Skip to content

Allow booleans in place of any top-level schema field #2598

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

Closed
wants to merge 1 commit into from

Conversation

MikeRalphson
Copy link
Member

@MikeRalphson MikeRalphson commented Jun 1, 2021

It has been pointed out that we didn't achieve 100% JSON Schema compatibility with the release of OAS v3.1.0. JSON Schema allows spelling the schema {} as true and the schema not: {} as false.

These are currently allowed in subschemas, but not where our schema field indicates an OpenAPI schemaObject (literally of type object as expected by the metaschema for example) is allowed.

This may only be of academic interest, as it is possible to spell the empty schema either as {} as above, or by omitting the schema keyword entirely (where allowed), and the false / not: {} schema has limited utility at the top-level in OAS - but for the sake of spec. and tooling compatibility, here is this PR.

It allows the schema fields to contain booleans in the same way we used to allow schemaObject | referenceObject in v3.0.x

This PR does not touch the metaschema as it is still somewhat in flux. I propose we pick up metaschema changes in the RC / post-release phase as with previous versions.

Edit: an alternative is to add wording similar to that we had in v3.0.x

Alternatively, anywhere a Schema Object can be used, a Reference Object boolean MAY be used in its place.

Signed-off-by: Mike Ralphson [email protected]

@karenetheridge
Copy link
Member

karenetheridge commented Jun 1, 2021

Wouldn't this require updates to the metaschemas as well? specifically

$defs:
  components:
    type: object
    properties:
      schemas:
        type: object
        additionalProperties:
          $dynamicRef: '#meta'

should change type: object to type: [ object, boolean ]

edit: actually no, the schemas themselves are one layer down, under the properties... and at the 'schema' definition, where the 'meta' dynamicAnchor is defined, we already have type: [ object, boolean ] there. So the schema is already correct; it is just the spec wording that is inconsistent with it.

@MikeRalphson
Copy link
Member Author

Wouldn't this require updates to the metaschemas as well?

@karenetheridge I explicitly said...

This PR does not touch the metaschema as it is still somewhat in flux. I propose we pick up metaschema changes in the RC / post-release phase as with previous versions.

... but thank you for pointing out that the v3.1 metaschemas may be out of step with the markdown source-of-truth. Ping @jdesrosiers

@char0n
Copy link
Contributor

char0n commented Jun 2, 2021

Great, thanks for clarification! Would it be possible to explicitly mention in 3.1 that Boolean JSON Schemas are not supported as top level Schema Objects? It's kind of implied because the name of the spec object is Schema Object but, when implementing tooling it gives space for interpretation and I was actually inclined to support Boolean JSON Schemas in 3.1 tooling (as top level Schema Objects) but eventually decided not to go for it.

@MikeRalphson
Copy link
Member Author

I was actually inclined to support Boolean JSON Schemas in 3.1 tooling (as top level Schema Objects) but eventually decided not to go for it.

Thanks also, @char0n for the data-point.

char0n added a commit to swagger-api/apidom that referenced this pull request Jun 2, 2021
@jdesrosiers
Copy link
Contributor

Instead of adding "| boolean" everywhere, I'd suggest just redefining "Schema Object" to include boolean. It allows the spec to only be changed in one place and is more correct (boolean schemas are a JSON Schema concept, not an OpenAPI concept).

I agree with @char0n that it's not explicitly stated in the spec that this value must be an object ("Schema Object" is a name, it's not specifying anything). Therefore, I think it's reasonable to fix this in the 3.1.1 patch rather than 3.2.0. I think it's clarifying an ambiguity, not changing a specified behavior.

@Relequestual
Copy link
Contributor

For the sake of not just adding a +1 but being on record... I agree with everything @jdesrosiers said above.

@MikeRalphson MikeRalphson marked this pull request as draft June 4, 2021 08:31
@MikeRalphson
Copy link
Member Author

Will replace with alternative PR on the v3.1.1-dev branch as discussed.

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.

5 participants