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

documented the discriminator property #1093

Merged
merged 5 commits into from May 19, 2017
Merged

documented the discriminator property #1093

merged 5 commits into from May 19, 2017

Conversation

fehguy
Copy link
Contributor

@fehguy fehguy commented May 8, 2017

Fixes #1085

versions/3.0.md Outdated
discriminator:
property: pet_type
mapping:
cat: '#/components/schemas/Cat'

Choose a reason for hiding this comment

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

Should this be

      cachorro: '#/components/schemas/Dog'

(introducing cat is confusing since the above example used "pet_type": "Cat")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DavidBiesack you are correct, I will update the example.

versions/3.0.md Outdated
- pet_type
properties:
pet_type:
type: string

Choose a reason for hiding this comment

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

Consider adding an enum for this as well, to allow for validation:

         pet_type:
           type: string
           enum:
             - Cat
             - cachorro

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would defeat the goal of this example--here, we're showing that the parent object Pet does not have knowledge of all of the composed types. Thus, any object which extends Pet with the allOf construct is a candidate for use

versions/3.0.md Outdated
bark:
type: string
```

Choose a reason for hiding this comment

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

Consider adding Lizard: here to be consistent with the previous 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.

sure, I think Lizards are cool

For example:

```
components:

Choose a reason for hiding this comment

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

remove components: and schemas: here since they are not used in the other examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I believe the components/schemas is important here, since we're showing references inside the example. I'll ensure the composition examples are consistent.


will map to `Dog` because of the definition in the `mappings` element.


Copy link

Choose a reason for hiding this comment

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

I suggest also adding a note:

Some OpenAPI definitions use external, reusable schema references such as

      properties:
        pet_type:
           type: string
           enum:
             - cat
             - cachorro
             - lizard
      oneOf:
      - $ref: 'http://www.example.com/schemas/Cat/schema.json'
      - $ref: 'http://www.example.com/schemas/Doc/schema.json'
      - $ref: 'http://www.example.com/schemas/Lizard/schema.json'
      discriminator:
        propertyName: pet_type
        mapping:
          cat: 'http://www.example.com/schemas/Cat/schema.json'
          cachorro: 'http://www.example.com/schemas/Doc/schema.json'
          lizard: 'http://www.example.com/schemas/Lizard/schema.json'

and an implicit name mapping based on schema names is not possible.
For such cases, a mapping is required.

The above $ref values are JSON Schema files, not OpenAPI documents. Schema may
also be referenced from schema definitions from an external OpenAPI definition:

      properties:
        pet_type:
           type: string
           enum:
             - cat
             - cachorro
             - lizard
      oneOf:
      - $ref: 'http://www.example.com/library.json#/components/schema/Cat.json'
      - $ref: 'http://www.example.com/library.json#/components/schema/Doc.json'
      - $ref: 'http://www.example.com/library.json#/components/schema/Lizard.json'
      discriminator:
        propertyName: pet_type
        mapping:
          cat: $ref: 'http://www.example.com/library.json#/components/schema/Cat.json'
          cachorro: $ref: 'http://www.example.com/library.json#/components/schema/Doc.json'
          lizard: $ref: 'http://www.example.com/library.json#/components/schema/Lizard.json'

[in particular, citing external JSON Schema, and not just #/definitions/SchemaName from other OpenAPI 2.0 documents, worked in 2.0 and we need to ensure this still works in 3.0, whether this involves discriminators or not.]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, I'll add language around this 👍

##### Fixed Fields
Field Name | Type | Description
---|:---:|---
<a name="propertyName"></a>propertyName | `string` | **required** the name of the property in the payload that will hold the discriminator value.
Copy link
Contributor

Choose a reason for hiding this comment

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

The examples are using property, here it is propertyName.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will fix

versions/3.0.md Outdated
- $ref: '#/components/schemas/Lizard'
```

which means the paylod _must_, by validation, match exactly one of the schemas described by `Cat`, `Dog`, or `Lizard`. In this case, a discriminator will act as a "hint" to bypass 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:
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure "bypass validation" is the right phrase here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'll rephrase. will ==> may, bypass ==> shortcut

versions/3.0.md Outdated

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, an optional `mapping` definition may be used:
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the discriminator value does not match a schema or mapping? We should probably document this as it will impact tooling.

@fehguy
Copy link
Contributor Author

fehguy commented May 11, 2017

@OAI/tdc updated per feedback above. Thanks!

@fehguy
Copy link
Contributor Author

fehguy commented May 12, 2017

@OAI/tdc please review and approve--changes from call have been merged.

- packSize
```

#### <a name="discriminatorObject"></a>Discriminator Object

Choose a reason for hiding this comment

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

I suggest renaming this anchor as discriminatorProperty / Discriminator Property so that OAS only uses "Object" when referring to a JSON object.

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 object described here actually is a JSON (or YAML) object in the API definition (In OpenAPI 2.0 it was a simple string), though it describes a single (string) property of the JSON payload.

I would say the name is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DavidBiesack you are correct. Using title is a bad idea, and I've disallowed inline schemas for consideration with the discriminator.

Choose a reason for hiding this comment

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

thanks, Tony

@RobDolinMS
Copy link
Contributor

#TDC: Merge it!

@RobDolinMS RobDolinMS merged commit e8d99bd into OpenAPI.next May 19, 2017
@webron webron deleted the issue-1085 branch June 16, 2017 23:46
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.

None yet

6 participants