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 #580

Closed
wants to merge 3 commits into from
Closed

Conversation

Relequestual
Copy link
Member

$ref is now delegation rather than inclusion by default, and if inclusion is required, it must not effect adjacent key words.

Attempt to resolve #523 and #514

I've spent the past two days refreshing my understanding of the current issues around $ref, what exactly a URI is and how they can be used, drawing diagrams of possible ways to resolve and handle $ref... so I figure I've thought this one through.

Previously...

A schema MUST NOT be run into an infinite loop against a schema.
...
Schemas SHOULD NOT make use of infinite recursive nesting like this; the behavior is undefined.

I can't think of any reason this should still be true, given the expected default behaviour should "now" be delegation rather than inclusion. As such, I've removed that section.

(I've done this as just a text document first to get feedback and thoughts as I can't bring myself to look at XML today. I'll update this branch as needed, and finally move the text to the actual XML for final the final PR state.)

@Relequestual Relequestual requested a review from a team April 12, 2018 14:56
@awwright
Copy link
Member

awwright commented Apr 12, 2018

I'm assuming this is section 8.3. Schema References With "$ref" (as of c872253).

I want to make sure I'm understanding this correct, what's the impact of this change? What's an example of something you couldn't do before that you can now?

Some nits:

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 result in a boolean assertion when the resolved schema is applied.

"boolean assertion" and "applied" need to be defined. We use "applied" in some other places but the usage is more colloquial I think.

The use of "$ref" MUST NOT effect adjacent keywords.

This is probably redundant, since keywords already don't affect each other except as explicitly described.

A schema with a "$ref" property where the value of the property is a string MUST be interpreted as a "$ref" reference. The value of the "$ref" property MUST be a URI reference.

Is it the schema that "MUST be interpreted as a $ref reference", or the property value? This seems to conflict with the first sentence from earlier.

According to RFC 3986, a URI may be a locator, a name, or both.

We already normatively reference RFC 3986, and I'm not sure what this helps with.

A URI reference without a protocol MUST be considered a plain name fragment, and the URI reference location resolved according to section 9.2. The "$id" Keyword.

This doesn't make any sense to me. Implementations should only be concerned with the URI that the URI Reference resolves to. In a URI base of http://example.com/, the URI References foo and http://example.com/foo should operate identically.

A URI reference with a network addressable locator defined MUST be provided with an interface to retrieve the network accessible resource.
Any URI may be resolvable by use of externally defined references as per section 9.2.2. External References.

Is changing how we dereference URIs supposed to be part of this issue?

Implementations SHOULD NOT attempt to dereference a schema by replacing any "$ref" keyword and value with a resolved reference schema.

SHOULD NOT implies that the discouraged behavior is still an option, and still legal (with a really good reason).

... and there's probably more but this is enough for now.

@awwright
Copy link
Member

And thanks for taking the time to write specific text, by the way! That's some good progress.

@handrews
Copy link
Contributor

@awwright I'll leave most of the replying to @Relequestual, but to address:

"boolean assertion" and "applied" need to be defined. We use "applied" in some other places but the usage is more colloquial I think.

I think if we just replace "result in a boolean assertion" with "produce a boolean assertion result" then everything works fine, and fits with the definition of assertions in the current text.

For "applied", there is now a "Subschema Application" section from a recent PR, which should cover this. We can certainly tweak the wording there if needed, either as part of this PR or separately.

@Relequestual
Copy link
Member Author

Relequestual commented Apr 17, 2018

I'm assuming this is section 8.3. Schema References With "$ref" (as of c872253).

Yes! 8.3. Thanks for clarifying.

I want to make sure I'm understanding this correct, what's the impact of this change? What's an example of something you couldn't do before that you can now?

The major difference is previously, all other keywords adjacent to $ref MUST be ignored. By making $ref work by delegation, we remove this requirement. This has been asked for more times than I can count. It can be achived by wraping in an allOf, but is really suboptimal and messy.

Some nits:

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 result in a boolean assertion when the resolved schema is applied.

"boolean assertion" and "applied" need to be defined. We use "applied" in some other places but the usage is more colloquial I think.

  • I'll make the adjustments @handrews suggested in the previous comment to this.
The use of "$ref" MUST NOT effect adjacent keywords.

This is probably redundant, since keywords already don't affect each other except as explicitly described.

Because previous it said it All other properties in a "$ref" object MUST be ignored. , I feel it's worth emphasising. Good tests cover the inverse, so a good spec shouldn't leave any room for ambiguity.

A schema with a "$ref" property where the value of the property is a string MUST be interpreted as a "$ref" reference. The value of the "$ref" property MUST be a URI reference.

Is it the schema that "MUST be interpreted as a $ref reference", or the property value? This seems to conflict with the first sentence from earlier.

  • Good spot. I'll reorder this scentence so the subject is correct.
According to RFC 3986, a URI may be a locator, a name, or both.

We already normatively reference RFC 3986, and I'm not sure what this helps with.

I don't see any harm in being really clear about where this is coming from. I personally find it helpful, so I'll leave it here.

A URI reference without a protocol MUST be considered a plain name fragment, and the URI reference location resolved according to section 9.2. The "$id" Keyword.

This doesn't make any sense to me. Implementations should only be concerned with the URI that the URI Reference resolves to. In a URI base of http://example.com/, the URI References foo and http://example.com/foo should operate identically.

I spent a little while familiarising myself with the correct termonology. The plain name fragment is indeed "foo", and section 9.2. shows resolution as you described. What did you understand this to mean?

A URI reference with a network addressable locator defined MUST be provided with an interface to retrieve the network accessible resource.
Any URI may be resolvable by use of externally defined references as per section 9.2.2. External References.

Is changing how we dereference URIs supposed to be part of this issue?

This isn't changing anything as far as I understand. Could you explain why you think this is a change vs emphasis on current working?

Implementations SHOULD NOT attempt to dereference a schema by replacing any "$ref" keyword and value with a resolved reference schema.

SHOULD NOT implies that the discouraged behavior is still an option, and still legal (with a really good reason).

That is correct. I checked RFC 2119. It's clear to me from discussions on slack and more than just a few SO questions and comments, that some people and implementations have fair and valid reasons for wanting to dereference the use of $ref by inclusion, so we shouldn't prohibit it.

@Relequestual
Copy link
Member Author

@awwright Given the previous comment is 3 levels deep, I'd appreciate splitting your reply up into multiple comments please.

@awwright
Copy link
Member

@Relequestual Most of your comments make sense, give me a day to write a proper reply though. Thanks!

@handrews
Copy link
Contributor

handrews commented Apr 20, 2018

@Relequestual

Because previous it said it All other properties in a "$ref" object MUST be ignored., I feel it's worth emphasising

I think it might be a good idea to address this in an appendix, similar to the appendix on keywords that have been moved that I added to the validation spec. This way people reading the new spec will get the correct impression that it's not special, and we can directly address the change in the appendix, with the clear context that only people moving from older drafts need to even read the appendix. It will be dropped well before RFC.

@handrews
Copy link
Contributor

@Relequestual regarding:

A URI reference without a protocol MUST be considered a plain name fragment, and the URI reference location resolved according to section 9.2. The "$id" Keyword.

I think I'm with @awwright on this. We're not changing anything at all about how the value of $ref is handled. We're only changing whether it works with adjacent keywords. We should keep the existing description of $ref values and how to resolve them. It hasn't changed, so if we want to tweak it for other reasons I'd like to separately discuss why. I'm fine with folding in some minor wording improvements, but since you didn't open this PR on the existing wording it's not clear to me that this qualifies.

@handrews
Copy link
Contributor

@Relequestual

A URI reference with a network addressable locator defined MUST be provided with an interface to retrieve the network accessible resource.
Any URI may be resolvable by use of externally defined references as per section 9.2.2. External References.

Again I'm kind of agreeing with @awwright . We seem to all agree that the functionality is not changing, so what is the purpose of the extra wording? What existing wording is it clarifying? This is where it gets hard to give feedback on text divorced from the current file. I'm guessing that if we see this as a direct diff it will be more clear what you're getting at.

@handrews
Copy link
Contributor

Implementations SHOULD NOT attempt to dereference a schema by replacing any "$ref" keyword and value with a resolved reference schema.

@Relequestual it's not clear to me that we want to address this in the spec at all. It's analogous to collapsing allOf branches. It follows from how the keywords are defined that you can often do that and produce a schema with identical effects.

If we do want to address this, then we need to talk about using allOf to wrap the dereferenced schemas. And then things get tricky as there are three cases:

  • $ref with no adjacent keywords (you can replace just as you could now, modulo circular references)
  • $ref with adjacent keywords, none of which are allOf (you can just add a one-element allOf containing the dereferenced schema
  • $ref with an adjacent allOf (you need to add a new allOf branch for the dereference.

None of this is required to understand or implement the spec. I'd also consider an appendix here, although one that would also be part of the RFC. That is a good way to convey interesting usage that is not obvious from the spec, but is not required for implementation.

@Relequestual
Copy link
Member Author

@handrews OK, I think you're seeing a change in behaviour where I'm seeing clarifications, with insight from the URI RFC.

I felt it was important to add these clarifications because they were discussed in the associated issues as needing clarification.

I don't feel we can dump in "ref is delegation" without also addressing the exsisting use cases of inlining references, and how they should be handled, otherwise we're replacing adding ambiguity to the issue.

#514 includes a number of issues around use of $ref, and you (and a number of others) upthumbed my comment about specifying how inclusion should work given this change: #514 (comment)

I spent considerable time reading through the two issues and made notes on key points required to resolve them.

If we think #514 is just too fuzzy, we can try and nail down the requirements to resolve in a new issue, but I didn't think that was required.

If you both feel this PR is far off what you were expecting, I suggest we close it, create a new issue in favour of #514, and define how it should be resolved alongside #523.

/cc @awwright

@Relequestual
Copy link
Member Author

One of the reasons I didn't want to do this as a diff initialy is because #514 is essentially "$ref isn't defined well in the spec, let's fix that and re-explain". I can attempt to solve both issues separatley, but given they would need to be done one after another, I thought it made sense to do both at the same time.

@Relequestual
Copy link
Member Author

Sorry, sorry. Third commit of this PR shows the diff of previous vs my suggested change. Just as text and not as the XML.

@awwright
Copy link
Member

awwright commented Apr 26, 2018

Because previous it said it All other properties in a "$ref" object MUST be ignored., I feel it's worth emphasising. Good tests cover the inverse, so a good spec shouldn't leave any room for ambiguity.

Can we use a CREF?

I don't have anything to add to the other points, at least not until I see the revision, if any.

Are you able to make a new PR that edits the XML sources?

@Relequestual
Copy link
Member Author

@awwright Thanks.
As you say a CREF can be used for "Any draft notes we’d like to make that would be removed before final publication", then this is probably a good solution.

I'll aim to make a new PR based on this one but in the XML within the next week.

@Relequestual
Copy link
Member Author

Closed in favour of #585
Requesting that no one else delete the branch for now please.

@Relequestual
Copy link
Member Author

@awwright @handrews I've made a new PR in favour of this one.
Just in case you missed it =]

@Relequestual Relequestual deleted the feature/ref-is-delegation branch December 18, 2018 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keywords alongside of $ref
3 participants