-
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?
GCI604 V.E.R.T Grc3 feature n+1 issue #108
Conversation
| public List<Author> smellGetAllAuthors() { | ||
| List<Author> authors = authorRepository.findAll(); | ||
| for (Author author : authors) { | ||
| List<Book> books = author.getBooks(); // // Noncompliant {{ Evitez le N+1 : utilisez un fetch join ou une récupération eager. }} |
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.
More precisely, it's the call to a method on getBooks() (for example, .size() or .get(5)) that triggers the initialization of the list, causing SQL queries to be executed — and that's where the problem occurs.
Calling getBooks() alone simply returns the proxy; it does not trigger any query by itself.
However, I believe that calling getBooks() is enough to justify raising the rule violation: why retrieve the proxy if nothing is done with it afterward?
That’s why we should ensure the rule’s description clearly explains why this is an issue.
✅ Agreed: no need to simulate a method call; just the getBooks() access is enough to be considered problematic.
✅ The comment should be written in English.
✅ Remove the double //.
✅ Add a proper rule description.
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.
Yes agree, changed the comment to match the specification
5d76fdd to
0b9e962
Compare
|
This PR has been automatically marked as stale because it has no activity for 30 days. |
| @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 "; |
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
| } | ||
| } | ||
|
|
||
| // Cas d'un foreach sur une variable issue de findAll() |
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 us english in comments in all files
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
comments contains example and not the algorithms description
| 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 comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this method call "withClassPath" ?
| } | ||
|
|
||
|
|
||
| public class Author { |
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.
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
sorry, but I don't understand how it works without JPA annotations ...
|
This PR has been automatically marked as stale because it has no activity for 30 days. |
Adding a new rule to check GRC3 : N+1 problem when calling a nested Object in an entity that causes multiples repository calls ->
#36