feat: Add allOf support for model definitions (#98)

[dbanty]

@bowenwr I attempted to implement what I was talking about in my review of #312 . I just cloned that branch and made the changes, so in theory they are based on top of your existing work and could be backported to your fork? Let me know if there's a problem with this implementation or you disagree that it's simpler.

[codecov]

Codecov Report

Merging #321 (e81165d) into main (dc05117) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #321   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1421      1478   +57     
=========================================
+ Hits          1421      1478   +57     

Impacted Files	Coverage Δ
...penapi_python_client/parser/properties/__init__.py	100.00% <100.00%> (ø)
..._python_client/parser/properties/model_property.py	100.00% <100.00%> (ø)
openapi_python_client/parser/properties/schemas.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc05117...e81165d. Read the comment docs.

[dbanty]

This PR is obviously still missing a bit of coverage and needs to be synced with main, but I'll wait on a response from the Benchling folks before putting any more effort into this branch.

CC @packyg as well since I believe they did the original implementation in the fork.

[kalzoo]

Interested to hear what @packyg and @forest-benchling think about this :) If it's just a matter of porting comments from #312 and syncing with main, I can contribute that. This feature will be great to have.

[packyg]

I think you need to account for a situation where one of the children of allOf specify a property as required and one does not (in which case should make it should be required)

[dbanty]

Yeah you're right. I think attrs will implement __eq__ and maybe even __hash__ so we can remove any optional properties from the required properties list. I think we can also get duplicate properties with the current implementation.

What do you think we should do if two properties of the same name are defined but not the same? Return an error, right?

[packyg]

Depends on why they're not the same, I think. If there are things in direct conflict (like different types) then error. But if it's one has it required, the other optional, then just make it required.

Different sub-properties in 2 different model props could also probably error

[packyg]

@dbanty This is much cleaner actually. I'm definitely not dogmatic about the implementation we did so definitely could get behind this. cc @bowenwr @dtkav @forest-benchling

I hadn't looked closely enough at build_schemas but I think you're right in your comment on #312 that the loop would handle this.

We had actually implemented our original version of this before the big refactor in #236/v0.7.0. At that time there was not support for forward references so we had to circle back and resolve them the end. That implementation was ported over after pulling v0.7 and worked more or less the same way.

[forest-benchling]

Sorry for the delay, this looks good to me. Thanks for implementing it. My only question is, why did the generated code for AModel change when it looks like openapi.json didn't touch it?

[dbanty]

why did the generated code for AModel change when it looks like openapi.json didn't touch it?

Because the schema for AModel actually had several allOfs in it already, they just weren't being used 😅. The JSON was originally copied from a FastAPI app so I guess it had included some and I never noticed we were missing the functionality.

[dbanty]

Alright, I think I fixed the issues pointed out above and finished test coverage so in theory this code is ready to go after some reviews.

Highlights

1. Moved build_model_property into model_property.py since it felt like it belonged there.
2. Split up build_model_property into a few sub-functions since it's getting pretty complex now.
3. Loop around through allOf and collect the different types of data that could be there into the final ModelProperty

Expected behaviors

1. Including a property schema in allOf means you'll get that property in your model.
2. Including a Reference in allOf means you'll get all the properties of that Reference (note this only applies to references that are models, references that are anything else will be ignored, not sure how to deal with them right now).
3. Two properties included from any source (allOf ref, allOf schema, or normal top level properties) that have the same name are conflicts. Conflicts cause errors unless they can be resolved. Right now, the only things that will be resolved are different nullable values or different required values. Any other differences will cause an error during generation

CC @packyg , @emann , @kalzoo , and @forest-benchling if any of y'all want to take another look at this.

[forest-benchling]

👍 No need to block on me, changes sound good!

[dbanty]

I'm gonna move this forward so it's not sitting around forever and include it in the new release (since there are other changes I want to get out).