openapi-python-client icon indicating copy to clipboard operation
openapi-python-client copied to clipboard

Single model `allOf` behavior is confusing

Open mtovts opened this issue 3 years ago • 1 comments

Describe the bug Models for schemas with allOf are not generated.

To Reproduce Steps to reproduce the behavior:

  1. Run generation for example OpenAPI (see below).
  2. See the generated package models contains only pet.py. No cat.py

Expected behavior Expected generated model for Cat.

OpenAPI Spec File

openapi: 3.0.3
info:
  title: allOf example
  description: allOf example
  version: 0.1.0
servers:
  - url: 'https://example.com'
paths:
  '/foo':
    delete:
      responses:
        '200':
          description: OK
components:
  schemas:
    Pet:
      type: object
      properties: { }
    Cat:
      type: object
      properties: { }
      allOf:
        - $ref: '#/components/schemas/Pet'

Desktop (please complete the following information):

  • OS: [e.g. macOS 10.15.1] Windows 10
  • Python Version: [e.g. 3.8.0] Python 3.9.2
  • openapi-python-client version [e.g. 0.1.0] 0.10.1

Additional context I saw the generated model for ModelFromAllOf in end_to_end_tests\golden-record\my_test_api_client\models\model_from_all_of.py. Tests are not full?

Also I have a question about allOf implementation: Why allOf sub-models are turned into attributes of the allOf model? Seems like allOf is a job for inheritance. I think it seems more obvious. Are there blockers out there to implement it this way?

For example, I was expecting something like:

class ModelFromAllOf(AllOfSubModel, AnotherAllOfSubModel):
...

Instead

class ModelFromAllOf:
   a_sub_property: Union[Unset, str] = UNSET
   another_sub_property: Union[Unset, str] = UNSET
...

mtovts avatar Aug 09 '21 18:08 mtovts

Hey @mtovts, what's actually happening here is that a model which is allOf a single other model is considered to just be "the same" as that model. It was a feature request a while back to not generate the effectively duplicate models with just other names and instead treat that as aliases. What should happen is if you have a route which uses Cat, the generated code will use Pet. I think this feature request was because some frameworks auto-generate duplicate models with allOf.

This could probably be more clear to consumers if we did a type alias instead—something like Cat = Pet and then still use Cat everywhere. Is that a feature you'd be interested in?

As far as inheritance, there are a few reasons not to use it but the biggest one is that it breaks whenever you inherit optional parameters and need to declare required ones as the order is enforced at runtime.

dbanty avatar Dec 18 '21 21:12 dbanty

My interpretation of allOf is that it means it's a type combining all the fields of the referenced models—so this is working properly. We won't be using inheritance in this project because it causes too many unexpected problems.

dbanty avatar Jul 08 '23 17:07 dbanty