Skip to content
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

Feature/ref is delegation 2 #585

Closed
wants to merge 5 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 41 additions & 10 deletions jsonschema-core.xml
Original file line number Diff line number Diff line change
Expand Up @@ -791,20 +791,21 @@

<section title='Schema References With "$ref"'>
<t>
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 applicator key word, applying the referenced
schema to the instance.
</t>
<t>
An object schema with a "$ref" property MUST be interpreted as a "$ref" reference.
The value of the "$ref" property MUST be a URI Reference.
By being an applicator key word, "$ref" allows the posibility to externalise or
segment a schema across multiple files, and provides the ability to validate recursive structures
through self-reference.
</t>
<t>
The value of the "$ref" property MUST be a string URI Reference.
Resolved against the current URI base, it identifies the URI of a schema to use.
All other properties in a "$ref" object MUST be ignored.
</t>
<t>
The URI is not a network locator, only an identifier. A schema need not be
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

downloadable from the address if it is a network-addressable URL, and
implementations SHOULD NOT assume they should perform a network operation when they
encounter a network-addressable URI.
A URI may be a locator, a name, or both, per <xref target="RFC3986">RFC 3986</xref>.
</t>
<t>
A schema MUST NOT be run into an infinite loop against a schema. For example, if two
Expand All @@ -814,7 +815,19 @@
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,
Copy link
Contributor

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 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

and the URI reference location resolved according to <xref target="id-keyword">"$id" keyword</xref> section.
</t>
<t>
A URI reference with a network addressable locator defined MAY be provided with an interface to resolve
the reference as a network accessible resource.
</t>
<t>
Any URI may be resolvable by use of externally defined references provided to the implementation as per the
<xref target="loading-references">Loading a referenced schema</xref> section.
</t>
<section title="Loading a referenced schema" anchor="loading-references">
<t>
The use of URIs to identify remote schemas does not necessarily mean anything is downloaded,
but instead JSON Schema implementations SHOULD understand ahead of time which schemas they will be using,
Expand Down Expand Up @@ -903,6 +916,24 @@
</t>
</section>
</section>
<section title="Dereferencing By Inclusion">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<t>
It MUST NOT be expected that any schema can be dereferenced by the means of replacing any object that
uses the "$ref" keyword with the resolved referenced schema (inclusion). An interface MAY be provided
to dereference a schema by means of inclusion, however it MUST NOT be the default behaviour.
</t>
<t>
The use of "$id" and "$ref" from external schemas MUST be evaluate correctly, and not evaluated after
any inclusion process.
</t>
<t>
The result of any inclusion process MUST NOT effect previously adjacent keywords to the original "$ref" keyword
</t>
<t>
A behaviour when a resolved schema which defines a schema version which is different to that of the base JSON Schema document
is not defined.
</t>
</section>

<section title='Schema Re-Use With "$defs"'>
<t>
Expand Down