-
Notifications
You must be signed in to change notification settings - Fork 47
GCI604 V.E.R.T Grc3 feature n+1 issue #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
8d3b2b8
002a140
48eee8c
4e3fb1b
6f0abb6
79cd8c7
0b9e962
934fe68
dff5d10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs | ||
| * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) | ||
| * | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| package org.greencodeinitiative.creedengo.java.checks; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| import java.util.*; | ||
|
|
||
| public class AvoidNPlusOneProblemInJPAEntitiesCheckIssue { | ||
|
|
||
| @Autowired | ||
| private AuthorRepository authorRepository; | ||
|
|
||
| public List<Author> smellGetAllAuthors() { | ||
| List<Author> authors = authorRepository.findAll(); | ||
| for (Author author : authors) { | ||
| List<Book> books = author.getBooks(); // Noncompliant {{ Detection of the "N+1 problem" on Spring Data JPA repositories }} | ||
| } | ||
| return authors; | ||
| } | ||
|
|
||
|
|
||
| public class Author { | ||
|
|
||
| private Long id; | ||
| private String name; | ||
|
|
||
| private List<Book> books; | ||
|
|
||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public void setName(String name) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| public List<Book> getBooks() { | ||
| return books; | ||
| } | ||
|
|
||
| public void setBooks(List<Book> books) { | ||
| this.books = books; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public class Book { | ||
|
|
||
| private Long id; | ||
| private String title; | ||
|
|
||
| private Author author; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getTitle() { | ||
| return title; | ||
| } | ||
|
|
||
| public void setTitle(String title) { | ||
| this.title = title; | ||
| } | ||
|
|
||
| public Author getAuthor() { | ||
| return author; | ||
| } | ||
|
|
||
| public void setAuthor(Author author) { | ||
| this.author = author; | ||
| } | ||
| } | ||
|
|
||
| public interface AuthorRepository extends JpaRepository<Author, Long> { | ||
|
|
||
| } | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| package org.greencodeinitiative.creedengo.java.checks; | ||
|
|
||
| import org.sonar.check.Rule; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.semantic.MethodMatchers; | ||
| import org.sonar.plugins.java.api.semantic.Symbol; | ||
| import org.sonar.plugins.java.api.semantic.Symbol.VariableSymbol; | ||
| import org.sonar.plugins.java.api.tree.*; | ||
|
|
||
| import java.util.*; | ||
|
|
||
| @Rule(key = "GCI604") | ||
| public class AvoidNPlusOneProblemInJPAEntitiesCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
| protected static final String RULE_MESSAGE = " Detection of the \"N+1 problem\" on Spring Data JPA repositories "; | ||
|
|
||
| private static final String SPRING_REPOSITORY = "org.springframework.data.repository.Repository"; | ||
|
|
||
| private static final MethodMatchers SPRING_REPOSITORY_METHOD_FIND_ALL = | ||
| MethodMatchers.create() | ||
| .ofSubTypes(SPRING_REPOSITORY) | ||
| .names("findAll") | ||
| .withAnyParameters() | ||
| .build(); | ||
|
|
||
| private final Map<Symbol, Tree> repositoryFindAllVars = new HashMap<>(); | ||
|
|
||
| @Override | ||
| public List<Tree.Kind> nodesToVisit() { | ||
| return Arrays.asList(Tree.Kind.VARIABLE, Tree.Kind.METHOD_INVOCATION, Tree.Kind.FOR_EACH_STATEMENT); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| if (tree.is(Tree.Kind.VARIABLE)) { | ||
| VariableTree variableTree = (VariableTree) tree; | ||
| ExpressionTree initializer = variableTree.initializer(); | ||
| if (initializer != null && initializer.is(Tree.Kind.METHOD_INVOCATION)) { | ||
| MethodInvocationTree methodInvocation = (MethodInvocationTree) initializer; | ||
| if (SPRING_REPOSITORY_METHOD_FIND_ALL.matches(methodInvocation)) { | ||
| VariableSymbol symbol = (VariableSymbol) variableTree.symbol(); | ||
| repositoryFindAllVars.put(symbol, tree); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Cas d'un foreach sur une variable issue de findAll() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please us english in comments in all files |
||
| if (tree.is(Tree.Kind.FOR_EACH_STATEMENT)) { | ||
| ForEachStatement forEach = (ForEachStatement) tree; | ||
| ExpressionTree iterable = forEach.expression(); | ||
| if (iterable.is(Tree.Kind.IDENTIFIER)) { | ||
| Symbol symbol = ((IdentifierTree) iterable).symbol(); | ||
| if (repositoryFindAllVars.containsKey(symbol)) { | ||
| // On marque la variable d'itération comme issue d'un findAll() | ||
| VariableSymbol loopVar = (VariableSymbol) forEach.variable().symbol(); | ||
| repositoryFindAllVars.put(loopVar, tree); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Détection d'un appel de getter sur une variable issue de findAll() | ||
| if (tree.is(Tree.Kind.METHOD_INVOCATION)) { | ||
| MethodInvocationTree methodInvocation = (MethodInvocationTree) tree; | ||
|
|
||
| // Check if the call is something like post.getAuthor() or post.getAuthor().getName() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comments contains example and not the algorithms description |
||
| ExpressionTree select = methodInvocation.methodSelect(); | ||
| if (select.is(Tree.Kind.MEMBER_SELECT)) { | ||
| MemberSelectExpressionTree memberSelect = (MemberSelectExpressionTree) select; | ||
| ExpressionTree root = memberSelect.expression(); | ||
|
|
||
| if (root.is(Tree.Kind.IDENTIFIER)) { | ||
| Symbol symbol = ((IdentifierTree) root).symbol(); | ||
| if (repositoryFindAllVars.containsKey(symbol) && isGetter(memberSelect.identifier().name())) { | ||
| reportIssue(methodInvocation, RULE_MESSAGE); | ||
| } | ||
| } | ||
|
|
||
| // Handle nested getter chains (e.g., post.getAuthor().getName()) | ||
| if (root.is(Tree.Kind.METHOD_INVOCATION)) { | ||
| MethodInvocationTree rootInvocation = (MethodInvocationTree) root; | ||
| ExpressionTree deeperSelect = rootInvocation.methodSelect(); | ||
| if (deeperSelect.is(Tree.Kind.MEMBER_SELECT)) { | ||
| MemberSelectExpressionTree deeperMemberSelect = (MemberSelectExpressionTree) deeperSelect; | ||
| ExpressionTree deeperRoot = deeperMemberSelect.expression(); | ||
|
|
||
| if (deeperRoot.is(Tree.Kind.IDENTIFIER)) { | ||
| Symbol rootSymbol = ((IdentifierTree) deeperRoot).symbol(); | ||
| if (repositoryFindAllVars.containsKey(rootSymbol) && isGetter(deeperMemberSelect.identifier().name())) { | ||
| reportIssue(methodInvocation, RULE_MESSAGE); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Méthode utilitaire pour détecter un getter | ||
| private boolean isGetter(String methodName) { | ||
| return methodName.startsWith("get") && methodName.length() > 3 && Character.isUpperCase(methodName.charAt(3)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| "GCI78", | ||
| "GCI79", | ||
| "GCI82", | ||
| "GCI94" | ||
| "GCI94", | ||
| "GCI604" | ||
| ] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,109 @@ | ||
| /* | ||
| * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs | ||
| * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) | ||
| * | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| package org.greencodeinitiative.creedengo.java.checks; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.data.jpa.repository.JpaRepository; | ||
|
|
||
| import java.util.*; | ||
|
|
||
| public class AvoidNPlusOneProblemInJPAEntitiesCheckIssue { | ||
|
|
||
| @Autowired | ||
| private AuthorRepository authorRepository; | ||
|
|
||
| public List<Author> smellGetAllAuthors() { | ||
| List<Author> authors = authorRepository.findAll(); | ||
| for (Author author : authors) { | ||
| List<Book> books = author.getBooks(); // Noncompliant {{ Detection of the "N+1 problem" on Spring Data JPA repositories }} | ||
| } | ||
| return authors; | ||
| } | ||
|
|
||
|
|
||
| public class Author { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see JPA annotations to link Author and Book objects, like the usage of @entity, @id, @onetomany, ... like described here https://www.baeldung.com/spring-hibernate-n1-problem |
||
|
|
||
| private Long id; | ||
| private String name; | ||
|
|
||
| private List<Book> books; | ||
|
|
||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getName() { | ||
| return name; | ||
| } | ||
|
|
||
| public void setName(String name) { | ||
| this.name = name; | ||
| } | ||
|
|
||
| public List<Book> getBooks() { | ||
| return books; | ||
| } | ||
|
|
||
| public void setBooks(List<Book> books) { | ||
| this.books = books; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| public class Book { | ||
|
|
||
| private Long id; | ||
| private String title; | ||
|
|
||
| private Author author; | ||
|
|
||
| public Long getId() { | ||
| return id; | ||
| } | ||
|
|
||
| public void setId(Long id) { | ||
| this.id = id; | ||
| } | ||
|
|
||
| public String getTitle() { | ||
| return title; | ||
| } | ||
|
|
||
| public void setTitle(String title) { | ||
| this.title = title; | ||
| } | ||
|
|
||
| public Author getAuthor() { | ||
| return author; | ||
| } | ||
|
|
||
| public void setAuthor(Author author) { | ||
| this.author = author; | ||
| } | ||
| } | ||
|
|
||
| public interface AuthorRepository extends JpaRepository<Author, Long> { | ||
|
|
||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /* | ||
| * creedengo - Java language - Provides rules to reduce the environmental footprint of your Java programs | ||
| * Copyright © 2024 Green Code Initiative (https://green-code-initiative.org/) | ||
| * | ||
| * This program is free software: you can redistribute it and/or modify | ||
| * it under the terms of the GNU General Public License as published by | ||
| * the Free Software Foundation, either version 3 of the License, or | ||
| * (at your option) any later version. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| * GNU General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU General Public License | ||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
| */ | ||
| package org.greencodeinitiative.creedengo.java.checks; | ||
|
|
||
| import org.greencodeinitiative.creedengo.java.utils.FilesUtils; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| class AvoidNPlusOneProblemInJPAEntitiesCheckTest { | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile("src/test/files/AvoidNPlusOneProblemInJPAEntitiesCheckIssue.java") | ||
| .withCheck(new AvoidNPlusOneProblemInJPAEntitiesCheck()) | ||
| .withClassPath(FilesUtils.getClassPath("target/test-jars")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do you need this method call "withClassPath" ? |
||
| .verifyIssues(); | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete de first space in the message