-
-
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
allArgConstructor for java #18405
allArgConstructor for java #18405
Conversation
run: | | ||
bash bin/generate-samples.sh | ||
# when a sample is deleted, you have to generate it twice for all files to get created | ||
bash bin/generate-samples.sh |
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.
this PR includes lots of changes not belonging to your original PR
can you please submit a new one with just the change for just "allArgConstruct for java"?
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 screwed up the merges.
Is it better now?
…onstructor # Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaCaskServerCodegen.java # modules/openapi-generator/src/main/resources/scala-cask/build.sbt.mustache # modules/openapi-generator/src/main/resources/scala-cask/build.sc.mustache # modules/openapi-generator/src/main/resources/scala-cask/project/plugins.sbt # samples/client/echo_api/java/okhttp-gson-user-defined-templates/docs/PetApiDocumentation.md # samples/client/echo_api/java/okhttp-gson-user-defined-templates/docs/StoreApiDocumentation.md # samples/client/echo_api/java/okhttp-gson-user-defined-templates/docs/UserApiDocumentation.md # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/main/java/org/openapitools/client/api/PetApi.java # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/main/java/org/openapitools/client/api/StoreApi.java # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/main/java/org/openapitools/client/api/UserApi.java # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/test/java/org/openapitools/client/api/PetApiTest.java # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/test/java/org/openapitools/client/api/StoreApiTest.java # samples/client/echo_api/java/okhttp-gson-user-defined-templates/src/test/java/org/openapitools/client/api/UserApiTest.java # samples/client/petstore/java/microprofile-rest-client-3.0-jackson-with-xml/src/main/java/org/openapitools/client/RFC3339DateFormat.java # samples/client/petstore/java/microprofile-rest-client-3.0-jackson/src/main/java/org/openapitools/client/RFC3339DateFormat.java # samples/documentation/html2/.openapi-generator/VERSION # samples/server/petstore/scala-cask/.openapi-generator/VERSION # samples/server/petstore/scala-cask/build.sbt # samples/server/petstore/scala-cask/build.sc # samples/server/petstore/scala-cask/project/plugins.sbt
if (cm.vars.size() + cm.parentVars.size() != cm.allVars.size()) { | ||
once(LOGGER).warn("Unexpected allVars for {} expecting:{} vars. actual:{} vars", cm.name, cm.vars.size() + cm.parentVars.size(), cm.allVars.size()); | ||
} | ||
cm.vendorExtensions.put("allArgsConstructorVars", constructorArgs); |
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.
for extensions, please add x-
as the prefix and use dash case
e.g. x-java-all-args-constructor-vars
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.
done
// vendorExtensions.allArgsConstructorVars should be equivalent to allVars | ||
// but it is not reliable if openapiNormalizer.REFACTOR_ALLOF_WITH_PROPERTIES_ONLY is disabled | ||
if (cm.vars.size() + cm.parentVars.size() != cm.allVars.size()) { | ||
once(LOGGER).warn("Unexpected allVars for {} expecting:{} vars. actual:{} vars", cm.name, cm.vars.size() + cm.parentVars.size(), cm.allVars.size()); |
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 sure if we need this warning as vars+ parentVars may not be equal to allVars when vars and parentVars have overlayed properties (e.g. both child and parent model has a property named id
defined)
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.
Do you have a sample? I don't reach line 789 when I generate all arg constructors using allOfDuplicatedProperties.yaml or allOfMappingDuplicatedProperties.yaml
Even when I add x-parent: true, allVars are still ok (but the code is uncompilable because the overriden getter)
I reach it from SpringCodegenTest.generateAllArgsConstructor() for Cat.
It might be something similar to #18340
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.
if (cm.vars.size() + cm.parentVars.size() != cm.allVars.size()) {
let's step back for a moment. did you encounter such condition before? if yes, can you please share the spec?
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 can find more evidences in #18340
So the merge of parentVars and allVars is needed to ensure that the constructors are correctly generated
modelsAttrs.getImports().add(toAdd); | ||
} | ||
} | ||
} |
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.
fyi above code block refactored into abstract java codegen.
@cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02) @Zomzog (2022/09) @martin-mfg (2023/08)
fyi. i've filed #18538 with resolved merge conflicts and minor change (removing EOL in pojo) |
cc |
closed via #18538 |
All Argument constructor for java client and spring
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
(upcoming 7.5.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)