| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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 allOf models for two models which have discriminators, since in the past, we ignored them. You've added a new failure mode, but this should be very rare. I've found that rare things happen all the time with this project :) It's a bit tricky to wrap your head around the meaning of recursive allOf's with discriminators, however I really like what you did, nice work! Now, let's chat about discriminators, and merging schemas with contain them. Say we have a top level type, Pet, with the petType discriminator as appears everywhere. Let's add another top level type Pest with the pestType discriminator. As I understand your code, if I was to make something like this pseudocode: schema:
Mouse:
allOf:
- Pet
- Pest
It would fail because of discriminator conflict. Now, is this a valid example? I don't know. If it worked, a Mouse would need to have petType and pestType properties, but they aren't in conflict with each other. A Mouse is still a Pet and it is also a Pest and anything which expects either of them should be able to accept a Mouse. I'm not sure if we should reject it. 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 allOf 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. |
Sorry, something went wrong.
|
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 Mouse example. I implemented support for this functionality and wrote tests. So Mouse with both petType and pestType properties is now valid, since they're different properties. This aligns with OpenAPI spec's property merging behavior. 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 Discriminator() method (allof-discriminator.tmpl). I'm wondering if this is necessary, given that:
I added it for compatibility with unionElements (union.tmpl). The only use case I can think of is for debugging. |
Sorry, something went wrong.
|
Hello @jamietanna, any updates on this issue? We’re also missing support for allOf + discriminator. |
Sorry, something went wrong.
|
Hi, we're also really looking forward to this feature |
Sorry, something went wrong.
|
Thanks for your use of oapi-codegen! |
Sorry, something went wrong.
| // Reject same discriminator property names in allOf | ||
| if allOf && s1.Discriminator != nil && s2.Discriminator != nil && s1.Discriminator.PropertyName == s2.Discriminator.PropertyName { | ||
| return openapi3.Schema{}, fmt.Errorf("merging two schemas with the same discriminator property name "+ | ||
| "(%q) in allOf is not supported", s1.Discriminator.PropertyName) | ||
| } | ||
|
|
There was a problem hiding this comment.
This PR actually solved an issue I was having with missing child schemas, unfortunately this change surfaced an error which made a schema that previously generated successfully error out which might be good to be aware of.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
I have added the implementation of code generation for discriminators used with allOf schemas. OpenAPI Spec
Feature for #666
Some notes
Code generation