-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
enhance python openapi generator #22600
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
base: master
Are you sure you want to change the base?
Changes from all commits
cbb6665
dec179a
bf5eff7
ddad4aa
5fd3663
81c2226
bc25f93
039a1bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,9 +1,6 @@ | ||||||||||
| # coding: utf-8 | ||||||||||
|
|
||||||||||
| {{>partial_header}} | ||||||||||
|
|
||||||||||
| from typing import Any, Optional | ||||||||||
| from typing_extensions import Self | ||||||||||
| from typing import Any, Optional, Self | ||||||||||
|
|
||||||||||
| class OpenApiException(Exception): | ||||||||||
| """The base exception class for all OpenAPIExceptions""" | ||||||||||
|
|
@@ -95,9 +92,9 @@ class ApiKeyError(OpenApiException, KeyError): | |||||||||
| class ApiException(OpenApiException): | ||||||||||
|
|
||||||||||
| def __init__( | ||||||||||
| self, | ||||||||||
| status=None, | ||||||||||
| reason=None, | ||||||||||
| self, | ||||||||||
| status=None, | ||||||||||
| reason=None, | ||||||||||
| http_resp=None, | ||||||||||
| *, | ||||||||||
| body: Optional[str] = None, | ||||||||||
|
|
@@ -119,14 +116,14 @@ class ApiException(OpenApiException): | |||||||||
| self.body = http_resp.data.decode('utf-8') | ||||||||||
| except Exception: | ||||||||||
| pass | ||||||||||
| self.headers = http_resp.headers | ||||||||||
| self.headers = http_resp.getheaders() | ||||||||||
|
|
||||||||||
| @classmethod | ||||||||||
| def from_response( | ||||||||||
| cls, | ||||||||||
| *, | ||||||||||
| http_resp, | ||||||||||
| body: Optional[str], | ||||||||||
| cls, | ||||||||||
| *, | ||||||||||
| http_resp, | ||||||||||
| body: Optional[str], | ||||||||||
| data: Optional[Any], | ||||||||||
| ) -> Self: | ||||||||||
| if http_resp.status == 400: | ||||||||||
|
|
@@ -160,11 +157,8 @@ class ApiException(OpenApiException): | |||||||||
| error_message += "HTTP response headers: {0}\n".format( | ||||||||||
| self.headers) | ||||||||||
|
|
||||||||||
| if self.body: | ||||||||||
| error_message += "HTTP response body: {0}\n".format(self.body) | ||||||||||
|
|
||||||||||
| if self.data: | ||||||||||
| error_message += "HTTP response data: {0}\n".format(self.data) | ||||||||||
| if self.data or self.body: | ||||||||||
| error_message += "HTTP response body: {0}\n".format(self.data or self.body) | ||||||||||
|
Comment on lines
+160
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Using if self.data is not None or self.body:
error_message += "HTTP response body: {0}\n".format(self.data if self.data is not None else self.body)Prompt for AI agents
Suggested change
|
||||||||||
|
|
||||||||||
| return error_message | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -9,169 +9,35 @@ import re # noqa: F401 | |||||||||
| {{#vendorExtensions.x-py-model-imports}} | ||||||||||
| {{{.}}} | ||||||||||
| {{/vendorExtensions.x-py-model-imports}} | ||||||||||
| from typing import Union, Any, List, Set, TYPE_CHECKING, Optional, Dict | ||||||||||
| from typing_extensions import Literal, Self | ||||||||||
| from pydantic import Field | ||||||||||
| from typing import Union, Any, List, Set, TYPE_CHECKING, Optional, Dict, Literal, Self | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Unused imports added/left over; symbols like Literal, Self, getfullargspec, json, pprint, TYPE_CHECKING, etc. are not used in the generated anyOf model, causing unused-import lint failures. Prompt for AI agents |
||||||||||
| from pydantic import Field, RootModel | ||||||||||
|
|
||||||||||
| {{#lambda.uppercase}}{{{classname}}}{{/lambda.uppercase}}_ANY_OF_SCHEMAS = [{{#anyOf}}"{{.}}"{{^-last}}, {{/-last}}{{/anyOf}}] | ||||||||||
|
|
||||||||||
| class {{classname}}({{#parent}}{{{.}}}{{/parent}}{{^parent}}BaseModel{{/parent}}): | ||||||||||
|
|
||||||||||
| class {{classname}}(RootModel[Union[{{#anyOf}}{{.}}{{^-last}}, {{/-last}}{{/anyOf}}]]): | ||||||||||
| """ | ||||||||||
| {{{description}}}{{^description}}{{{classname}}}{{/description}} | ||||||||||
| """ | ||||||||||
|
|
||||||||||
| {{#composedSchemas.anyOf}} | ||||||||||
| # data type: {{{dataType}}} | ||||||||||
| {{vendorExtensions.x-py-name}}: {{{vendorExtensions.x-py-typing}}} | ||||||||||
| {{/composedSchemas.anyOf}} | ||||||||||
| if TYPE_CHECKING: | ||||||||||
| actual_instance: Optional[Union[{{#anyOf}}{{{.}}}{{^-last}}, {{/-last}}{{/anyOf}}]] = None | ||||||||||
| else: | ||||||||||
| actual_instance: Any = None | ||||||||||
| any_of_schemas: Set[str] = { {{#anyOf}}"{{.}}"{{^-last}}, {{/-last}}{{/anyOf}} } | ||||||||||
|
|
||||||||||
| model_config = { | ||||||||||
| "validate_assignment": True, | ||||||||||
| "protected_namespaces": (), | ||||||||||
| } | ||||||||||
| {{#discriminator}} | ||||||||||
|
|
||||||||||
| discriminator_value_class_map: Dict[str, str] = { | ||||||||||
| {{#children}} | ||||||||||
| '{{^vendorExtensions.x-discriminator-value}}{{name}}{{/vendorExtensions.x-discriminator-value}}{{#vendorExtensions.x-discriminator-value}}{{{vendorExtensions.x-discriminator-value}}}{{/vendorExtensions.x-discriminator-value}}': '{{{classname}}}'{{^-last}},{{/-last}} | ||||||||||
| {{/children}} | ||||||||||
| } | ||||||||||
| {{/discriminator}} | ||||||||||
|
|
||||||||||
| def __init__(self, *args, **kwargs) -> None: | ||||||||||
| if args: | ||||||||||
| if len(args) > 1: | ||||||||||
| raise ValueError("If a position argument is used, only 1 is allowed to set `actual_instance`") | ||||||||||
| if kwargs: | ||||||||||
| raise ValueError("If a position argument is used, keyword arguments cannot be used.") | ||||||||||
| super().__init__(actual_instance=args[0]) | ||||||||||
| else: | ||||||||||
| super().__init__(**kwargs) | ||||||||||
|
|
||||||||||
| @field_validator('actual_instance') | ||||||||||
| def actual_instance_must_validate_anyof(cls, v): | ||||||||||
| {{#isNullable}} | ||||||||||
| if v is None: | ||||||||||
| return v | ||||||||||
|
|
||||||||||
| {{/isNullable}} | ||||||||||
| instance = {{{classname}}}.model_construct() | ||||||||||
| error_messages = [] | ||||||||||
| {{#composedSchemas.anyOf}} | ||||||||||
| # validate data type: {{{dataType}}} | ||||||||||
| {{#isContainer}} | ||||||||||
| try: | ||||||||||
| instance.{{vendorExtensions.x-py-name}} = v | ||||||||||
| return v | ||||||||||
| except (ValidationError, ValueError) as e: | ||||||||||
| error_messages.append(str(e)) | ||||||||||
| {{/isContainer}} | ||||||||||
| {{^isContainer}} | ||||||||||
| {{#isPrimitiveType}} | ||||||||||
| try: | ||||||||||
| instance.{{vendorExtensions.x-py-name}} = v | ||||||||||
| return v | ||||||||||
| except (ValidationError, ValueError) as e: | ||||||||||
| error_messages.append(str(e)) | ||||||||||
| {{/isPrimitiveType}} | ||||||||||
| {{^isPrimitiveType}} | ||||||||||
| if not isinstance(v, {{{dataType}}}): | ||||||||||
| error_messages.append(f"Error! Input type `{type(v)}` is not `{{{dataType}}}`") | ||||||||||
| else: | ||||||||||
| return v | ||||||||||
|
|
||||||||||
| {{/isPrimitiveType}} | ||||||||||
| {{/isContainer}} | ||||||||||
| {{/composedSchemas.anyOf}} | ||||||||||
| if error_messages: | ||||||||||
| # no match | ||||||||||
| raise ValueError("No match found when setting the actual_instance in {{{classname}}} with anyOf schemas: {{#anyOf}}{{{.}}}{{^-last}}, {{/-last}}{{/anyOf}}. Details: " + ", ".join(error_messages)) | ||||||||||
| else: | ||||||||||
| return v | ||||||||||
|
|
||||||||||
| @classmethod | ||||||||||
| def from_dict(cls, obj: Dict[str, Any]) -> Self: | ||||||||||
| return cls.from_json(json.dumps(obj)) | ||||||||||
|
|
||||||||||
| @classmethod | ||||||||||
| def from_json(cls, json_str: str) -> Self: | ||||||||||
| """Returns the object represented by the json string""" | ||||||||||
| instance = cls.model_construct() | ||||||||||
| {{#isNullable}} | ||||||||||
| if json_str is None: | ||||||||||
| return instance | ||||||||||
|
|
||||||||||
| {{/isNullable}} | ||||||||||
| error_messages = [] | ||||||||||
| {{#composedSchemas.anyOf}} | ||||||||||
| {{#isContainer}} | ||||||||||
| # deserialize data into {{{dataType}}} | ||||||||||
| try: | ||||||||||
| # validation | ||||||||||
| instance.{{vendorExtensions.x-py-name}} = json.loads(json_str) | ||||||||||
| # assign value to actual_instance | ||||||||||
| instance.actual_instance = instance.{{vendorExtensions.x-py-name}} | ||||||||||
| return instance | ||||||||||
| except (ValidationError, ValueError) as e: | ||||||||||
| error_messages.append(str(e)) | ||||||||||
| {{/isContainer}} | ||||||||||
| {{^isContainer}} | ||||||||||
| {{#isPrimitiveType}} | ||||||||||
| # deserialize data into {{{dataType}}} | ||||||||||
| try: | ||||||||||
| # validation | ||||||||||
| instance.{{vendorExtensions.x-py-name}} = json.loads(json_str) | ||||||||||
| # assign value to actual_instance | ||||||||||
| instance.actual_instance = instance.{{vendorExtensions.x-py-name}} | ||||||||||
| return instance | ||||||||||
| except (ValidationError, ValueError) as e: | ||||||||||
| error_messages.append(str(e)) | ||||||||||
| {{/isPrimitiveType}} | ||||||||||
| {{^isPrimitiveType}} | ||||||||||
| # {{vendorExtensions.x-py-name}}: {{{vendorExtensions.x-py-typing}}} | ||||||||||
| try: | ||||||||||
| instance.actual_instance = {{{dataType}}}.from_json(json_str) | ||||||||||
| return instance | ||||||||||
| except (ValidationError, ValueError) as e: | ||||||||||
| error_messages.append(str(e)) | ||||||||||
| {{/isPrimitiveType}} | ||||||||||
| {{/isContainer}} | ||||||||||
| {{/composedSchemas.anyOf}} | ||||||||||
|
|
||||||||||
| if error_messages: | ||||||||||
| # no match | ||||||||||
| raise ValueError("No match found when deserializing the JSON string into {{{classname}}} with anyOf schemas: {{#anyOf}}{{{.}}}{{^-last}}, {{/-last}}{{/anyOf}}. 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 None: | ||||||||||
| return "null" | ||||||||||
| root: Union[{{#anyOf}}{{.}}{{^-last}}, {{/-last}}{{/anyOf}}] = Field( | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Nullable anyOf schemas now reject null because the required root Union field no longer includes Optional/nullable handling. Prompt for AI agents
Suggested change
|
||||||||||
| ...{{#discriminator}}, discriminator="{{discriminatorName}}"{{/discriminator}} | ||||||||||
| ) | ||||||||||
|
|
||||||||||
| if hasattr(self.actual_instance, "to_json") and callable(self.actual_instance.to_json): | ||||||||||
| return self.actual_instance.to_json() | ||||||||||
| else: | ||||||||||
| return json.dumps(self.actual_instance) | ||||||||||
| def __getattr__(self, name): | ||||||||||
| """ | ||||||||||
| Delegate attribute access to the root model if the attribute | ||||||||||
| doesn't exist on the main class. | ||||||||||
| """ | ||||||||||
|
|
||||||||||
| def to_dict(self) -> Optional[Union[Dict[str, Any], {{#anyOf}}{{.}}{{^-last}}, {{/-last}}{{/anyOf}}]]: | ||||||||||
| """Returns the dict representation of the actual instance""" | ||||||||||
| if self.actual_instance is None: | ||||||||||
| return None | ||||||||||
| if name in self.__dict__: | ||||||||||
| return super().__getattribute__(name) | ||||||||||
|
|
||||||||||
| if hasattr(self.actual_instance, "to_dict") and callable(self.actual_instance.to_dict): | ||||||||||
| return self.actual_instance.to_dict() | ||||||||||
| else: | ||||||||||
| return self.actual_instance | ||||||||||
| root = self.__dict__.get('root') | ||||||||||
| if root is not None: | ||||||||||
| return getattr(root, name) | ||||||||||
|
|
||||||||||
| def to_str(self) -> str: | ||||||||||
| """Returns the string representation of the actual instance""" | ||||||||||
| return pprint.pformat(self.model_dump()) | ||||||||||
| raise AttributeError(f"'{self.__class__.__name__}' object has no attribute '{name}'") | ||||||||||
|
|
||||||||||
| {{#vendorExtensions.x-py-postponed-model-imports.size}} | ||||||||||
| {{#vendorExtensions.x-py-postponed-model-imports}} | ||||||||||
|
|
||||||||||
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.
P1: Downloaded file path uses unsanitized Content-Disposition filename, allowing path traversal writes outside temp directory.
Prompt for AI agents
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.
content_disposition = response.headers.get("Content-Disposition")
VS
content_disposition = response.getheader("Content-Disposition")
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.
@padznich The difference between these two approaches is about the API used to access HTTP headers:
response.headers.get("Content-Disposition")(old code)headersas a dict-like objectresponse.headerswas an HTTPHeaderDictresponse.getheader("Content-Disposition")(new code)getheader()andgetheaders()methodsYour PR correctly updates this to
getheader()to be compatible with urllib3 >= 2.1.0 (as specified in your pyproject.mustache). This is the right change.However, my P1 review comment is still valid — it's about a security issue, not the API change:
The code extracts a filename from the
Content-Dispositionheader and uses it directly in a file path:If a malicious server returns
Content-Disposition: attachment; filename="../../etc/passwd", this could write outside the temp directory. You should sanitize the filename:This prevents path traversal attacks regardless of whether you use
.get()or.getheader().