-
-
Notifications
You must be signed in to change notification settings - Fork 313
URI Template resolution as pseudocode #425
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
Conversation
This consolidates all of the URI Template resolution sections into one section. The new section is at the top level, as it applies to both LDO keywords ("href", "anchor", and all of the algorithm modifiers) and to a schema keyword ("base"). As much of the algorithm as possible is now given in pseudocode, although whether this is the most readable form is debatable. It is certainly more concise, and forced detailed consideration of some steps which had been under-specified, particularly with respect to "hrefSchema". The main changes are that setting any applicable subschema for a template variable to "false" in "hrefSchema" excludes that variable from accepting input, and that the input data for "hrefSchema" is prepopulated only with instance data that validates against all applicable subschemas within "hrefSchema". Expressing this properly requires the "applicability" concept from PR json-schema-org#424. Previously, the only mention of how to exclude a field from being used to pre-populate input was the advice to set the "properties" subschema to "false". However, this does not hold up once you start using more complex schemas.
An alternative approach here would be to say that an implementation MUST provide the from-instance template data set to callers alongside of If we went that route, I would still like to include this pre-population approach as an option that an implementation SHOULD support directly as a convenience, as I think it will be the most common and interoperable approach. But I could see requiring a more flexible option alongside of it. |
While I've been reading the algorithms a couple of times, I'm still having hard time following... So it's hard for me to figure out the scope of the changes of this PR and make an opinion. One issue with the "pseudocode" form is that it's part of a normative section of the spec (as opposed to in an appendix for example), so it can hardly be skipped. I have the impression that such extensive use of pseudo-code in a normative section is not an usual practice. I can't really tell how this would look like in another form: certainly be more verbose, but maybe easier to follow? For instance, would something like the Implementation Hints in RFC6570 work here? |
@dlax it's mostly that I needed to write the pseudocode in order to work out the steps that needed to be done for it to actually work. I'm not at all attached to it as the form of explanation. I just posted this for feedback on the direction. |
@dlax I added some English descriptions of the various steps, see if that makes it easier to comprehend. I added the same commit to the main rewrite PR as well. |
As reading the rewrite PR, I haven't seen any obvious problem with this approach of presenting the URI template resolution consolidated into a dedicated 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've been rereading this as part of #427 so here is some more questions. (Commenting here instead of in the other PR as it seems more appropriate to me.)
Since nobody has a better idea than the pseudocode approach, maybe we should stick to it and move forward.
jsonschema-hyperschema.xml
Outdated
<![CDATA[ | ||
templateData = populateDataFromInstance(T, ldo, instance) | ||
|
||
if resolving "href" or "base" for "href" and ldo.hrefSchema 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.
I don't understand this line. Perhaps resolving
needs to be explain or expressed as a function? Then it's not clear how things come together, perhaps adding some parentheses would help.
jsonschema-hyperschema.xml
Outdated
template resolution data set | ||
</t> | ||
</list> | ||
</t> |
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 paragraph (the whole <t>
) does not look correct to me, or I'm not understanding it. It seems to me that it does not match with the pseudocode below. Specifically, I don't get the "Look in the immediate instance for remaining values".
Also I wouldn't say that "various data sources in the instance" are used (or, again, I may be missing what you mean by "various data sources"); to me the instance is just one source.
@dlax I've reworked some wording based on your feedback, please take a look and see if that's more clear. |
@handrews It's more clear, 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.
LGTM overall.
Just spotted a couple of typos and suggesting minor improvements.
jsonschema-hyperschema.xml
Outdated
<preamble> | ||
This is the high-level algorithm as pseudocode. "T" comes from either | ||
"href" or "anchor" within the LDO, or from "base" in a containing schema. | ||
Pseudocode for each step follows. "anchorOrHref" indicates which of the |
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 something like templateKind
instead of anchorOrHref
.
jsonschema-hyperschema.xml
Outdated
All three keywords share the same algorithm for resolving variables from | ||
instance data, which makes use of the "templatePointers" and "templateRequired" | ||
keywords. When resolving "href", both it and any "base" templates | ||
needed for resolution to an absolute URI, the algorithm is modfied to |
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.
typo: modfied
jsonschema-hyperschema.xml
Outdated
appears in that keyword's value | ||
</t> | ||
<t> | ||
Otherwise, look for a property names matching the variable in |
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.
typo: names -> name
jsonschema-hyperschema.xml
Outdated
<preamble> | ||
"InputForm" represents whatevers sort of input mechanism is appropriate. | ||
This may be a literal web form, or may be a more programmatic construct | ||
such as a callback funciton accepting specific fields and data types, |
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.
typo: funciton
</figure> | ||
</section> | ||
|
||
<section title="Accepting input for template 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.
Might be worth mentioning that this step corresponds to the acceptInput
call in this code block above.
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.
Really? I have to say that "Accepting input" == "acceptInput"? Even when it's in the right order and it's the same words already? Let me think on if there is a better way to present this if it's that confusing as is.
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's certainly not confusing now that you mention it. Perhaps I just read it without enough concentration... Let's leave this comment out for the moment.
Define the condition for using hrefSchema more clearly (I hope). Use consistent language around "locations" in a schema rather than introduce a new term "data sources" which is not defined. Attempt a better explanation of how to find values in the instance. I'm not entirely sure it's successful but at least it's different.
URI Template resolution as pseudocode
NOTE: This is a chunk of work that I have done as part of the rewrite. This probably isn't quite the right way to specify it, but seems like the best way to get some feedback on the approach and changes before putting a lot of work into new prose. Please assume that perfect examples are present wherever you want them to be :-) I have not yet consolidated the examples into a more focused set so they're not included in this PR.
This is one of the two most complex things an implementation must do in order to implement hyper-schema (the other thing being finding all applicable schemas per #424).
This consolidates all of the URI Template resolution sections into
one section. The new section is at the top level, as it applies
to both LDO keywords ("href", "anchor", and all of the algorithm
modifiers) and to a schema keyword ("base").
As much of the algorithm as possible is now given in pseudocode,
although whether this is the most readable form is debatable.
It is certainly more concise, and forced detailed consideration
of some steps which had been under-specified, particularly with
respect to "hrefSchema".
The main changes are that setting any applicable subschema for
a template variable to "false" in "hrefSchema" excludes that
variable from accepting input, and that the input data for
"hrefSchema" is prepopulated only with instance data that
validates against all applicable subschemas within "hrefSchema".
Expressing this properly requires the "applicability" concept
from PR #424.
Previously, the only mention of how to exclude a field from being
used to pre-populate input was the advice to set the "properties"
subschema to "false". However, this does not hold up once you
start using more complex schemas.