Skip to content

correctly resolve references to a type that is itself just a single allOf reference #1103

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 7 commits into from
Aug 25, 2024

Conversation

eli-bl
Copy link
Collaborator

@eli-bl eli-bl commented Aug 22, 2024

Fixes #1091

Example of a valid spec that triggered this bug: https://gist.github.com/eli-bl/8f5c7d1d872d9fda5379fa6370dab6a8

In this spec, CreateCat contains only an allOf with a single reference to CreateAnimal. The current behavior of openapi-python-client in such a case is that it treats CreateCat as simply an alias for CreateAnimal; any references to it are treated as references to CreateAnimal, and it doesn't bother creating a model class for CreateCat. And if the spec contained only those two types, then this would be successful.

(Note, the term "alias" doesn't exist in OpenAPI/JSON Schema, but I'm using it here to mean "a type that extends one other type with allOf, with no changes." Whether that should be treated as a separate thing in any way is not really a concern of OpenAPI; it's an issue for us only because we are generating code for model classes. See also: #1104)

Anyway, in this case the spec also contains UpdateCat, which extends CreateCat with an additional property. This should be exactly the same as extending CreateAnimal... but, prior to this fix, it resulted in a parsing error. The problem happened like this:

  1. In _create_schemas, we create a ModelProperty for each of the three schemas.
  • The one for CreateCat is handled slightly differently: its data attribute points to the exact same schema as CreateAnimal, and we do not add it into schemas.classes_by_name because we don't want to generate a separate Python class for it.
  1. In _process_models, we're attempting to finish filling in the property list for each model.
  • That might not be possible right away because there might be a reference to another model that hasn't been fully processed yet. So we iterate as many times as necessary until they're all fully resolved. However...
  • What we are iterating over is schemas.classes_by_name. There's an incorrect assumption that every named model is included in that dict; in this case, CreateCat is not in it.
  • Therefore, CreateCat remains in an invalid state, and the reference from CreateAnimal to CreateCat causes an error.

My solution is to use classes_by_name only for the purpose of determining what Python classes to generate, and add a new collection, models_to_process, which includes every ModelProperty including ones that are aliases.

After the fix, generating a client from the example spec succeeds. The only Python model classes created are CreateAnimal and UpdateCat; the post endpoint that referenced CreateCat uses the CreateAnimal class. Again, that's consistent with how openapi-python-client currently handles these type aliases; the difference is just that it no longer fails when it sees a reference to the alias.

@eli-bl eli-bl marked this pull request as ready for review August 22, 2024 00:35
dbanty
dbanty previously approved these changes Aug 25, 2024
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.

Thanks so much for tackling this problem!

@dbanty dbanty changed the title fix: correctly resolve references to a type that is itself just a single allOf reference correctly resolve references to a type that is itself just a single allOf reference Aug 25, 2024
@dbanty dbanty enabled auto-merge August 25, 2024 02:53
@dbanty dbanty added this pull request to the merge queue Aug 25, 2024
Merged via the queue into openapi-generators:main with commit cf181c2 Aug 25, 2024
19 checks passed
@knope-bot knope-bot bot mentioned this pull request Aug 25, 2024
github-merge-queue bot pushed a commit that referenced this pull request Aug 25, 2024
> [!IMPORTANT]
> Merging this pull request will create this release

## Fixes

### Allow OpenAPI 3.1-style `exclusiveMinimum` and `exclusiveMaximum`

Fixed by PR #1092. Thanks @mikkelam!

### Add missing `cast` import when using `const`

Fixed by PR #1072. Thanks @dorcohe!

### Correctly resolve references to a type that is itself just a single
allOf reference

PR #1103 fixed issue #1091. Thanks @eli-bl!

### Support `const` booleans and floats

Fixed in PR #1086. Thanks @flxdot!

Co-authored-by: knope-bot[bot] <152252888+knope-bot[bot]@users.noreply.github.com>
@eli-bl eli-bl deleted the issue-1091-allof-single branch August 26, 2024 17:51
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.

allOf fails if it references a type that also uses allOf with just single item
2 participants