-
-
Notifications
You must be signed in to change notification settings - Fork 122
Crjvm205 #122
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
Crjvm205 #122
Changes from 42 commits
8969840
5d3d380
bda60fe
36cef91
0bdc73d
2ed3514
1d68d3f
7409bc6
2341685
580dc34
d64c35b
50fe804
d87da8c
6619a8f
2187338
ef11e33
c4f8f0c
782e39f
6e9fee7
4188277
c2983a4
58055ab
08f8145
7fa808c
44b9e80
a5c0fb0
d3f2024
eeaaadc
bb43a16
071d082
06d2e10
34be768
2bf8921
2c8e50d
8f978ea
a798cab
bdcf8ea
ed6b6c4
55677d7
94fde4d
5dd3483
3785981
ba397b1
04d5ec7
beee006
ad66c3d
a8e886a
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,93 @@ | ||
| package fr.greencodeinitiative.java.checks; | ||
|
|
||
| import org.sonar.check.Priority; | ||
| 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.tree.Tree.Kind; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Rule(key = "EC80", name = "Developpement", | ||
|
Member
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. there is a new way to declare configuration of one rule : please see other rules as example |
||
| description = ForceUsingLazyFetchTypeInJPAEntity.MESSAGERULE, | ||
| priority = Priority.MINOR, | ||
| tags = {"bug"}) | ||
| public class ForceUsingLazyFetchTypeInJPAEntity extends IssuableSubscriptionVisitor { | ||
|
|
||
| protected static final String MESSAGERULE = "Force the use of LAZY FetchType"; | ||
| private static final String EAGER_KEYWORD = "EAGER"; | ||
| private static final String FETCH_KEYWORD = "fetch"; | ||
| private static final String ONE_TO_MANY = "OneToMany"; | ||
| private static final String MANY_TO_ONE = "ManyToOne"; | ||
| private static final String ONE_TO_ONE = "OneToOne"; | ||
| private static final String MANY_TO_MANY = "ManyToMany"; | ||
|
|
||
| @Override | ||
| public List<Kind> nodesToVisit() { | ||
| return List.of(Kind.VARIABLE); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitNode(Tree tree) { | ||
| VariableTree variableTree = (VariableTree) tree; | ||
| List<AnnotationTree> annotations = variableTree.modifiers().annotations(); | ||
|
Member
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. did you check if null pointer could be raised here ? 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. After searching, ModifiersTree is not annotated @nullable. Accessing annotations by this interface method is normally safe, so no intermediate checks are required. |
||
| // get all annotations on the attribute | ||
| for (AnnotationTree annotationTree : annotations) { | ||
| if (!needToCheckExplicitAnnotation(annotationTree.annotationType().symbolType().name())) { | ||
| // no Explicit annotation (@OneToMany, @ManyToOne, @ManyToMany, @OneToOne) was found, | ||
| // and we don't need to investigate further | ||
| continue; | ||
| } | ||
| Arguments arguments = annotationTree.arguments(); | ||
| performsCheck(arguments, annotationTree, tree); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * perform the check for two case : | ||
| * fetch keyword is found -> parse the annotation argument and search for eager keyword | ||
| * fetch keyword was not found -> we use the default value of fetch type by join type : | ||
| * OneToMany: Lazy | ||
| * ManyToOne: Eager | ||
| * ManyToMany: Lazy | ||
| * OneToOne: Eager | ||
| * reporting the issues if necessary | ||
| */ | ||
| private void performsCheck(Arguments arguments, AnnotationTree annotationTree, Tree tree) { | ||
|
Member
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. here, no need to give |
||
| boolean fetchFound = false; | ||
| for (ExpressionTree argument : arguments) { | ||
| AssignmentExpressionTree assignmentExpression = (AssignmentExpressionTree) argument; | ||
| IdentifierTree variable = (IdentifierTree) assignmentExpression.variable(); | ||
|
|
||
| if (!FETCH_KEYWORD.equals(variable.name())) { | ||
| // no need to continue checking this argument | ||
| continue; | ||
| } | ||
| String fetchValue = ((MemberSelectExpressionTree) assignmentExpression.expression()).identifier().name(); | ||
| fetchFound = true; | ||
| if (EAGER_KEYWORD.equals(fetchValue)) { | ||
| reportIssue(tree, MESSAGERULE); | ||
| } | ||
| } | ||
| //- The default case of the ManyToOne and the OneToOne | ||
| // the fetch keyword is not explicit | ||
| if (fetchFound) { | ||
| return; | ||
| } | ||
| String symbolType = annotationTree.annotationType().symbolType().name(); | ||
| if ((MANY_TO_ONE.equals(symbolType) || ONE_TO_ONE.equals(symbolType))) { | ||
| reportIssue(tree, MESSAGERULE); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @param symbolType the annotation type name : @OneToMany, @ManyToOne... | ||
| * @return true if searched annotation was found and checks need to be performed | ||
| */ | ||
| private boolean needToCheckExplicitAnnotation(String symbolType) { | ||
| return ONE_TO_MANY.equals(symbolType) | ||
| || MANY_TO_ONE.equals(symbolType) | ||
| || ONE_TO_ONE.equals(symbolType) | ||
| || MANY_TO_MANY.equals(symbolType); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| <p>Consider using LAZY mode on your FetchType for collections in JPA type entities. This will reduce the amount of data loaded into memory.</p> | ||
|
|
||
| <h2>Non-compliant Code Example</h2> | ||
| <pre> | ||
| @Entity | ||
| @Table(name = "purchaseOrder") | ||
| public class Order implements Serializable { | ||
|
|
||
| @OneToMany(mappedBy = "order", fetch = FetchType.EAGER) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| ... | ||
|
|
||
| } | ||
|
|
||
| </pre> | ||
| <h2>Compliant Code Example</h2> | ||
| <pre> | ||
| @Entity | ||
| @Table(name = "purchaseOrder") | ||
| public class Order implements Serializable { | ||
|
|
||
| @OneToMany(mappedBy = "order", fetch = FetchType.LAZY) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| ... | ||
|
|
||
| } | ||
|
|
||
| </pre> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| { | ||
| "title": "Force the use of FetchType LAZY on collections in Entity JPA", | ||
|
Member
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. now, it isn't the good way to declare rule properties but inside |
||
| "type": "CODE_SMELL", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "eco-design", | ||
| "performance", | ||
| "bug", | ||
| "ecocode" | ||
| ], | ||
| "defaultSeverity": "Minor" | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
|
Member
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 don't you give all use cases in a single file ? why several test files ? |
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseAllInOne implements Serializable { | ||
|
|
||
| @OneToMany(mappedBy = "myOrders", fetch = FetchType.EAGER) // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| @OneToMany(mappedBy = "myOrders", fetch = FetchType.LAZY) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| @Column(name = "ORDER", length = 50, nullable = false, unique = false) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
|
Member
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 as a new use case with default value for fetch keywork (thus, without "fetch" variable assigned) |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseCompliant implements Serializable { | ||
|
|
||
| @OneToMany(mappedBy = "myOrders", fetch = FetchType.LAZY) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseFalse implements Serializable { | ||
|
|
||
| @ManyToOne // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private Order order; | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseFalse implements Serializable { | ||
|
|
||
| @OneToOne // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private OrderInformation orderInformations; | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseFalseTest implements Serializable { | ||
|
Member
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 dont use suffix "Test" for POJO classes to not confuse with Unit Test class |
||
|
|
||
| @Column(name = "ORDER", length = 50, nullable = false, unique = false) | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseFalseTest implements Serializable { | ||
|
Member
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 dont use suffix "Test" for POJO classes to not confuse with Unit Test class |
||
|
|
||
| @ManyToOne(fetch = FetchType.EAGER) // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private Order order; | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseNonCompliant implements Serializable { | ||
|
|
||
| @OneToMany(mappedBy = "myOrders", fetch = FetchType.EAGER) // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private Set<OrderItem> items = new HashSet<OrderItem>(); | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import javax.persistence.Column; | ||
| import javax.persistence.Entity; | ||
| import javax.persistence.EnumType; | ||
| import javax.persistence.Enumerated; | ||
| import javax.persistence.GeneratedValue; | ||
| import javax.persistence.GenerationType; | ||
| import javax.persistence.Id; | ||
| import javax.persistence.Table; | ||
| import javax.persistence.Temporal; | ||
| import javax.persistence.TemporalType; | ||
| import javax.persistence.Transient; | ||
|
|
||
| @Entity | ||
| @Table(name = "myTable") | ||
| public class ForceLazyFetchTypeUseFalseTest implements Serializable { | ||
|
|
||
| @OneToOne(fetch = FetchType.EAGER) // Noncompliant {{Force the use of LAZY FetchType}} | ||
| private OrderInformation orderInformations; | ||
|
|
||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,7 @@ void checkNumberRules() { | |
| final JavaCheckRegistrar registrar = new JavaCheckRegistrar(); | ||
| registrar.register(context); | ||
|
|
||
| assertThat(context.checkClasses()).hasSize(19); | ||
| assertThat(context.checkClasses()).hasSize(20); | ||
|
Member
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. maybe value has to be changed. to recheck locally, please |
||
| assertThat(context.testCheckClasses()).isEmpty(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.