Sort by inheritance first and java all arguments constructor by jpfinne · Pull Request #18219 · OpenAPITools/openapi-generator

jpfinne · 2024-03-24T20:25:43Z

Fix #17542 by adding the additionalProperty sortModelByInheritanceFirst. This is available for all generators.

For the java generators (spring and client), add the option to generate a constructor for all vars: generatedConstructorWithAllArgs (disabled by default).
It can replace the lombok allArgsConstructor annotation (found only in the spring generator)

This PR implements both features because it makes sense to have a predicatable order for all the arguments in the new constructor.

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 [7.5.0] (https://github.com/OpenAPITools/openapi-generator/wiki/Git-Branches): master (upcoming 7.1.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.

Dear technical committee, please review

Java	@bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @martin-mfg (2023/08)
Java Spring	@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)

# Conflicts: # modules/openapi-generator-cli/pom.xml # modules/openapi-generator-core/pom.xml # modules/openapi-generator-gradle-plugin/pom.xml # modules/openapi-generator-maven-plugin/pom.xml # modules/openapi-generator-online/pom.xml # modules/openapi-generator/pom.xml # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java # pom.xml

# Conflicts: # pom.xml

…iOptions

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

wing328 · 2024-03-25T17:49:29Z

please put this on hold before we discuss it this week

jpfinne · 2024-03-26T08:00:08Z

please put this on hold before we discuss it this week

@wing328 Ok. I've created a slack account (jean-paulfinne.slack.com)

wing328 · 2024-03-26T09:50:49Z

can you please join https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g and PM me ?

welshm · 2024-03-26T11:08:44Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java


    // Whether to automatically hardcode params that are considered Constants by OpenAPI Spec
    protected boolean autosetConstants = false;
+//    protected SortByInheritanceFirstBuilder sortByInheritanceFirstBuilder;


Intentionally left commented out?

Sorry, my mistake

welshm · 2024-03-26T11:10:05Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java

            }
        }

