Skip to content

Crjvm205 - #122

Closed
AntoineMeheut wants to merge 47 commits into
green-code-initiative:mainfrom
AntoineMeheut:CRJVM205
Closed

Crjvm205#122
AntoineMeheut wants to merge 47 commits into
green-code-initiative:mainfrom
AntoineMeheut:CRJVM205

Conversation

@AntoineMeheut

Copy link
Copy Markdown
Contributor

CRJVM205 - Force the use of FetchType LAZY on collections in Entity JPA

  • Team: 17%
  • Analyzed language: java
  • Implemented rule: search in the java code for FetchTypes for which the developer has not set LAZY in order to avoid unnecessarily loading data clusters into memory.
  • Result of this rule: reduce the amount of useless data loaded into memory, to reduce the consumption of RAM and therefore of electricity.
  • Unit tests: passed
  • Definition of done : test in sonarqube passed with ecoCode-java-test-project and we did a merge request of the tests in ecoCode-java-test-project

Pitch : Consider using LAZY mode on your FetchType for collections in JPA type entities. This will reduce the amount of data loaded into memory. And having less data loaded into memory reduces power consumption. Any electricity not produced is good for our planet.

@dedece35

dedece35 commented Jul 3, 2023

Copy link
Copy Markdown
Member

Hi @dirdr,
no, I didn't work on this PR, sorry. Once current task is over, this is my next priority ... with two others CRJVM205 JIRAs :p

@github-actions

github-actions Bot commented Aug 3, 2023

Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@dedece35

Copy link
Copy Markdown
Member

Hi @dirdr,
sorry there is another conflict to resolve, please.
and then, check DoD list here : https://github.com/green-code-initiative/ecoCode-common/blob/main/doc/starter-pack.md#definition-of-done-of-a-pr

@github-actions

Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has no activity for 30 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@github-actions github-actions Bot added the stale label Oct 25, 2023
* OneToOne: Eager
* reporting the issues if necessary
*/
private void performsCheck(Arguments arguments, AnnotationTree annotationTree, Tree tree) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here, no need to give arguments as input parameter because it already is present inside annotationTree input parameter


import java.util.List;

@Rule(key = "EC80", name = "Developpement",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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

@@ -0,0 +1,16 @@
{
"title": "Force the use of FetchType LAZY on collections in Entity JPA",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 ecocode-rules-specifications (please see another rule as example)


@Column(name = "ORDER", length = 50, nullable = false, unique = false)
private Set<OrderItem> items = new HashSet<OrderItem>();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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)

import javax.persistence.Temporal;
import javax.persistence.TemporalType;
import javax.persistence.Transient;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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 ?

registrar.register(context);

assertThat(context.checkClasses()).hasSize(19);
assertThat(context.checkClasses()).hasSize(20);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe value has to be changed. to recheck locally, please

@dedece35

Copy link
Copy Markdown
Member

@dedece35

dedece35 commented Jan 1, 2024

Copy link
Copy Markdown
Member

to discuss in core-team :

  • good argument to prove it's a "green" rule
  • no native Sonarqube rule found

but I think we can't force developers not to use EAGER key word if they really need it.

do we refuse this rule or not ?

@jhertout

jhertout commented Jan 3, 2024

Copy link
Copy Markdown
Contributor

Hello,

we do not force the user to not use "EAGER", it is just a code smell. He can always ignore it. The true question is, when we use JPA entities, is the "EAGER" keyword a classic use? If in 90% of the case its use is justified may be we should not raise the code smell to avoid to pollute the analysis.
However, I think this rule is specific enough to not raise too much (not like a problem we had with a rule on Exceptions) and I think it is not a bad thing to throw the code smell because as I understand the rule, the usage of "EAGER" must be done in specific justified cases.

@github-actions

github-actions Bot commented Mar 4, 2024

Copy link
Copy Markdown
Contributor

This PR has been automatically marked as stale because it has no activity for 60 days.
Please add a comment if you want to keep the issue open. Thank you for your contributions!

@dedece35

dedece35 commented Aug 7, 2026

Copy link
Copy Markdown
Member

Hi @AntoineMeheut, @dirdr , @jhertout

several PR have worked on this same subject (FetchType Lazy instead of Eager for OneToMany et ManyToMany relations).

After worked on a complete analysis to check if this rule is relevant :

  • first, I found that the rule already exists in SonarQube as a built-in rule : S6904
  • secondly, no proof was found to check relevancy
  • thirdly, I decided to launch an analysis with EnergyTracer tool which can do real measures

Thus I created a new PR to :

  • add this analysis
  • clean all old references to this same subject

Here is the PR : #487

Sorry, but I have to close this PR.
thank you for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🗃️ rule rule improvment or rule development or bug 🏆 challenge2023 🏆 Work done during the ecoCode Challenge 2023 java 🚀 enhancement New feature or request

Projects

Development

Successfully merging this pull request may close these issues.

7 participants