[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.
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.
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.
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.
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.
Is 'Extensions' defined anywhere? Should the following be part of the allOf array?
There was a problem hiding this comment.
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.
Should $ref be a required property? Otherwise I get false matches of both halves of a oneOf.
| type: object | ||
| patternProperties: | ||
| '^[a-zA-Z0-9\.\-_]+$': | ||
| oneOf: |
There was a problem hiding this comment.
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.
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.
@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.
I think this requires an additionalProperties: false otherwise responses do match this sub-schema.
There was a problem hiding this comment.
@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: falsewhich 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.
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.
Is the Schema object missing the items property?
| oneOf: | ||
| - $ref: '#/definitions/Schema' | ||
| - $ref: '#/definitions/Reference' | ||
| examples: |
There was a problem hiding this comment.
Similarly, I had to make examples a required property to make the oneOf detection work correctly.
There was a problem hiding this comment.
Can you explain this one?
There was a problem hiding this comment.
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.
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.
Typo wrapperd for wrapped.
| - number | ||
| - object | ||
| - string | ||
| not: |
There was a problem hiding this comment.
Are we also missing definitions for the allOf, oneOf and anyOf arrays?
schemas/v3.0/schema.yaml
Outdated
| default: {} | ||
| $ref: | ||
| type: string | ||
| format: uri-ref |
There was a problem hiding this comment.
Is this uriref in Draft 05? Multiple occurences.
There was a problem hiding this comment.
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.
Is this correct, or is it now a Map as elsewhere?
There was a problem hiding this comment.
This requires a complete rework.
schemas/v3.0/schema.yaml
Outdated
| description: | ||
| type: string | ||
| value: | ||
| type: string |
There was a problem hiding this comment.
Example.value should be an empty schema?
|
@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.
Should this format be uri-ref / uriref not url ?
There was a problem hiding this comment.
No. We changed the namespace definition to be an absolute URI.
There was a problem hiding this comment.
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.
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.
@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-referenceas a format is entirely valid even in draft-05
Any objections?
There was a problem hiding this comment.
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.
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.
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.
Think this should be openIdConnectUrl.
schemas/v3.0/schema.yaml
Outdated
| properties: | ||
| url: | ||
| type: string | ||
| format: uriref |
There was a problem hiding this comment.
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}|\*)?)*\})*$/iThere was a problem hiding this comment.
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.
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.
Makes sense, will remove.
schemas/v3.0/schema.yaml
Outdated
| $ref: '#/definitions/PathItem' | ||
| patternProperties: | ||
| '^x-': {} | ||
| additionalProperties: false |
There was a problem hiding this comment.
The additionalProperties: false should be removed, as it overrides the other additionalProperties property when converted to JSON. This is the only such occurrence.
|
@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.
Should not it be Callback or Reference:
callbacks | Map[string, Callback Object | Reference Object]
|
@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.
serverVariables.enum should be an array of string. @webron
There was a problem hiding this comment.
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). |
|
🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 |
* 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`
MikeRalphson
left a comment
There was a problem hiding this comment.
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:
contentandschema(and a few other cases).The reasons for creating a new PR instead of suggesting fixes to #1236:
Odd things about this version:
allOfto 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/explodeinside 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.