tazatechnology / openapi_spec

Dart based OpenAPI specification generator and parser
BSD 3-Clause "New" or "Revised" License
8 stars 5 forks source link

Union types improvements #41

Closed davidmigloz closed 1 week ago

davidmigloz commented 9 months ago

This PR reworks union classes:

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 commented 9 months ago

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 commented 9 months ago

@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:

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

davidmigloz commented 9 months ago

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 commented 9 months ago

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

davidmigloz commented 9 months ago

I've moved the first commit of this PR to a separate one (https://github.com/tazatechnology/openapi_spec/pull/45) as it's not related to unions.

I'll try to work on this PR today.

walsha2 commented 1 week ago

Closing in favor of newer solutions to come soon.

davidmigloz commented 1 week ago

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

walsha2 commented 1 week ago

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 commented 1 week ago

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.