Feature/ref is delegation 2 #585

Relequestual · 2018-05-09T14:50:32Z

#628 Opened in favour of this PR.

Retrying to ~~resolve~~ #523 and ~~resolve~~ #514.
Opened in favour of #580 !

Possibly considered a sidetrack, but I think it's part of #514 :
Redacted previous change of removing comment on infinite loops.

A schema MUST NOT be run into an infinite loop against a schema. For example, if two
schemas "#alice" and "#bob" both have an "allOf" property that refers to the other,
a naive validator might get stuck in an infinite recursive loop trying to validate
the instance.
Schemas SHOULD NOT make use of infinite recursive nesting like this; the behavior is
undefined.

Do we actually want this? Later we say "Validators MUST NOT fall into an infinite loop." which suggests we expect that could be possible based on the schema.

Maybe I just missunderstand the intesnion. Happy to file a new issue if you (collective) find this preferable.

Other properties of an object containing $ref as a key no longer MUST be ignored.

…d using an interface provided by the implementation. May be just a reiteration of the "loading a referenced schema", but I've added a use case.

…he default behaviour, but MAY be provided as an option.

handrews

Lots of great stuff here @Relequestual, and I'm really glad you're getting into the PR work!

My overall thought here is that I'd like to get a really solid description of the new behavior, as if it were the only behavior that ever existed, nailed down, and then worry about how to make sure people who have been following the project understand the changes.

There are other things here where I may be missing the reasons that you have included some things, so if it seems that I've questioned something you feel is essential it may just be that I'm a bit too deep in the existing spec wording.

handrews · 2018-05-14T04:09:02Z

jsonschema-core.xml

-                    The "$ref" keyword is used to reference a schema, and provides the ability to
-                    validate recursive structures through self-reference.
+                    The "$ref" keyword can be used to reference a schema which is to be applied to the
+                    current instance location. "$ref" is an assertion key word, which MUST produce a boolean


$ref is an applicator rather than an assertion. It does not produce its own result, it merely conveys the result of the subschema that it applies (in this case the subschema is by reference rather than a literal subschema).

$ref should return all of the results of the subschema (assertion and annotations).

These were your words buddy...

Delegation means that the target of the $ref is processed against the current instance location, and the "results" (boolean assertion outcome and optionally the collected annotations) of $ref are simply the results of the target schema.

#514 (comment)

The part I'm objecting to is '"$ref" is an assertion keyword', which is not something that I say in that text that you quote. I didn't explicitly classify it, and I'd be fine with not classifying it, but assertion keywords evaluate a condition based on a non-schema value (e.g. "maxLength": 5) which is not what $ref does.

Applicators apply a subschema and return its results (assertion and annotations). $ref applies a subschema by reference rather than directly, so I wasn't particularly trying to fit it into the existing classifications, but if we are doing so then applicator is the correct one (and we might need to tweak the wording defining applicators if it talks specifically about an immediate subschema).

And this change sort of obscures the purpose of $ref. We should probably still say it "provides the ability to validate recursive structures through self-reference".

I'll chang this and mention recursive structures again.

handrews · 2018-05-14T04:14:23Z

jsonschema-core.xml

                </t>
                <t>
-                    The URI is not a network locator, only an identifier. A schema need not be


This is still true. It relates to section 8.3.1 about loading schema references. You may have something like "$ref": "https://example.com/schemas/foo", but that host may be down, or you may be running behind a firewall. In which case you probably have the schema available locally and SHOULD NOT automatically fetch it.

We do need some improved wording about when it's OK to automatically fetch $ref'd schemas, as some environments will depend on such dynamic fetching. But that was true before this change so I'd suggest we work on that separately.

handrews · 2018-05-14T04:17:27Z

jsonschema-core.xml

@@ -814,7 +816,30 @@
                    Schemas SHOULD NOT make use of infinite recursive nesting like this; the behavior is
                    undefined.
                </t>
-                <section title="Loading a referenced schema">
+                <t>
+                    A URI reference without a protocol MUST be considered a plain name fragment,


I don't understand this part. There is nothing at all special about URI references in "$ref" values. Everything that needs to be said about resolving $ref and $id is already said in this document, and none of it changes as the result of the whole inclusion/delegation change.

I think you have both hit the nail on the head, and missed the point.

The reason for this section change, is trying to highlight specific aspects of how URI work, as the above removed lines (ending 807) clearly wern't suffecient to explain. There have been so many Stack Overflow questions on how resolution works that I've lost count.

These changes don't CHANGE any of the behaviour, but seek to clairfy it with a little structure. Everything that NEEDS to be said technically yes, but not everything that needs to be said to make the technical aspect clear enough in my experinece.

Even when I've pointed people to the spec section, it wasn't enough. So I tried to write stuff which was a simple form of discussions which resulted in people understanding the intent.

handrews · 2018-05-14T04:34:31Z

jsonschema-core.xml

+                    the user to specify a method to perform a network operation (or other operation) to retrieve the reference
+                    content. For example, the behaviour of a URI with a protocol of "file" is not defined. The implementation
+                    provides an interface to define a function which is called when it encounters a URI which uses the specified
+                    URI protocol (which is "file" in this case). The user defines "file" as the protocol, and includes a function


This whole concept (the regular paragraph on lines 822-825 and this CREF) are implementation notes of the sort that we have not had before. Whether we need them or not, I don't think that delegation vs inclusion makes them more or less necessary.

If we had already gotten agreement in an issue that we need this sort of change, and if the main point of this PR (delegation vs inclusion) was otherwise clear, I'd have no problem doing this in one PR. But I'm not sold on the need for this section, so I'd like to get the agreed-upon changes clear and committed first before opening a new topic on what sort of implementation guidance does or does not need to be added.

I thought a better explanation of how things work was in scope under #514 ? "What is "$ref" and how does it work?" is pretty broad. I may want to write additional comments to this part before awaiting a reply =]

The issue title was pretty broad as I was trying not to prejudice the outcome. It's not that I'm against clarifying, it's that this level of implementation detail doesn't seem quite right to me. Perhaps I'm just not understanding it the way you intended.

handrews · 2018-05-14T05:13:00Z

jsonschema-core.xml

@@ -903,6 +928,24 @@
                    </t>
                </section>
            </section>
+            <section title="Dereferencing By Inclusion">


As I mentioned in the previous PR (I think, or maybe on Slack), I think that if this is just to explain the difference from prior behavior, it belongs in an appendix (which would be xref'd from a brief note in the main text). This is what I've been doing for all confusing changes, like the dependencies split, $defs vs definitions, and keywords that moved from validation to core.

If we do want to provide guidance on an inclusion mechanism for things like json-schema-ref-parser that want to do so to the extent it is possible, then it should describe the use with allOf per #523.

But I don't think there should be a dereference inclusion process as part of the standard. Just a clear description of the behavior, and tools that want to make schema transformations that preserve that behavior can do so (for instance, I know of at least three tools for collapsing allOf branches into a single schema, which is not part of the spec but follows directly from the described behavior).

Here I was trying to address the issues raised by making this change (ref is delegation) as per #523.

I make no reference to prior behaviour, so it's not explaining a difference.

If we do want to provide guidance on an inclusion mechanism for things like json-schema-ref-parser that want to do so to the extent it is possible, then it should describe the use with allOf per #523.

I thought we wanted to avoid specific implementation directives where possible (like saying "you should do this by using allOf)? More, this section puts the nessecary constraints on any process, which may need to resolve by inclusion, as to avoid any interoperability issues.

I think this is REQUIRED. If we don't define constraints around how to implement inclusion, I feel it will be done any number of invalid ways and cause problems.

handrews · 2018-05-14T05:15:23Z

jsonschema-core.xml

                </t>
+                <cref anchor="REF1" source="BH">
+                    The use of "$ref" MUST NOT effect adjacent keywords.
+                    Given previously the use of "$ref" would negate the use of other keywords in that object,


I think that most of this can be made clear by thoroughly describing the applicator behavior. Otherwise I would put this in the appendix for people who want to understand the changes, as noted in another comment.

I guess we should discuss this as it looks to me like your opinion on applicator or assertation for $ref has changed since the issue was raised.

I'm not aware of my opinion changing although I certainly accept that I may have been unclear.

handrews · 2018-05-24T23:02:06Z

@Relequestual note that #589 reorganizes where the $ref section fits (it's now a subsection of "Schema Referencing") and makes some very small tweaks to its wording. More importantly, the new section intro for "Schema Referencing" explains the applicator nature of $ref and $recurse.

I tried to avoid impacting the areas you're working on here as much as possible, but I needed this changes for $recurse to make sense.

Relequestual · 2018-06-20T15:05:23Z

@handrews The issue you mentioned has since been closed with a hope to revisit later. I'll continue with this one.

handrews · 2018-06-20T18:54:41Z

@Relequestual yes, don't worry about the specifics of #589. The general idea of static vs dynamic references as two forms of in-place applicators will still be present in the next approach, but I can worry about merging the specifics when I get there.

…and not an assertion key word. Removed sections which explain implementation detail or use case. Slightly modified language around network addressable locators. Mention recursive structures.

handrews · 2018-06-22T22:30:38Z

After discussing this with @Relequestual off-github, we decided to split this into three PRs/Issues:

One to address just the basics of the "this works as delegation, and as a consequence adjacent keywords are now allowed" topic (What is "$ref" and how does it work? #514/Keywords alongside of $ref #523), which is pretty close to settled- I've just asked @Relequestual to review the latest PRs I have on applicators to make sure the language aligns.
One to address exactly what to say about implementations performing some sort of inclusion, and where to say it. This is part of What is "$ref" and how does it work? #514 (it's in @Relequestual's action item list in the last comment), but the PR made us realize that we had differing impressions of what this meant. This will probably be an issue first as part of the differing views involves where to put it in the spec, and whether certain parts belong in the spec or on the web site.
One to address any clarifications on the process of resolving $ref against $id, which is not significantly impacted by What is "$ref" and how does it work? #514/Keywords alongside of $ref #523. @Relequestual I could see this as going back to an issue or just becoming a new PR. Since you have noted that there are a lot of Stack Oveflow questions motivating this work, I would find an issue linking to some typical SO questions to be very helpful in understanding what you're trying to accomplish. I think my confusion over some of your points is due to lacking this context.

philsturgeon · 2018-06-25T18:46:25Z

How's this one coming along @Relequestual?

Relequestual · 2018-06-27T14:31:13Z

@philsturgeon FYI, those type of comments on github most likely won't reach me due to volume of notifications. Consider Slack as a preference for nudges =]

Relequestual · 2018-06-27T14:31:57Z

I was on leave for the last two days, and the remainder of this week is split between this and another project, so I strongly expect this to be delivered again this week.

Relequestual · 2018-06-28T12:53:16Z

Opened #628 in favour

Relequestual · 2018-06-28T15:26:10Z

@handrews I believe this can now be closed, as I've opened a new PR in favour, and created two new issues, as specified by your previous comment. Please close if you concurr.

I'm happy for you to delete the branch, as I believe I'll still retain it on my fork. (And at the least, I'll have a local copy.)

handrews · 2018-06-29T00:21:26Z

@Relequestual thanks for all the work revising and splitting this! I agree we can close this.

Relequestual added 3 commits May 4, 2018 13:46

$ref is applicable to the current instance.

3318210

Other properties of an object containing $ref as a key no longer MUST be ignored.

Added information about dereferencing URIs where a protocol is define…

cf2aea0

…d using an interface provided by the implementation. May be just a reiteration of the "loading a referenced schema", but I've added a use case.

Defined behaviour for dereferencing by inclusion, which MUST NOT be t…

649dba9

…he default behaviour, but MAY be provided as an option.

Relequestual requested review from awwright and handrews May 9, 2018 14:50

Relequestual mentioned this pull request May 9, 2018

Feature/ref is delegation #580

Closed

handrews suggested changes May 14, 2018

View reviewed changes

handrews mentioned this pull request May 24, 2018

Add "$recurse" for extending recursive schemas #589

Closed

This was referenced Jun 15, 2018

Example of $recursiveRoot and $recursiveRef #605

Closed

Keywords alongside of $ref #523

Closed

Relequestual and others added 2 commits June 21, 2018 10:42

Changed language to make clear that $ref is an applicator key word …

1bb0f5a

…and not an assertion key word. Removed sections which explain implementation detail or use case. Slightly modified language around network addressable locators. Mention recursive structures.

Fix misplaced <t> elements

97c3c29

This was referenced Jun 28, 2018

Ref is delegation - try 3 #628

Merged

Quantify issues people face around resolving $ref against $id #630

Closed

Should we define how Inclusion (transclusion) should be implemented given $ref is delegation? #629

Closed

handrews closed this Jun 29, 2018

Relequestual deleted the feature/ref-is-delegation-2 branch December 18, 2018 09:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ref is delegation 2 #585

Feature/ref is delegation 2 #585

Relequestual commented May 9, 2018 •

edited

Loading

handrews left a comment

handrews May 14, 2018

Relequestual May 18, 2018 •

edited

Loading

handrews May 18, 2018

awwright May 19, 2018

Relequestual Jun 21, 2018

handrews May 14, 2018

handrews May 14, 2018

Relequestual Jun 21, 2018

handrews May 14, 2018

Relequestual May 18, 2018

handrews May 18, 2018

handrews May 14, 2018

Relequestual Jun 21, 2018

handrews May 14, 2018

Relequestual May 18, 2018

handrews May 18, 2018

handrews commented May 24, 2018

Relequestual commented Jun 20, 2018

handrews commented Jun 20, 2018

handrews commented Jun 22, 2018 •

edited by Relequestual

Loading

philsturgeon commented Jun 25, 2018

Relequestual commented Jun 27, 2018

Relequestual commented Jun 27, 2018

Relequestual commented Jun 28, 2018

Relequestual commented Jun 28, 2018

handrews commented Jun 29, 2018

Feature/ref is delegation 2 #585

Feature/ref is delegation 2 #585

Conversation

Relequestual commented May 9, 2018 • edited Loading

#628 Opened in favour of this PR.

handrews left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Relequestual May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

handrews commented May 24, 2018

Relequestual commented Jun 20, 2018

handrews commented Jun 20, 2018

handrews commented Jun 22, 2018 • edited by Relequestual Loading

philsturgeon commented Jun 25, 2018

Relequestual commented Jun 27, 2018

Relequestual commented Jun 27, 2018

Relequestual commented Jun 28, 2018

Relequestual commented Jun 28, 2018

handrews commented Jun 29, 2018

Relequestual commented May 9, 2018 •

edited

Loading

Relequestual May 18, 2018 •

edited

Loading

handrews commented Jun 22, 2018 •

edited by Relequestual

Loading