Add constructor with required parameter for Spring by alucas · Pull Request #14822 · OpenAPITools/openapi-generator

alucas · 2023-02-27T00:30:53Z

Fix #9789

Add constructor with required parameter for Spring

@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog

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: 🚨 ./bin/generate-samples.sh update a lot .py / .rb / .rs that have nothing to do with this PR, I dit not include them
```
./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
In case you are adding a new generator, run the following additional script : 🚨 The script name is ./bin/utils/ensure-up-to-date(without .sh), this template is outdated
```
./bin/utils/ensure-up-to-date.sh
```
Commit all changed files.
File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks) 🚨 The next version is 6.5.0 right ? This template is outdated.
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

Thanks for the PR!

Just some minor suggestions

bin/generate-samples.sh

welshm · 2023-02-27T02:49:01Z

modules/openapi-generator/src/test/resources/3_0/spring/issue_9789.yml

Can you make some of these nullable to excercise the nullability check as well?

I added a nullable & required parameter 🙂

samples/client/petstore/spring-cloud-async/src/main/java/org/openapitools/model/Pet.java

borsch

@alucas thanks for the PR. Unfortunately there is a big think which block us from this being implemented earlier - inheritance. Currently your changes will work perfectly fine until you have inheritance in your models, so if you really want this to be merged you have to do you changes with this in mind

alucas · 2023-02-27T23:47:51Z

@alucas thanks for the PR. Unfortunately there is a big think which block us from this being implemented earlier - inheritance. Currently your changes will work perfectly fine until you have inheritance in your models, so if you really want this to be merged you have to do you changes with this in mind

@borsch isn't inheritance implemented with ‘allOf:‘ ? Because I have such case in my tests. Are you thinking of something else ?

borsch · 2023-02-28T08:38:08Z

@alucas you can check generated sample from configuration bin/config/spring-boot-delegate-j8.yaml there is generated model Cat extends Animal

alucas · 2023-02-28T10:33:04Z

@borsch Thx for pointing that problem, the generate code didn't even compile. I think I fixed it. We should probably add the execution of those generated tests in the CI ?

.../openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java

modules/openapi-generator/src/test/resources/3_0/petstore.yaml

Fix OpenAPITools#9789

borsch · 2023-03-01T12:49:21Z

@alucas I found that there was one issue related to code generation due to which your custom test was running against incorrect classes. I've fixed this

Once all pipelines would be completed I'll merge this PR
Thanks for contribution

welshm · 2023-03-01T13:45:26Z

samples/server/petstore/springboot/src/test/java/org/openapitools/JacksonTest.java

+    }
+
+    @Test
+    void shouldDeserializeObjectWithInheritanceWithoutLoss() throws JsonProcessingException {


👏 Thank you for adding these tests! Merci beaucoup pour le addition!

OlliL · 2023-05-06T19:15:37Z

Marking the default Constructor deprecated is not a good thing to do imho. A POJO requires a default constructor. Otherwise most if not all those mapping frameworks out there won't work. So there is no point in marking it deprecated. It just polutes the code with unnecessary warnings. The constructor with only required parameters adds no benefit as there is for example no null check in it - so it gives you the impression to make things safer but it just does not. With a default constructor available and a constructor with required parameters but without null checks in it no "is required" is enforced. Removing the default constructor would break the Pojo spec so it will never be gone anyway.... so please remove that pointless deprecated annotation....

alucas · 2023-05-07T03:45:17Z

Hello,

You're right, there is no plan to remove the default constructor, that would be breaking.

Can you give us some example of warnings poluting your code ? Are they from the mapping framework ?

Note that you can disable this feature with the generatedConstructorWithRequiredArgs config

borsch · 2023-05-07T08:12:10Z

@alucas I believe it's just Java compiler warnings saying that you use deprecated API(deprecated constructor)

OlliL · 2023-05-07T08:40:22Z

yeah - its static code analysis tools like Sonar which start complaining for using those deprecated constructors. Yes, the warning could be turned of, or the code could be changed but as I tried to explain the new constructor does not add that much safety imho so i don't see that much of a point to change all the affected code just to get rid of the deprecation warning.

Thanks for letting me know about the config switch. I could have checked the documentation first 😄

MelleD · 2023-05-07T08:46:26Z

Don’t understand why Sonar should check generated code.

And off course this constructor add more safety and increase the readability also for client side code, because then you know what is mandatory and what not. That’s a old best practice to add mandatory fields into the constructor

alucas force-pushed the feat/spring-add-constructor branch from d5ad550 to a6bf771 Compare February 27, 2023 00:46

welshm reviewed Feb 27, 2023

View reviewed changes

alucas force-pushed the feat/spring-add-constructor branch from a6bf771 to 890ef92 Compare February 27, 2023 08:13

borsch requested changes Feb 27, 2023

View reviewed changes

alucas force-pushed the feat/spring-add-constructor branch from 890ef92 to 891919f Compare February 27, 2023 17:58

alucas force-pushed the feat/spring-add-constructor branch from 891919f to d390242 Compare February 28, 2023 10:27

alucas force-pushed the feat/spring-add-constructor branch from d390242 to 1a6c611 Compare February 28, 2023 14:37

borsch reviewed Feb 28, 2023

View reviewed changes

.../openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java Outdated Show resolved Hide resolved

alucas force-pushed the feat/spring-add-constructor branch 2 times, most recently from e367315 to 855e9aa Compare February 28, 2023 22:53

borsch reviewed Mar 1, 2023

View reviewed changes

modules/openapi-generator/src/test/resources/3_0/petstore.yaml Outdated Show resolved Hide resolved

alucas force-pushed the feat/spring-add-constructor branch from 855e9aa to 32faca3 Compare March 1, 2023 11:19

Add constructor with required parameter for spring

5f363b8

Fix OpenAPITools#9789

alucas force-pushed the feat/spring-add-constructor branch from 32faca3 to 5f363b8 Compare March 1, 2023 11:26

borsch approved these changes Mar 1, 2023

View reviewed changes

[Java][Spring] constructor with required args

0af91b8

borsch added the Server: Spring label Mar 1, 2023

borsch added this to the 6.5.0 milestone Mar 1, 2023

borsch merged commit e1ab25c into OpenAPITools:master Mar 1, 2023

welshm reviewed Mar 1, 2023

View reviewed changes

alucas deleted the feat/spring-add-constructor branch March 3, 2023 17:44

borsch mentioned this pull request Mar 12, 2023

[BUG] issue with Spring required args constructor #14934

Closed

6 tasks

Kilrath mentioned this pull request May 12, 2023

[Java][Spring] Don't generate constructors if already there (with lombok) #15494

Closed

Uh oh!

Conversation

alucas commented Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

welshm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

welshm Feb 27, 2023

Choose a reason for hiding this comment

Uh oh!

alucas Feb 27, 2023 • 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!

Uh oh!

borsch left a comment

Choose a reason for hiding this comment

Uh oh!

alucas commented Feb 27, 2023

Uh oh!

borsch commented Feb 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alucas commented Feb 28, 2023

Uh oh!

Uh oh!

Uh oh!

borsch commented Mar 1, 2023

Uh oh!

welshm Mar 1, 2023

Choose a reason for hiding this comment

Uh oh!

OlliL commented May 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alucas commented May 7, 2023

Uh oh!

borsch commented May 7, 2023

Uh oh!

OlliL commented May 7, 2023

Uh oh!

MelleD commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alucas commented Feb 27, 2023 •

edited

Loading

alucas Feb 27, 2023 •

edited

Loading

borsch commented Feb 28, 2023 •

edited

Loading

OlliL commented May 6, 2023 •

edited

Loading

MelleD commented May 7, 2023 •

edited

Loading