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

Polymorphism support doesn't allow custom discriminator values #366

Closed
brjohnstmsft opened this issue Sep 25, 2015 · 7 comments
Closed

Polymorphism support doesn't allow custom discriminator values #366

brjohnstmsft opened this issue Sep 25, 2015 · 7 comments

Comments

@brjohnstmsft
Copy link
Member

The Swagger spec for Schema Object says this about discriminator (emphasis mine):

Adds support for polymorphism. The discriminator is the schema property name that is used to differentiate between other schema that inherit this schema. The property name used MUST be defined at this schema and it MUST be in the required property list. When used, the value MUST be the name of this schema or any schema that inherits it.

The constraint on value makes it impossible to model OData polymorphism using Swagger. In OData V4, the discriminator is always an annotation (JSON property) called @odata.type and its value is defined as a # followed by the fully-qualified name of the type in the EDM model. In the example of Dog inheriting from Pet, Swagger expects petType to be Dog, while OData requires it to be something like #Microsoft.Azure.Dog (assuming a namespace of Microsoft.Azure in the EDM model).

We need a way to model polymorphism in existing REST APIs that use OData without having to change those APIs.

@brjohnstmsft
Copy link
Member Author

Here's the related discussion from the Swagger folks: OAI/OpenAPI-Specification#403

Looks like a proper fix is a long way off. Any suggested workarounds?

@dolmen
Copy link

dolmen commented Sep 29, 2015

It looks like the implementation idea I submitted in swagger-api/swagger-spec#403 (using enum values for matching) could solve your problem... But this is just an idea, not code...

@devigned
Copy link
Member

My vote goes to option 4 from OAI/OpenAPI-Specification#403.

The last option is to add a new field, like we added the discriminator, such as class (or whatever), and its value would determine the value, in case the user doesn't want to assume the model name used under definitions. While this is a more elegant solution, it is technically a breaking change in the spec (even though it only adds, and doesn't mutate/remove from it).
We should implement it as an x-ms extension, though.

It sounds like this will end up as the path going forward, but it's definitely breaking right now. Thoughts @brjohnstmsft, @Azure/adx-sdk-team?

@brjohnstmsft
Copy link
Member Author

Seems like the extension is the cleanest solution. It doesn't have to be breaking if we ensure that it's optional (i.e. -- fall back to the current behavior if it's not there).

@brjohnstmsft
Copy link
Member Author

Since I haven't heard any objections, I'm going to start implementing a fix based on adding a new extension (tentatively named x-ms-discriminator-value).

@devigned
Copy link
Member

👍

@brjohnstmsft
Copy link
Member Author

Still working on this. Found this related bug along the way: #456

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

No branches or pull requests

4 participants