-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
OpenAPI v3.1 as a JSON Schema "Vocabulary" #1957
Conversation
3b23868
to
f58d2c0
Compare
f58d2c0
to
f9ed32b
Compare
ce1b4b6
to
dde3733
Compare
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.
Lots of great work here, and thank you for bravely being the first person other than me to try to write a vocabulary 😝 This is very helpful in understanding where we should improve the documentation.
The things that are not quite right here are things that are definitely complicated, and are features that we expect the vast majority of JSON Schema users will never have to think about (most people will not design new vocabularies and publish them).
- description - [CommonMark syntax](http://spec.commonmark.org/) MAY be used for rich text representation. | ||
- format - See [Data Type Formats](#dataTypeFormat) for further details. While relying on JSON Schema's defined formats, the OAS offers a few additional predefined formats. | ||
- default - The default value represents what would be assumed by the consumer of the input as the value of the schema if one is not provided. Unlike JSON Schema, the value MUST conform to the defined type for the Schema Object defined at the same level. For example, if `type` is `string`, then `default` can be `"foo"` but cannot be `1`. |
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.
In place of "represents what would be assumed" perhaps "documents the required behavior"? I'm not sure I love either phrase, but this is a weird concept that everyone (myself included) has struggled with. So I don't think it should be removed entirely- ideally folks would get this from the JSON Schema spec, but I would not object to reinforcing it locally for OAS.
The key requirement is that tooling MUST NOT automatically write default values into instances. Although they MAY offer that as an option. This allows applications to note when the default is relied upon, and also avoids a potentially dramatic expansion of the instance document, which could be a problem in constrained environments.
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'm not sure I follow your suggestion here. I know you don't love it, but could you throw a few more words at what you're trying to say and I can turn that into specific words for the spec once I understand the intention?
The original sentence makes more sense to me than "documents the required behavior" because its basically "The default value represents something a user might provide as input for this value" which is one way to talk about a default (in OAS land anyway).
|
||
##### Fixed Fields | ||
Field Name | Type | Description | ||
---|:---:|--- | ||
<a name="schemaNullable"></a>nullable | `boolean` | Allows sending a `null` value for the defined schema. Default value is `false`. | ||
<a name="schemaNullable"></a>nullable | `boolean` | Allows sending a `null` value for the defined schema. Default value is `false`. Deprecated. Move towards using `"type": "null"` & type arrays , e.g: `"type": ["string", "null"]`. |
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.
As noted elsewhere, saying that the default value of "nullable"
is false
is deeply problematic. Technically it makes the empty schema {}
mean "anything except null
", which is a gross violation of JSON Schema principles.
Also, leaving the default of "nullable"
as false
technically means that {"type": ["integer", "null"]}
is equivalent to {"type": ["integer", "null"], "nullable": false}
. By normal JSON Schema validation rules, the results of the keywords are ANDed, so that would forbid null
from validating. The only way to allow a null
in the instance would be to specify "null"
in "type"
and specify "nullable": true
.
I know there are compatibility concerns here for 3.1 vs 4.0, but this is so incredibly broken there is no way to use a regular JSON Schema validator and have it behave the same way that this default value of `"nullable" requires.
Can we at least drop the explicit default value?
- When
"type"
is present and does not contain"null"
, the default value for"nullable"
is redundant - When
"type"
is present and does contain"null"
, the default value for"nullable"
effectively causes the"null"
in"type"
to be ignored in any JSON Schema implementation that conforms to the general processing model - When
"type"
is absent, the default value breaks the fundamental JSON Schema rule that absent keywords MUST NOT cause validation to fail
Instead, can we just allow the same behavior for "nullable"
when it is present but say that it has no default behavior? If it's absent, nothing happens, which would actually make things work properly.
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.
+1 for deprecating nullable
!
Also +1 for changing its default as suggested.
But I don't think we can deprecate it until we allow at least the simple form of type arrays that you proposed in #1389:
type
MUST be a string or a two-element array with unique items- if an array, one of the unique items MUST be
"null"
... or just add support for type arrays, without restricting it to the simplified two-element form. I'm not sure there's really any benefit to that restriction, given that it's already possible to define union types (in a less elegant way) using oneOf
.
If we can make these changes in 3.1, then the proposed JSON Schema vocabulary can assume that change will already be in effect.
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.
@tedepstein "or just add support for type arrays" this PR does that. :)
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.
@handrews yeah I need to write a thing in the spec which says "nullable: true" should be treated the same as a null appearing in the array, instead of leaving it as default false. they're certainly not meant to contradict each other.
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.
The greater issue here is that the default, as in "if omitted", has a constricting behaviour.
Omitting keywords should not cause additional validation constraints to be imposed, according to JSON Schema principals.
schemas/v3.1/openapi-vocabulary.json
Outdated
"https://json-schema.org/draft/2019-WIP/vocab/content": true, | ||
"https://specs.openapis.org/oas/3.1/vocab/openapi": true | ||
}, | ||
"$recursiveAnchor": true, |
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.
This is not doing what you want it to do- the OAS file schema is not a recursive extension of a regular meta-schema. It is a schema for a totally separate document format that embeds extended schemas. Take a look at http://json-schema.org/work-in-progress/WIP-links.json for an example of how this sort of thing should work.
As noted in the comment on the vocabulary meta-schema you need three documents:
- A schema for the vocabulary (which can work as a meta-schema on its own, but is not very useful that way)
- A meta-schema for the OAS Schema Object (essentially the same as the standard JSON Schema
schema
, but with the OAS vocabulary added as the 7th entry in$vocabularies
, and the OAS vocabulary schema as the 7th entry in theallOf
) - A schema for the OAS file format, which references the meta-schema for the OAS Schema Object
The OAS Schema Object meta-schema needs to be a standalone document because of how $recursiveAnchor
and $recursiveRef
work. $recursiveRef
can only target root schemas (entire schema documents) because otherwise it would be incredibly difficult to implement.
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.
Also, if we're moving to the new draft for the OAS meta-schema, I would strongly recommend starting from https://gist.github.com/handrews/6dfebd56ef97328f9e4dc7a47a1e8bc7 which is more concise, although not tested and not necessarily correct.
}, | ||
"additionalProperties": false | ||
}, | ||
"Contact": { |
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.
These definitions are nothing to do with the schema object, which is what this (meta?)-schema file is describing in "properties", should I make another file which is just defs?
309622c
to
2bb6f11
Compare
@MikeRalphson fixed the whitespace! :) |
2bb6f11
to
0967d49
Compare
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.
This is a great improvement! The remaining comments are minor and relate to the somewhat confusing situation of OAS being a format that contains a schema, but not a schema itself. Which definitely makes the distinction between non-schema format, schema, and meta-schema a bit confusing.
I did not review the OAS format schema for correctness at all, I just reviewed the vocabulary-ish stuff. Which is looking good.
schemas/v3.1/meta/schema.json
Outdated
{"$ref": "http://json-schema.org/draft/2019-WIP/meta/meta-data"}, | ||
{"$ref": "http://json-schema.org/draft/2019-WIP/meta/format"}, | ||
{"$ref": "http://json-schema.org/draft/2019-WIP/meta/content"}, | ||
{"$ref": "meta/oas"} |
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.
You want "../meta/oas"
or just "oas"
"properties": { | ||
"nullable": { | ||
"type": "boolean", | ||
"default": false, |
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 still think this breaks things.
schemas/v3.1/meta/oas.json
Outdated
"^x-": { | ||
} | ||
}, | ||
"unevaluatedProperties": false, |
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.
Actually on second thought, I think you don't want this here, but you do want to add it in schemas/v3.1/meta/schema.json
. unevaluatedProperties
can only "see" fields that are either in the same schema object (just like additionalProperties
), and in static (schema authoring time0 or dynamic (evaluation time) subschemas. So if you put it here, then when schemas/v3.1/meta/schema.json
allOf
s it together with the standard vocabularies, this unevaluatedProperties
won't be able to see those other vocabularies' keywords.
TL;DR: Always put off adding unevaluatedProperties
until the last possible place. It effectively seals an object against further extension, which you only really want to do for the OAS Schema Object itself.
} | ||
}, | ||
"unevaluatedProperties": false, | ||
"$defs": { |
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.
While it make sense to have Discriminator
and XML
, which are used in this file, defined here, I'm a little unclear on why you put all of the other definitions here. They are describing other aspects of the OAS format, not parts of the schema, so they don't make sense to me for storing in the vocabulary meta-schema. I'd put them in schemas/v3.1/schema.json
Since OAS is a document format that embeds schemas, the thing that describes the schema format is a meta-schema. The thing that describes the overall format is not a meta-schema, it's just a schema. Schemas have a root schema at the.. um... root of the document, which is not how the root object of OAS behaves.
518dad3
to
2ceaad8
Compare
This one got too confusing because I took on too much at once. Instead of worrying about updating the spec AND updating the schema, let's just talk about the spec. This can be revisited later. |
As a follow-up to the June 13th TSC call, I am working on showing you what OpenAPI v3.1 on "JSON Schema Draft 8" could look like. The WIP JSON Schema draft added a fantastic new feature called vocabularies, which allow for extension languages like OpenAPI to add their own keywords.
Basically, instead of the OpenAPI spec listing out every single similarity and difference in its subset superset sideset confusing situation, OpenAPI just extends the JSON Schema core and valaidation vocabs and adds a few keywords of its own.
This solves the OpenAPI <≠> JSON Schema divergence nicely. The biggest thing here is explaining how exclusiveMinimum and exclusiveMaximum are different from latest JSON Schema, which was done to support BC between v3.0 and v3.1.
The most controversial part of this is my choice to mark things like
nullable: true
andexample
as deprecated. This can certainly be removed, but I think it would be a good idea to start pushing folks towards one way of doing things instead of two.Timeline for removal would essentially be maybe v4.0, or v5.0, or never, but deprecation itself is a good way to move folks in the right direction, and allow tooling to start giving warnings.