-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Add a new Python client generator - python-nextgen #14157
Conversation
modules/openapi-generator/src/main/resources/python-nextgen/model_generic.mustache
Show resolved
Hide resolved
I did some tests and I like this approach. It gives us new features like support for type hints and it is still compatible with older generator (even with python-legacy with aiohttp). Thanks 👍 |
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 have mostly a user perspective. From what I saw, the generated code looked easy to use; so I like the approach.
However, as it stands, the generator didn't generate valid code for the API I tested it on, cf. my inline comments.
modules/openapi-generator/src/main/resources/python-nextgen/api.mustache
Outdated
Show resolved
Hide resolved
{{#-first}} | ||
_response_types_map = { | ||
{{/-first}} | ||
{{^isWildcard}} |
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.
The API spec I tried the generator on defines responses as follows:
responses:
200:
description: Ok
content:
application/json:
schema:
type: array
items:
$ref: '#/components/schemas/SearchProfile'
4XX:
$ref: '#/components/responses/4XX'
default:
$ref: '#/components/responses/InternalError'
The generated snippet looks like this:
_response_types_map = {
200: "List[SearchProfile]",
4XX: "ErrorResponse",
}
Obviously, 4XX
is not a valid literal. And the default response has been omitted.
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've used string as the key to avoid compilation error for the time being.
still need to work on better handling of response status code using wild card.
will do it with separate PR after merging this one.
modules/openapi-generator/src/main/resources/python-nextgen/requirements.mustache
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python-nextgen/rest.mustache
Outdated
Show resolved
Hide resolved
@chludwig-haufe thanks for the feedback. will try to incorporate those this week. |
I ran this on a small spec of mine and generally like the generated code as it is surprisingly readable. On top, the generation is blazing fast. Particularly like the |
The following component produces a broken from_json for requiredTime.
from __future__ import annotations
from inspect import getfullargspec
import pprint
import json
import re # noqa: F401
from typing import Any, List, Optional
from pydantic import BaseModel, Field, StrictInt, StrictStr, ValidationError, validator
from typing import Any, List
from pydantic import StrictStr, Field
QUERYSTATUSREQUIREDTIME_ONE_OF_SCHEMAS = ["int", "none_type"]
class QueryStatusRequiredTime(BaseModel):
# data type: int
__oneof_schema_1: Optional[StrictInt] = None
actual_instance: Any
one_of_schemas: List[str] = Field(QUERYSTATUSREQUIREDTIME_ONE_OF_SCHEMAS, const=True)
class Config:
validate_assignment = True
@validator('actual_instance')
def actual_instance_must_validate_oneof(cls, v):
error_messages = []
match = 0
# validate data type: int
if type(v) is not int:
error_messages.append(f"Error! Input type `{type(v)}` is not `int`")
else:
match += 1
if match > 1:
# more than 1 match
raise ValueError("Multiple matches found when deserializing the JSON string into QueryStatusRequiredTime with oneOf schemas: int, none_type. Details: " + ", ".join(error_messages))
elif match == 0:
# no match
raise ValueError("No match found when deserializing the JSON string into QueryStatusRequiredTime with oneOf schemas: int, none_type. Details: " + ", ".join(error_messages))
else:
return v
@classmethod
def from_dict(cls, obj: dict) -> QueryStatusRequiredTime:
return cls.from_json(json.dumps(obj))
@classmethod
def from_json(cls, json_str: str) -> QueryStatusRequiredTime:
"""Returns the string representation of the model"""
instance = cls()
error_messages = []
match = 0
if match > 1:
# more than 1 match
raise ValueError("Multiple matches found when deserializing the JSON string into QueryStatusRequiredTime with oneOf schemas: int, none_type. Details: " + ", ".join(error_messages))
elif match == 0:
# no match
raise ValueError("No match found when deserializing the JSON string into QueryStatusRequiredTime with oneOf schemas: int, none_type. Details: " + ", ".join(error_messages))
else:
return instance
def to_json(self) -> str:
"""Returns the JSON representation of the actual instance"""
if self.actual_instance is not None:
return self.actual_instance.to_json()
else:
return "null"
def to_dict(self) -> dict:
"""Returns the dict representation of the actual instance"""
if self.actual_instance is not None:
return self.actual_instance.to_dict()
else:
return dict()
def to_str(self) -> str:
"""Returns the string representation of the actual instance"""
return pprint.pformat(self.dict()) |
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.
Sorry, i found these typos while testing
modules/openapi-generator/src/main/resources/python-nextgen/model_anyof.mustache
Outdated
Show resolved
Hide resolved
modules/openapi-generator/src/main/resources/python-nextgen/model_oneof.mustache
Outdated
Show resolved
Hide resolved
Thank you. I will fix those. |
I appreciate how the new structure of the generated client helps with readability. Its easy to at a glance understand where code that controls a given path lives |
@awildturtok thanks for reporting the issue. I'll try to address it in another PR after merging this one. |
@wing328 Great work! Looks really good 👍 Edit: nvm, just found out that there is a docker build and pushed from master 👍 |
Pretty great work! A question for understanding: |
Right, those logics are not yet added to the auto-generated python sdk at the moment. We definitely welcome PRs to add those enhancement to delivery a better experience using OAuth 2.0 with the auto-generated Python SDK. |
Thank you! Using this instead of the default Python generator fixes a known bug with nullable references (#5180):
|
@TimoGuenther saying that the Nullable is working in the
Nullable is not allowed as a keyword adjacent to $ref per the openapi 3.0.3 spec. In 3.1.0 it is allowed adjacent but our tooling cannot yet handle 3.1.0 specs. Per the 3.0.3 spec: |
@spacether, you are right that the child:
allOf:
- $ref: '#/components/schemas/Child'
nullable: true After all:
However, according to the convoluted definition of |
@awildturtok we've added a rule in OpenAPI Normalizer to handle that: #14777 Please give it a try with the latest master and let me know if you're still facing the same issue. |
I'm atm just using the models in conjunction with fastapi.
I think in the sub-models, in this case eg. Happy to provide a reproduction and/or open a separate issue |
@Linus-Boehm Did you try the latest master or release v6.4.0? We've made some improvements but not sure if it will address issue in your use case. Can you also try enabling the option |
Tried this on one of our APIs and like it. One minor thing after first glance - can we remove the |
@philippslang thanks for the feedback. I've filed #15071 to remove it. |
Thanks @wing328 - think there's still a print function import in the generated readme. Also I think there s a bug for float values - it errors when the values in the json are ints (no decimal point). most of json renderers will omit a trailing .0 |
Are you using the latest master? There's a new option to map the number.
The default should work for your use case. |
about readme still referencing the import, it will be removed as part of #15380 |
To test the new python-nextgen generator, please run the following:
git clone --depth 1 -b python-nextgen https://github.com/openapitools/openapi-generator cd openapi-generator ./mvnw clean package -DskipTests -Dmaven.javadoc.skip=true java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g python-nextgen -i https://raw.githubusercontent.com/OpenAPITools/openapi-generator/master/modules/openapi-generator/src/test/resources/3_0/petstore.yaml --additional-properties packageName=openapi_client -o /tmp/openapi_client/
Please give it a try and let us know if you've any feedback by replying to this PR.
Welcome PR for improvements, bug fixes, etc targeting this branch
python-nextgen
and I'll review accordingly.This new generator is still in beta and maybe subject to breaking changes without further notice.
(please ignore the python-legacy test for the time being)
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
(6.3.0) (minor release - breaking changes with fallbacks),7.0.x
(breaking changes without fallbacks)cc @OpenAPITools/generator-core-team