-
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
[WIP] Alternative OAS3 JSON Schema #1270
Conversation
👍 |
Glad to see effort going into this. The schema that I automatically generated from the spec is easy to reexport as YAML, which I'll append for comparison. The most apparent difference to me is that you've flattened out some intermediate structures.
|
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.
With the changes implied by these comments I was able to validate a converted v2 petstore definition. It is possible by making change X I have weakened the schema, thus missing problem Y, so I would appreciate your thoughts @webron on each individually.
schemas/v3.0/schema.yaml
Outdated
openapi: | ||
type: string | ||
enum: | ||
- 3.0.0 |
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.
Replaced this with a regex pattern to allow testing of 3.0.0-rc2 compatible documents. This would keep schema churn to a minimum.
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.
Yeah, this should actually change to a pattern that matches anything starting with 3.0 as according to semver, any patch level changes should not affect validation. That said, I'm embarrassed to say I've avoided learning regex as much as I can 😁
schemas/v3.0/schema.yaml
Outdated
type: string | ||
type: | ||
type: string | ||
enum: |
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.
Incorrect indentation of enum:. Yay, yaml FTW! :)
schemas/v3.0/schema.yaml
Outdated
|
||
Responses: | ||
allOf: | ||
- $ref: '#/definitions/Extensions' |
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.
Is 'Extensions' defined anywhere? Should the following be part of the allOf
array?
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.
That was from a previous life of the schema, and I just forgot to remove it. Will do.
schemas/v3.0/schema.yaml
Outdated
Reference: | ||
type: object | ||
properties: | ||
$ref: |
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.
Should $ref
be a required
property? Otherwise I get false matches of both halves of a oneOf
.
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.
Yes, will fix.
type: object | ||
patternProperties: | ||
'^[a-zA-Z0-9\.\-_]+$': | ||
oneOf: |
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.
Both Schema
and Reference
include a $ref
property and Reference
allows additionalProperties
. Either change to anyOf
or disallow additionalProperties
in Reference
objects - which would be a change to the OAS? Multiple examples.
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.
That's an excellent question. I've revisited the spec. I'll either make it anyOf
or remove the $ref
from the Schema
definition and make sure it's always interchangeable with the Reference
in the JSON 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.
@MikeRalphson - not that I recall, but it seems that right now, there's no $ref
property in Schema
and it stayed oneOf
wherever a Schema is called. I think that should be fine. Please let me know what you're thinking - if it's fine, I'll create a definition for SchemaOrReference
and use that across the board instead of repeating the oneOf
everywhere.
schemas/v3.0/schema.yaml
Outdated
not: | ||
type: object | ||
patternProperties: | ||
'^x-': {} |
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 think this requires an additionalProperties: false
otherwise responses
do match this sub-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.
@webron @MikeRalphson why is this not:
block here? Filtering out other stuff, you have
Responses:
type: object
patternProperties:
'^x-': {}
additionalProperties: false
not:
type: object
patternProperties:
'^x-': {}
additionalProperties: false
which is an impossible schema. {"type": "object", "not": {"type": "object"}}
alone makes it impossible. Any keywords that are the same inside and outside of a not
by definition are impossible to satisfy.
Am I misreading something here?
schemas/v3.0/schema.yaml
Outdated
'^x-': {} | ||
additionalProperties: false | ||
|
||
ParameterWithSchemaWithExamples: |
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 seem to be having problems with these - as neither example
or examples
is required in ParameterWithSchemaWithExampleInQuery
or ParameterWithSchemaWithExamplesInQuery
and there is no ParameterWithSchemaWithoutExampleInQuery
type, how is the correct type to match the oneOf
to be determined?
I temporarily changed examples
to be a required property in ParameterWithSchemaWithExamplesIn*
.
|
||
Schema: | ||
type: object | ||
properties: |
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.
Is the Schema
object missing the items
property?
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.
Yeah, will add.
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.
Fixed.
oneOf: | ||
- $ref: '#/definitions/Schema' | ||
- $ref: '#/definitions/Reference' | ||
examples: |
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.
Similarly, I had to make examples
a required property to make the oneOf
detection work correctly.
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.
Can you explain this one?
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.
Sorry, I made this edit after another comment, but it appears earlier in the schema. I believe this is MediaTypeWithExample
vs MediaTypeWithExamples
. A media-type object with neither example
or examples
properties matches both definitions. Suggest making examples
a required property of MediaTypeWithExamples
.
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 can also make things mutually exclusive by forbidding the other property rather than requiring the property that you have:
oneOf:
- properties:
example:
properties:
...
examples:
not: {}
- properties:
example:
not: {}
examples:
patternProperties:
...
schemas/v3.0/schema.yaml
Outdated
attribute: | ||
type: boolean | ||
default: false | ||
wrapperd: |
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.
Typo wrapperd
for wrapped
.
- number | ||
- object | ||
- string | ||
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.
Are we also missing definitions for the allOf
, oneOf
and anyOf
arrays?
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.
Yes, added.
schemas/v3.0/schema.yaml
Outdated
default: {} | ||
$ref: | ||
type: string | ||
format: uri-ref |
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.
Is this uriref
in Draft 05? Multiple occurences.
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.
Yes, that's a miss on my part. I actually looked it up before, and went with the wrong one 😕
schemas/v3.0/schema.yaml
Outdated
- $ref: '#/definitions/Schema' | ||
- $ref: '#/definitions/Reference' | ||
examples: | ||
type: array |
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.
Is this correct, or is it now a Map as elsewhere?
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 requires a complete rework.
schemas/v3.0/schema.yaml
Outdated
description: | ||
type: string | ||
value: | ||
type: string |
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.
Example.value should be an empty 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.
Yup, will fix.
@webron with the changes implied by the above comments, I've now successfully validated 574 converted OpenAPI v2 definitions - with only one minor bug found in my converter (thank you). Does
include |
schemas/v3.0/schema.yaml
Outdated
type: string | ||
namespace: | ||
type: string | ||
format: url |
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.
Should this format be uri-ref
/ uriref
not url
?
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.
No. We changed the namespace
definition to be an absolute URI.
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.
Hi @webron, as far as I can tell, url isn't a format that's in the official specification. It sounds like you're implying here that it's like a uri, but required to be absolute?
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.
uri
is the correct format for an absolute URI.
uriref
(which became uri-reference
in the next draft so that is really preferred) is for relative URI references.
It would be really great to avoid url
as having uri
and url
is horribly confusing, particularly as RFC 3986 goes out of its way to explain that there is no hard line between url
and urn
(e.g. you cannot make the distinction based on the scheme)
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.
@webron @MikeRalphson I'm planning to change this to uri-reference
, even if we are saying that this schema is a draft-wright-*-00 (a.k.a. "draft-05") schema, and that draft used uriref
. I'm doing this because:
- Literally no one has ever implemented a validator for draft-wright-*-00. Every implementation that I'm aware of skipped from draft-04 (draft-zyp-json-schema-04 + draft-fge-json-schema-validation-00) to draft-06 (draft-wright-*-01).
- There is no official meta-schema for draft-05, and there never will be, so there is no programmatic way to inform validators that you are using draft-05
- Format is extensible, so using
uri-reference
as a format is entirely valid even in draft-05
Any objections?
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.
Ideally we could also publish a draft-06 version where uri-reference
would be recognized, but format
validation in general is such a hazy weird optional thing that, really, no one should rely on it unless they are using a specific validator and know it's exact behavior. e.g. some validators try to use a complex regex to validate the email
format, but at least one popular validator just checks to see if there's an "@" character in it somewhere and doesn't bother with anything else.
@MikeRalphson No, I wasn't referring to adding the Right now I'm more focused on the functionality. |
@MikeRalphson Updated with your comments a possible a few additional things I've found. Would appreciate a second look. |
schemas/v3.0/schema.yaml
Outdated
- $ref: '#/definitions/Schema' | ||
- $ref: '#/definitions/Reference' | ||
items: | ||
type: array |
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.
Wondering why array's item type is array
and not an object
(schema or reference). From the OAS3 spec:
items - Value MUST be an object and not an array. Inline or referenced schema MUST be of a Schema Object and not a standard JSON 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.
That's me being too tired. Good catch, will fix shortly.
schemas/v3.0/schema.yaml
Outdated
type: object | ||
required: | ||
- type | ||
- openIdConnect |
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.
Think this should be openIdConnectUrl
.
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.
Indeed.
schemas/v3.0/schema.yaml
Outdated
properties: | ||
url: | ||
type: string | ||
format: uriref |
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 think this should be a uri-template
format, but this doesn't exist in JSON schema Draft 5. If we replace format
with pattern
a suitable regex seems to be (lifted from ajv which quotes https://tools.ietf.org/html/rfc6570)
/^(?:(?:[^\x00-\x20"'<>%\\^`{|}]|%[0-9a-f]{2})|\{[+#.\/;?&=,!@|]?(?:[a-z0-9_]|%[0-9a-f]{2})+(?:\:[1-9][0-9]{0,3}|\*)?(?:,(?:[a-z0-9_]|%[0-9a-f]{2})+(?:\:[1-9][0-9]{0,3}|\*)?)*\})*$/i
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 rather not get into that. I'm not sure what the difference is between uriref
and uri-template
. If it's too permissive, in this case, I'm ok with it. If it's too constrict, we can drop the format altogether.
I find adding complex regex impossible to read and maintain. We could have added a regex to define media types as well and we didn't (we had a similar discussion about it around 2.0's 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.
Understood. I'm seeing that it's too restrictive being a uriref
, not allowing {}
characters. Dropping the format
seems the best option.
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.
Makes sense, will remove.
schemas/v3.0/schema.yaml
Outdated
$ref: '#/definitions/PathItem' | ||
patternProperties: | ||
'^x-': {} | ||
additionalProperties: 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.
The additionalProperties: false
should be removed, as it overrides the other additionalProperties
property when converted to JSON. This is the only such occurrence.
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.
Thanks.
@MikeRalphson thanks for the review - any other comments or thoughts? |
schemas/v3.0/schema.yaml
Outdated
callbacks: | ||
type: object | ||
additionalProperties: | ||
$ref: '#/definitions/Callback' |
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.
Should not it be Callback or Reference:
callbacks | Map[string, Callback Object | Reference Object]
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.
Yup.
@webron I'm seeing a rate of about 0.35% errors over 35,000 converted 'valid' OpenAPI 2.0 documents. All the failures appear to be correctly identified - i.e. things which have changed in the spec that the converter can't process automatically or that the 2.0 validators missed. This schema certainly validates more of the spec than the @timburks / #1236 schema, but I do think you may have made a rod for your own back by multiplying out the combinations of (for example) parameters What I'm trying to say is that maintenance of this schema is going to be somewhat harder going forward (depending on what changes come along in 3.1 and subsequent minor versions) as it has more redundancy and a slightly different mental model to grok rather than modelling only those objects that the spec defines. When (if?) we have a test-suite and a reference parser/validator, some of these issues go away. The schema seems to be performant even on large documents, though the use of That said... add the |
@MikeRalphson Thanks for the feedback. Trust me, I don't like the breakdown as it is now, and I agree it's harder to maintain. The issue is derived from the way JSON Schema combines |
schemas/v3.0/schema.yaml
Outdated
- default | ||
properties: | ||
enum: | ||
type: string |
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.
serverVariables.enum
should be an array
of string
. @webron
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.
Thanks, should be fixed now.
@webron In this schema, |
@hkosova it's by design. A parameter can have either |
@webron |
Thanks for clarifying @handrews - I obviously misremembered the situation. |
Reduce duplications and complexity in v3.0 JSON Schema
Added `id`, `$schema`, `description`
@MikeRalphson, @handrews - I've updated the schema with the missing metadata ( If you give the thumbs up, and we get an official @OAI/tsc vote, I'll add a conversion of the schema to JSON, and merge it (at last). |
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.
Newly added metadata looks good, @webron
🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 |
* adopt @webron's OpenAPI 3.0 schema from OAI/OpenAPI-Specification#1270 permalink: https://github.com/OAI/OpenAPI-Specification/blob/92e15eba1d4591ebfe8c11898c48241e72854381/schemas/v3.0/schema.yaml * use literal `$ref` key in Reference Object schema * `"$ref"` -> `$ref`
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.
LGTM. No regressions found.
Added the converted JSON file and a README with some relevant details. Thanks everyone for making it happen. |
this has been merged into |
@cebe sigh just shows how old this PR was. I'll move the content to |
* adopt @webron's OpenAPI 3.0 schema from OAI/OpenAPI-Specification#1270 permalink: https://github.com/OAI/OpenAPI-Specification/blob/92e15eba1d4591ebfe8c11898c48241e72854381/schemas/v3.0/schema.yaml * add ajv-errors * address error messages for #1808's Swagger 2.0 example clarifies the schema and adds custom error messages for unclear error conditions * address error messages for #1808's OpenAPI 3.0 example * restrict underlying JSON Schema `type` field to simple types only (for #1832) * fix limitation in JSON Pointer conversion helper * add clear `not` error message (for #1489) * add additionalProperties message (for #1394) * add ajv-keywords * use `switch` to intelligently identify inline vs referenced content (for #1853) * use `switch` to XOR `schema` and `content` (for #1853) * use `switch` to pivot security scheme based on type (for #1672) * use switch to fall-through to inline security scheme validation (for #1672) * rewrite more Reference oneOfs (for #1519) * add custom message for `Schema.required` type error (for #1519) * rewrite Response/Reference oneOf (for #1489) * use switch in ParameterLocation validation (for #1797) * define pivot key switches for SecurityDefinitions (for #1711) * give helpful `format: uri` messages for SecurityDefinitions (for #1711) * eliminate NonBodyParameter; pivot on `Parameter.in` with a switch (for #1511) * oneOf -> switch for Parameters.items reference * (for #1711) * remove redundant semantic validator (for #1511) * adjust wording of custom error message (for #1853) * add regression tests for all related issues * revert to expect@^1.20.2 * linter fixes * fix messaging flaw for #1832 * improve messaging for #1394 * use literal key for `$ref` in Reference Object * remove commented legacy data from OAS3 schema * remove superfluous quotation marks * normalize test case paths to `/` * normalize openapi fields to 3.0.0 * drop unused `paths` information * ensure clear errors for 3.0 Parameter style/content exclusivity * add `required` assertions to switch statements that pivot on a key's value this prevents false positives when the pivot key is missing entirely * remove stray space
hehe, I thought this was part of your workflow :) |
…ger-api#1984) * adopt @webron's OpenAPI 3.0 schema from OAI/OpenAPI-Specification#1270 permalink: https://github.com/OAI/OpenAPI-Specification/blob/92e15eba1d4591ebfe8c11898c48241e72854381/schemas/v3.0/schema.yaml * use literal `$ref` key in Reference Object schema * `"$ref"` -> `$ref`
…i#1985) * adopt @webron's OpenAPI 3.0 schema from OAI/OpenAPI-Specification#1270 permalink: https://github.com/OAI/OpenAPI-Specification/blob/92e15eba1d4591ebfe8c11898c48241e72854381/schemas/v3.0/schema.yaml * add ajv-errors * address error messages for swagger-api#1808's Swagger 2.0 example clarifies the schema and adds custom error messages for unclear error conditions * address error messages for swagger-api#1808's OpenAPI 3.0 example * restrict underlying JSON Schema `type` field to simple types only (for swagger-api#1832) * fix limitation in JSON Pointer conversion helper * add clear `not` error message (for swagger-api#1489) * add additionalProperties message (for swagger-api#1394) * add ajv-keywords * use `switch` to intelligently identify inline vs referenced content (for swagger-api#1853) * use `switch` to XOR `schema` and `content` (for swagger-api#1853) * use `switch` to pivot security scheme based on type (for swagger-api#1672) * use switch to fall-through to inline security scheme validation (for swagger-api#1672) * rewrite more Reference oneOfs (for swagger-api#1519) * add custom message for `Schema.required` type error (for swagger-api#1519) * rewrite Response/Reference oneOf (for swagger-api#1489) * use switch in ParameterLocation validation (for swagger-api#1797) * define pivot key switches for SecurityDefinitions (for swagger-api#1711) * give helpful `format: uri` messages for SecurityDefinitions (for swagger-api#1711) * eliminate NonBodyParameter; pivot on `Parameter.in` with a switch (for swagger-api#1511) * oneOf -> switch for Parameters.items reference * (for swagger-api#1711) * remove redundant semantic validator (for swagger-api#1511) * adjust wording of custom error message (for swagger-api#1853) * add regression tests for all related issues * revert to expect@^1.20.2 * linter fixes * fix messaging flaw for swagger-api#1832 * improve messaging for swagger-api#1394 * use literal key for `$ref` in Reference Object * remove commented legacy data from OAS3 schema * remove superfluous quotation marks * normalize test case paths to `/` * normalize openapi fields to 3.0.0 * drop unused `paths` information * ensure clear errors for 3.0 Parameter style/content exclusivity * add `required` assertions to switch statements that pivot on a key's value this prevents false positives when the pivot key is missing entirely * remove stray space
This is a proposal for an alternative JSON Schema provided by #1236.
The work done in #1236 is good but:
content
andschema
(and a few other cases).The reasons for creating a new PR instead of suggesting fixes to #1236:
Odd things about this version:
allOf
to reuse things didn't work for me due toadditionalProperties: false
. If anyone wants to take a crack and minimizing it, by all means... I just wanted to get a working version.What's missing:
style
/explode
inside Request Bodies where it's multipart. That was just too much for me for now.Pinging @darrelmiller as he asked to be pinged.
Pinging @MikeRalphson for assistance in testing - would really appreciate if you can take a crack at it.
I'm sure converting the YAML to JSON is going to be easy enough.