-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
[Python] remove default value #21736
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?
Conversation
applying defaults on the client side is incorrect, so this change removes them. ref: https://swagger.io/docs/specification/v3_0/describing-parameters/#default-parameter-values
dd261b2
to
eb37b1a
Compare
thanks for the PR there are use cases in which users only generate the models so I think we need to keep the default values for those use cases |
Thank you for your feedback. Default values defined in OpenAPI are supposed to be handled by the server, not by the client. Having defaults on the client side risks introducing inconsistencies, especially when the server changes its defaults. Other generators, such as the TypeScript client, also do not assign default values, so removing them in Python would align the behavior across languages. In addition, even in Python, primitive types currently get defaults, but arrays/maps do not. This means the "models only" use case is already not fully supported. Since this change corrects the client behavior to match the OpenAPI specification, I consider it a fix rather than a new breaking change, which is why I have targeted the 7.x branch for this PR. |
what about adding a new rule (e.g. CLEAR_DEFAULT_VALUES) in openapi normalizer to remove default values from schemas? that should work for all generators if users prefer not to have a default value in the output (fyi java client supports default value for array/map as well) |
@wing328 For users who want to utilize default values, we could consider providing a flag like IMPLEMENT_DEFAULT_VALUE. |
I am not completely following the scenario where the one would want to retain them in the model for the case where you only generate the models? Is the purpose for documentation or what is it used for? As the OP highlight it is incorrect for the client to send values based on the default. They should only send the default if they are sure that they want to explicitly send that (i.e., they want to retain that value even if the server rotates its default). I understand keeping it in models as documentation. This so I as a developer can know that the server has a default value, and that that is then something that I should have to consider "are we fine with that value (and following any changes to the default value), or do we want to send something different?". But should this not rather exist as a documentation section in a generated model rather than the value that is sent by default? |
Hi @wing328 Since OpenAPI Generator is meant to generate clients according to the OpenAPI specification, I believe this PR is reasonable and aligns with the spec. I understand the concern that some users might want to use default values on the client side. For now, would it be possible to approve this PR and remove the default values from the client? |
Default values defined in OpenAPI should be handled by the server, and the client should not arbitrarily assign values. Therefore, this PR remove the default values from the Pydantic models.
For members with default values defined in OpenAPI, if no explicit value
is provided, they temporarily hold None and are omitted when sent.
PR checklist
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*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)"fixes #123"
present in the PR description)@cbornet (2017/09) @tomplus (2018/10) @krjakbrjak (2023/02) @fa0311 (2023/10) @multani (2023/10)