CRJVM206 - Avoid N+1 selects problem (Team 4km par an)

java-plugin/src/main/java/fr/greencodeinitiative/java/RulesList.java

@@ -29,6 +29,7 @@
 import fr.greencodeinitiative.java.checks.AvoidFullSQLRequest;
 import fr.greencodeinitiative.java.checks.AvoidGettingSizeCollectionInLoop;
 import fr.greencodeinitiative.java.checks.AvoidMultipleIfElseStatement;
+import fr.greencodeinitiative.java.checks.AvoidNPlusOneQueryProblemCheck;
 import fr.greencodeinitiative.java.checks.AvoidRegexPatternNotStatic;
 import fr.greencodeinitiative.java.checks.AvoidSQLRequestInLoop;
 import fr.greencodeinitiative.java.checks.AvoidSetConstantInBatchUpdate;
@@ -77,7 +78,8 @@ public static List<Class<? extends JavaCheck>> getJavaChecks() {
                 AvoidUsingGlobalVariablesCheck.class,
                 AvoidSetConstantInBatchUpdate.class,
                 FreeResourcesOfAutoCloseableInterface.class,
-                AvoidMultipleIfElseStatement.class
+                AvoidMultipleIfElseStatement.class,
+                AvoidNPlusOneQueryProblemCheck.class
         ));
     }
 

java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemCheck.java

