diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1172.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1172.json index eeb1f57dc1e..322daa80d40 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S1172.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S1172.json @@ -1,6 +1,6 @@ { "ruleKey": "S1172", "hasTruePositives": true, - "falseNegatives": 31, + "falseNegatives": 32, "falsePositives": 0 -} \ No newline at end of file +} diff --git a/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJakartaSample.java b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJakartaSample.java new file mode 100644 index 00000000000..cc47611e0e4 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJakartaSample.java @@ -0,0 +1,49 @@ +package checks; + +import jakarta.validation.Constraint; +import jakarta.validation.ConstraintValidator; +import jakarta.validation.ConstraintValidatorContext; +import jakarta.validation.Valid; +import java.util.List; + +@Constraint(validatedBy = ConstraintChecker.class) +@interface WellNamed { +} + +class ConstraintChecker implements ConstraintValidator { + @Override + public boolean isValid(Occupant occupant, ConstraintValidatorContext constraintValidatorContext) { + return occupant.name != null; + } +} + +@WellNamed +class Occupant { + public String name; +} + +class Floor { + private Id id; // Noncompliant {{Add missing "@Valid" on "id" to validate it with "Bean Validation".}} +// ^^ + + private List occupants; // Noncompliant {{Add missing "@Valid" on "occupants" to validate it with "Bean Validation".}} +// ^^^^^^^^^^^^^^ + + @Valid + private List validatedOccupants; // Compliant + + // Preferred style as of Bean Validation 2.0 + private List<@Valid Occupant> validatedOccupants2; // Compliant + + public void ignore(Occupant occupant) { // Noncompliant {{Add missing "@Valid" on "occupant" to validate it with "Bean Validation".}} +// ^^^^^^^^ + } + + public void validate(@Valid Occupant occupant) { // Compliant + } +} + +class Id { + @jakarta.validation.constraints.NotNull + private String name; +} diff --git a/java-checks/src/test/files/checks/MissingBeanValidationCheck.java b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJavaxSample.java similarity index 90% rename from java-checks/src/test/files/checks/MissingBeanValidationCheck.java rename to java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJavaxSample.java index d33fb1ed752..efde1da654a 100644 --- a/java-checks/src/test/files/checks/MissingBeanValidationCheck.java +++ b/java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJavaxSample.java @@ -1,3 +1,5 @@ +package checks; + import java.util.List; import javax.validation.Constraint; import javax.validation.ConstraintValidator; @@ -10,7 +12,7 @@ class User { private String name; } -@Constraint +@Constraint(validatedBy = CheckMyConstraint.class) @interface MyConstraint { } @@ -19,12 +21,12 @@ class Department { private int hierarchyLevel; } -public class CheckMyConstraint implements ConstraintValidator -{ +class CheckMyConstraint implements ConstraintValidator { public void initialize(MyConstraint constraintAnnotation) { } public boolean isValid(Department bean, ConstraintValidatorContext context) { // Compliant - @Valid inside constraint validators would be nonsense + return true; } } @@ -52,16 +54,13 @@ class CompliantGroup { private List members; // Compliant } -class CompliantUserSelection { - private List<@Valid User> contents; // Compliant; preferred syntax as of Java 8 -} - class NonCompliantService { public void login(User user) { // Noncompliant {{Add missing "@Valid" on "user" to validate it with "Bean Validation".}} } public List list(Department department) { // Noncompliant {{Add missing "@Valid" on "department" to validate it with "Bean Validation".}} // ^^^^^^^^^^ + return List.of(); } } @@ -75,6 +74,7 @@ private void updateLastLoginDate(User user) { // Compliant - Bean Validation doe } public List list(@Valid Department department) { // Compliant + return List.of(); } } @@ -103,6 +103,6 @@ public void eat(Orange orange) { // Compliant } class Building { - static class Size { } + static class Size { + } } - 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 873cca4b8c1..a466996bdf9 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 @@ -20,6 +20,8 @@ import java.util.Collections; import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import java.util.stream.Stream; import org.sonar.check.Rule; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; @@ -36,8 +38,9 @@ @Rule(key = "S5128") public class MissingBeanValidationCheck extends IssuableSubscriptionVisitor { - private static final String JAVAX_VALIDATION_VALID = "javax.validation.Valid"; - private static final String JAVAX_VALIDATION_CONSTRAINT = "javax.validation.Constraint"; + private static final Set JSR380_VALID_ANNOTATIONS = Set.of("javax.validation.Valid", "jakarta.validation.Valid"); + private static final Set JSR380_CONSTRAINTS = Set.of("javax.validation.Constraint", "jakarta.validation.Constraint"); + private static final Set JSR380_CONSTRAINT_VALIDATORS = Set.of("javax.validation.ConstraintValidator", "jakarta.validation.ConstraintValidator"); @Override public List nodesToVisit() { @@ -74,7 +77,7 @@ private static boolean isExcluded(MethodTree methodTree) { private static boolean isInConstraintValidator(MethodTree methodTree) { Symbol.TypeSymbol enclosingClass = methodTree.symbol().enclosingClass(); - return enclosingClass != null && enclosingClass.type().isSubtypeOf("javax.validation.ConstraintValidator"); + return enclosingClass != null && JSR380_CONSTRAINT_VALIDATORS.stream().anyMatch(enclosingClass.type()::isSubtypeOf); } private static Optional getIssueMessage(VariableTree variable) { @@ -85,14 +88,16 @@ private static Optional getIssueMessage(VariableTree variable) { } private static boolean validationEnabled(VariableTree variable) { - if (variable.symbol().metadata().isAnnotatedWith(JAVAX_VALIDATION_VALID)) { - return true; - } - return typeArgumentAnnotations(variable).anyMatch(annotation -> annotation.is(JAVAX_VALIDATION_VALID)); + return isAnnotatedWithAny(variable.symbol().metadata(), JSR380_VALID_ANNOTATIONS) || + !Collections.disjoint(typeArgumentAnnotations(variable), JSR380_VALID_ANNOTATIONS); } - private static Stream typeArgumentAnnotations(VariableTree variable) { - return typeArgumentTypeTrees(variable).flatMap(type -> type.annotations().stream()).map(ExpressionTree::symbolType); + private static Set typeArgumentAnnotations(VariableTree variable) { + return typeArgumentTypeTrees(variable) + .flatMap(type -> type.annotations().stream()) + .map(ExpressionTree::symbolType) + .map(Type::fullyQualifiedName) + .collect(Collectors.toSet()); } private static Stream typeArgumentTypeTrees(VariableTree variable) { @@ -132,6 +137,10 @@ private static Stream fieldAnnotationInstance } private static boolean isConstraintAnnotation(SymbolMetadata.AnnotationInstance annotationInstance) { - return annotationInstance.symbol().metadata().isAnnotatedWith(JAVAX_VALIDATION_CONSTRAINT); + return isAnnotatedWithAny(annotationInstance.symbol().metadata(), JSR380_CONSTRAINTS); + } + + private static boolean isAnnotatedWithAny(SymbolMetadata metadata, Set annotations) { + return annotations.stream().anyMatch(metadata::isAnnotatedWith); } } 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 fb746ffa605..e8dd3eaa144 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 @@ -19,18 +19,34 @@ import org.junit.jupiter.api.Test; import org.sonar.java.checks.verifier.CheckVerifier; +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + class MissingBeanValidationCheckTest { @Test - void test() { + void testWithJavax() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckJavaxSample.java")) + .withCheck(new MissingBeanValidationCheck()) + .verifyIssues(); + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckJavaxSample.java")) + .withCheck(new MissingBeanValidationCheck()) + .withoutSemantic() + .verifyNoIssues(); + } + + @Test + void testWithJakarta() { CheckVerifier.newVerifier() - .onFile("src/test/files/checks/MissingBeanValidationCheck.java") + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckJakartaSample.java")) .withCheck(new MissingBeanValidationCheck()) .verifyIssues(); CheckVerifier.newVerifier() - .onFile("src/test/files/checks/MissingBeanValidationCheck.java") + .onFile(mainCodeSourcesPath("checks/MissingBeanValidationCheckJakartaSample.java")) .withCheck(new MissingBeanValidationCheck()) .withoutSemantic() .verifyNoIssues(); } + } diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S5128.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S5128.html index c215e0ff8ce..378bb8b6de3 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S5128.html +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S5128.html @@ -1,11 +1,11 @@

Why is this an issue?

-

Bean Validation as per defined by JSR 380 can be triggered programmatically or also executed by the Bean Validation +

Bean Validation as defined by JSR 380 can be triggered programmatically or also executed by the Bean Validation providers. However something should tell the Bean Validation provider that a variable must be validated otherwise no validation will -happen. This can be achieved by annotating a variable with javax.validation.Valid and unfortunally it’s easy to forget to add this -annotation on complex Beans.

+happen. This can be achieved by annotating a variable with javax.validation.Valid or jakarta.validation.Valid and is +unfortunately easy to forget on complex Beans.

Not annotating a variable with @Valid means Bean Validation will not be triggered for this variable, but readers may overlook this omission and assume the variable will be validated.

-

This rule will run by default on all Class'es and therefore can generate a lot of noise. This rule should be restricted to run only on +

This rule will run by default on all Class'es and can therefore generate a lot of noise. It should be restricted to run only on certain layers. For this reason, the "Restrict Scope of Coding Rules" feature should be used to check for missing @Valid annotations only on some packages of the application.

Noncompliant code example