-
-
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
Add constructor with required parameter for Spring #14822
Add constructor with required parameter for Spring #14822
Conversation
d5ad550
to
a6bf771
Compare
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.
Thanks for the PR!
Just some minor suggestions
param1: | ||
type: string | ||
param3: | ||
type: string | ||
param2: | ||
type: string |
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.
Can you make some of these nullable to excercise the nullability check as well?
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.
I added a nullable & required parameter 🙂
samples/client/petstore/spring-cloud-async/src/main/java/org/openapitools/model/Pet.java
Show resolved
Hide resolved
a6bf771
to
890ef92
Compare
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.
@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
890ef92
to
891919f
Compare
@borsch isn't inheritance implemented with ‘allOf:‘ ? Because I have such case in my tests. Are you thinking of something else ? |
@alucas you can check generated sample from configuration |
891919f
to
d390242
Compare
@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 ? |
d390242
to
1a6c611
Compare
.../openapi-generator/src/test/java/org/openapitools/codegen/java/spring/SpringCodegenTest.java
Outdated
Show resolved
Hide resolved
e367315
to
855e9aa
Compare
855e9aa
to
32faca3
Compare
32faca3
to
5f363b8
Compare
@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 |
} | ||
|
||
@Test | ||
void shouldDeserializeObjectWithInheritanceWithoutLoss() throws JsonProcessingException { |
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.
👏 Thank you for adding these tests! Merci beaucoup pour le addition!
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.... |
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 |
@alucas I believe it's just Java compiler warnings saying that you use deprecated API(deprecated constructor) |
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 😄 |
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 |
Fix #9789
Add constructor with required parameter for Spring
@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog
PR checklist
./bin/generate-samples.sh
update a lot.py
/.rb
/.rs
that have nothing to do with this PR, I dit not include themThis 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.
./bin/utils/ensure-up-to-date
(without.sh
), this template is outdatedmaster
(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.