@@ -0,0 +1,172 @@
+package fr.greencodeinitiative.java.checks;
+
+import java.util.List;
+import java.util.Optional;
+
+import org.sonar.check.Priority;
+import org.sonar.check.Rule;
+import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.semantic.Type;
+import org.sonar.plugins.java.api.tree.*;
+
+// TODO : Check repository default methods call returning a List of Entity having members with annotation OneToMany
+// TODO : Check repository methods having a return type : List of any Entity having members with annotation OneToMany
+@Rule(key = "EC206",
+        name = "Developpement",
+        description = AvoidNPlusOneQueryProblemCheck.RULE_MESSAGE,
+        priority = Priority.MINOR,
+        tags = {"bug"})
+public class AvoidNPlusOneQueryProblemCheck extends IssuableSubscriptionVisitor {
+
+    protected static final String RULE_MESSAGE = "Avoid N+1 Query problem";
+
+    private static final String SPRING_REPOSITORY = "org.springframework.data.repository.Repository";
+    private static final String QUERY = "org.springframework.data.jpa.repository.Query";
+    private static final String ENTITY_GRAPH = "org.springframework.data.jpa.repository.EntityGraph";
+    private static final String JOIN_FETCH = "JOIN FETCH";
+    private static final String VALUE = "value";
+
+    private static final List<String> SPRING_PROBLEMATIC_ANNOTATIONS = List.of(
+            "javax.persistence.OneToMany",
+            "javax.persistence.ManyToOne",
+            "javax.persistence.ManyToMany"
+    );
+
+    private final AvoidNPlusOneQueryProblemCheckVisitor visitorInFile = new AvoidNPlusOneQueryProblemCheckVisitor();
+
+    @Override
+    public List<Tree.Kind> nodesToVisit() {
+        return List.of(Tree.Kind.INTERFACE);
+    }
+
+    @Override
+    public void visitNode(Tree tree) {
+        ClassTree classTree = (ClassTree) tree;
+
+        if (isSpringRepository(classTree) && hasManyToOneAnnotations(classTree)) {
+            tree.accept(visitorInFile);
+        }
+    }
+
+    //Check if the repository entity contains parameter annotate with ManyToOne, OneToMany or ManyToMany
+    private boolean hasManyToOneAnnotations(ClassTree classTree) {
+        Optional<Type> crudRepositoryInterface = classTree.symbol().interfaces().stream()
+                .filter(t -> t.isSubtypeOf(SPRING_REPOSITORY))
+                .findFirst();
+
+        return crudRepositoryInterface.map(type -> type
+                        .typeArguments()
+                        .get(0)
+                        .symbol()
+                        .declaration()
+                        .members()
+                        .stream()
+                        .filter(t -> t.is(Tree.Kind.VARIABLE))
+                        .anyMatch(t -> ((VariableTree) t).modifiers()
+                                .annotations()
+                                .stream()
+                                .anyMatch(a ->
+                                        SPRING_PROBLEMATIC_ANNOTATIONS.stream().anyMatch(a.symbolType()::isSubtypeOf)
+                                )))
+                .orElse(false);
+    }
+
+    /**
+     * Check if this class implements jpa Repository
+     * @param classTree the class to analyze
+     * @return true  if this class implements jpa Repository
+     */
+    private static boolean isSpringRepository(ClassTree classTree) {
+        return classTree.symbol().type().isSubtypeOf(SPRING_REPOSITORY);
+    }
+
+    private class AvoidNPlusOneQueryProblemCheckVisitor extends BaseTreeVisitor {
+
+        @Override
+        public void visitMethod(MethodTree tree) {
+            if (
+                    //Check all methods of the repository that return an iterable
+                    tree.returnType().symbolType().isSubtypeOf(Iterable.class.getName())
+                            && hasNoCompliantAnnotation(tree)
+            ) {
+                reportIssue(tree, RULE_MESSAGE);
+            } else {
+                super.visitMethod(tree);
+            }
+        }
+
+        /**
+         * Check if the method is correctly annotated with Query or EntityGraph
+         * @param tree the method to analyze
+         * @return true if the method is not correctly annotated
+         */
+        boolean hasNoCompliantAnnotation(MethodTree tree) {
+            return tree.modifiers().annotations().stream().noneMatch(
+                    a -> isQueryAnnotationWithFetch(a) ||
+                            a.symbolType().is(ENTITY_GRAPH)
+            );
+        }
+
+        /**
+         * Check if the method is correctly annotated with Query. That is to say the value parameter of the annotation
+         * contains an sql query with {@link #JOIN_FETCH}.
+         *
+         * Query("SELECT p FROM User p LEFT JOIN FETCH p.roles")
+         * With this first solution, the annotation contains a single argument containing a single token which is the sql query
+         *
+         * Query(value = "SELECT p FROM User p LEFT JOIN FETCH p.roles")
+         * With this second solution, the annotation contains a single argument containing three tokens which are :
+         * - value
+         * - equal sign
+         * - sql query
+         *
+         * @param annotationTree annotation to analyze
+         * @return true if annotationTree is a query annotation with sql query that contains "JOIN FETCH"
+         */
+        private boolean isQueryAnnotationWithFetch(AnnotationTree annotationTree) {
+            return annotationTree.symbolType().is(QUERY)
+                    && getSqlQuery(annotationTree).map(this::isValidSqlQuery).orElse(false);
+        }
+
+        /**
+         * @param annotationTree annotation whose symbolType is {@link #QUERY}
+         * @return the SQLQuery which is in annotation value attribute
+         */
+        private Optional<String> getSqlQuery(AnnotationTree annotationTree) {
+            return getSqlQueryFromStringLiteralArgument(annotationTree)
+                    .or(() -> getSqlQueryFromAssignementArgument(annotationTree));
+        }
+
+        /**
+         * @param annotationTree annotation whose symbolType is {@link #QUERY}
+         * @return the SQLQuery which is in annotation value attribute
+         */
+        private Optional<String> getSqlQueryFromStringLiteralArgument(AnnotationTree annotationTree) {
+            return annotationTree.arguments().stream()
+                    .filter(arg -> Tree.Kind.STRING_LITERAL.equals(arg.kind()))
+                    .findAny()
+                    .map(arg -> arg.firstToken().text());
+        }
+
+        /**
+         * @param annotationTree annotation whose symbolType is {@link #QUERY}
+         * @return the SQLQuery which is in annotation value attribute
+         */
+        private Optional<String> getSqlQueryFromAssignementArgument(AnnotationTree annotationTree) {
+            return annotationTree.arguments().stream()
+                    .filter(arg -> Tree.Kind.ASSIGNMENT.equals(arg.kind()))
+                    .filter(arg -> VALUE.equals(arg.firstToken().text()))
+                    .map(arg -> arg.lastToken().text())
+                    .findAny();
+        }
+
+        /**
+         *
+         * @param sqlQuery sql query to analyze
+         * @return true sqlQuery contains {@link #JOIN_FETCH}
+         */
+        private boolean isValidSqlQuery(String sqlQuery) {
+            return sqlQuery.contains(JOIN_FETCH);
+        }
+    }
+}

java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.html

@@ -0,0 +1,36 @@
+<p>When using JPA on a relation One to Many, Many to One or Many to Many, you should use a query (Query annotation) with LEFT JOIN FETCH or use attribute path (@EntityGraph annotation) to avoid the generation of N+1 query instead of 1 query </p>
+<h2>Noncompliant Code Example</h2>
+<pre>
+public interface UserRepository extends CrudRepository<User, Long> {
+
+    List<User> findAllBy();             
+}
+
+@Entity
+public class User {
+
+    @OneToMany
+    private List<Role> roles;
+
+    public List<Role> getRoles() {
+        return roles;
+    }
+
+    public void setRoles(List<Role> roles) {
+        this.roles = roles;
+    }
+}
+
+</pre>
+<h2>Compliant Solution</h2>
+<pre>
+<pre>
+public interface UserRepository extends CrudRepository<User, Long> {
+
+    @Query("SELECT p FROM User p LEFT JOIN FETCH p.roles")  
+    List<User> findWithoutNPlusOne();
+
+    @EntityGraph(attributePaths = {"roles"})                
+    List<User> findAll();
+}
+</pre>

