Skip to content

Improve "Discriminator object" section of the OAS specification. #2216

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.

Sign up for GitHub

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 11 commits into from
Closed

Improve "Discriminator object" section of the OAS specification. #2216

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
343b5e8
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
0e8a077
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
2efae8d
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
591bf43
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
9d4acf1
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
47dd48a
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
fd5c9e3
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
fa38c08
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
501b829
Improve OAS spec for discriminator
sebastien-rosset Apr 26, 2020
3edee8d
Change sentence to take Justin's feedback into consideration
sebastien-rosset May 1, 2020
172b357
Merge branch 'v3.1.0-dev' of https://github.com/OAI/OpenAPI-Specifica…
sebastien-rosset Aug 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions versions/3.1.0.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2325,7 +2325,6 @@ The OpenAPI Specification allows combining and extending model definitions using
While composition offers model extensibility, it does not imply a hierarchy between the models.
To support polymorphism, the OpenAPI Specification adds the `discriminator` field.
When used, the `discriminator` will be the name of the property that decides which schema definition validates the structure of the model.
As such, the `discriminator` field MUST be a required field.
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I have moved this statement in the "Discriminator Object" section. Grouping all the normative statements in one place helps to understand the specification and how multiple normative statements interact with each other.

There are two ways to define the value of a discriminator for an inheriting instance.
- Use the schema name.
- Override the schema name by overriding the property with a new value. If a new value exists, this takes precedence over the schema name.
Expand Down Expand Up @@ -2676,7 +2675,14 @@ components:

When request bodies or response payloads may be one of a number of different schemas, a `discriminator` object can be used to aid in serialization, deserialization, and validation. The discriminator is a specific object in a schema which is used to inform the consumer of the document of an alternative schema based on the value associated with it.

When using the discriminator, _inline_ schemas will not be considered.
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am proposing to reword this sentence because it is ambiguous.

Problem 1: the verb "consider" is not conducive to normative precision. consider: "to think carefully about (something), typically before making a decision." "take (something) into account when making an assessment or judgment."...

Problem 2: does this sentence mean the discriminator is not required when using inline schema?The sentence is too ambiguous to conclude decisively, especially when you put that sentence side-by-side with this other sentences:

The discriminator field MUST be a required field in the schema object.

and:

The expectation now is that a property with name petType MUST be present in the response payload.

The discriminator object is legal only when using a schema that has one or more of the composite keywords `oneOf`, `anyOf`, `allOf`. Both _inline_ and references can be used as composite child schemas.
When a child schema is a reference, the discriminator property _MUST_ be present in the payload.
A discriminator MAY act as a "hint" to shortcut validation and selection of the matching schema which may be a costly operation, depending on the complexity of the schema.
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I moved this sentence up because it was in the middle of an example. Isn't this sentence always true regardless of the specific example that was provided? If so, I think it would make sense to put it outside the example section and along with other normative statements.

In addition, I have added one more proposed sentence. The spec should explicitly mention that even if the discriminator is not used as a hint (since the above sentence uses "MAY"), the payload must be validated against the JSON schema.

Copy link

@spacether spacether May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

How about using one of the composite keywords -> using one or more of the composite keywords
You can have a schema that includes allOf and oneOf where the payload conforms to both definitions, the allOf validation passes and those properties are loaded in as additional properties in the chosen oneOf 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.

How about using one of the composite keywords -> using one or more of the composite keywords
You can have a schema that includes allOf and oneOf where the payload conforms to both definitions, the allOf validation passes and those properties are loaded in as additional properties in the chosen oneOf schema.

Thank you I have updated the sentence accordingly.

Even when the discriminator is used as shortcut hint, the payload MUST still be validated against the JSON schema.
When a child schema is specified _inline_, the discriminator property is not required to be present in the payload. Validation is applied to determine if the payload matches the child JSON 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.

As a special case, what if all child schemas are specified inline? Does this mean the discriminator is completely ignored and has no effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a proposal to clarify When using the discriminator, inline schemas will not be considered. I am not claiming my proposed change is necessarily correct AS IS. The problem is that we find that sentence ambiguous. I wrote something in the hope that it can incrementally lead to a better wording of the spec.


The discriminator `propertyName` field MUST be a required field in the schema object. However, as shown below, the discriminator is not required to be present in the payload for _inline_ schemas.


##### Fixed Fields
Field Name | Type | Description
Expand All @@ -2686,32 +2692,34 @@ Field Name | Type | Description

This object MAY be extended with [Specification Extensions](#specificationExtensions).

The discriminator object is legal only when using one of the composite keywords `oneOf`, `anyOf`, `allOf`.
##### Discriminator Object Example
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this section title to clearly differentiate between the normative statements versus the example section.


In OAS 3.0, a response payload MAY be described to be exactly one of any number of types:
A payload MAY be described to be exactly one of any number of types:
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Remove "In OAS 3.0" words:

  1. It is the 3.1 spec, not 3.0
  2. There is no need to start every sentence with "In OAS X.Y". The entire document is about a specific version of the OAS spec (in this case 3.1).

Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest removing the word "response" both in the sentence and YAML example, because the discriminator can be used in the request and response. I know this is just an example, but IMHO it's better to show examples that cover as many use cases as possible


```yaml
MyResponseType:
MyType:
oneOf:
- type: 'null'
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Apr 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

One special case is the "null" type, which is introduced in 3.1. I am suggesting to provide one "null" type example to present the implications of the "null" type when it is used along with a discriminator object and oneOf/anyOf. There is only one sentence in this 3.1 draft spec that mentions the "null" type.

My understanding is this schema is supported, and it is the new recommended approach in lieu of the deprecated "nullable". But the "null" type does not have a discriminator field in its payload, so adding it as an example would help to clarify that it's ok to use.

On one hand, the "null" type does not have a discriminator value which breaks the normative statement about the discriminator being a required field. But on the other hand, it seems to be valid because of the following sentence: When using the discriminator, inline schemas will not be considered.

Similarly, it would be great to provide a discriminator example with inline primitive types, e.g. string, boolean:

MyType:
oneOf:
  - type: 'null'
  - type: string
  - $ref: '#/components/schemas/Cat'
  - $ref: '#/components/schemas/Dog'
  - $ref: '#/components/schemas/Lizard'
discriminator:
    propertyName: petType	    propertyName: petType

- $ref: '#/components/schemas/Cat'
- $ref: '#/components/schemas/Dog'
- $ref: '#/components/schemas/Lizard'
```

which means the payload _MUST_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. In this case, a discriminator MAY act as a "hint" to shortcut validation and selection of the matching schema which may be a costly operation, depending on the complexity of the schema. We can then describe exactly which field tells us which schema to use:
which means the payload _MUST_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. We can then describe exactly which field tells us which schema to use:


```yaml
MyResponseType:
MyType:
oneOf:
- type: 'null'
- $ref: '#/components/schemas/Cat'
- $ref: '#/components/schemas/Dog'
- $ref: '#/components/schemas/Lizard'
discriminator:
propertyName: petType
```

The expectation now is that a property with name `petType` _MUST_ be present in the response payload, and the value will correspond to the name of a schema defined in the OAS document. Thus the response payload:
The expectation now is that a property with name `petType` _MUST_ be present in the payload, and the value will correspond to the name of a schema defined in the OAS document. Thus the payload:

```json
{
Expand All @@ -2725,7 +2733,7 @@ Will indicate that the `Cat` schema be used in conjunction with 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:

```yaml
MyResponseType:
MyType:
oneOf:
- $ref: '#/components/schemas/Cat'
- $ref: '#/components/schemas/Dog'
Expand Down