Skip to content

Don't generate extra models for allOf, oneOf, or anyOf of one model #361

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

Merged
merged 6 commits into from
Mar 29, 2021

Conversation

forest-benchling
Copy link
Collaborator

Currently, we have tons of places where we do something like

	AaSequence:
      properties:
        customFields:
          allOf:
            - $ref: "#/components/schemas/CustomFields"
          description: Custom fields set on the AA sequence.

which causes an unnecessary model AaSequenceCustomFields to be generated, rather than directly referencing the existing CustomFields model.

The OpenAPI 3.1 spec is going to add support for direct sibling properties:

	AaSequence:
      properties:
        customFields:
          $ref: "#/components/schemas/CustomFields"
          description: Custom fields set on the AA sequence.

But since that may be a long ways away, we directly implement a workaround in this PR.

This workaround also applies for oneOf and anyOf.

@forest-benchling
Copy link
Collaborator Author

Replaces #350, since that PR got auto-closed when its base branch got deleted.

@dbanty @emann

@codecov
Copy link

codecov bot commented Mar 24, 2021

Codecov Report

Merging #361 (da5c8a9) into main (95a29a8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main      #361   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        47           
  Lines         1478      1483    +5     
=========================================
+ Hits          1478      1483    +5     
Impacted Files Coverage Δ
...penapi_python_client/parser/properties/__init__.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 95a29a8...da5c8a9. Read the comment docs.

emann
emann previously approved these changes Mar 24, 2021
@emann
Copy link
Collaborator

emann commented Mar 24, 2021

@dbanty How do you want to do the commit message for this? I feel like doing a fix: would be weird as this is really a fix on an upcoming feature so we probably wouldn't want this showing up in the changelog

@dbanty dbanty mentioned this pull request Mar 27, 2021
Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for putting this together! This will fix a bug that just got reported as well, which is awesome. I have a few changes to request, then we're good to go!

@@ -0,0 +1,154 @@
from typing import Optional
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main branch no longer has golden-record-custom. I removed the full generation there since it was redundant and we really just needed to test that a custom template would be applied. So these files can be removed

Comment on lines +119 to +122
if not isinstance(json_nullable_model_prop, Unset) and json_nullable_model_prop is not None:
params.update(json_nullable_model_prop)
if json_nullable_required_model_prop is not None:
params.update(json_nullable_required_model_prop)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the right call, it matches the intended default of style=query,explode=true, but it is a breaking change we'll have to remember to note.

Comment on lines 410 to 415
for attribute in ["allOf", "anyOf", "oneOf"]:
sub_data = getattr(data, attribute)
if sub_data and len(sub_data) == 1 and isinstance(sub_data[0], oai.Reference):
return _property_from_ref(
name=name, required=required, nullable=data.nullable, data=sub_data[0], schemas=schemas
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's anything illegal about having more than one of those properties, so we only want to do the pass through if only 1 ref exists in all of those properties. So something like:

Suggested change
for attribute in ["allOf", "anyOf", "oneOf"]:
sub_data = getattr(data, attribute)
if sub_data and len(sub_data) == 1 and isinstance(sub_data[0], oai.Reference):
return _property_from_ref(
name=name, required=required, nullable=data.nullable, data=sub_data[0], schemas=schemas
)
sub_data = (data.allOf or []) + data.anyOf + data.oneOf
if len(sub_data) == 1 and isinstance(sub_data[0], oai.Reference):
return _property_from_ref(
name=name, required=required, nullable=data.nullable, data=sub_data[0], schemas=schemas
)

This also means no getattr so we get better type checking!

@dbanty
Copy link
Collaborator

dbanty commented Mar 27, 2021

@dbanty How do you want to do the commit message for this? I feel like doing a fix: would be weird as this is really a fix on an upcoming feature so we probably wouldn't want this showing up in the changelog

@emann this is also fixing the issue for oneOf and anyOf so I think fix: is the right call. Although it will be fix!: since it's a breaking change.

@dbanty dbanty added 🥚breaking This change breaks compatibility 🐞bug Something isn't working labels Mar 27, 2021
@dbanty dbanty added this to the 0.9.0 milestone Mar 27, 2021
@forest-benchling
Copy link
Collaborator Author

Also noting that it is a breaking change because people may be depending on the models that are getting removed by this PR (Benchling certainly is)

@forest-benchling forest-benchling requested a review from dbanty March 29, 2021 00:19
@dbanty dbanty merged commit 078b417 into openapi-generators:main Mar 29, 2021
@eli-bl eli-bl deleted the forest-allof-3 branch November 26, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🥚breaking This change breaks compatibility 🐞bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants