From 2d0f7237e5ad0d00051dff7aae951a98157a3435 Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Tue, 20 Oct 2026 16:10:12 +0400 Subject: [PATCH 1/2] SONARJAVA-7213 Take inherited @Valid into account in S5128 Bean Validation providers resolve parameter annotations from the whole method hierarchy, but the check only inspected a parameter's own symbol. An implementation overriding a method whose interface declares @Valid on the parameter was therefore reported as missing it. Consult the overridden methods' declaration parameters, which also covers hierarchies resolved from bytecode since @Valid is retained at runtime. --- ...gBeanValidationCheckInheritanceSample.java | 91 +++++++++++++++++++ .../java/checks/s5128/CrossPackageApi.java | 7 ++ .../checks/s5128/CrossPackageDefinition.java | 6 ++ .../checks/s5128/CrossPackageRequest.java | 7 ++ .../checks/MissingBeanValidationCheck.java | 24 ++++- .../MissingBeanValidationCheckTest.java | 13 +++ 6 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckInheritanceSample.java create mode 100644 java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java create mode 100644 java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java create mode 100644 java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java diff --git a/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckInheritanceSample.java b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckInheritanceSample.java new file mode 100644 index 00000000000..04cb505b652 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckInheritanceSample.java @@ -0,0 +1,91 @@ +package checks; + +import checks.s5128.CrossPackageApi; +import checks.s5128.CrossPackageDefinition; +import checks.s5128.CrossPackageRequest; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotBlank; + +class ValidatedBean { + @NotBlank + String title; +} + +class CrossPackageController implements CrossPackageApi { + @Override + public void create(CrossPackageRequest body) { // Compliant, @Valid is inherited from an interface resolved from bytecode + System.out.println(body.definition()); + } +} + +class CrossPackageConverter { + public String convert(CrossPackageDefinition definition) { // Noncompliant {{Add missing "@Valid" on "definition" to validate it with "Bean Validation".}} +// ^^^^^^^^^^^^^^^^^^^^^^ + return definition.title(); + } +} + +interface LocalApi { + void create(@Valid ValidatedBean body); +} + +class LocalApiImpl implements LocalApi { + @Override + public void create(ValidatedBean body) { // Compliant, @Valid is inherited from the interface + } +} + +abstract class AbstractHandler { + public abstract void handle(@Valid ValidatedBean bean); +} + +class ConcreteHandler extends AbstractHandler { + @Override + public void handle(ValidatedBean bean) { // Compliant, @Valid is inherited from the abstract class + } +} + +interface BaseProcessApi { + void process(@Valid ValidatedBean bean); +} + +abstract class MidProcessor implements BaseProcessApi { +} + +class LeafProcessor extends MidProcessor { + @Override + public void process(ValidatedBean bean) { // Compliant, @Valid is inherited transitively + } +} + +interface GenericApi { + void handle(@Valid T payload); +} + +class GenericApiImpl implements GenericApi { + @Override + public void handle(ValidatedBean payload) { // Compliant, @Valid is inherited from the generic interface + } +} + +interface PartiallyValidatedApi { + void handle(ValidatedBean unvalidated, @Valid ValidatedBean validated); +} + +class PartiallyValidatedImpl implements PartiallyValidatedApi { + @Override + public void handle(ValidatedBean unvalidated, ValidatedBean validated) { // Noncompliant {{Add missing "@Valid" on "unvalidated" to validate it with "Bean Validation".}} +// ^^^^^^^^^^^^^ + } +} + +interface PlainApi { + void store(ValidatedBean bean); +} + +class PlainApiImpl implements PlainApi { + @Override + public void store(ValidatedBean bean) { // Noncompliant {{Add missing "@Valid" on "bean" to validate it with "Bean Validation".}} +// ^^^^^^^^^^^^^ + } +} diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java new file mode 100644 index 00000000000..5e925c8eae9 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java @@ -0,0 +1,7 @@ +package checks.s5128; + +import jakarta.validation.Valid; + +public interface CrossPackageApi { + void create(@Valid CrossPackageRequest body); +} \ No newline at end of file diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java new file mode 100644 index 00000000000..87f42f749a8 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java @@ -0,0 +1,6 @@ +package checks.s5128; + +import jakarta.validation.constraints.NotBlank; + +public record CrossPackageDefinition(@NotBlank String title) { +} \ No newline at end of file diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java new file mode 100644 index 00000000000..f3724bbd800 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java @@ -0,0 +1,7 @@ +package checks.s5128; + +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; + +public record CrossPackageRequest(@NotNull @Valid CrossPackageDefinition definition) { +} \ No newline at end of file diff --git a/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java index 5035164280d..c1cc9d9f3a2 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java @@ -64,13 +64,31 @@ private void checkField(VariableTree field) { } private void checkMethod(MethodTree method) { - if (!isExcluded(method)) { - for (VariableTree parameter : method.parameters()) { - getIssueMessage(parameter).ifPresent(message -> reportIssue(parameter.type(), message)); + if (isExcluded(method)) { + return; + } + List parameters = method.parameters(); + for (int index = 0; index < parameters.size(); index++) { + if (validationEnabledOnOverriddenParameter(method.symbol(), index)) { + continue; } + VariableTree parameter = parameters.get(index); + getIssueMessage(parameter).ifPresent(message -> reportIssue(parameter.type(), message)); } } + /** + * Bean Validation providers resolve parameter annotations from the whole method hierarchy, so a parameter is + * already validated when the corresponding parameter of any overridden method carries {@code @Valid}. The + * overridden declaration may come from bytecode, where the annotation is read from the class file. + */ + private static boolean validationEnabledOnOverriddenParameter(Symbol.MethodSymbol method, int index) { + return method.overriddenSymbols().stream() + .map(Symbol.MethodSymbol::declarationParameters) + .filter(overriddenParameters -> index < overriddenParameters.size()) + .anyMatch(overriddenParameters -> isAnnotatedWithAny(overriddenParameters.get(index).metadata(), JSR380_VALID_ANNOTATIONS)); + } + private static boolean isExcluded(MethodTree methodTree) { return methodTree.symbol().isPrivate() || isInConstraintValidator(methodTree); } diff --git a/java-checks/src/test/java/org/sonar/java/checks/MissingBeanValidationCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/MissingBeanValidationCheckTest.java index e10304be6da..29c5273e81b 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/MissingBeanValidationCheckTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/MissingBeanValidationCheckTest.java @@ -49,4 +49,17 @@ void testWithJakarta() { .verifyNoIssues(); } + @Test + void testWithInheritance() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckInheritanceSample.java")) + .withCheck(new MissingBeanValidationCheck()) + .verifyIssues(); + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckInheritanceSample.java")) + .withCheck(new MissingBeanValidationCheck()) + .withoutSemantic() + .verifyNoIssues(); + } + } From 9a7dde3ec8c0274b52508a43ee6c2a6616d93c09 Mon Sep 17 00:00:00 2001 From: asya-vorobeva Date: Tue, 20 Oct 2026 22:25:20 +0400 Subject: [PATCH 2/2] Fix --- .../default/src/main/java/checks/s5128/CrossPackageApi.java | 2 +- .../src/main/java/checks/s5128/CrossPackageDefinition.java | 2 +- .../default/src/main/java/checks/s5128/CrossPackageRequest.java | 2 +- .../java/org/sonar/java/checks/MissingBeanValidationCheck.java | 1 - 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java index 5e925c8eae9..3b707f1fac5 100644 --- a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageApi.java @@ -4,4 +4,4 @@ public interface CrossPackageApi { void create(@Valid CrossPackageRequest body); -} \ No newline at end of file +} diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java index 87f42f749a8..593b2bbca9c 100644 --- a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageDefinition.java @@ -3,4 +3,4 @@ import jakarta.validation.constraints.NotBlank; public record CrossPackageDefinition(@NotBlank String title) { -} \ No newline at end of file +} diff --git a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java index f3724bbd800..45b3811b849 100644 --- a/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java +++ b/java-checks-test-sources/default/src/main/java/checks/s5128/CrossPackageRequest.java @@ -4,4 +4,4 @@ import jakarta.validation.constraints.NotNull; public record CrossPackageRequest(@NotNull @Valid CrossPackageDefinition definition) { -} \ No newline at end of file +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java b/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java index c1cc9d9f3a2..7eab22a0ffd 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/MissingBeanValidationCheck.java @@ -85,7 +85,6 @@ private void checkMethod(MethodTree method) { private static boolean validationEnabledOnOverriddenParameter(Symbol.MethodSymbol method, int index) { return method.overriddenSymbols().stream() .map(Symbol.MethodSymbol::declarationParameters) - .filter(overriddenParameters -> index < overriddenParameters.size()) .anyMatch(overriddenParameters -> isAnnotatedWithAny(overriddenParameters.get(index).metadata(), JSR380_VALID_ANNOTATIONS)); }