Skip to content
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] Add enum support when building default values for model properties when using $ref #18796

Merged

Conversation

welshm
Copy link
Contributor

@welshm welshm commented May 30, 2024

Add in better processing for enums in Python when they are a $ref

  • Currently, doesn't properly populate default values so when models are de/serialized, the enum value is dropped.
  • Adds support for checking the $ref when getting the default value for a Schema
  • Updates how enum variable names are referenced
    • Update DefaultCodegen.java with public String toEnumDefaultValue(CodegenProperty property, String value)
    • Allows checking if the property is an enum reference - and if so, handle it as a first class enum model in Python
    • Otherwise, use the inline variable value (as it does today)

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    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.
  • File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@welshm welshm changed the title Add enum support when building default values for model properties Add enum support when building default values for model properties when using $ref May 30, 2024
@welshm welshm marked this pull request as ready for review May 30, 2024 13:42
@welshm
Copy link
Contributor Author

welshm commented May 30, 2024

@wing328 @spacether @krjakbrjak I don't see Flask/Connexion as being supported (just FastAPI) but this impacts all of Python.

@welshm welshm changed the title Add enum support when building default values for model properties when using $ref [Python] Add enum support when building default values for model properties when using $ref May 30, 2024
@@ -6750,7 +6762,7 @@ public void updateCodegenPropertyEnum(CodegenProperty var) {
}
}
if (enumName != null) {
var.defaultValue = toEnumDefaultValue(enumName, var.datatypeWithEnum);
var.defaultValue = toEnumDefaultValue(var, enumName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi @OpenAPITools/generator-core-team

@@ -1,4 +1,4 @@
generatorName: python-flask
outputDir: samples/server/petstore/python-flask
outputDir: samples/server/petstore/2_0/python-flask
inputSpec: modules/openapi-generator/src/test/resources/2_0/python-flask/petstore.yaml
templateDir: modules/openapi-generator/src/main/resources/python-flask
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't test 2.0 spec so no need to keep this sample folder and yaml config file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - thanks!

@welshm welshm requested a review from jimschubert as a code owner June 5, 2024 14:00
@welshm
Copy link
Contributor Author

welshm commented Jun 5, 2024

Something went wrong doing the rebase - will fix

@welshm
Copy link
Contributor Author

welshm commented Jun 5, 2024

The code is now correct, but the commit history is a bit mangled. I can open another PR if that's a problem.

Let me know! @wing328

@welshm welshm requested a review from wing328 June 6, 2024 14:46
@welshm welshm force-pushed the welshm.add_enum_support_python_codegen branch from 23300fb to 1d8dd73 Compare June 6, 2024 14:51
@wing328
Copy link
Member

wing328 commented Jun 8, 2024

thanks for the PR. the commit history looks ok as we will squash the commits during merge.

tested python client with the new test schemas and the result is good: #18882

@wing328 wing328 merged commit a5a9958 into OpenAPITools:master Jun 8, 2024
25 checks passed
@wing328 wing328 added this to the 7.7.0 milestone Jun 8, 2024
@wing328
Copy link
Member

wing328 commented Jun 8, 2024

fyi. filed #18883 to add the tests in python client as well

@wing328
Copy link
Member

wing328 commented Jun 12, 2024

@welshm when you've time, can you please PM me via Slack? thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants