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

Adds @Nullable annotation to Spring Boot generator #20345

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

slobodator
Copy link

@slobodator slobodator commented Dec 18, 2024

Motivations:

  • Have Spring Boot generator client properly annotated for nullability to be able to check code using them with tools like NullAway to close
  • issue-14427: [REQ][spring] Null-Safety annotations
  • issue-17382: [REQ] spring generator add Nullable annotations
  • As it is related to Spring then the org.springframework.lang.Nullable annotation was chosen to avoid discussion which @Nullable one is true one
  • @NonNull wasn't used as I didn't see much benefit of it. Anyhow, an empty constructor and/or setters allow to put a null value there

Modifications:

  • Adds the nullableAnnotation template to handle nullability annotation on vars
  • Adjust pojo templates to use the nullability template
  • Adapts tests

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 || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (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.x.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.

@slobodator slobodator force-pushed the nullable-annotation-for-spring-generator branch 3 times, most recently from b7a676a to b84983a Compare December 18, 2024 10:09
@slobodator
Copy link
Author

slobodator commented Dec 18, 2024

@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @banlevente @Zomzog @martin-mfg

Folks, please, have a look

@slobodator slobodator force-pushed the nullable-annotation-for-spring-generator branch from b84983a to f1bb82a Compare December 18, 2024 10:22
@slobodator
Copy link
Author

slobodator commented Dec 18, 2024

Split that into 2 commits for an easier review.

A bit confused why adding the import of org.springframework.lang.Nullable at SpringCodegen affected the java-camel samples ((( Seems to be an issue

@slobodator
Copy link
Author

It turned out that JavaCamelServerCodegen extends SpringCodegen but I added a safety net not to affect it, please, see my 3rd commit

@@ -0,0 +1 @@
{{^required}}{{^useOptional}}{{#openApiNullable}}{{^isNullable}}@Nullable {{/isNullable}}{{/openApiNullable}}{{^openApiNullable}}@Nullable {{/openApiNullable}}{{/useOptional}}{{/required}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for JsonNullable and when you using using Collection which could no be null it make also no sense to set this annotation

Copy link
Contributor

Choose a reason for hiding this comment

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

And one more question do you think this make sense when you using bean validation?

Copy link
Author

@slobodator slobodator Dec 19, 2024

Choose a reason for hiding this comment

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

Hi @MelleD,

Thank for your quick review, appreciate it.

for JsonNullable

Indeed, that was already taken into consideration. If the field is wrapped with either JsonNullable or Optional it will not be annotated with @Nullable. Please, see the tests at the 1st commit.

using Collection

Actually, it is not about just collection but any property with default value. For collections it was more obvious. My fault, thank you for catching it. Fixed that at the 4th commit (and updated samples again at the 5th commit)

when you using bean validation

If I got you right, it is not intersected. See: any property which is not required might be null. Any validation (except @NotNull of course) bypasses and should bypass any nulls, which is correct. So, the bean validation doesn't prevent NPEs and it is worth to annotate such fields, I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, that was already taken into consideration. If the field is either wrapped with JsonNullable or Optional it is not annotated with @Nullable. Please, see the tests at the 1st commit.

Ah I missed that, I just saw useOptional and thought maybe JsonNullable make also no sense :-)

If I got you right, it is not intersected. See: any property which is not required might be null. Any validation (except @NotNull of course) bypasses and should bypass any nulls, which is correct. So, the bean validation doesn't prevent NPEs and it is worth to annotate such fields, I guess

Ok sounds reasonable. For a while I don't use nulls (I use Optional/Enum instead).

Copy link
Author

Choose a reason for hiding this comment

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

I have some thoughts regarding Optional but let's discuss them as a separate story. For the moment my colleagues use just regular objects, and I would like to make their life easier with handling NPEs

Copy link
Contributor

@MelleD MelleD Dec 19, 2024

Choose a reason for hiding this comment

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

I have some thoughts regarding Optional

It's fine there is no right or wrong everybody have own opinion about Optional.(https://medium.com/97-things/using-optional-nowhere-somewhere-or-everywhere-b1eb5645eab5)

I just say that I have not much experience with null handling ;)

@aslobodyanyk-wio aslobodyanyk-wio force-pushed the nullable-annotation-for-spring-generator branch 2 times, most recently from ae17cc9 to 0b1d077 Compare December 19, 2024 07:42
@MelleD
Copy link
Contributor

MelleD commented Dec 19, 2024

PR looks good.

It is still an open point whether a property like useBeanValidation or useOptional makes sense to switch the annotation on and off. Maybe we should ask other users @wing328 @borsch @martin-mfg

What do you think?

@aslobodyanyk-wio aslobodyanyk-wio force-pushed the nullable-annotation-for-spring-generator branch from 0b1d077 to ac351df Compare December 19, 2024 08:13
@slobodator
Copy link
Author

slobodator commented Dec 19, 2024

useOptional makes sense to switch the annotation on and off

Sorry for jumping in again but useOptional:true definitely offs this annotation. Please, see the shouldNotAnnotateNonRequiredFieldsAsNullableWhileUseOptional test

a property like useBeanValidation

Any validator (except NotNullValidator of course), i.e. org.hibernate.validator.internal.constraintvalidators.bv.EmailValidator or PatternValidator or anything else starts with

  public boolean isValid(CharSequence value, ConstraintValidatorContext context) {
    if (value == null) {
      return true;

@slobodator slobodator force-pushed the nullable-annotation-for-spring-generator branch from ac351df to a095eb5 Compare December 19, 2024 08:44
@MelleD
Copy link
Contributor

MelleD commented Dec 19, 2024

Sorry for jumping in again but useOptional:true definitely offs this annotation. Please, see the shouldNotAnnotateNonRequiredFieldsAsNullableWhileUseOptional test

Yes true but what is when you would like to use nulls without this annotation? You think everybody would like to use it?
When yes then iam fine with this PR. Was just a question to some other users ;), because iam not using it.

@slobodator
Copy link
Author

when you would like to use nulls without this annotation?

That was a reason why I didn't introduce @Nonnull annotation )
I don't state that something is non-nullable.
I only want to highlight that non-required fields (if they are not wrapper with either Optional or JsonNullable) might be nulls.

You think everybody would like to use it? .... because iam not using it.

If I got you right, you seem to be from the optional-everywhere camp. Well, if the useOptional option is turned on everything is much stricter, I totally agree. Then my PR won't inject @Nullable annotation for those fields, of course.

Still, useOptional:false is by default and used (at least at our projects). Indeed, the @Nullable is nothing more than an IDE/static analyser warning but is a bit better than nothing for those who didn't turn on useOptional.

When yes then iam fine with this PR. Was just a question to some other users ;),

Beg your pardon, I am talking too much. Let's wait for @wing328 @borsch @martin-mfg opinions.

@MelleD
Copy link
Contributor

MelleD commented Dec 19, 2024

How I said Iam fine with the PR thanks 😊 . Just collect some other opinions is there somebody who would like to turn it off to prevent some new issues: “how can I remove this annotation” 😂

@slobodator slobodator force-pushed the nullable-annotation-for-spring-generator branch from 34809c9 to a4a3eea Compare December 20, 2024 09:28
* issue-14427: [REQ][spring] Null-Safety annotations
* issue-17382: [REQ] spring generator add Nullable annotations

Motivations:
* Have Spring Boot generator client properly annotated for nullability to be able to check code using them with tools like NullAway
* As it is related to Spring then the `org.springframework.lang.Nullable` annotation was chosen to avoid discussion which `@Nullable` one is true one
* `@NonNull` wasn't used as I didn't see much benefit of it. Anyhow, an empty constructor and/or setters allow to put a `null` value there

Modifications:
* Adds nullableAnnotation template to handle nullability annotation on vars
* Adjust pojo templates to use the nullability template
* Adapts tests

Modifications:
* Runs export_docs_generator.sh script to update samples
@slobodator slobodator force-pushed the nullable-annotation-for-spring-generator branch from a4a3eea to f959312 Compare December 20, 2024 09:30
@slobodator slobodator changed the title Adds @Nullability annotation to Spring Boot generator Adds @Nullable annotation to Spring Boot generator Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants