-
Notifications
You must be signed in to change notification settings - Fork 9.1k
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
base: v3.2-dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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.
@jeremyfiel |
There was a problem hiding this 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
Co-authored-by: Jeremy Fiel <[email protected]>
I feel like this line should be updated with the language of This was not part of your changes, but it's related and the perfect time to make additional clarifications OpenAPI-Specification/src/oas.md Line 3459 in 7ebde14
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be overkill to link all of the anchors, but I think that's how the spec usually does it. Feel free to ignore
There is one typo with the defaultMapping
naming that should be corrected if you choose to ignore the anchor links.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As problematic as the allOf
form is, I'm not sure I'm OK with removing the example entirely. People do use it, and we can't remove it in a minor version, and it's extremely hard to understand even with an example.
src/oas.md
Outdated
| <a name="discriminator-mapping"></a> mapping | Map[`string`, `string`] | An object to hold mappings between payload values and schema names or URI references. | | ||
| <a name="default"></a> defaultMapping | `string` | The schema name or URI reference to a schema that is expected to validate the structure of the model when the discriminating property is not present in the payload. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name defaultMapping
suggests to me that this is the default whenever there isn't a defined mapping. So not just when the property is missing, but when the property has a value that cannot otherwise be resolved. Is this the intention? If not, I'd suggest a name tying to absence rather than "default."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was not the intention. But it is an interesting idea. This could be accomplished with a slightly different schema for the defaultMapping. Instead of not: required: ['petType']
it could be:
properties:
petType:
not:
enum: ['dog', 'cat']
This might get unwieldy for a schema with many variants, but there is some attractiveness to this.
Thoughts?
I pushed some commits that address most of the outstanding comments. I made musts and mays uppercase. I added the language @handrews suggested about how discriminating values are used for I fixed the instance where I did not make every instance of "Discriminator Object" into a link. When there were multiple occurrences of Discriminator Object in a section, I made the first one a link and left the rest as plain text. I think other "Object" types are handled similarly -- for example only 13 of the 20 occurrences of "Path Item Object" are links, the rest are plain text. I left open the question about the name |
Co-authored-by: Jeremy Fiel <[email protected]>
@@ -3433,7 +3488,7 @@ MyResponseType: | |||
- $ref: '#/components/schemas/Lizard' | |||
``` | |||
|
|||
which means the payload _MUST_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. Deserialization of a `oneOf` can be a costly operation, as it requires determining which schema matches the payload and thus should be used in deserialization. This problem also exists for `anyOf` schemas. A `discriminator` MAY be used as a "hint" to improve the efficiency of selection of the matching schema. The Discriminator Object cannot change the validation result of the `oneOf`, it can only help make the deserialization more efficient and provide better error messaging. We can specify the exact field that tells us which schema is expected to match the instance: | |||
which means the payload _MUST_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. Deserialization of a `oneOf` can be a costly operation, as it requires determining which schema matches the payload and thus should be used in deserialization. This problem also exists for `anyOf` schemas. A `discriminator` MAY be used as a "hint" to improve the efficiency of selection of the matching schema. The [Discriminator Object](#discriminator-object) cannot change the validation result of the `oneOf`, it can only help make the deserialization more efficient and provide better error messaging. We can specify the exact field that tells us which schema is expected to match the instance: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which means the payload _MUST_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. Deserialization of a `oneOf` can be a costly operation, as it requires determining which schema matches the payload and thus should be used in deserialization. This problem also exists for `anyOf` schemas. A `discriminator` MAY be used as a "hint" to improve the efficiency of selection of the matching schema. The [Discriminator Object](#discriminator-object) cannot change the validation result of the `oneOf`, it can only help make the deserialization more efficient and provide better error messaging. We can specify the exact field that tells us which schema is expected to match the instance: | |
which means a valid payload has to match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. Deserialization of a `oneOf` can be a costly operation, as it requires determining which schema matches the payload and thus should be used in deserialization. This problem also exists for `anyOf` schemas. A `discriminator` can be used as a "hint" to improve the efficiency of selection of the matching schema. The [Discriminator Object](#discriminator-object) cannot change the validation result of the `oneOf`, it can only help make the deserialization more efficient and provide better error messaging. We can specify the exact field that tells us which schema is expected to match the instance: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid "MUST" and "MAY" in Examples section
@@ -3438,7 +3511,7 @@ The expectation now is that a property with name `petType` _MUST_ be present in | |||
|
|||
will indicate that the `Cat` schema is expected to match this payload. | |||
|
|||
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: | |||
In scenarios where the value of the discriminating property does not match the schema name or implicit mapping is not possible, an optional `mapping` definition MAY be used: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In scenarios where the value of the discriminating property does not match the schema name or implicit mapping is not possible, an optional `mapping` definition MAY be used: | |
In scenarios where the value of the discriminating property does not match the schema name or implicit mapping is not possible, an optional `mapping` definition can be used: |
@@ -3458,6 +3531,30 @@ 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 discriminating property is defined as optional, the Discriminator Object MUST include a `defaultMapping` 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the discriminating property is defined as optional, the Discriminator Object MUST include a `defaultMapping` 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 Object has to include a `defaultMapping` 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. |
This PR changes the
discriminator
to allow the discriminator property to be an optional field, so that when it is not present thedefault
field can be used to specify which schema of theanyOf
oroneOf
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: