Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Propose better JSON-LD processing text #1302
Propose better JSON-LD processing text #1302
Changes from 1 commit
6812f81
a43e85b
67bc6ff
c3422a0
39494a8
228b5bc
ff77608
36af583
807f364
a87a036
6c3f953
fb61da7
dc102f7
60b603c
3b6199e
f3ca404
96fdf7a
517d91f
afd7cda
9b6dbd2
4ba55fd
ca8324f
3ea8261
9979be7
94f9221
027506a
3123715
91e5a37
9959a04
65e3a8e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notion of "RDF processing" is not defined, and it can mean many things. Better not go there. Besides, it took me some time to understand what (I think) you meant in this section... What about a rewrite of the whole paragraph along the lines of:
Also, in view of what you write later, I have the feeling that you have in mind two type of "processing":
if that is correct, this may be the place to define these, because otherwise the text below is very unclear. That being said, I do not like the two terms; for me, "JSON-LD Processing" means to perform the processing steps defined in the JSON-LD 1.1 Processing Algorithms and API specification, and this seems to contradict this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this is correct, my goal is to basically say this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is not a thing. You never "ignore"
@context
. You can hard-code your application to only work with some specific contexts, that's fine. But you don't "ignore"@context
; that is not at all the same thing. Perhaps this is part of a core misunderstanding here?The comments about context immutability are a different kind of issue. We can have text here that says: "if you're going to hard-code to specific contexts, they must be immutable in order to guarantee the same interpretation as someone who doesn't hard-code". That is a simple fact and encouraging the use of immutable contexts for VCs is a good thing, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd take "Some scenarios where limiting RDF processing"... but I won't take removal of RDF entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear what "intended expression of the Verifiable Credential Graphs" means, or how this is different from "Minimal JSON-LD Processing" (which is what we should rename the JSON Processing section to, IMHO).
Perhaps we should have these sections: "JSON-LD Processing", which contains two sub-sections: "Processing without a JSON-LD Library" and "Processing as RDF"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this suggestion, I will update to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments on the same section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw now @dlongley's comment that the whole bullet point should be removed in view of the recent changes in RDFC; I would be o.k. with that, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to give direct security advice here, conforming documents are JSON, schema validation can protect against poisoning... telling people to read the RDF spec without spelling this out is harmful to the security posture of our document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dataset poisoning is an issue in general but, I have the impression, it is very rarely an issue with v. credentials or presentations, and the text we are writing here relates to those. (It has been my suspicion for a long time that a VC/VP dataset is, from an RDF point of view, "well-behaved" 90% of the time, which means that the "tricky" part of the RDFC 1.0 algorithm, that may run amok, is not applied in the first place. But I cannot prove that.) The VCDM spec does not even talk about defining a blank node explicitly, something that is usually done to create poisoned datasets.
(Let alone the fact that, per RDFC 1.0 spec, each implementation may have its own way of defending itself against poisoned datasets, whose parameters (that the user may fiddle with) are therefore also implementation dependent. The RDFC standard only stipulates that a defense must exist and have a reasonable default parameter setting; the test suite does include a few poisoned datasets that the implementation should pass.)
As a conclusion, I do not believe that going to the details of the RDFC 1.0 is really an issue for this text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13, please respond to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decline to implement feedback, but perhaps a stronger rewarding regarding the security impact would be acceptable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, "Naive JSON processing" is never allowed ... with any JSON. I guess I don't know what this means. When you are processing JSON ... or anything generally, you shouldn't be naive. There is also no special danger of confusion or deviation of interpretation from generalized processors if the rules are followed, so we should focus there.
An attempt at rewording:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think your suggestion makes it clearer. The issuer is required to use
@context
, since it's required in conforming documents.Perhaps you mean additional context beyond the v2 core context? can you adjust your suggestion to take that into account?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made an adjustment to say "Issues might use additional contexts... and consumers are advised...".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13 please respond to this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to define "shapes that are well-known", assuming some version of this PR will land:
#1320
I can accept this suggestion as it exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13,
Yes, please, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the intention of the issuer is preserved... because it's a conforming document. The issuer produced a conforming document, the verifier is consuming that conforming document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because in the case the issuer included structure in the context, and a verifier ignores the context, the verifier ignores the issuers intent.
The verifier has no way to know if the issuer meant for them to ignore the context or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No verifier can ignore
@context
; that is not conformant processing. You can only consume properties from contexts you understand. If that's only the core context, that's fine -- but you can't read properties from other contexts unless you understand them.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decline to implement feedback, although as I said in the other comments, while the current language is effective, it might be improved in a future editorial PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is true now with some clean up above and it's simpler:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not true, but could be true modulo some defense language around contexts changing, and it would be nice to say it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlongley @msporny I am not intending to accept this suggestion, in the interest of getting this PR to mergable state, I suggest we resolve any threads that we feel don't need further discussion on the PR, if you need issue markers to resolve a thread, please file issues, and I will address all issues filed in a subsequent PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find an alternative subtitle? There is nothing "Advanced" in what is in the section, at least not from an RDF point of view...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we remove this subsection. It's not clear what "expressions of intention" would actually be. The closing paragraph in this section also states that...
The claims being made in the separate graphs are no more "separate" conceptually in JSON than in RDF and any "intention" toward their use would be on the Verifiers (or whomever) to "unite" as/when desired.
The other items about
id
/type
and value ordering introduce more confusion than value in their current form. Folks familiar with processing RDF out of JSON-LD will know about aliasing and value ordering, and (as @iherman mentions) there's nothing "advanced" about it...just par for the course when using term aliasing and graph data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention was to clearly communicate that there is "JSON-LD processing" where you don't apply the context (This was previously called "JSON Processing"... and there is JSON-LD processing, where you do apply the context (this is currently titled "Advanced RDF Processing".).
I'm not attached to the names, I am attached to signaling that both processing rules are in the context of RDF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not call the first "JSON-LD Processing", because it is very misleading. The term "JSON-LD Processing" is a term coined by the JSON-LD WG and has a well-defined meaning. It is actually closer to what you call "Advanced RDF Processing".
What we describe in the spec handling VC files in JSON-LD format but without a full JSON-LD Processing is, though fairly general, proprietary to the VC specification. Maybe using something like "Basic VC Processing" would do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Manu suggested calling in RDF processing, I think thats a fair compromise... its the "JSON-LD... but with transformations that care about effort put into the context" processing kind...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlongley suggested "Static Processing" vs. "Dynamic Processing"... where "Static Processing" covers things like "Use a JSON Schema to check the well-formed-ness of the payload and then process it like you would a JSON payload... and where "Dynamic Processing" is using a JSON-LD processor to compact/expand or convert to NQuads.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the "Dynamic" vs. "Static". I do not lie down on the road over this, but these two terms do not speak to me in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this statement is correct, the usage of
@protected
is way more far-reaching than that: it is used to protect all terms define in this specification. That is what should be emphasized, not only these two imho.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, do you you have any concrete text, or maybe its better for us to just refer to https://www.w3.org/TR/json-ld11/#protected-term-definitions ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the JSON-LD spec alone is too terse for those who would really dive into this section. Something like
"Implementers should also be aware that all terms, defined in this specification, are marked as
@protected
[link to the JSON-LD spec], meaning that subsequent, application specific context files would not be able to change them."I leave the details to native Anglo-Saxon speakers... :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we addressed this, and we'll need to resolve things that are addressed so I can process the remaining blockers. can you confirm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this addressed? The text has not changed...
To be more precise: in §6.1.1 of the spec, the role of
@protected
is indeed explained (in the last bullet), so is the fact that@id
and@type
are aliased. In fact, this paragraph does not add any information that would not be explicitly stated in §6.1.1. Maybe the best is to remove it altogether.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.