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

Allow discriminator to be optional #4339

Open
wants to merge 4 commits into
base: v3.2-dev
Choose a base branch
from

Conversation

mikekistler
Copy link
Contributor

This PR changes the discriminator to allow the discriminator property to be an optional field, so that when it is not present the default field can be used to specify which schema of the anyOf or oneOf is expected to validate the structure of the model.

I've also massaged the descriptions of polymorphism to emphasize that JSON Schema can and should be used to describe polymorphism and that discriminator is an optional extension of that support.

I also dropped some of the JSON examples when the same example was shown in yaml just to cut down on redundancy.

Tick one of the following options:

  • schema changes are included in this pull request
  • schema changes are needed for this pull request but not done yet
  • no schema changes are needed for this pull request

@mikekistler mikekistler requested review from a team as code owners February 6, 2025 19:23
Copy link
Contributor

@jeremyfiel jeremyfiel left a comment

Choose a reason for hiding this comment

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

I'm not totally on board with the naming of default.

The definition is different from JSON Schema and what you are trying to accomplish here by specifying a schema instance to validate against vs the commonly, well used, instance value.

src: https://json-schema.org/draft/2020-12/json-schema-validation#section-9.2

There are no restrictions placed on the value of this keyword. When multiple occurrences of this keyword are applicable to a single sub-instance, implementations SHOULD remove duplicates.

This keyword can be used to supply a default JSON value associated with a particular schema. It is RECOMMENDED that a default value be valid against the associated schema.

The other concern I have is the referenced schemas in oneOf/anyOf may have all properties optional, there is no requirement to have at least one required property to satisfy a oneOf subschema. The only requirement is that the validation succeeds which can be accomplished with other means besides a property name.

src/oas.md Outdated Show resolved Hide resolved
src/oas.md Outdated Show resolved Hide resolved
@handrews
Copy link
Member

handrews commented Feb 6, 2025

@jeremyfiel default could instead be ifAbsent or whenAbsent or something like that

Copy link
Contributor

@jeremyfiel jeremyfiel left a comment

Choose a reason for hiding this comment

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

a few minor edits for clarification

src/oas.md Outdated

When the `discriminator` property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema that is expected to validate the structure of the model when the `discriminator` property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
When the discriminating property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema that is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the discriminating property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema that is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
When the discriminating property is defined as optional, the `discriminator` schema must include a `default` field that specifies a schema that is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think changing "field" to "schema" is right here. Can you explain why you have suggesting this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

the discriminator keyword defines a schema. It's not a standalone field. It requires the propertyName keyword per the spec.

discriminator:
  propertyName: thing
  default: Dog # < your new keyword

vs

discriminator: ??

Copy link
Contributor

Choose a reason for hiding this comment

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

there were a few locations where this wasn't updated. They are in the Outdated conversations below. Is that an oversight or intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change discriminator field to discriminator keyword. Would that address your concerns?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like OAS fields should be fields even in the Schema Object. Sometimes when talking about the Schema Object as one of many Objects I just use "fields" because the context is clearly OAS. I use "keyword" when the more important context it JSON Schema. Although I have no stronger argument for this usage. Perhaps @ralfhandl has an opinion here?

Copy link
Contributor

Choose a reason for hiding this comment

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

the reason I raised this is because the discriminator field is not actually a thing. it's a schema. You can't use the field by itself, as shown in my example. Referring to it as a field is not correct, imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremyfiel I don't know what you mean when you say "the discriminator field is not actually a thing". discriminator is a "fixed field" defined by OpenAPI, so by our established terminology, we refer to it as a "field".

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the field is described by OAS, but the field itself is a schema. It's never a standalone field such as additionalProperties: false or additionalProperties: {} where it can be both.

In this reference language, I believe the discriminator field should be referenced as a schema because you are referencing the behavior of the field. The use of field is correct when referencing the defaultMapping requirement.

When the discriminating property is defined as optional, the discriminator schema must include a defaultMapping field that specifies a schema

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay ... I think I see a way to resolve this. What we are talking about here is the "Discriminator Object" -- the value of the discriminator field. So we could use "Discriminator Object" rather than "discriminator field`" in the places you have flagged. I think this would be consistent with language/terminology elsewhere in the spec. Would this work for you @jeremyfiel ?

src/oas.md Outdated
@@ -2877,7 +2877,7 @@ These keywords can be used to describe polymorphism, where a single field can ac

The OpenAPI specification extends the JSON Schema support for polymorphism by adding the [`discriminator`](#schema-discriminator) field.
When used, the `discriminator` indicates the name of the property that hints which schema of an `anyOf` or `oneOf` is expected to validate the structure of the model.
The `discriminator` property may be defined as required or optional, but when defined as an optional property the `discriminator` field must include a `default` field that specifies which schema of the `anyOf` or `oneOf` is expected to validate the structure of the model.
The discriminating property may be defined as required or optional, but when defined as an optional property the `discriminator` field must include a `default` field that specifies which schema of the `anyOf` or `oneOf` is expected to validate the structure of the model.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The discriminating property may be defined as required or optional, but when defined as an optional property the `discriminator` field must include a `default` field that specifies which schema of the `anyOf` or `oneOf` is expected to validate the structure of the model.
The discriminating property may be defined as required or optional, but when defined as an optional property the `discriminator` schema must include a `default` field that specifies which schema of the `anyOf` or `oneOf` is expected to validate the structure of the model.

src/oas.md Outdated Show resolved Hide resolved
src/oas.md Outdated
@@ -3476,7 +3476,7 @@ Here the discriminating value of `dog` will map to the schema `#/components/sche

When used in conjunction with the `anyOf` construct, the use of the discriminator can avoid ambiguity for serializers/deserializers where multiple schemas may satisfy a single payload.

When the `discriminator` property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema of the `anyOf` or `oneOf` is expected to validate the structure of the model when the `discriminator` property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
When the discriminating property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema of the `anyOf` or `oneOf` is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When the discriminating property is defined as optional, the `discriminator` field must include a `default` field that specifies a schema of the `anyOf` or `oneOf` is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.
When the discriminating property is defined as optional, the `discriminator` schema must include a `default` field that specifies a schema of the `anyOf` or `oneOf` is expected to validate the structure of the model when the discriminating property is not present in the payload. This allows the schema to still be validated correctly even if the discriminator property is missing.

@jeremyfiel
Copy link
Contributor

I feel like this line should be updated with the language of discriminating value

This was not part of your changes, but it's related and the perfect time to make additional clarifications

In scenarios where the value of the `discriminator` field does not match the schema name or implicit mapping is not possible, an optional `mapping` definition MAY be used:

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