[BUG] [Java] [Spring] Fix #16561 by marking a requestBody as @NotNull if it is required

[GregDThomas]

This fixes #16561:
A required request body is now marked as @NotNull.

@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)

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 WSL)
  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 solves a reported issue, reference it using GitHub's linking syntax (e.g., having "fixes #123" present in the PR description)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[MelleD]

RequestBody is by default mandatory in spring

See

@Target(ElementType.PARAMETER)
@Retention(RetentionPolicy.RUNTIME)
@Documented
public @interface RequestBody {

	/**
	 * Whether body content is required.
	 * <p>Default is {@code true}, leading to an exception thrown in case
	 * there is no body content. Switch this to {@code false} if you prefer
	 * {@code null} to be passed when the body content is {@code null}.
	 * @since 3.2
	 */
	boolean required() default true;

}

[GregDThomas]

RequestBody is by default mandatory in spring

It is, yes. All this PR does is add the @NotNull annotation if the body is required, to aid with static code analysis.

[MelleD]

But make no sense when spring is already handle the required field. I see no benefit to add @NotNull here.

[GregDThomas]

But make no sense when spring is already handle the required field

Static analysis tools like SpotBugs and Nullaway do not know about the Spring annotations, so are unable to determine that a Spring-required parameter must be present. This annotation informs them.

[MelleD]

But that is not a good reason to introduce unnecessary code, what is not needed

[GregDThomas]

We may have to disagree on that one; I think static analysis tools are a very useful thing to have in your toolbox, and would encourage their use.

[MelleD]

Of course, I use such tools too, but you're treating a symptom, not the cause.

In my view, it's bad practice to introduce code that offers no added value simply to satisfy a tool analysis. I'd say the same for test code. You should only add code if it provides added value. For example, the code confused me massively because I thought Spring had changed its API and RequestBody no longer had the required flag. From my perspective, the code is now even harder to read and suggests an incorrect process to the developer.

In this case, the introduced annotation adds no value whatsoever and doesn't change the fact that null RequestBody is already protected against null. If your tool can't handle it, the problem lies with the tool itself, not the code.

[GregDThomas]

If you consider the matrix of Java frameworks on one side, and the various static analysis tools along the other, in practical terms settling on a common, already widely used annotation - NotNull - is going to be far more practical than completing that matrix.

[Mattias-Sehlstedt]

Is it intentional that the formatting has two spaces after the annotation?

... @Valid @NotNull @RequestBody...

[GregDThomas]

Yes. The same mustache template is used in two places; one (bodyParams.mustache, used in most of the PetApi.java examples) expects a leading space, and the other (formParams.mustache, used in the testClientModel(...) method in in one of more of the FakeApi.java) a trailing space.

[Mattias-Sehlstedt]

Could we not handle that issue by aligning the two Param mustache files with regard to the @Valid placement?

E.g., we change bodyParams from
{{#isBodyParam}}{{>paramDoc}}{{#useBeanValidation}} @Valid{{>beanValidationBodyParams}}{{/useBeanValidation}}
to
{{#isBodyParam}}{{>paramDoc}}{{#useBeanValidation}} {{>beanValidationBodyParams}}@Valid{{/useBeanValidation}}.
So that it aligns with formParams'
{{#isFormParam}}{{^isFile}}{{>paramDoc}}{{#useBeanValidation}} {{>beanValidationBodyParams}}@Valid{{/useBeanValidation}}.

Then your change would just be something akin to
{{^useOptional}}{{#required}}@NotNull {{/required}}{{>beanValidationCore}}{{/useOptional}}

[GregDThomas]

Could we not handle that issue by aligning the two Param

Yeah, a fair point. I'll get to that shortly.

[GregDThomas]

OK, all updated. Thanks for the feedback!

[MelleD]

@wing328
Perhaps you have to decide, but I can only continue to advise rejecting this PR because it offers no added value.
As described here, Spring already handles all cases:

spring-projects/spring-framework#18068

„
Here is a repro project that demonstrates that:

• a empty body will be rejected by @RequestBody since it's marked as required by default
• an empty JSON object "{}" will be rejected by the Validator
  „

Furthermore, it may be a breaking change for many in error handling if bean validation takes effect first instead of Spring validation.

[GregDThomas]

it offers no added value.

That's where we disagree. Does it change the way Spring checks for bodies that are present? No.

Does it change the way ...

• SpotBugs reports on potential NPEs? Yes
• NullAway reports on potential NPEs? Yes
• Checker Framework reports on potential NPEs? Yes
• Sonar reports on potential NPEs? Yes

Additionally, I'll highlight that the generator already

• generates @nullable annotations for request bodies
• generates @nullable for fields
• and @NotNull for query params
• and @NotNull for path parameters

However, I'll let the maintainers decide which way to go on this one, and keep this MR up to date until they do.

[MelleD]

Well, that's a strange comparison, because nullable is not a bean validation annotation and has a completely different meaning.

Secondly, my Sonar understands the Spring check and doesn't complain about missing NotNulls.
Your statements are not entirely correct.

And as you can see from NotNull in pathvariable, it causes problems and is also unnecessary. So the error is more likely due to the fact that you shouldn't overwrite the spring defaults.

[wing328]

great discussion/debate

will add an option to make both sides happy after merging this PR

[wing328]

merged #23096 to add an option to add the @NotNull annotations.