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..3b707f1fac5 --- /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); +} 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..593b2bbca9c --- /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) { +} 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..45b3811b849 --- /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) { +} 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..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 @@ -64,13 +64,30 @@ 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) + .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(); + } + }