-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Serialize model query parameters with style=form, explode=true BNCH-18023
#39
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
Conversation
style=form, explode=true BNCH-18023
| _json_type_string: ClassVar[str] = "Dict[str, Any]" | ||
|
|
||
| template: ClassVar[str] = "model_property.pyi" | ||
| is_dict: ClassVar[bool] = True |
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.
not that happy with this, but couldn't think of a better way
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 might want a more broad type here in the future, as arrays and other might need more special handling but honestly this is "good enough" for now.
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 is this for, to tell if the property is a ModelProperty?
Other places on the codegen check if property.template is set to tell if it's a templated (modeled) property (as opposed to a primitive one). Maybe something similar could be used here?
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.
Yeah. Right now it only applies to ModelProperties, not any other type of property.
| _json_type_string: ClassVar[str] = "Dict[str, Any]" | ||
|
|
||
| template: ClassVar[str] = "model_property.pyi" | ||
| is_dict: ClassVar[bool] = True |
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 might want a more broad type here in the future, as arrays and other might need more special handling but honestly this is "good enough" for now.
packyg
left a comment
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 is the style=form, expload=true referenced in the PR title?
| _json_type_string: ClassVar[str] = "Dict[str, Any]" | ||
|
|
||
| template: ClassVar[str] = "model_property.pyi" | ||
| is_dict: ClassVar[bool] = True |
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 is this for, to tell if the property is a ModelProperty?
Other places on the codegen check if property.template is set to tell if it's a templated (modeled) property (as opposed to a primitive one). Maybe something similar could be used here?
| {% set property_name = "json_" + property.python_name if property.template else property.python_name %} | ||
| if {% if not property.required %}not isinstance({{ property_name }}, Unset) and {% endif %}{{ property_name }} is not None: | ||
| {% if property.is_dict %} | ||
| params.update({{ property_name }}) |
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.
Does this do the prefixing as expected?
I think I would expect something like
| params.update({{ property_name }}) | |
| params.update({ | |
| f"{property.name}.{p}": v, for p, v in {{ property_name }}.items() | |
| }) |
in order for it to build the params with e.g. schemaField.Plasmid=seq_XYZ instead of just Plasmid=seq_XYZ
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.
It does not handle the prefixing. According to the OpenAPI spec, the parameter name itself does not show up at all in the serialization.
See https://swagger.io/specification/ under "Style Examples"
Sorry, it's a bit abstruse, it's OpenAPI terminology for how to serialize query parameters. And is currently the default (and only) behavior in |
d5073d9 to
2ad8d29
Compare
|
Commit history got messed up because I changed the base from upstream to |
style=form, explode=true BNCH-18023style=form, explode=true BNCH-18023
04f43f0 to
7424688
Compare
style=form, explode=true BNCH-18023style=form, explode=true BNCH-18023
packyg
left a comment
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.
Just the one thing
| additional_properties: Union[bool, Property] | ||
|
|
||
| template: ClassVar[str] = "model_property.pyi" | ||
| json_is_dict: ClassVar[bool] = True |
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.
You should add this classvar to the parent class to be consistent:
json_is_dict: ClassVar[bool] = FalseIt's not erroring now because Jinja is permissive, but if you were to do property.json_is_dict outside of a template, it would error for non-model properties.
Fixes openapi-generators#295.
Upstream PR: openapi-generators#316
When I pulled this change into
openapi-specsand made the changes in https://github.com/benchling/openapi-specs/pull/138, the generated code looked like this:This should allow us to do schema field queries with by passing in a dict