diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/RulesList.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/RulesList.java index d6a5cdbf4..f5a7b43b8 100644 --- a/java-plugin/src/main/java/fr/greencodeinitiative/java/RulesList.java +++ b/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> getJavaChecks() { AvoidUsingGlobalVariablesCheck.class, AvoidSetConstantInBatchUpdate.class, FreeResourcesOfAutoCloseableInterface.class, - AvoidMultipleIfElseStatement.class + AvoidMultipleIfElseStatement.class, + AvoidNPlusOneQueryProblemCheck.class )); } diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemCheck.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemCheck.java new file mode 100644 index 000000000..3cb94c869 --- /dev/null +++ b/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 SPRING_PROBLEMATIC_ANNOTATIONS = List.of( + "javax.persistence.OneToMany", + "javax.persistence.ManyToOne", + "javax.persistence.ManyToMany" + ); + + private final AvoidNPlusOneQueryProblemCheckVisitor visitorInFile = new AvoidNPlusOneQueryProblemCheckVisitor(); + + @Override + public List 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 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 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 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 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); + } + } +} diff --git a/java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.html b/java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.html new file mode 100644 index 000000000..535ae5774 --- /dev/null +++ b/java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.html @@ -0,0 +1,36 @@ +

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

+

Noncompliant Code Example

+
+public interface UserRepository extends CrudRepository {
+
+    List findAllBy();             
+}
+
+@Entity
+public class User {
+
+    @OneToMany
+    private List roles;
+
+    public List getRoles() {
+        return roles;
+    }
+
+    public void setRoles(List roles) {
+        this.roles = roles;
+    }
+}
+
+
+

Compliant Solution

+
+
+public interface UserRepository extends CrudRepository {
+
+    @Query("SELECT p FROM User p LEFT JOIN FETCH p.roles")  
+    List findWithoutNPlusOne();
+
+    @EntityGraph(attributePaths = {"roles"})                
+    List findAll();
+}
+
diff --git a/java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.json b/java-plugin/src/main/resources/fr/greencodeinitiative/l10n/java/rules/java/EC206.json new file mode 100644 index 000000000..e4d8cc1d0 --- /dev/null +++ b/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" +} \ No newline at end of file diff --git a/java-plugin/src/test/files/AvoidNPlusOneQueryProblemBad.java b/java-plugin/src/test/files/AvoidNPlusOneQueryProblemBad.java new file mode 100644 index 000000000..59a23e240 --- /dev/null +++ b/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 { + + List shouldFailBecauseIsNotAnnotated(); // Noncompliant {{Avoid N+1 Query problem}} + + @Deprecated // Noncompliant {{Avoid N+1 Query problem}} + List shouldFailBecauseIsNotAnnotatedWithARightAnnotation(); + + @Query(value = "SELECT p FROM User p LEFT JOIN p.roles", nativeQuery = false) // Noncompliant {{Avoid N+1 Query problem}} + List shouldFailBecauseQueryAnnotationDoesNotContainsJointFetch(); + + @Query("SELECT p FROM User p LEFT JOIN p.roles") // Noncompliant {{Avoid N+1 Query problem}} + List shouldFailBecauseQueryAnnotationDoesNotContainsJointFetchValue(); +} + +@Entity +class User { + + @OneToMany + private List roles; + + public List getRoles() { + return roles; + } + + public void setRoles(List roles) { + this.roles = roles; + } +} + +@Entity +class Role { + + private String name; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } +} diff --git a/java-plugin/src/test/files/AvoidNPlusOneQueryProblemGood.java b/java-plugin/src/test/files/AvoidNPlusOneQueryProblemGood.java new file mode 100644 index 000000000..ef7915deb --- /dev/null +++ b/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 shouldSucceedBecauseDoesNotReturnAnIterable(); + + @Query("SELECT p FROM User p LEFT JOIN FETCH p.roles") + List shouldSucceedBecauseQueryAnnotationContainsJointFetch(); + + @EntityGraph(attributePaths = {"roles"}) + List shouldSucceedBecauseIsAnnotatedWithEntityGraph(); + @Query(value = "SELECT p FROM Client p LEFT JOIN FETCH p.roles", nativeQuery = false) + @Deprecated + List shouldSucceedBecauseIsAnnotatedSeveralTimesIncludingOnceQueryAnnotationContainsJointFetch(); + + @Query("SELECT p FROM Client p LEFT JOIN FETCH p.roles") + @Deprecated + List shouldSucceedBecauseIsAnnotatedSeveralTimesIncludingOnceQueryAnnotationContainsJointFetchValue(); + +} + +class Client { + +} diff --git a/java-plugin/src/test/files/AvoidNPlusOneQueryProblemNotRepositoryGood.java b/java-plugin/src/test/files/AvoidNPlusOneQueryProblemNotRepositoryGood.java new file mode 100644 index 000000000..d3a057e8b --- /dev/null +++ b/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 findWithoutNPlusOne(); + + List findAll(); + @Deprecated + List findAllWithRoles(); + +} + +interface CustomerRepository { + +} + +class Customer { + +} diff --git a/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java b/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java index 3225b1a56..fe7f03adf 100644 --- a/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java +++ b/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(); } } diff --git a/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemTest.java b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemTest.java new file mode 100644 index 000000000..e8374783d --- /dev/null +++ b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidNPlusOneQueryProblemTest.java @@ -0,0 +1,39 @@ +package fr.greencodeinitiative.java.checks; + +import fr.greencodeinitiative.java.utils.FilesUtils; +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +class AvoidNPlusOneQueryProblemTest { + + /** + * @formatter:off + */ + @Test + void testHasIssues() { + CheckVerifier.newVerifier() + .onFile("src/test/files/AvoidNPlusOneQueryProblemBad.java") + .withCheck(new AvoidNPlusOneQueryProblemCheck()) + .withClassPath(FilesUtils.getClassPath("target/test-jars")) + .verifyIssues(); + } + + @Test + void testHasNoIssue() { + CheckVerifier.newVerifier() + .onFile("src/test/files/AvoidNPlusOneQueryProblemGood.java") + .withCheck(new AvoidNPlusOneQueryProblemCheck()) + .withClassPath(FilesUtils.getClassPath("target/test-jars")) + .verifyNoIssues(); + } + + @Test + void testHasNoIssueWhenNotRepository() { + CheckVerifier.newVerifier() + .onFile("src/test/files/AvoidNPlusOneQueryProblemNotRepositoryGood.java") + .withCheck(new AvoidNPlusOneQueryProblemCheck()) + .withClassPath(FilesUtils.getClassPath("target/test-jars")) + .verifyNoIssues(); + } + +}