-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add "hrefPointers" for adjusting resolution #386
Conversation
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.
As for "anchorPointer" #385, this looks promising and I'll do a second review later.
The "tree" example is not so easy to follow I think, but maybe I'm missing (relative) JSON pointer practice.
hyper-schema.json
Outdated
@@ -21,6 +21,8 @@ | |||
"type": "string", | |||
"format": "uri-template" | |||
}, | |||
"hrefPointers": { | |||
"description": "a map of variable names to relative instance JSON Pointers, which adjust the instance location from which template variable resolution begins", |
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.
Missing closing }
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.
Also maybe a "type" property.
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 avoided a "type" on purpose. While most will be objects, it would not be that odd to have an array. I can't think of a situation where this sort of data would be best handled as a string or a number but I couldn't thing of a compelling reason to restrict it either.
We can always see how it gets used and tighten it in future drafts if it really seems necessary. In general, the meta-schema errs on the side of permissiveness.
jsonschema-hyperschema.xml
Outdated
<xref target="I-D.luff-relative-json-pointer">Relative JSON Pointer</xref>, | ||
which is evaluated relative to the position in the instance from which | ||
<xref target="href">"href"</xref> template variable resolution would | ||
normolly begin. |
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: normolly
jsonschema-hyperschema.xml
Outdated
"rel": "self", | ||
"href": "/trees{/root,parent,id}" | ||
"hrefPointers": { | ||
} |
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 guess we'd expect something within hrefPointers
:)
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.
left as an exercise for the reader :-P
(I'll fix it)
jsonschema-hyperschema.xml
Outdated
<t hangText='"/children/0/children/0/children/0"'>/trees/0/1/2</t> | ||
<t hangText='"/children/0/children/0/children/0/children/0"'>/trees/2/3</t> | ||
</list> | ||
omitted, giving us the desired URI with two components after "/trees". 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.
This line is weird and unfinished.
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.
Yeah... I thought I'd copied over the example from the issue, but I have no idea what actually happened here.
@dlax the tree example should now be a lot easier to follow with actual pointers in the the schema, and without a stray line of explanation duplicated into a really confusing place. That will teach me to post a PR without actual proofreading. I would not be surprised if it's still a bit unclear, please do bring up those issues and I will improve the wording. |
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.
Tried to understand the tree example, but failed :(
In fact, I wonder if the example is not too convoluted or abstract. In particular, I was trying to think of a server-side router that'd get queried through these "self" links: the root node's URI would be /trees/0
rather than /trees/0/0
which would be the child of root with id=0 (or maybe the first child in children collection). Similarly, I'd expect /trees/0/1/2/3
to be the URI of inner node ({"id": 3}
), but the href model does not allow this. Arguably, that's not the only possible routing model, but it we were to explain (or assume) a more sophisticated one, I think it'd be a bit too much for readers.
Sorry if my comment does not look very constructive. I'd like to think about a better example, perhaps more "practical", but, at the moment, I don't have one...
jsonschema-hyperschema.xml
Outdated
"id": 2, | ||
"children": [ { | ||
"id": 3 | ||
} ] } ] } ] }]]> |
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.
Did you gather closing brackets on the last line on purpose? The block renders strangely 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.
Yes, because it takes up absurd amounts of space otherwise. I can do it closer the normal way, though. I'll still leave the "[ {" / "} ]" pairs together because otherwise it really takes up too much vertical space. But I'll put each closing pair on its own line, indented.
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.
Much better now!
jsonschema-hyperschema.xml
Outdated
<t hangText='"" (the root node)'>/trees/0/0</t> | ||
<t hangText='"/children/0/children/0"'>/trees/0/0/1</t> | ||
<t hangText='"/children/0/children/0/children/0"'>/trees/0/1/2</t> | ||
<t hangText='"/children/0/children/0/children/0/children/0"'>/trees/0/2/3</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.
I'm trying to apply the explanation in <postamble>
literally and can't get the last three examples working:
- "/children/0/children/0" points to
{"id": 2, "children": [{"id": 3}]}
, so it's URI should be/trees/0/1/2
because it's parent has "id=1" and the node has "id=2" - "/children/0/children/0/children/0" points to
{"id": 3}
, so it's URI should be/trees/0/2/3
(parent's id=2, context node's id=3) - "/children/0/children/0/children/0/children/0" does not resolve
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.
OMG I am so sorry, all of these have an extra /children/0 in them, no wonder none of this makes sense to you. :-(
I'll update this ASAP, but you can see that your results work perfectly if you chop off that extra path component. I think this was a copy-paste error from when I had more levels in the tree, and for some reason I cut the wrong part of the example when trimming it down.
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.
Similarly, I'd expect /trees/0/1/2/3 to be the URI of inner node ({"id": 3}), but the href model does not allow this.
This is where I always have trouble with examples. Perhaps this is just because the current example literally makes no sense the way I messed it up. But I find that so many people just want to redesign the example to do something else instead of reading the premise.
The intention was specifically not to do a normal tree layout.
The point is to show the need for both absolute and relative pointers. I'm not attached to this example, it just (when written properly) shows it more concisely than anything else. I try not to do this one with collections because inevitably, someone complains "I just wouldn't do collections that way."
I hate writing examples, and everyone always hates my examples :-P
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.
Although you bring up an interesting point that it's not possible to do that "normal' tree layout with hrefPointers
AFAIK. Hmm... I don't think we absolutely need to solve every URI construction approach, but it's worth thinking about.
jsonschema-hyperschema.xml
Outdated
"links": [ | ||
{ | ||
"rel": "self", | ||
"href": "/trees{/root,parent,id}" |
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.
trailing ,
missing
@dlax hopefully the example finally actually works now. I am very sorry for posting such a mess :-( I have added some wording around not being able to do the tree layout you expect, although I'm not sure if that makes things more or less confusing. I am totally open to replacing this example with another one (or perhaps two instead of trying to show everything at once?) Any ideas are welcome. |
@dlax any further thoughts on this? Does the current example work? Would you prefer to see multiple examples illustrating things a piece at a time? Any specific requests? I'm happy to write them but I'm notoriously bad at choosing compelling examples, so any requirements you can provide will help a lot. |
This looks really interesting, and I think I understand it, but it could do with a more simplistic example before getting into the tree example. I think it would help those with little-to-no context understand the feature. |
@philsturgeon thanks! If there are any pointers you can give me about what individual topics you'd like to see that would help. I have always struggled to write examples that others find compelling (I work from abstract to concrete, which is the opposite of most people and a recurring communication challenge for me). |
Yes, the current example works, thanks for improving it! (The "abstract" part is also fine, by the way.) One thing that makes the example difficult to follow is the URI structure. So if we try to keep resulting "href" flat (i.e. only one variable path segment), the reader would only need to focus on the
Does this cover all the cases you wanted to highlight? |
@dlax yes, this is great! Conveys the same information much more simply. This is exactly what I mean when I say I struggle with examples. My inclination is always to overload or go for the most pathological situation, and I have no idea how long it would have been before it occurred to me to simply do each case like that :-P @philsturgeon is @dlax's version easier to follow for you as well? |
jsonschema-hyperschema.xml
Outdated
"href": "/trees{/root,parent,id}", | ||
"hrefPointers": { | ||
"root": "/id", | ||
"parent": "2/id" |
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 2 is a bit of a magic number at this point, as explanation of IDs 1, 2, 3, 4 only comes after. on line 731.
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.
Makes sense, I'll address that.
This does create a bit of confusion as to what the "up" link should produce for the root node. In the complex example, it still produced a valid URI (however convoluted it was to understand). In this version, you'll just get This is actually a pretty good example of issue #49 about nonsensical template expansion results. |
What about {
"type": "object",
"required": ["id"],
"properties": {
"id": {"type": "integer", "minimum": 0},
"children": {"type": "array", "items": {"$ref": "#"}}
},
"links": [
{
"rel": "self",
"href": "/trees/{rootId}/{id}",
"hrefPointers": {
"rootId": "/id"
}
},
{
"rel": "up",
"href": "/trees/{rootId}/{parentId}",
"hrefPointers": {
"parentId": "2/id",
"rootId": "/id"
}
}
]
} which would make the parent of the root be |
I like it! |
I think that I like the variation I did, with two links, a bit better- @philsturgeon are you OK with that one? Not sure which of the two you were commenting on. The pathological case of the root's parent will require a bit of explanation, but is no worse of a problem than it is without |
I am also happy to go with the three-link option if that is more clear. As noted, I'm not really the best judge of examples. |
The point of having several different links is that of them illustrates a specific situation (standard context resolution without
Maybe changing the href to
Yes, just bikeshedding about the example :) |
Yes, this is what I meant. "self" can show the use of absolute pointers, and "up" relative ones. We don't need to re-display the standard process, just show how it looks in context (not present in
This is actually exactly what I have in my workspace, but the "up" link is still |
Not enough internet to check the PR. Don't hold up on my behalf, looks like you two have got to the *root* of the problem. 😂
|
@philsturgeon it's got 2 days of the comment period left, and since I'm about to push a new version of it, I'll probably give it a few days extra. No obligation for further review, but you certainly don't need to get it done today. |
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.
Suggested a couple of minor changes in handrews#3
3738422
to
b131bfe
Compare
@dlax I accepted one of your commits (by rebasing it directly from your fork into my workspace) and took an alternate approach to the other one. And fixed the links.json meta-schema which was underspecified for some reason. |
This is a companion to "anchorPointer", as they use the same mechanism for adjusting locations within the instance. While "anchorPointer" adjusts the context, "hrefPointers" adjust how template variables are resolved. "hrefPointers" replaces and is much more functional than the preprocessing rules that were removed in draft wright-01. Those rules only allowed adjusting to use the sub-instance as a whole intead of individual properties or elements. "hrefPointers" supports that as well as using data from any containg or sub-instance.
Also note limitations of the hrefPointer system.
This example simplifies the concepts by getting rid of the confusing variable-length URIs and just acknowledging that the parent URI for the root node does not make any sense. This sort of situation is addressed elsewhere in the spec, so add an xref to it. The reworked example uses two links: one to illustrate standard resolution (not appearing in "hrefPointers") alongside of an "hrefPointers" adjustment using an absolute pointer, and then another illustratig both absolute and relative pointers.
Specify the actual object and property types, and drop the description as we no longer keep those in the meta-schema.
This is a companion to "anchorPointer" (#385), as they use the same
mechanism for adjusting locations within the instance. While
"anchorPointer" adjusts the context, "hrefPointers" adjust
how template variables are resolved.
"hrefPointers" replaces and is much more functional than the
preprocessing rules that were removed in draft wright-01.
Those rules only allowed adjusting to use the sub-instance
as a whole intead of individual properties or elements.
"hrefPointers" supports that as well as using data from
any containg or sub-instance.