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] readonly constructors #9409

Merged
merged 7 commits into from May 11, 2021

Conversation

gbmarc1
Copy link
Contributor

@gbmarc1 gbmarc1 commented May 5, 2021

Constructor for readOnly parameters

@spacether

Related to #9296

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

docs/generators/python.md Outdated Show resolved Hide resolved
readOnly: true
taste:
type: string
readOnly: false
Copy link
Contributor

@spacether spacether May 5, 2021

Choose a reason for hiding this comment

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

This spec example looks great! Thank you for adding it.
What do you think about also including?

  • a parameter that omits defining readOnly and is optional
  • a parameter that omits defining readOnly and is required

That would vet the unset readOnly -> readOnly: false use case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I will do

assert mole.smell == "dirt"
assert mole.taste == "kfc"
assert mole.blind is True
assert mole.touch is False
Copy link
Contributor

@spacether spacether May 5, 2021

Choose a reason for hiding this comment

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

These tests look great, thank you for adding them!
A couple of points.

# includes required parameters that are not readOnly
mole = Mole(smell="dirt")

# includes required parameters that are not readOnly and an optional readOnly false arg
mole = Mole(smell="dirt", taste="kfc")

# passing in required readOnly parameters raises an exception
with self.assertRaises(Exception):
    Mole(smell="dirt", blind=True

# passing in optional readOnly parameters raises an exception
with self.assertRaises(Exception):
    Mole(smell="dirt", touch=True)

# passing in required an optional parameters with readOnly true or false works with from_openapi_data
mole = Mole.from_openapi_data(smell="dirt", taste="kfc", blind=True, touch=False)
  • For your assertRaises, can you check for specific ApiX errors ApiAttributeError etc? It helps to cast all exceptions into our defined exceptions because then our users can capture all OpenApiExceptions or ApiAttributeError or AttributeError because all of those are included in the ApiAttributeError class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the test in https://github.com/OpenAPITools/openapi-generator/tree/master/samples/openapi3/client/petstore/python/test where suppose to the Model class. The auto-generated tests are empty and therefore meaningless. Are you planing to generate tests that are at least a bit useful?

https://github.com/OpenAPITools/openapi-generator/tree/master/samples/openapi3/client/petstore/python/tests_manual
was more to test the endpoints.

Anyway, I can do that if there is a plan for auto-generated tests

Copy link
Contributor

@spacether spacether May 5, 2021

Choose a reason for hiding this comment

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

We have autogen tests in tests for models and apis. They are a scaffold that can be built upon.
The tests in tests_manual also include model and api tests any any other specific tests we want.
We have lots of useful manual tests:

Long term we should improve the tests in tests and auto-generate some positive and negative test cases.
One runs into the issue that for composed schemas, one does not know which oneOf, anyOf, allOf schemas to select when making an example payload, so generating a payload to test with is non-trivial.
For primitive models and object models it's easy and doable. We also run into that issue of we are generating a sample payload for an object model where any property is of type ComposedSchema.

Some effort has been invested in generating some model tests in the python-legacy client, as seen here
But that effort results in broken autogen python tests because our java code doesn't know how to generate composed schema tests, or values for properties that are of type composed schema.

Copy link
Contributor

@spacether spacether May 5, 2021

Choose a reason for hiding this comment

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

For a composed schema one can have complicated use cases like this:

OneOfSchema:
  oneOf:
    - type: object
      properties:
        a:
          type: integer
    - type: object
      properties:
        b:
          type: integer
    - type: object
      properties:
        c:
          type: integer

ComplicatedComposed:
  properties:
    id:
      type: integer
  allOf:
    - OneOfSchema
  oneOf:
    - type: object
      properties:
        d:
          type: integer
    - type: object
      properties:
        e:
          type: integer
    - type: object
      properties:
        d:
          type: integer

What should go in the payload for ComplicatedComposed?
We need one of a,b,c and id, and one of d, e f
So {'a': 1, 'id': 1, 'd': 1} is valid, but it must also validate for each of the schemas:

  1. OneOfSchema
  2. the chosen oneOf schema in OneOfSchema
  3. ComplicatedComposed
  4. the chosen oneOf schema in ComplicatedComposed

And this example doesn't even cover :

  • cases where variables have the same name but different types in schemas
  • parameter may be different types via oneOf definition
  • payloads not meeting formatting or validation constraints
  • schemas that include cyclic references to themself
  • two enum schemas being used and valid values coming from their intersection

@spacether spacether mentioned this pull request May 5, 2021
5 tasks
@@ -0,0 +1,66 @@
@classmethod
@convert_js_args_to_python_args
def from_openapi_data(cls, *args, **kwargs): # noqa: E501
Copy link
Contributor

@spacether spacether May 6, 2021

Choose a reason for hiding this comment

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

What if someone wanted to name an object property from_openapi_data?
How about using _from_openapi_data to reduce the likelihood of a naming collision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess now I change the target branch to master since 5.2 was merged today

@gbmarc1 gbmarc1 changed the base branch from 5.2.x to master May 7, 2021 20:32
@spacether
Copy link
Contributor

Did you regenerate all the python samples?
In the drone.io CI tests I am seeing uncommitted changes. Can you regenerate the samples?

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

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

This looks great; thank you for adding this feature!

@spacether
Copy link
Contributor

CIrcleCi errors are unrelated to this PR

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.

None yet

2 participants