-
-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
[python] readonly constructors #9409
Conversation
readOnly: true | ||
taste: | ||
type: string | ||
readOnly: false |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
- Can you move it to https://github.com/OpenAPITools/openapi-generator/tree/master/samples/openapi3/client/petstore/python/tests_manual? That's where we store manually written tests. We like to have the tests in tests be the autogen tests with no human written content
- can you add comments describing what each instantiation is testing?
# 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- OneOfSchema
- the chosen oneOf schema in OneOfSchema
- ComplicatedComposed
- 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
@@ -0,0 +1,66 @@ | |||
@classmethod | |||
@convert_js_args_to_python_args | |||
def from_openapi_data(cls, *args, **kwargs): # noqa: E501 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
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
Did you regenerate all the python samples? |
There was a problem hiding this 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!
CIrcleCi errors are unrelated to this PR |
Constructor for readOnly parameters
@spacether
Related to #9296
PR checklist
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.
master
,5.1.x
,6.0.x