Conversation
…excessive data loading
|
This PR has been automatically marked as stale because it has no activity for 30 days. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Sonar rule (GCI1111) that enforces pagination on Hibernate queries to prevent excessive data loading.
- Adds the
DataInHibernateMustBePaginatedrule implementation - Provides unit and integration tests for compliant and non-compliant cases
- Registers the rule in the plugin registrar and updates the Sonar profile; bumps the rules-specifications version
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/greencodeinitiative/creedengo/java/checks/DataInHibernateMustBePaginated.java | New rule checking repository methods return paged collections |
| src/test/java/org/greencodeinitiative/creedengo/java/checks/DataInHibernateMustBePaginatedTest.java | Unit tests for issue detection and no-issue scenarios |
| src/test/files/DataInHibernateMustBePaginated.java | Test case triggering rule violations |
| src/test/files/DataInHibernateMustBePaginatedNoIssue.java | Test case demonstrating acceptable pagination usage |
| src/it/java/org/greencodeinitiative/creedengo/java/integration/tests/GCIRulesIT.java | Integration tests added for GCI1111 |
| src/main/resources/org/greencodeinitiative/creedengo/java/creedengo_way_profile.json | Sonar profile updated with new rule key |
| src/main/java/org/greencodeinitiative/creedengo/java/JavaCheckRegistrar.java | Plugin registrar updated to include the new rule |
| pom.xml | Updated creedengo-rules-specifications.version to 1.0-SNAPSHOT |
Comments suppressed due to low confidence (3)
src/main/java/org/greencodeinitiative/creedengo/java/checks/DataInHibernateMustBePaginated.java:91
- The method name
retrournAllDatacontains a typo and is unclear; consider renaming it toreturnsAllDataorisReturningAllDatafor clarity.
public boolean retrournAllData(String returnType) {
src/main/java/org/greencodeinitiative/creedengo/java/checks/DataInHibernateMustBePaginated.java:65
- [nitpick] The method name
extractedis non-descriptive; consider renaming it to something likecheckPaginationRequirementorreportMissingPagination.
private void extracted(boolean returnsAllData, boolean hasPaginationParam, MethodTree methodTree, boolean usesQueryAnnotation) {
pom.xml:81
- The
creedengo-rules-specificationsversion was changed from2.2.2to1.0-SNAPSHOT, which appears to be a downgrade; please confirm this is intentional to avoid compatibility issues.
<creedengo-rules-specifications.version>1.0-SNAPSHOT</creedengo-rules-specifications.version>
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
||
| import org.sonar.check.Rule; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.tree.*; | ||
| import org.sonar.plugins.java.api.semantic.Symbol; | ||
| import org.sonar.plugins.java.api.semantic.Type; | ||
|
|
||
| import java.util.Collections; | ||
| import java.util.List; | ||
|
|
There was a problem hiding this comment.
[nitpick] Duplicate import of Collections detected; remove redundant imports (Collections and List) to clean up the imports section.
| import java.util.Collections; | |
| import java.util.List; | |
| import org.sonar.check.Rule; | |
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | |
| import org.sonar.plugins.java.api.tree.*; | |
| import org.sonar.plugins.java.api.semantic.Symbol; | |
| import org.sonar.plugins.java.api.semantic.Type; | |
| import java.util.Collections; | |
| import java.util.List; | |
| import org.sonar.check.Rule; | |
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | |
| import org.sonar.plugins.java.api.tree.*; | |
| import org.sonar.plugins.java.api.semantic.Symbol; | |
| import org.sonar.plugins.java.api.semantic.Type; |
| if (returnsAllData && !hasPaginationParam) { | ||
| reportIssue(methodTree.simpleName(), | ||
| MESSAGE); | ||
| } else if (usesQueryAnnotation && returnsAllData && !hasPaginationParam) { | ||
| reportIssue(methodTree.simpleName(), | ||
| MESSAGE); |
There was a problem hiding this comment.
This else if condition is redundant because the previous if (returnsAllData && !hasPaginationParam) already covers the same logic; you can simplify by merging these branches.
| if (returnsAllData && !hasPaginationParam) { | |
| reportIssue(methodTree.simpleName(), | |
| MESSAGE); | |
| } else if (usesQueryAnnotation && returnsAllData && !hasPaginationParam) { | |
| reportIssue(methodTree.simpleName(), | |
| MESSAGE); | |
| if ((returnsAllData && !hasPaginationParam) || (usesQueryAnnotation && returnsAllData && !hasPaginationParam)) { | |
| reportIssue(methodTree.simpleName(), | |
| MESSAGE); |
There was a problem hiding this comment.
please correct this Copilot feedback
|
|
||
| private static boolean isUsesQueryAnnotation(MethodTree methodTree) { | ||
| boolean usesQueryAnnotation = methodTree.modifiers().annotations().stream() | ||
| .anyMatch(ann -> ann.annotationType().toString().equals("Query")); |
There was a problem hiding this comment.
Using annotationType().toString() may not reliably match fully qualified annotation names; consider checking the annotation's symbol (e.g., ann.annotationType().symbolType().fullyQualifiedName()) for robustness.
| .anyMatch(ann -> ann.annotationType().toString().equals("Query")); | |
| .anyMatch(ann -> "org.springframework.data.jpa.repository.Query".equals(ann.annotationType().symbolType().fullyQualifiedName())); |
| } | ||
|
|
||
|
|
||
| // Récupérer le nom de la méthode |
There was a problem hiding this comment.
please translate comments to english
| String returnType = methodTree.returnType().symbolType().toString(); | ||
|
|
||
| // Vérifier s’il retourne une collection entière (sans pagination) | ||
| boolean returnsAllData = retrournAllData(returnType); |
There was a problem hiding this comment.
typo error in method name (please use something like "isToto" because it returns a boolean
| return usesQueryAnnotation; | ||
| } | ||
|
|
||
| private static boolean isHasPaginationParam(MethodTree methodTree) { |
There was a problem hiding this comment.
please delete "is" prefix because "has" prefix is enough
|
|
||
| } | ||
|
|
||
| private void extracted(boolean returnsAllData, boolean hasPaginationParam, MethodTree methodTree, boolean usesQueryAnnotation) { |
There was a problem hiding this comment.
please use a method name more explicit ... when I read the method name I must understand what it will do
| if (returnsAllData && !hasPaginationParam) { | ||
| reportIssue(methodTree.simpleName(), | ||
| MESSAGE); | ||
| } else if (usesQueryAnnotation && returnsAllData && !hasPaginationParam) { | ||
| reportIssue(methodTree.simpleName(), | ||
| MESSAGE); |
There was a problem hiding this comment.
please correct this Copilot feedback
|
This PR has been automatically marked as stale because it has no activity for 30 days. |
3 participants
Hibernate queries must be paginated to avoid …
Issue link :