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

Review draft-08/core/9.4 - $defs: Clarify its a recommendation #778

Closed
about-code opened this issue Aug 10, 2019 · 9 comments · Fixed by #783
Closed

Review draft-08/core/9.4 - $defs: Clarify its a recommendation #778

about-code opened this issue Aug 10, 2019 · 9 comments · Fixed by #783
Labels
clarification Items that need to be clarified in the specification

Comments

@about-code
Copy link
Contributor

In requests for reviewing draft-08 I would like to comment on

Core-Spec, Section 9.4. Schema Re-Use With "$defs"

The "$defs" keyword provides a standardized location for schema authors to inline re-usable JSON Schemas into a more general schema. [...]

In the original issue #512 @handrews stated that $defs is actually more of a recommendation while definitions and basically any other arbitrary keys (see #512 (comment)) should continue to work, too.

Reading previous versions of the specification as well as draft-08, though, it is easy to read it as if $defs (and formerly definitions) were the only possible spec-conformant place where to put subschemas. It is not very clear that it has been a recommendation. I guess the fine distinction to catch is the use of the phrase a standardized location rather than the standardized location and an absence of MUST. However, standardized sounds more reliable than it obviously is and doesn't indicate a recommendation at all.

I think the spec would profit from using the wording the recommended location or from becoming even more clear with something like

It MUST NOT be assumed that $defs is the only permitted location for subschemas. This is, because schemas and subschemas are identified by an URI which itself is agnostic to the actual keyword used to locate subschemas (see 8.3.2 Dereferencing). Schema authors SHOULD consider using $defs over any alternative for conventional purposes, though.

@about-code about-code changed the title Review draft-08/core/9.4 - Keyword does not directly affect validation result? Review draft-08/core/9.4 - $defs: Clarify its a recommendation Aug 10, 2019
@handrews handrews added this to the draft-08 milestone Aug 10, 2019
@handrews
Copy link
Contributor

@about-code does section 9.3.4 "References to Possible Non-Schemas" clarify this at all? It discusses things that could happen when putting re-usable subschemas under unexpected keywords.

Perhaps a reference to that in 9.4 would help?

@stain
Copy link

stain commented Aug 15, 2019

In particular, a (sub)schema URI might be used as a format/conformance identifier within other representations.

@handrews
Copy link
Contributor

@stain I'm not sure I follow how that relates to this issue, could you elaborate? I agree that using a subschema's URI as an identifier outside of the schema is valid, I'm just not sure what that has to do with the recommended vs required nature of $defs?

@about-code did you see my question above about the "References to Possible Non-Schemas" section?

@Relequestual
Copy link
Member

@handrews That's a great section you linked to.
Could we add a specific requirement to this, like...

"subschemas SHOULD NOT be the value of unknown keywords and they SHOULD NOT be treated as a subschema, or be the target of a reference."
or / and
"references to objects which appear to be subschemas as the value of unknown keywords MAY result in an error being thrown"

I want to suggest to implementers that they may want to watch out for this and SHOULD thrown an error, in effort to guide schema authors away from this practice.

@handrews
Copy link
Contributor

@Relequestual we want people to create new applicator keywords in their vocabularies if they need them, so we don't actually want to discourage creating keywords with subschema values.

"Unknown" really means "unknown to your implementation". There's nothing really unusual going on here- if your implementation doesn't understand a keyword, it's not going to work. This whole section just emphasizes that this is just as true for what I think of as "placeholder" keywords ($defs, definitions, $comment) as it is for assertions, annotations, and applicators. While placeholder keywords don't do anything in the same way that the other keyword classes do, placeholders that hold subschemas do inform the implementation that it should look at those subschemas as subschemas.

@garethsb
Copy link
Contributor

FWIW, I agree with the OP on a minor point, that it's worth being careful with the use of standard and standardized in the specification. Especially in industries that rely on Standards Organizations, the "level of force" intended by standardized could be unclear. An RFC 2119 term like RECOMMENDED would be better if appropriate.

@Relequestual
Copy link
Member

@garethsb-sony Thanks for the concern.
In the updated PR, the word "standard" is used, but it is not an RFC 2119 word or phrase, nor is it in all caps. In this case it simply means regular. Could do /standard/regular/, but if you fail to read the spec with the lense of RFC2119, then there may be many points of confusion. That's the whole reason for using RFC 2119 in the first place.

@garethsb
Copy link
Contributor

OK, I do agree, using RFC 2119 terms if and only if 'force' is intended ought to be enough.

Avoiding using RFC 2119 terms in lowercase (e.g. does 'should not' in https://github.com/json-schema-org/json-schema-spec/blob/master/jsonschema-core.xml#L327 have a different force than 'SHOULD' in the previous sentence and if so, why?), and similarly forceful sounding words, will help some readers, but it can often make the language tortuous!

@Relequestual
Copy link
Member

This is defind at the top of the document, as per the majority of RFC documents.

That example you've picked specifically, maybe it could do with some tidying.
Please file a new issue for this.
There's a lot in flux, and there will be another review period to allow multiple core team members to do a full reading.

@gregsdennis gregsdennis added clarification Items that need to be clarified in the specification and removed Type: Maintenance labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification Items that need to be clarified in the specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants