Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn that using dynamic processor features might lead to less interoperability #1276

Merged
merged 8 commits into from
Nov 17, 2023

Conversation

msporny
Copy link
Member

@msporny msporny commented Sep 11, 2023

This PR attempts to address issue #1206 by warning document authors that using dynamic JSON-LD processor features, such as full URLs like https://schema.org/Person for terms/types, might lead to less interoperability with static JSON-LD processors.


Preview | Diff

Copy link
Contributor

@decentralgabe decentralgabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there any static validation tools implementers can use to not footgun themselves? maybe better suited for an impl guide...but worth having

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@TallTed
Copy link
Member

TallTed commented Sep 14, 2023

I think I preserved intended meaning. Negation of negative negation is challenging.

index.html Outdated Show resolved Hide resolved
Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still does not address what the value of the non compact JSON-LD is... and if that value is not elaborated on substantially, we should recommend no JSON-LD processing at all.

I would like to see language that specifically addressed application n-quads, or JSON-LD with @graph

@OR13
Copy link
Contributor

OR13 commented Sep 14, 2023

I recognize it might be better addressed in the value of JSON-LD PR. Before we add more text saying you don't need to do JSON-LD processing, and should avoid creating problems when JSON-LD processing occurs we need to address the value of JSON-LD processing, and in particular the value of RDF... which is ultimately what JSON-LD is about.

@iherman
Copy link
Member

iherman commented Sep 15, 2023

The issue was discussed in a meeting on 2023-09-14

  • no resolutions were taken
View the transcript

