-
-
Notifications
You must be signed in to change notification settings - Fork 313
$ref across schema versions clarification #845
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
Conversation
…d differently MAY be supported, or an error SHOULD be thrown Fixes json-schema-org#808
jsonschema-core.xml
Outdated
@@ -1767,6 +1767,10 @@ | |||
URI to identify more than one schema. When multiple schemas try to identify | |||
as the same URI, validators SHOULD raise an error condition. | |||
</t> | |||
<t> | |||
As with all schemas, a referenced schema SHOULD be processed according to its define "$schema" |
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 feel like this ought (OUGHT?) to be MUST, except that I don't think we ever actually say that $schema
MUST be respected at all? But we do say that $vocabulary
MUST be respected at least to the point of the implementation failing in the face of required but unsupported vocabularies. So that sort-of implies a MUST for respecting $schema
. Maybe?
Looking at this now, it occurs to me that how a schema is loaded and processed is a very fundamental thing that deserves more emphasis than we are giving it. Loading a referenced schema is an important use case, but generally loading schemas is the same whether you found the schema directly, through $ref
, or through hypermedia. As @awwright noted in #808, $ref
is not really a special case for this.
I think we should have a section about loading schemas that covers the general load-and-process model, and notes any specifics needed for $ref
and for hypermedia. My initial thought was to put this early in the spec (general considerations?) but you really need to understand the core vocabulary to do all of the steps.
So I say we should add a new top-level section between section 8 (Core Vocabulary) and section 9 (Applicator Vocabulary) on schema loading. It should include most or all of:
8.2.4.5 Loading a referenced schema (but just Loading a schema)
8.2.4.6 Dereferencing (maybe tweak to cover other ways you get a schema other than just $ref
)
8.2.4.3 Guarding against infinite recursion
8.2.4.4 References to possible non-schemas
8.1.4 Detecting a meta-schema
11 Usage in hypermedia (demote to subsection?)
While this sounds like a lot, it's mostly just moving stuff around.
I've gotten several questions that basically boil down to "How do I load schemas?" Consolidating this information would make an essential process much more clear and emphasize its importance. It would also trim the extremely long Core Vocabulary section and avoid some of the really deep nesting that causes key sections to not show up in the Table of Contents right now.
Thoughts?
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.
(maybe I should have put all that in the issue, can move if we'd prefer)
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.
Sounds reasonable. As such, want to create a new issue for this?
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.
OK- yeah I'm happy with you merging this after taking into account @gregsdennis 's comment, and I'll file the above so that we can consider it separately.
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'd probably go as far as to say "Implementations MAY throw an error if a $schema is not provided"... but hey =]
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'll hold this as unresolved till I see a related new issue.
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.
@Relequestual no, it's not fair- there is no requirement to provide $schema
. A linter can raise an issue, but an implementation? Definitely not.
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.
JSON Schema implementations should generally be forgiving. If the spec specifically allows something (and saying that $schema
SHOULD be present is more or less equivalent to saying that it MAY be absent) then all implementations should always accept that thing even if we are clearly indicating that doing so is a bad idea.
Things that are bad ideas but neither outright forbidden (requiring an error) nor explicitly have undefined or implementation-defined behavior (allowing an error as that is one possible behavior) need to be allowed. Not warnings, not errors, allowed. A linter can do "warnings" for those things as a separate piece.
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.
Apologies, it wasn't clear enough that suggesting implementers throw an error on lack of $schema
was not a serious suggestion.
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.
@Relequestual ah, I see. I usually read these right after waking up in hopes of catching you and Phil at the end of your days, so subtleties are lost on me.
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.
Good for now. Let's be sure to revisit this as @handrews suggests.
Closing PRs that will get rolled into the overall "how to load and process schemas" work, per discussions with @Relequestual on slack. |
Note that referenced schemas which identify as needing to be processed differently MAY be supported, or an error SHOULD be thrown
Fixes #808