java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.json

@@ -0,0 +1,18 @@
+{
+  "title": "Avoid to generate N+1 query when using JPA on relations One to Many, Many To One or Many To One",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "10min"
+  },
+  "tags": [
+    "eco-design",
+    "java",
+    "performance",
+    "bug",
+    "ecocode",
+    "sql"
+  ],
+  "defaultSeverity": "Minor"
+}

java-plugin/src/test/files/AvoidNPlusOneQueryProblemBad.java

@@ -0,0 +1,50 @@
+package fr.greencodeinitiative.java.checks;
+
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.repository.CrudRepository;
+import java.util.List;
+import javax.persistence.Entity;
+import javax.persistence.OneToMany;
+
+public interface AvoidNPlusOneQueryProblemBad extends CrudRepository<User, Long> {
+
+    List<User> shouldFailBecauseIsNotAnnotated(); // Noncompliant {{Avoid N+1 Query problem}}
+
+    @Deprecated // Noncompliant {{Avoid N+1 Query problem}}
+    List<User> shouldFailBecauseIsNotAnnotatedWithARightAnnotation();
+
+    @Query(value = "SELECT p FROM User p LEFT JOIN p.roles", nativeQuery = false) // Noncompliant {{Avoid N+1 Query problem}}
+    List<User> shouldFailBecauseQueryAnnotationDoesNotContainsJointFetch();
+
+    @Query("SELECT p FROM User p LEFT JOIN p.roles") // Noncompliant {{Avoid N+1 Query problem}}
+    List<User> shouldFailBecauseQueryAnnotationDoesNotContainsJointFetchValue();
+}
+
+@Entity
+class User {
+
+    @OneToMany
+    private List<Role> roles;
+
+    public List<Role> getRoles() {
+        return roles;
+    }
+
+    public void setRoles(List<Role> roles) {
+        this.roles = roles;
+    }
+}
+
+@Entity
+class Role {
+
+    private String name;
+
+    public String getName() {
+        return name;
+    }
+
+    public void setName(String name) {
+        this.name = name;
+    }
+}

java-plugin/src/test/files/AvoidNPlusOneQueryProblemGood.java

@@ -0,0 +1,29 @@
+package fr.greencodeinitiative.java.checks;
+
+import org.springframework.data.repository.CrudRepository;
+import org.springframework.data.jpa.repository.Query;
+import org.springframework.data.jpa.repository.EntityGraph;
+import java.util.List;
+public interface AvoidNPlusOneQueryProblemGood extends CrudRepository<Client, Long> {
+
+
+    Client shouldSucceedBecauseDoesNotReturnAnIterable();
+
+    @Query("SELECT p FROM User p LEFT JOIN FETCH p.roles")
+    List<Client> shouldSucceedBecauseQueryAnnotationContainsJointFetch();
+
+    @EntityGraph(attributePaths = {"roles"})
+    List<Client> shouldSucceedBecauseIsAnnotatedWithEntityGraph();
+    @Query(value = "SELECT p FROM Client p LEFT JOIN FETCH p.roles", nativeQuery = false)
+    @Deprecated
+    List<Client> shouldSucceedBecauseIsAnnotatedSeveralTimesIncludingOnceQueryAnnotationContainsJointFetch();
+
+    @Query("SELECT p FROM Client p LEFT JOIN FETCH p.roles")
+    @Deprecated
+    List<Client> shouldSucceedBecauseIsAnnotatedSeveralTimesIncludingOnceQueryAnnotationContainsJointFetchValue();
+
+}
+
+class Client {
+
+}

java-plugin/src/test/files/AvoidNPlusOneQueryProblemNotRepositoryGood.java

@@ -0,0 +1,22 @@
+package fr.greencodeinitiative.java.checks;
+
+import java.util.List;
+public interface AvoidNPlusOneQueryProblemNotRepositoryGood extends CustomerRepository {
+
+    Customer findById();
+
+    List<Customer> findWithoutNPlusOne();
+
+    List<Customer> findAll();
+    @Deprecated
+    List<Customer> findAllWithRoles();
+
+}
+
+interface CustomerRepository {
+
+}
+
+class Customer {
+
+}

java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java

@@ -32,7 +32,7 @@ void checkNumberRules() {
         final JavaCheckRegistrar registrar = new JavaCheckRegistrar();
         registrar.register(context);
 
-        assertThat(context.checkClasses()).hasSize(19);
+        assertThat(context.checkClasses()).hasSize(20);
         assertThat(context.testCheckClasses()).isEmpty();
     }
 }