1.7. Provide guidance to use compact terms/types instead of full URLs (pr vc-data-model#1276)

See github pull request vc-data-model#1276.

Brent Zundel: Few requests for changes from TallTed. Not a lot of review yet, raised 2 days ago.
� We'd like to see how to move this forward.

Manu Sporny: This is in response to an issue that DavidC raised.
� We're suggesting to people that they do not use full URLs for things that have json-ld terms. Doing so would harm interop. Not illegal, just a suggestion to not do it.

Brent Zundel: Please review folks.

index.html Outdated Show resolved Hide resolved
index.html Outdated
<li>
Always using the compact form for JSON-LD terms and types and always ensuring
that those terms and types are defined in application-specific JSON-LD context
files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest replacing the above three lines by:

Always using the compact form for JSON-LD terms and types when these are defined in JSON-LD context files.
Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Chadwick -- I'm confused by your comments. Note that the Use ... files in green in my change request, which replaced the three lines you're suggesting be replaced by something else, is an item in unordered list of "avoid using these JSON-LD features" ... and you seem to be saying "don't tell implementers to use JSON-LD context files" which is already happening. Part of my confusion stems, I think, from your having made 3 comments on the same bullet point, which comments don't seem to be fully connected thoughts, nor to take my existing change request into consideration.

My sense is that you're reading the PR Preview, which doesn't reflect suggested changes until/unless the suggestions are merged.... Please take another look at the HTML source, and base your suggestion on the total context of that unordered list, starting with line 3991 and continuing to line 4011?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TallTed Your change request is just that, a request. It has not been merged nor accepted. When it has been I can comment on it. But until then my review is based on the text that is displayed to me when I perform the review. This is the text I am asked to review. So your sense that I am reading the PR Preview is correct.
However I have also looked at your change request and it does not appear to change the semantics of the original text, as it appears to only remove the word 'always'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[@David-Chadwick] it appears to only remove the word 'always'

I strongly suggest that you re-examine my change request for that paragraph (bullet point), and tell me again whether it appears to only remove the word 'always'. If so, I will have little choice but to ignore your comments, as you are not responding to the full flesh of my own; indeed, you're acting as if my change requests do not exist.

PRs on GitHub are conversations. PR Preview is a helper but it only shows the result of the PR as initially drafted, and as/when conversation on the PR results in revision of the PR (which is to revise the text of the document). Ignoring the conversational aspect of this tool does a disservice to all involved, yourself included.

(To be most explicit: My change request significantly rewords the content of that bullet, which is to make it make sense in context of my small rewording of the intro to those bullets, along with my suggested changes to the other bullets in the list, which are to make them all agree with one another. Your own change only applies to the single bullet, not to the intro, nor the other bullets, and does not appear to me to help in bringing clarity to this segment of the document.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Chadwick wrote:

Always using the compact form for JSON-LD terms and types when these are defined in JSON-LD context files.

This seems fine.

Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.

Hrm, but we have a base @vocab that makes it so they don't have to do that. I believe there were objections in the group to using long-form URLs and adding the text you suggest above would re-raise those objections. @OR13, I believe you were one of the people that wanted clear guidance here -- thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I escaped the thread: #1276 (comment)

The linked comment is probably a better way to address this issue.... the default behavior should not appear to be illegal, imo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Application specific terms and types MAY use the long (URI) form when the compact forms are not defined in any JSON-LD context file.
The reason for adding this text is that it still allows compact local names to be used because of the @vocab term, but it also allows implementors who wish to use a global long (URI) name to use that as well, ensuring that no other VC Issuer will have a name clash with their invented compact local name when they do not want to start creating their own @context files

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've attempted to address this issue in the latest series of edits, please re-review @David-Chadwick.

@OR13
Copy link
Contributor

OR13 commented Sep 28, 2023

@msporny I think this comment addresses the impact that @vocab has on @type... #1290 (comment)

^ if folks don't understand what the @vocab is doing in the v2 context... they are probably not ready to use W3C Verifiable Credentials... the context is now normative.

@iherman
Copy link
Member

iherman commented Oct 12, 2023

The issue was discussed in a meeting on 2023-10-11

  • no resolutions were taken
View the transcript

1.13. Provide guidance to use compact terms/types instead of full URLs (pr vc-data-model#1276)

See github pull request vc-data-model#1276.

Brent Zundel: Guidance for the use of compact terms and types rather than full URLs. I know this is related to the light processing PRs.

Manu Sporny: There's an issue raised to provide clear guidance on using compact terms vs. full URLs. I think Orie is saying that this PR doesn't address this issue and it might be better address in the value of JSON-LD PR. I don't know what is needed to make it so that Orie is ok with it. It is a PR that would be closed because we couldn't get to consensus.

Brent Zundel: Thank you, Manu. We're at time for the meeting. Appreciate folks joining, kinda crazy with people at IIW, etc.
… Thank you for scribing, Dave, thanks for all the work people have done, see you later!


@brentzundel brentzundel added the pending close Close if no objection within 7 days label Oct 17, 2023
@msporny
Copy link
Member Author

msporny commented Oct 17, 2023

@TallTed @OR13 @David-Chadwick Your change requests have resulted in this PR being marked as pending close. If you feel you can save this PR w/ change suggestions, please do, if not, the PR will be closed and the issue will go unresolved in this iteration of the WG.

index.html Outdated
</li>
<li>
Use of full URLs for JSON-LD terms and types, instead of the compact forms of
those values as are defined in application-specific JSON-LD context files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about
"instead of the compact forms of those values whether or not they are defined in application-specific JSON-LD context files"
This covers the cases when a new context file is created or when a new context file is not created.

Copy link
Member

@TallTed TallTed Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Chadwick — There is no "compact form" of such URL values, if they're not mapped (I think "defined" remains the wrong verb for what @context files do, whether I accidentally re-introduced it or overlooked it, in a relevant comment or suggestion related to this thread) by an @context file, no matter whether that file is old, new, generic, application-specific, etc.

The thing called "compact form" here — probably better to be called the "unmapped form" — is the naked word, the non-URL. The full URL is what that "unmapped form" is mapped to by some @context identified by the JSON-LD document in question, which is probably one of the documents cited in the VC specifications, unless the "unmapped form" in question is not mapped by any of those VC @context files.

These "unmapped forms" might be validly referred to as "JSON keys" given that a JSON file contains some number of key/value pairs; and a JSON-LD file contains some number of key/value pairs where the JSON-LD-relevant keys are mapped by a file identified by the @context property, and some additional number of key/value pairs involving keys that remain un-mapped by any identified @context are ignored as non-conforming or uncrecognized.

Suggested change
those values as are defined in application-specific JSON-LD context files
those values as are mapped by application-specific JSON-LD context files

Beyond that, I don't believe I drafted these bullets nor their introductory paragraph, though I did content-edit both, so I'm hoping earlier author(s) will chime in on whether they see their meaning as being preserved, lost, clarified, distorted, etc., by current efforts.

index.html Outdated
</li>
<li>
Using full URLs for JSON-LD terms and types instead of the compact form of those
values defined in application-specific JSON-LD context files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace with
"Use of full URLs for JSON-LD terms and types, instead of the compact forms of
those values which may or may not be defined in application-specific JSON-LD context files"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suggestion makes no sense in context of the existing thread (started by your 19 hour older comment, followed on by my 10 hour older comment). The verb tense here ("Using") breaks from the intro preceding the <ul> and differs from that in both bullets including this one. I had much to say about your "values which may or may not be defined"/"values whether or not they are defined" in my previous comment, but that comment appears to have been ignored completely.

Copy link
Contributor

@David-Chadwick David-Chadwick Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get your point. But I am not sure you get the point I am trying to make, which is: an issuer that only uses the default at context files, but defines their own application types, wishes to use the full URLs for these, since the VCDM requires them to be URLs. The issuer does not want to use a local string that maps via the at vocab into a URL, since another issuer could use the same local string that maps into the same URL via the at vocab, whilst it has a different semantic. Using the full URL is conformant to the VCDM but the proposed text says that it might affect interoperability. However using a local string could cause a bigger car crash because recipients would interpret two different application 'local string' types to be the same one by mapping them both via the at vocab.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I understand what you're trying to say here. I think it may not be possible (nor necessary) to wedge all that nuance into this list item. I have a new draft for the list item, which changes both existing lines, so it's not going directly on this thread (which is only attached to one line of text). I suggest trying to craft a new list item, if the existing list items do not now cover your concern as elaborated above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we could change the text of this PR to be more precise, which you could help by wordsmithing the following e.g.

"using full URLs for JSON-LD terms and types instead of the compact form of those
values that are either explicitly defined in application-specific JSON-LD context files, or are implicitly defined by the @vocab feature of the base @context file https://www.w3.org/ns/credentials/v2

@brentzundel brentzundel added post-CR and removed pending close Close if no objection within 7 days labels Oct 31, 2023
@iherman
Copy link
Member

iherman commented Oct 31, 2023

The issue was discussed in a meeting on 2023-10-31

  • no resolutions were taken
View the transcript

1.11. Warn that using dynamic processor features might lead to less interoperability (pr vc-data-model#1276)

See github pull request vc-data-model#1276.

Brent Zundel: This one is already marked pending close.
… David is requesting changes, Orie is requesting changes.
… It's been two weeks.
… We did get some more feedback from David.
… Orie seems fine with it being closed. That's where we're at. I'm planning on closing after the call at this point.

Manu Sporny: The reason it was marked pending close is because Orie was objecting but just didn't use the button to object. I don't see a way around ... we need Orie to respond or it's getting closed and I expect closure.
… If this is closed, we should also close the feeder issue, which was raised by Orie.
… There are a subset of people in the WG that are pushing for all JSON-LD features and not to say anything else. This PR was about saying that interop will be better if you don't use all features all the time.
… This is why we were going to say that if you use fewer features and profile down that you'll get more interop and it's fine and legal to do that both that and with more features. This PR was saying that the more features you use, the less interop you may see.
… I don't know how that affects the original issue Orie raised, but if we close this PR, we should close that other issue as well.
… Noting what Dave is saying -- this is a post-CR issue, we could mark it as that.
… That's another path forward here. I want to make it clear that David Chadwick has been saying he wants to use full URLs in certain places, there's no spec text stopping that, it's just going to lead to interop issues and the spec won't have had any warnings about that.
… We will continue to just stay vague.

Manu Sporny: This is also related to #1206.

Brent Zundel: I appreciate Dave pointing out that this is a post-CR PR.
… It would be inline with the other PRs that we don't have consensus on to mark it as post-CR if we feel like it's possible to have a path forward that leads to some sort of consensus.

Dave Longley: If I recall, Orie's objection was around recommending that people don't use certain JSON-LD features. I think we've changed the text to say it's not the goal... we want to inform people that if they use a lot of features, they might get less interop.
… I don't know if we've gotten sufficient feedback from Orie on that, but that might be a path forward in the post-CR phase.

Brent Zundel: Thank you, Dave, I appreciate that comment, I've marked it post-CR.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
<li>
Use of full URLs for JSON-LD terms and types (e.g.,
`https://www.w3.org/2018/credentials#VerifiableCredential`), instead of the
unmapped forms of any such values (e.g., `VerifiableCredential`) that are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(e.g., https://www.w3.org/2018/credentials#VerifiableCredential or https://example.org/somenewtype) instead of the unmapped forms of any such values (e.g., VerifiableCredential or somenewtype) that are

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@David-Chadwick -- "that are" what? Your comment above seems to have been cut off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no its not. This is a replacement for lines 3905 and 3906 so that 3907 continues as before

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: The below is not my change suggestion, and I do not support it. I would draft further changes that might help, if it is re-suggested using the tools as designed. — @TallTed


@David-Chadwick — Ah! Your comment started with (e.g., which made me think it was starting a new thought, because no line above starts with (e.g.,.

When you're making a change suggestion, there's a toolbar, with an ± icon as highlighted below.

https://docs.github.com/assets/cb-26290/mw-1440/images/help/pull_requests/suggestion-block.webp

If you click that, it will copy the lines you've selected to comment on to the text entry box, within a suggestion block -- that is, with a leading line of —

```suggestion

— and a closing line of —

```

— and if you edit those lines to be what you want to suggest, something like this —

```suggestion
(e.g., 
`https://www.w3.org/2018/credentials#VerifiableCredential` or `https://example.org/somenewtype`) instead of the 
unmapped forms of any such values (e.g., `VerifiableCredential` or `somenewtype`) that are
```

— then future readers will see something like this (now that I understand what you were trying to say), with the existing text directly above your suggested text, with deletions highlighted in red and additions highlighted in green —

Suggested change
unmapped forms of any such values (e.g., `VerifiableCredential`) that are
(e.g.,
`https://www.w3.org/2018/credentials#VerifiableCredential` or `https://example.org/somenewtype`) instead of the
unmapped forms of any such values (e.g., `VerifiableCredential` or `somenewtype`) that are

Now, this only shows line 3906 as being changed, because you only selected that line for your comment. Next time, try click-and-hold on 3904 (where the (e.g., is found) and then drag down to 3906 (which ends with that are), to clearly suggest changes to those three lines.

If you use this tool as designed, you'll find your suggestions are much clearer to your readers.


NOTE: The above is not my change suggestion, and I do not support it. I would draft further changes that might help, if it is re-suggested using the tools as designed. — @TallTed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thankyou. I will try this next time. I was not aware of the feature. I only thought you could edit line by line

index.html Outdated
`https://www.w3.org/2018/credentials#VerifiableCredential`), instead of the
unmapped forms of any such values (e.g., `VerifiableCredential`) that are
among active JSON-LD `@context` mappings (e.g.,
`https://www.w3.org/ns/credentials/v2`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add
" through either explicit mappings or the @vocab feature that covers all other unlisted values"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[@David-Chadwick ] add
" through either explicit mappings or the @vocab feature that covers all other unlisted values"

I don't understand what you intend to communicate by this addition, so I cannot suggest revision. My inability to decipher your meaning might suggest a need for rephrasing such that others — who do not have even the benefit of our discussions to date — to understand it.

Copy link
Contributor

@David-Chadwick David-Chadwick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to use an example of the problem that I am raising

@TallTed
Copy link
Member

TallTed commented Nov 13, 2023

[@David-Chadwick] It would be helpful to use an example of the problem that I am raising

Please do provide such an example!

@David-Chadwick
Copy link
Contributor

@TallTed Can you please wordsmith this to be precise

Use of full URLs for JSON-LD terms and types (e.g., 
`https://www.w3.org/2018/credentials#VerifiableCredential` or `https://example.org/somenewtype`) instead of the
unmapped forms of any such values (e.g., `VerifiableCredential` or `somenewtype`) that either are
among explicit JSON-LD `@context` mappings (e.g.,
`https://www.w3.org/ns/credentials/v2`) or are implicitly mapped via the @vocab feature that covers all unlisted values.

@iherman
Copy link
Member

iherman commented Nov 14, 2023

The issue was discussed in a meeting on 2023-11-14

  • no resolutions were taken
View the transcript

1.2. Warn that using dynamic processor features might lead to less interoperability (pr vc-data-model#1276)

See github pull request vc-data-model#1276.

Brent Zundel: this is post-cr, so would like to understand status today.

Manu Sporny: I believe DavidC was asking for changes and manu requested and received re-review with some subsequent discussion.
… I think that we need an example from DavidC to move this along.
… sounds like DavidC is opposed to this PR as it stands, would like some changes, and we need explicit change requests.

David Chadwick: I did produce the text, but TallTed had suggested reformat as a block.
… at the moment there is one example, which uses a term defined in @context and uses the alias for that.
… I wanted to add a second example with an alias not explicitly defined in @context that would be turned into a URI using @vocab.

Ted Thibodeau Jr.: that explanation is helpful, but I don't see that communicated in the current text.
… if you update your change request with this text I think it will be easier to move forward.

David Chadwick: I had some difficulty with the GitHub UI trying to add multiple lines to the change request.
… I will try just adding a block of text to bypass this (maybe broken) UI issue.

Manu Sporny: Can we change this one to pre-cr? I've been ignoring post-cr issues, but this one seems like it may have a resolution this week.

Brent Zundel: It might make sense to do that, yes.
… it seems like we are close, and ideally this one will get merged this week.

@msporny
Copy link
Member Author

msporny commented Nov 17, 2023

@David-Chadwick I've edited your content here: #1276 (comment)

@msporny
Copy link
Member Author

msporny commented Nov 17, 2023

Editorial, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny merged commit 60cd81b into main Nov 17, 2023
1 check passed
@msporny msporny deleted the msporny-use-terms branch November 17, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants