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

Discriminator updates for v3.2 #2621

Closed

Conversation

jdesrosiers
Copy link
Contributor

These are some changes I propose for discriminator that are minor, but would probably be considered breaking changes for 3.1.

  • The discriminator keyword is an annotation. It doesn't affect validation, so it would not be ideal for it to cause validation failures. This changes the behavior of discriminator from being an error if the propertyName is not a required property in the object to being a no-op. This is also more consistent with other JSON Schema keywords that ignore things that don't apply to them, like properties only applying to instances that are objects.

  • There is no way for implementations to tell the difference between a value that is intended to be a schema name and a value that is expected to be a URI. This changes mapping to allow only URIs and not schema names. Requiring all mappings to use URIs avoids the problem while not losing any functionality. The only consequence is that some mappings would be a little more verbose than they otherwise would have needed to be (example: #/components/schemas/Dog vs Dog).

  • It doesn't make sense that the discriminator keyword would affect allOf. There's no point in discriminating if all schemas need to validate successfully. This removes mention of discriminator being used with allOf.

There are a couple bigger changes I'd like to make as well, but I'll leave those for separate PRs. I'd like to remove the "schema name" addressing concept, the mapping sub-keyword, and the parent discriminator feature. I'm going to open a couple issues to try to learn more about why these things exist before trying to remove them.

Note: This builds on #2618 which is not merged yet. I'm going to put this in "draft" status so it doesn't get merged until the other is merged, but please do review even though it's in draft. The first commit brings in the changes from the other PR, so it might be more effective to view just the last three commits that cove the actual changes.

The `discriminator` keyword is an annotation. It doesn't affect
validation, so I would not be ideal for it to cause a failure if not
used properly. This changes the behavior of `discriminator` from being
an error if the propertyName is not a required property in the object to
being a no-op. This is also more consistent with other JSON Schema
keywords that ignore things that don't apply to them, like `properties`
only applying to instances that are objects.
There is no way for implementations to tell the difference between a
value that is intended to be a schema name and a value that is expected
to be a URI. Requiring all mappings to use URIs avoids this problem
while not losing any functionality. The only consequence is that some
mappings would be a little more verbose than they otherwise would have
needed to be (example: `#/components/schemas/Dog` vs `Dog`).
It doesn't make sense that the `discriminator` keyword would affect
`allOf`. There's no point in discriminating if all schemas need to
validate successfully.
@lornajane
Copy link
Contributor

Thanks @jdesrosiers . I realise this one has been open for some time! As things stand, there's discussions on whether the next major version release of OpenAPI will support features like discriminator. With that context, I think it is fair to say that there's little appetite to introduce this level of change in a minor release on the 3.x branch. I'm therefore closing this pull request (but with thanks, as always) - check out the Moonwalk repo discussions if you want to talk more about what we should (and should not) do with discriminator in the future of OpenAPI.

@lornajane lornajane closed this Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants