Discriminator Mapping Support for oneOf composition #1574
Conversation
|
mzronek
changed the title
|
I thought about this some more and my PR now handles all oneOf + mapping combinations. Also the automatically inferred oneOf case, that you already implemented. I adapted the unit test as well. My code is a little bit more intrusive now as I try to avoid the WithRequired<> handling for the already handled oneOfs. I still need to adapt the test cases that remain changed so all unit tests are successful. In my opinion the failed unit tests are showing the improvement of the new types very well. That's why I left them as is for now. We should also discuss the description, that I set when the discriminator enum is appended or replaces an existing enum. We can use the existing description and append some statement to it as well, so no information is lost. I appreciate your opinion on this. There is probably some opportunity to shrink my code a bit or make it more robust. Please feel free to change my proposal as you see fit. |
There was a problem hiding this comment.
This looks great! 👏 Fantastic investigation, great tests, and great bugfix to discriminators!
Bug Notes
You correctly identified that disciminator is handled both in 6.x and 7.x, but you had a usecase that wasn’t in the tests, and wasn’t one I had personally encountered in a schema before (not to say it’s uncommon; I agree it seems like a very useful pattern; again, just weren’t testing it). I think your fix could also be applied to the 6.x branch as well to get a stable release (this is the 7.x branch), and would also love a PR for that, too, unless I beat you to it over the next couple weeks (and no worries if that happens).
Discriminators in general have lots of weird quirks that the spec outlines, that people don’t realize. But your fix just seems to extend functionality without breaking some of the intentional quirks that are correctly captured with the existing tests.
Pull Request Notes
Re: some of your points about execution, this looks great to me. Your code comments are great, and just looking at your implementation I understand what’s going on here. At-a-glance, this also seems like it would execute fast and there don’t seem to be any obvious issues with performance (an undocumented, but present, principle of this library is using more performance- and memory-conscious patterns such as for … of which do make big differences in execution speed).
In the GlobalContext object, I don’t mind having discriminators and discriminatorRefsHandled side-by-side as that object is 100% internal, and doesn’t affect consumers. It’s only meant to hang on to relevant state during transformation, and this discriminator work fits the bill. Both are 2 different structures fundamentally. Perhaps we could namespace them both under discriminators somehow, for cleanliness, but that’s such a minor cleanup thing, and doesn’t affect any output, and can be handled at any point in the future (but again, I’m fine with it as-is).
Thanks again for such a great fix!
|
Thank you for the feedback. I am glad you like it. I can backport the discriminator changes to 6.x, no problem. Let's finish the necessary changes for this PR and I will PR to the 6.x branch in the next few days. My usecase is actually the default usecase as stated by the swagger.io documentation. The oneOf+mapping is the most frequent discriminator pattern I have come across in the corporate world. I added the discriminator objects and refs handled into a GlobalContext's discriminators object. I hope it is cleaner now, although I had to adjust more files. I updated the examples now as well and the unit tests complete successfully. There is a warning when running the update of the DigitalOcean schema: |
|
Quick update: I also added support for multiple discriminator enum values pointing to the same refs now. This will result in multiple enum values, if necessary. |
Oh yes that is ignorable. DigitalOcean is a “bad” schema, and and has some errors, but it’s huge. And I like testing any schema that’s used in production (as long as it’s not irreparably broken), and make sure that openapi-typescript can generate types for it. When I started this project, so many OpenAPI libraries just used that simple “PetStore” example from Swagger but could never handle full schemas, and I’m really proud of all the work the community has done (including yourself!) to make this such a robust project that can handle anything you can throw at it. Thanks again for making it better! I didn’t merge earlier because we had some minor test failures, but everything is passing. Thanks again! |
| @@ -71,7 +71,7 @@ export default async function openapiTS( | |||
| additionalProperties: options.additionalProperties ?? false, | |||
| alphabetize: options.alphabetize ?? false, | |||
| defaultNonNullable: options.defaultNonNullable ?? true, | |||
| discriminators: scanDiscriminators(schema), | |||
| discriminators: scanDiscriminators(schema, options), | |||
There was a problem hiding this comment.
Ah, even cleaner, thank you! That was such a minor optional thing that you didn’t have to do, but appreciate you doing it. I had nothing else to comment on in your PR 😄
Changes
This PR tries to solve the ignored discriminator mappings for oneOf compositions. More details here: #1572
I am unsure if changing the schemaObject is something we really want to do. But this is very fast and even solves the allOf compositions for discriminated objects with inheritance.
Added 2 tests as well for the new cases.
How to Review
All other discriminator tests are now failing. I checked a few cases and saw actual improvements to the generated types for example in the DigitalOcean types. This needs to be discussed of course.
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)