$recursiveRef part 2 of 3: $recursiveRef and $recursiveAnchor #654
Conversation
handrews
force-pushed
the
rec-ref-anchor
branch
from
27f96c3
to
319421c
Compare
There was a problem hiding this comment.
A few gramatical and preference changes requested. Otherwise:
I feel I have a correct understanding of how this works now, however I struggled because of the use of "basic" and "extension" schema naming.
The schemas I've been working with work on an entity component style architecture, so I kept assuming that an extension was used within a basic schema, which caused me to initially read the last paragraph (of additions) to be the wrong way round.
In the examples you've used, you have a basic schema, and are extending functionality with the "extension" schema, rather than including the extension within the basic schema.
In terms of usage in your examples, the "extension" schema is the one you would use, not the "basic" schema. Maybe renaming it to "extended" or "extendedBasic" would alleviate this confusion.
If I got a little confused, others might be too.
| description of the <spanx style="verb">"things"</spanx> | ||
| property. What we want is for it to resolve | ||
| to the combined schema as well. | ||
| </postamble> |
There was a problem hiding this comment.
Checking my understanding:
The problem is you want the $ref in the base schema to reference the not just itself statically, but the root schema, whatever the root schema is when referneced by another schema, as if it as transclueded / included. Right?
There was a problem hiding this comment.
Um... maybe? The whole transcluded/included thing does not really help me.
jsonschema-core.xml
Outdated
| just fine as a normals static reference. And the | ||
| "$recursiveRef" in the "extended" schema does not behave at | ||
| all differently, because the "$recursiveAnchor" in its | ||
| referred schema is the outermost "$recursiveAnchor" in the |
There was a problem hiding this comment.
I think I understand this, but I'm not sure, and end up confused because of the use of the phrase "in its referred schema". Maybe it's worth adding "(the extended schema root)" after "its referred schema"?
I am reviewing this while distracted and tired to simulate a first time reader, so feel free to dissagree on this =]
jsonschema-core.xml
Outdated
| referred schema is the outermost "$recursiveAnchor" in the | ||
| dynamic scope. However, the "$recursiveRef" in the "basic" | ||
| schema referrs to a "$recursiveAnchor" that is not the | ||
| outermost such keyword in the dynamic scope. That is still |
jsonschema-core.xml
Outdated
| the "$recursiveAnchor" in the "extension" schema. | ||
| Therefore, when processing starts with the extension | ||
| schema, the "$recursiveRef" in the basic schema actually | ||
| referrs to the "extension" schema's root schema. |
There was a problem hiding this comment.
I'd like to see this paragraph broken up a little bit to make it easier to read.
|
@Relequestual yeah, I don't really like the basic/extension either, couldn't come up with anything better and just wanted it posted. I'll think through some alternatives (and work on the rest of the feedback-thanks!). I don't follow the logic of "extendedBasic" so I'll just go back to the drawing board entirely. Maybe also talk about the relationship between what each of them validates in terms of instances more, rather than relying on the naming to convey behavior. |
|
Maybe just use |
jsonschema-core.xml
Outdated
| <postamble> | ||
| Now lets consider the evaluation of the "extension" schema. | ||
| Note that the "$ref": "basic" was not changed, as it works | ||
| just fine as a normals static reference. And the |
|
|
||
| <section title="Guarding Against Inifinite Recursion"> | ||
| <t> | ||
| A schema MUST NOT be run into an infinite loop against an instance. For |
There was a problem hiding this comment.
A schema MUST NOT be run into an infinite loop against an instance.
This doesn't really make sense. The explanation afterward does, but I don't get this sentence.
There was a problem hiding this comment.
I believe I just copy-pasted that from wherever it was before, so if we want to consider reworking it please file a new issue on it. I'm fine with discussing it, it's just not changed here so I don't want to add to the PR.
| <section title="Guarding Against Inifinite Recursion"> | ||
| <t> | ||
| A schema MUST NOT be run into an infinite loop against an instance. For | ||
| example, if two schemas "#alice" and "#bob" both have an "allOf" property |
There was a problem hiding this comment.
Can we remove the allOf usage from this example now that $ref can be used alongside other kewords?
There was a problem hiding this comment.
I think it's still valid as written, so as with the previous comment if you want to figure out a better way to do this whole paragraph let's discuss in an issue or separate PR.
These keywords allow for recursive extension of schemas, in the way needed by meta-schemas. This is not Object-Oriented extension, rather it is a shortcut for the kind of "allOf" combination that is already allowed, but without the need to redeclare all recursive keywords in the extension. It works by both the original target of the reference and the dynamically chosen target of the reference opting in to the dynamic behavior with "$recursiveAnchor", and the source of the reference opting in by using "$recursiveRef". This ensures that regular "$ref" usage is unchanged and and predictable, and also that schemas not intended for recursive extension cannot be extended without being changed.
Hopefully this version of the description and example are easier to follow. Also replaced "reffered" and "referring" with "referenced" and "referencing", which seems to read a lot more smoothly.
handrews
force-pushed
the
rec-ref-anchor
branch
from
1301082
to
062b037
Compare
|
@Relequestual @gregsdennis I have finally gotten back to this and added a second commit based on the feedback here. I more or less completely rewrote the description and example and I think the new version is much more straightforward. Where the text was still the same, I applied the typo / wording corrections. I also replaced "referred" and "referring" with "referenced" and "referencing" as that feels much more straightforward- I'd even previously changed a paragraph to use the parenthetically, so just being consistent about it seems better. As noted, the "Guarding against..." section was only moved by this PR, not changed, so I have not changed it based on the feedback here. Please open an issue or submit a PR if you want to pursue that. |
|
This looks great. To be honest I did have a hard time understand what the use case was for this based on just reading the description for this PR and the contents of the changes as they are now. It all made sense once I read #558 and saw that hyperschema was having to copy and paste stuff, but I am not sure how that, or anything else that made it more clear, could go into the PR. It's probably fine, ship it! 💃 |
|
@Relequestual told me that it was fine to have @philsturgeon review this as he is on vacation and/or busy for a while. So I will go ahead and merge this even though he has not lifted the "requested changes" flag. Of course, we can keep working on the wording here, and probably will once we get to the feature-complete full spec review. |
|
Yup that's fine =] |
NOTE: I'm breaking this up into three parts to keep each one a bit more focused. The first two are on the core spec file, but do not overlap in line numbers or even sections. There are some references between them but figuring them out should be straightforward. I'm not entirely happy with this write up (particularly the 2nd part), but I have been struggling with this for at least two months and just need to get something out there to unblock progress. Hopefully we can edit this into something workable.
Addresses #558
These keywords allow for recursive extension of schemas,
in the way needed by meta-schemas. This is not Object-Oriented
extension, rather it is a shortcut for the kind of "allOf"
combination that is already allowed, but without the need to
redeclare all recursive keywords in the extension.
It works by both the original target of the reference and the
dynamically chosen target of the reference opting in to the
dynamic behavior with "$recursiveAnchor", and the source of
the reference opting in by using "$recursiveRef".
This ensures that regular "$ref" usage is unchanged and
and predictable, and also that schemas not intended for
recursive extension cannot be extended without being changed.