Skip to content
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

[python-experimental] Allow models to have properties of type self #4888

Merged

Conversation

spacether
Copy link
Contributor

@spacether spacether commented Dec 29, 2019

This updates the python-experimental generator to allow a model to have properties of type self.
An example model is a Player which stores an enemyPlayer also of type player.

See this use case example:

  Player:
    type: object
    required:
      - name
    properties:
      name:
        type: string
      enemyPlayer:
        type: object
        $ref: '#/components/schemas/Player'

And our python code demonstrating that it works:

        jim = petstore_api.Player(
            name="Jim",
            enemy_player=petstore_api.Player(name="Sam")
        )

This was fixed by converting class.openapi_types into a class method, which prevents the openapi_types code from being run at import time.
We also removed the module name from the dataType of the property if the property in self's model.

Test Verification

A test has been added demonstrating that this works at https://github.com/spacether/openapi-generator/blob/issue_4885_allow_self_properties/samples/client/petstore/python-experimental/test/test_player.py#L32

If merged, this will resolve:
#4885

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Python technical-committee:
@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11) @spacether (2019/11)

@ethan92429

@eak24
Copy link
Contributor

eak24 commented Jan 2, 2020

@spacether I just ran into another issue using this PR. It is in the resolution of the discriminator. If a model specifies that it is a grandchild of the base model, the discriminator for that grandkid won't be found because the client only looks in the discriminators of the base. The way around this would be to specify every subtype possible, which would be extraordinarily ugly for a large tree that inherits all from one main type (a common pattern for my company). Instead, I think the client should recursively search the discriminator tree to find the correct type. Specifically, here we should look for a match recursively down the discriminator tree. Would that still honor the OAS?

@spacether
Copy link
Contributor Author

@ethan92429 I won't know until I see an example of your use case. Can you create and share a sample spec?

@eak24
Copy link
Contributor

eak24 commented Jan 2, 2020

Example spec:

components:
  schemas:
    Animal:
      type: object
      required:
      - type_key
      properties:
        type_key:
          type: string
      mapping:
        pet: '#/components/schemas/Pet'
    Pet:
      type: object
      properties:
        type_key:
          type: string
      discriminator:
        propertyName: type_key
        mapping:
          cat: '#/components/schemas/Cat'
    Cat:
      type: object
      properties:
        type_key:
          age: number

Now if the client tries to deserialize something that looks like {'type_key' : 'cat'} and it is known to be of type 'Animal', it will just not find the correct discriminated class (the Cat class). To work correctly, it would need to recursively search through all the discriminators of Pet as well. However, I'm not sure this is supported in OAS... Is the right way to solve this by making all my discriminators class-specific? For instance, changing 'type_key' to 'animal_type_key' and 'pet_type_key', respectively? That way an instnance of a Cat would specify both 'animal_type_key' and 'pet_type_key'... Thoughts?

@eak24
Copy link
Contributor

eak24 commented Jan 2, 2020

@spacether an example of code that would support this behavior: eak24@f606cfb

@spacether
Copy link
Contributor Author

Thank you for making that sample. It looks like this issue is unrelated to properties of type self. To help us fix it can you please post it as a new issue?
Should Cat allOf inherit Pet? Shoiuld Pet allOf inherent Animal? Why not combine the Pet and Animal classes?

@eak24
Copy link
Contributor

eak24 commented Jan 2, 2020

Yes - all inherit Animal and Cat inherits Pet. This is just the simplest example I could come up with to illustrate the issue - the spec that I'm using is 77,000 lines of JSON, so posting that wouldn't have been helpful :-) . There are many, many examples of this type of inheritance in our codebase. In our homegrown clients, we have a single 'discriminator mapping' dictionary that can relate any of our type mapping key values to their respective type. This way we flatten the type graph and achieve good performance. It would be really neat if there was some way to support this use case. I can write up an issue, but TBH, I'm not sure what the right solution is...

@spacether
Copy link
Contributor Author

Your posted diff is the right solution. I understand your use case better now. Let's move the discussion there or to an issue if you make it. Either is good.

@eak24
Copy link
Contributor

eak24 commented Jan 2, 2020

#4912 is the issue I posted. Moving discussion there.

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM

@wing328 wing328 merged commit cbc1254 into OpenAPITools:master Jan 10, 2020
@spacether spacether deleted the issue_4885_allow_self_properties branch January 10, 2020 13:29
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jan 11, 2020
* master: (187 commits)
  [core] Initial FeatureSet structures and definitions (OpenAPITools#3614)
  Add Cisco to the user list (OpenAPITools#4971)
  comment out php slim4 in ensure-up-to-date
  update samples
  [Python] Allow models to have properties of type self (OpenAPITools#4888)
  Add npmRepository option to javascript generators (OpenAPITools#4956)
  [Slim4] Add ref support to Data Mocker (OpenAPITools#4932)
  Fix auto-labeler for jax-rs (OpenAPITools#4943)
  [doc] full generator details (OpenAPITools#4941)
  comment out python flask 2 test (OpenAPITools#4949)
  [jaxrs-spec][quarkus] update to version 1.1.1.Final (OpenAPITools#4935)
  [cli] Full config help details (OpenAPITools#4928)
  Add RequestFile to typescript-node model template (OpenAPITools#4903)
  [csharp] enum suffix changes enumValueNameSuffix to enumValueSuffix (OpenAPITools#4927)
  [C#] allow customization of generated enum suffixes (OpenAPITools#4301)
  [Kotlin] Correct isInherited flag for Kotlin generators (OpenAPITools#4254)
  [Rust Server] Fix panic handling headers (OpenAPITools#4877)
  Initial CODEOWNERS (OpenAPITools#4924)
  [scala] Support for Set when array has uniqueItems=true (OpenAPITools#4926)
  remove nodejs server samples, scripts (OpenAPITools#4919)
  ...
@wing328 wing328 changed the title [Python] Allow models to have properties of type self [python-experimental] Allow models to have properties of type self Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants