-
-
Notifications
You must be signed in to change notification settings - Fork 1k
feat: add code generation for discriminator with allOf #2114
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
base: main
Are you sure you want to change the base?
feat: add code generation for discriminator with allOf #2114
Conversation
|
Hey @jamietanna, we meet again! How's it going?? Our teams have just run into this issue as well. Seems like this has been unsupported since 2022 (first issue). Totally get there's likely a reason as well. Any chance that this PR could get looked at sooner than later? Totally cool if not, but figured I'd shoot my shot. |
|
This is a nice change, however, it's also a bit scary, and I do believe it is breaking, so that it would have to be put behind a default-disabled flag. allOf/anyOf/oneOf have been very difficult to support and I tried all kinds of things until we got where we are today as a least broken approach. The reason it's breaking is that you can no longer generate It's a bit tricky to wrap your head around the meaning of recursive Now, let's chat about discriminators, and merging schemas with contain them. Say we have a top level type, As I understand your code, if I was to make something like this pseudocode: It would fail because of discriminator conflict. Now, is this a valid example? I don't know. If it worked, a I think the only real problem happens if both of the constituent schemas share a discriminator by the same name. Should the top level merged type even have a discriminator? I'm not sure if it needs one to work, but it does need to have one given how OpenAPI specifies that properties are merged in Anyhow, edge cases like these are why I avoided solving as much of this problem as possible, and left it to application logic to figure it out. |
ced092f to
4e63505
Compare
|
Hi, @mromaszewicz . Thanks for your feedback! Regarding the breaking change - indeed, my changes were introducing a breaking change. I reconsidered the discriminator logic - I had code that inherited discriminators when merging allOf. Now, the discriminator is only extracted from the original schema. I agree with the The only case that still errors out is when both schemas have discriminators with the same property name - I don't know what to do with this case, so I'm rejecting it. I pushed the changes, rebased, and squashed the commits. Also, I would like to ask a question about the necessity of the
I added it for compatibility with |
|
Hello @jamietanna, any updates on this issue? We’re also missing support for |
|
Hi, we're also really looking forward to this feature |
4e63505 to
f8829bd
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Thanks for your use of |
|
It's on our TODO list: #2170 |
I have added the implementation of code generation for discriminators used with
allOfschemas. OpenAPI SpecFeature for #666
Some notes
Code generation
allof-discriminator.tmpltemplate for discriminator-based type generationDiscriminator()- returns discriminator property valueIs<Type>()- type checking helpers (e.g.,IsCat(),IsDog())As<Type>()- type casting methods with validation (e.g.,AsCat(),AsDog())ValueByDiscriminator()- type resolution based on discriminator valueUnmarshalJSON()- unmarshaling to preserve raw JSON for type conversion