Skip to content

Union types improvements #41

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

Closed
wants to merge 2 commits into from

Conversation

davidmigloz
Copy link
Contributor

@davidmigloz davidmigloz commented Nov 14, 2023

This PR reworks union classes:

  • Fixes oneOf root schemas not generating union classes
  • Allows to rename factory constructors via onSchemaUnionFactoryName (resolves Support customizing factory constructors names from union types #33)
  • Allows to rename union subclasses via onSchemaName
  • Improves union sub-classes naming (I don't think we need the Union prefix to be the default)
  • Improves list and maps primitive unions default factory names by including their inner types in the name
  • Make union sub-classes public so that users can access them (e.g. for using Dart pattern matching instead of freezed map)
  • Adds support nullable primitive unions
  • Propagates descriptions in factory constructors and union classes
  • Make default names more aligned to Dart nomenclature (e.g. list instead of array)

The last commit is a (real) example of the code it generates that touches several edge cases (I've included it just for the sake of reviewing the PR, we can remove it before merging).

I've also regenerated and tested the openai_dart client with these changes (you can see the diff here). All is working fine and the new factories look much nicer 🙂

Some of the code could be improved in the future to reduce duplications.

Let me know what you think!

cc @walsha2

@walsha2
Copy link
Contributor

walsha2 commented Nov 15, 2023

I've also regenerated and tested the openai_dart client with these changes (you can see the diff here). All is working fine and the new factories look much nicer 🙂

That is a pretty client API!

Let me know what you think!

The changes in lib/src/generators/schema.dart break a proprietary API spec and in turn the code generation. I need to figure out where exactly the breaking change comes from and also add a test for that edge case. Spoiler, its union related. Let me get back to you when I have a second to work through the changes and see where things are breaking.

@walsha2
Copy link
Contributor

walsha2 commented Nov 15, 2023

@davidmigloz Checkout the latest main branch and execute the new "Unions" test (#42):

make test TEST_ARGS="-n Unions"

Inspect the resulting output on main to see what the expected results should look like in test/tmp/unions outputs.


Not comprehensive, but it exercises the anyof unions that are composed of components as opposed to just primitives. The OpenAI spec that is in the test folder just has primitive unions. Things get a lot more complicated when you have unions of components and when there is overlap and name clashes 😵‍💫 . Some of your changes in lib/src/generators/schema.dart seem to have broken this logic that was otherwise working.

Anyway, if you execute those tests on this branch you will see a bunch of things start to fail and some schemas are not even generated (hence the file exist checks). Let me know if you have troubles resolving the failing test. Here are two things I saw:

  • Some component schemas were not generated at all in this PR
  • There are name clashes for the constructors

https://github.com/tazatechnology/openapi_spec/actions/runs/6872633743/job/18691395383

@davidmigloz
Copy link
Contributor Author

ahh good one. I think I know how to fix it easily. We just need to treat fields with anyOf composed of only objects the same way as root schemas with only anyOf. I'll give it a try this evening.

@davidmigloz
Copy link
Contributor Author

Sorry, I haven't got time to look into it yet. I'll have some time this weekend!

@davidmigloz
Copy link
Contributor Author

I've moved the first commit of this PR to a separate one (#45) as it's not related to unions.

I'll try to work on this PR today.

@walsha2
Copy link
Contributor

walsha2 commented Aug 27, 2024

Closing in favor of newer solutions to come soon.

@walsha2 walsha2 closed this Aug 27, 2024
@davidmigloz
Copy link
Contributor Author

Sorry @walsha2! I never found the time to finish this work..

@walsha2
Copy link
Contributor

walsha2 commented Aug 28, 2024

Sorry @walsha2! I never found the time to finish this work..

No worries! I have plans for a rewrite to get it to a 1.0 sometime in the near future. Just need to find the time 😅

@davidmigloz
Copy link
Contributor Author

Sounds great, count with me if you need help.

I have this branch with some minor fixes that I was going to send you PRs for, but never did:
https://github.com/davidmigloz/openapi_spec/commits/langchain/

It's the fork I use to generate all the clients we use in LangChain.dart.

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.

Support customizing factory constructors names from union types
2 participants