+        if (sortModelPropertiesByInheritanceFirst) {


Should these options be exclusive?

Good question. I'll add a comment to clarify the behaviour.

The current behaviour is the following:

sort by properties first

then optionally sort again by required flag (and hopefully keeping the above order for the required properties)

I have a doubt about the validity of the comparator. It should have a secondary order, otherwise there is no guarantee for the final order when there are more than 32 values (See java.util.ComparableTimSort.MIN_MERGE)

if (one.required == another.required) return 0;

Anyway,
In my implementation,

sort by properties last

then optionally sort again by required flag (and hopefully keeping the above order for the required properties)

# Conflicts: # modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java # modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java # samples/server/petstore/java-play-framework-api-package-override/public/openapi.json # samples/server/petstore/java-play-framework-async/public/openapi.json # samples/server/petstore/java-play-framework-controller-only/public/openapi.json # samples/server/petstore/java-play-framework-fake-endpoints-with-security/public/openapi.json # samples/server/petstore/java-play-framework-fake-endpoints/public/openapi.json # samples/server/petstore/java-play-framework-no-bean-validation/public/openapi.json # samples/server/petstore/java-play-framework-no-exception-handling/public/openapi.json # samples/server/petstore/java-play-framework-no-interface/public/openapi.json # samples/server/petstore/java-play-framework-no-nullable/public/openapi.json # samples/server/petstore/java-play-framework-no-wrap-calls/public/openapi.json # samples/server/petstore/java-play-framework/public/openapi.json # samples/server/petstore/java-undertow/src/main/java/org/openapitools/handler/PathHandlerInterface.java # samples/server/petstore/java-undertow/src/main/resources/config/openapi.json

wing328 · 2024-04-05T07:07:26Z

i wonder if you can file a PR just for the following:

For the java generators (spring and client), add the option to generate a constructor for all vars: generatedConstructorWithAllArgs (disabled by default).

we will get that merge first.

wing328 · 2024-04-05T07:09:35Z

Fix #17542 by adding the additionalProperty sortModelByInheritanceFirst. This is available for all generators.

Generally I like the new option but will take some more time to review and test.

wing328 · 2024-04-05T07:13:56Z

.../openapi-generator/src/main/java/org/openapitools/codegen/SortByInheritanceFirstBuilder.java

+import java.util.*;
+import java.util.stream.Collectors;
+
+public class SortByInheritanceFirstBuilder {


would it be possible to add some unit tests for this new class, especially the reorder method?

There is SpringCodegenTest.testAllArgsConstructor_inheritanceFirst

But you're right. As this class is (nearly) a standalone class, we can unit test it.
I will do it.

There is SpringCodegenTest.testAllArgsConstructor_inheritanceFirst

I saw that too 👍

for every new class, we want some unit tests to make sure things are good as a starting point before further modifying it down the road.

wing328 · 2024-04-05T07:28:46Z

.../openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractJavaCodegen.java

+     */
+    protected void handleGenerateConstructorWithAllArgs(CodegenModel codegenModel) {
+        if (this.generatedConstructorWithAllArgs && !codegenModel.vars.isEmpty()) {
+            codegenModel.vendorExtensions.put("generatedConstructorWithAllArgs", true);


why we need this extension?

shouldn't generatedConstructorWithAllArgs set via additionalProperties be enough?

why we need this extension?

shouldn't generatedConstructorWithAllArgs set via additionalProperties be enough?

We don't want two no arg constructors.
(one generated by the default template and one generated by the "all arg" constructor, the arg count being 0)

what about checking allVars to see if it has zero argument or not?

Maybe.

I had an issue with allVars and supportsInheritance.
allVars did not contain the parent vars. I don't reproduce it anymore on master.
Is it something that was fixed recently?

I need to investigate more.

allvars should contain parent vars

don't recall a recent fix.

allVars was invalid for the contract issue_16797.yaml

See #18340

replied to that. works for me in the latest master

Jean-Paul Finne and others added 26 commits December 13, 2023 10:22

fix(java): x-discriminator-value should not produce @JsonTypeName

aef3b64

fix(java): Remove unused getDiscriminatorValue()

4199d11

build at Fednot

77a34df

build at Fednot

53832b0

build at Fednot: skip sonar

ad9959e

scm for release at fednot

eff380a

Merge remote-tracking branch 'origin/master'

8fb89ae

# Conflicts: # pom.xml

build fednot not 7.5.0-FEDNOT-SNAPSHOT

5a29795

build fednot not 7.5.0-FEDNOT-SNAPSHOT

8e83bee

Fix recursion in discriminatorFound

f41adf3

Sort by inheritance first

d55c699

SortByInheritanceFirst + all args constructor

f36505a

Merge remote-tracking branch 'origin/master'

e91cde1

test all vars

ca5b2e6

test all vars

7f1fe3e

generate allArgsConstructor

4791f09

revert temp folder

f70860b

allArgConstructor for java client and spring

4157884

generatedConstructorWithAllArgs and sortModelByInheritanceFirst in cl…

3a73ef9

…iOptions

avoid all arg comment if generatedConstructorWithAllArgs is disabled

d3310a5

generate all args constructor enabled by default (why not?)

8c0521d

rollback custom pom.xml

9263f4c

Merge branch 'master' into sortByInheritanceFirst

b73d813

# Conflicts: # modules/openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

Add tests for GENERATE_CONSTRUCTOR_WILL_ALL_ARGS false

bb61675

remove wrong commit

c11ba47

jpfinne requested a review from jimschubert as a code owner March 25, 2024 17:45

revert wrong commit

78b90bd

welshm reviewed Mar 26, 2024

View reviewed changes

jpfinne added 4 commits April 1, 2024 09:59

remove unused field

2a6e5ef

Make allArgsConstructor disabled by default

6d84e75

Make allArgsConstructor disabled by default

23a97b6

wing328 reviewed Apr 5, 2024

View reviewed changes

Uh oh!

Conversation

jpfinne commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

wing328 commented Mar 25, 2024

Uh oh!

jpfinne commented Mar 26, 2024

Uh oh!

wing328 commented Mar 26, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpfinne Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wing328 commented Apr 5, 2024

Uh oh!

wing328 commented Apr 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jpfinne commented Mar 24, 2024 •

edited

Loading

jpfinne Mar 27, 2024 •

edited

Loading