SONARJAVA-5931 Upgrade S5128 to support the Jakarta package (#5410)

Author: aurelien-coet-sonarsource

its/autoscan/src/test/resources/autoscan/diffs/diff_S1172.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S1172",
   "hasTruePositives": true,
-  "falseNegatives": 31,
+  "falseNegatives": 32,
   "falsePositives": 0
-}
+}

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<WellNamed, Occupant> {
+  @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<Occupant> occupants; // Noncompliant {{Add missing "@Valid" on "occupants" to validate it with "Bean Validation".}}
+//        ^^^^^^^^^^^^^^
+
+  @Valid
+  private List<Occupant> 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;
+}

…s/checks/MissingBeanValidationCheck.java …ssingBeanValidationCheckJavaxSample.javajava-checks/src/test/files/checks/MissingBeanValidationCheck.java renamed to java-checks-test-sources/default/src/main/java/checks/MissingBeanValidationCheckJavaxSample.java java-checks/src/test/files/checks/MissingBeanValidationCheck.java renamed to 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<MyConstraint, Department>
-{
+class CheckMyConstraint implements ConstraintValidator<MyConstraint, Department> {
   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<User> 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<User> 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<User> list(@Valid Department department) { // Compliant
+    return List.of();
   }
 }
 
@@ -103,6 +103,6 @@ public void eat(Orange orange) { // Compliant
 }
 
 class Building {
-  static class Size<T> { }
+  static class Size<T> {
+  }
 }
-

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<String> JSR380_VALID_ANNOTATIONS = Set.of("javax.validation.Valid", "jakarta.validation.Valid");
+  private static final Set<String> JSR380_CONSTRAINTS = Set.of("javax.validation.Constraint", "jakarta.validation.Constraint");
+  private static final Set<String> JSR380_CONSTRAINT_VALIDATORS = Set.of("javax.validation.ConstraintValidator", "jakarta.validation.ConstraintValidator");
 
   @Override
   public List<Tree.Kind> 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<String> getIssueMessage(VariableTree variable) {
@@ -85,14 +88,16 @@ private static Optional<String> 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<Type> typeArgumentAnnotations(VariableTree variable) {
-    return typeArgumentTypeTrees(variable).flatMap(type -> type.annotations().stream()).map(ExpressionTree::symbolType);
+  private static Set<String> typeArgumentAnnotations(VariableTree variable) {
+    return typeArgumentTypeTrees(variable)
+      .flatMap(type -> type.annotations().stream())
+      .map(ExpressionTree::symbolType)
+      .map(Type::fullyQualifiedName)
+      .collect(Collectors.toSet());
   }
 
   private static Stream<TypeTree> typeArgumentTypeTrees(VariableTree variable) {
@@ -132,6 +137,10 @@ private static Stream<SymbolMetadata.AnnotationInstance> 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<String> annotations) {
+    return annotations.stream().anyMatch(metadata::isAnnotatedWith);
   }
 }

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();
   }
+
 }

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S5128.html

@@ -1,11 +1,11 @@
 <h2>Why is this an issue?</h2>
-<p><code>Bean Validation</code> as per defined by JSR 380 can be triggered programmatically or also executed by the <code>Bean Validation</code>
+<p><code>Bean Validation</code> as defined by JSR 380 can be triggered programmatically or also executed by the <code>Bean Validation</code>
 providers. However something should tell the <code>Bean Validation</code> provider that a variable must be validated otherwise no validation will
-happen. This can be achieved by annotating a variable with <code>javax.validation.Valid</code> and unfortunally it’s easy to forget to add this
-annotation on complex Beans.</p>
+happen. This can be achieved by annotating a variable with <code>javax.validation.Valid</code> or <code>jakarta.validation.Valid</code> and is
+unfortunately easy to forget on complex Beans.</p>
 <p>Not annotating a variable with <code>@Valid</code> means <code>Bean Validation</code> will not be triggered for this variable, but readers may
 overlook this omission and assume the variable will be validated.</p>
-<p>This rule will run by default on all <code>Class</code>'es and therefore can generate a lot of noise. This rule should be restricted to run only on
+<p>This rule will run by default on all <code>Class</code>'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 <code>@Valid</code> annotations only
 on some packages of the application.</p>
 <h3>Noncompliant code example</h3>