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

For discussion only; support JSON Schema Draft 6 in 3.1 #1766

Closed
wants to merge 2 commits into from

Conversation

MikeRalphson
Copy link
Member

I think this is all that would need to be done to get us off "the draft that never was". Draft 6 being expired should not be a problem as Draft 5 is expired, so is Draft 7 and Draft 8 isn't out yet. I think it can be done in a semver-minor compatible way.

@handrews
Copy link
Member

@MikeRalphson this looks good at a glance. The exclusive* keywords were the only outright incompatibilities since you both were and still are ignoring id/$id.

One question: Why not go to draft-07? It adds several useful things and is strictly compatible with draft-06. At most you might want to check if we have any slight incompatibilities for readOnly and writeOnly which were moved/added to the validation spec in that draft.

draft-07 would also allow using content* keywords for binary data encoded in strings, or alternate media type documents stored as JSON strings, which could be used alongside the existing options for that for compatibility purposes.

@MikeRalphson
Copy link
Member Author

Isn't 7 the draft which gets closer to Turing completeness?

versions/3.1.0.md Outdated Show resolved Hide resolved
- `properties` - Property definitions MUST be a [Schema Object](#schemaObject) and not a standard JSON Schema (inline or referenced).
- `additionalProperties` - Value can be boolean or object. Inline or referenced schema MUST be of a [Schema Object](#schemaObject) and not a standard JSON Schema. Consistent with JSON Schema, `additionalProperties` defaults to `true`.
- `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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably make sure none of the formats added in the newer draft conflict with the ones OpenAPI defines. Putting this here as a reminder to check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you should run a copy of our registry site and we can both cross-reference before adding anything which conflicts?

@handrews
Copy link
Member

@MikeRalphson I don't know if you wanted an actual discussion of this, but I figure it might be of interest so here goes...

TL;DR: if can always be replaced with already-supported keywords, so it doesn't really add new behavior. However, due to increased clarity, it can replace discriminator in a way that is less magical and not specific to OpenAPI.

Isn't 7 the draft which gets closer to Turing completeness?

LOL. In all seriousness, if, then, and else are the only complex and/or controversial additions to core or validation. And I was the last holdout on those.

if and the *Of keywords

There is a really easy solution for implementations, which is that

if: X, then: Y, else: Z

is equivalent to

oneOf: [allOf: [X, Y], allOf: [not: X, Z]]

so if you include a line like "implementations MAY treat if... identically to oneOf..., you've immediately reduced it to keywords you already support. if & friends are really just readability options, they don't add any new capabilities.

This is actually the reason I ultimately supported adding it after keeping it from going into draft 6. There were also some debatable rationales for why it's still declarative and not imperative control-flow, but I never found those all that compelling.

What I did find compelling was the number of people who struggled to write exactly that kind of oneOf condition, or do anything with it. And showed up to complain.

if and discriminator

That sort of oneOf that drove complaints is basically the problem you solved with discriminator.

So this:

MyResponseType:
  oneOf:
  - $ref: '#/components/schemas/Cat'
  - $ref: '#/components/schemas/Dog'
  - $ref: '#/components/schemas/Lizard'
  discriminator:
    propertyName: petType

could be written as this:

MyResponseType:
  oneOf:
  - if: { properties: { petType: { const: Cat } } }
    then: { $ref: '#/components/schemas/Cat' }
  - if: { properties: { petType: { const: Dog } } }
    then: { $ref: '#/components/schemas/Dog'
  - if: { properties: { petType: { const: Lizard } } }
    then: { $ref: '#/components/schemas/Lizard'

While it's a bit more verbose, any draft-07 JSON Schema implementation can handle it. It's also a regular enough pattern (oneOf where all child schemas are just if/then, and the if is for a single const value of the same property) that it would not be that hard to detect it and present it specially in documentation, or generate a factory class in code, etc. And it doesn't rely on an exact match of values and component key names, which can be a maintenance challenge.

You may ask why we didn't just add a switch keyword. It's because the argument over fall-through behavior and default cases was incredibly confusing, with no consensus. And even if switch can technically be reduced to *Of and not, the general case is extremely complex.

Plus, for the most common no-fallthrough, no-default use case, it actually ended up being the same number of lines of schema 😁

I did make an effort to fit discriminator into JSON Schema's processing model around the same time, but it's not possible (if anyone wants the details on that, LMK).

Models are defined using the [Schema Object](#schemaObject), which is an extended subset of JSON Schema Specification Wright Draft 00.
Primitive data types in the OAS are based on the types supported by the [JSON Schema Specification Wright Draft 01](https://tools.ietf.org/html/draft-wright-json-schema-01#section-4.2).
`null` is only supported as a type modifier (see [`nullable`](#schemaNullable) for an alternative and equivalent method of achieving nullable types).
Models are defined using the [Schema Object](#schemaObject), which is a proper superset of JSON Schema Specification Wright Draft 01.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is proper meant in this case? If it's only there to differentiate from the current improper (subset/superset/sideset) situation, maybe we could drop it, or find another word which explains that a bit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, will peruse the thesaurus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intention was this specific meaning: https://mathinsight.org/definition/proper_superset

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@philsturgeon
Copy link
Contributor

This is excellent. AFAIK the only reason draft 6 was not seem as an acceptable move for OpenAPI v3.1, is that folks currently validating their OpenAPI with JSON Schema Draft 4 validators will no longer be able to? That is me roughly quoting from @webron from a TSC call many months ago, so please correct me if I'm wrong Ron.

That scenario was a bit odd to me, because most folks cannot use a straight up Draft 4 parser anyway, and if they are they're very very lucky that they didn't bump into nullable or any of the other discrepancies.

#1736 will be great, and give folks not just a way to use JSON Schema proper (any draft that the tooling supports) along with other schema systems entirely (any that the tooling supports) but this would be a strong win right off the bat without pushing such a large set of variables onto the tooling vendors.

Most validators out there worth their salt support Draft 6, and Draft 6 is a lot better than Draft 4, and the ability to wipe out these discrepancies will stop folks needing to worry about using workarounds like https://github.com/wework/json-schema-to-openapi-schema and Speccy's Resolve Command, etc.

If OpenAPI folks want to jump up to draft 7 that'd be great too, but if draft 6 is used and we can shrink the discrepancies list like this PR does, I certainly won't be sad.

@MikeRalphson
Copy link
Member Author

Closing in favour of #1977 - thanks all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants