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

feat: add support for discriminator #5012

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

feat: add support for discriminator #5012

wants to merge 13 commits into from

Conversation

marikaner
Copy link
Contributor

@marikaner marikaner commented Sep 18, 2024

This provides an initial implementation for the discriminating properties of OpenAPI specs: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

According to the documentation this can only happen on oneOf and anyOf schemas and there can either be an explicit mapping or it is implicitly derived from the referenced schema names.

Of course, the Azure OpenAPI spec does not use oneOf but type: object.
Also, the Azure OpenAPI spec has a circular reference like:

- SchemaA:
  discriminator:
    propertyName: discriminatorProp
    mapping:
      value1: => SchemaB
      value2: => SchemaC
- SchemaB:
  allOf:
    - SchemaA // this references back to the original type
    - ...

This would result in something like this:

type SchemaA = { discriminatorProp: 'value1'} & SchemaB;
type SchemaB = SchemaA & ... // <-- ERROR

To solve this I added a post parsing sanitization step that checks explicitly for oneOf schemas with discriminator mapping and child schemas with allof mapping for circular refs.

TODO:

  • Add test cases to test services
  • Add docs

Comment on lines 560 to 561
SchemaA: { properties: { c: { type: 'string' } } },
SchemaB: { properties: { c: { type: 'integer' } } }
Copy link
Member

Choose a reason for hiding this comment

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

I might not fully understand this test, but I think it should be:

Suggested change
SchemaA: { properties: { c: { type: 'string' } } },
SchemaB: { properties: { c: { type: 'integer' } } }
SchemaA: { properties: { discriminatingProperty: { type: 'string' }, c: { type: 'string' } } },
SchemaB: { properties: { discriminatingProperty: { type: 'string' }, c: { type: 'integer' } } }

The discriminator has to be a property of all schemas.

It is important that all the models mentioned below anyOf or oneOf contain the property that the discriminator specifies.

Don't ask me why this redundancy is necessary, IDK 😄
I guess it would also be possible for it to have different types, e.g. a string in SchemaA and a number in SchemaB.

Unfortunately, after checking again, the Azure spec seems to simply use this discriminator incorrectly. If you look at it in the swagger editor you can see that it is simply ignored and the type of the messages array is incorrect 😬

Copy link
Member

@MatKuhr MatKuhr Sep 19, 2024

Choose a reason for hiding this comment

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

Actually, they fixed this in the preview 2024-08-01, ... but stopped using the discriminator 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh damn, good catch. I read this multiple times yesterday, but still ignored it ;)
Unfortunately that makes things quite a bit more complicated, but let's see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, it doesn't make it more complicated. types should still work as expected.

@jjtang1985
Copy link
Contributor

jjtang1985 commented Sep 19, 2024

5 cases with discriminator: detected from the new spec, where 2 of them are not relevant (onYourData...)

Would it be ok to:

  1. merge Deeksha's PR first.
  2. use the Cloud SDK generator on a branch for fixing the AI SDK

With that:

  • we dont even need a release from Cloud SDK
  • we see smaller PR only related to discriminator

I would expect the version 2024-08-01 should have been released or soon in the future, by checking the name.

@marikaner marikaner changed the title feat: initial implementation for discriminator feat: add support for discriminator Sep 19, 2024
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.

4 participants