From 00c60e17ece64e164bf455efd749e4d98caf4585 Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Fri, 27 Oct 2023 17:51:09 +0200 Subject: [PATCH 1/7] [ISSUE 112] get stale code of PRs 151 and 157 to merge them into this branch and upgrade code --- CHANGELOG.md | 1 + RULES.md | 2 +- .../src/main/rules/EC1/EC1.json | 2 +- .../src/main/rules/EC1/java/EC1.asciidoc | 19 ++- .../java/JavaCheckRegistrar.java | 4 +- .../AvoidSpringRepositoryCallInLoopCheck.java | 66 ---------- ...ringRepositoryCallInLoopOrStreamCheck.java | 115 ++++++++++++++++++ .../AvoidSpringRepositoryCallInLoopCheck.java | 2 +- ...voidSpringRepositoryCallInStreamCheck.java | 94 ++++++++++++++ .../java/JavaCheckRegistrarTest.java | 1 + ...idSpringRepositoryCallInLoopCheckTest.java | 2 +- ...SpringRepositoryCallInStreamCheckTest.java | 35 ++++++ 12 files changed, 270 insertions(+), 73 deletions(-) delete mode 100644 java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheck.java create mode 100644 java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java create mode 100644 java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java create mode 100644 java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInStreamCheckTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 56a05ed33..8ebba73b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#140](https://github.com/green-code-initiative/ecoCode/issues/140) Upgrade rule EC3 for Python : no implementation possible for python - [#136](https://github.com/green-code-initiative/ecoCode/issues/136) Upgrade rule EC53 for Python : no implementation possible for python - [#128](https://github.com/green-code-initiative/ecoCode/pull/128) Adding EC35 rule for Python and PHP : EC35 rule replaces EC34 with a specific use case ("file not found" sepcific) +- [#112](https://github.com/green-code-initiative/ecoCode/issues/112) Updating EC1 rule to add controls on streams ### Deleted diff --git a/RULES.md b/RULES.md index bb4581d9e..6d1d739e1 100644 --- a/RULES.md +++ b/RULES.md @@ -36,7 +36,7 @@ Some are applicable for different technologies. | EC69 | Calling a function in the declaration of a for loop | Avoid calling the function each time the loop is iterated. | [cnumr best practices (3rd edition) BP_069 (no longer exists in edition 4)](https://www.greenit.fr/2019/05/07/ecoconception-web-les-115-bonnes-pratiques-3eme-edition/) | ✅ | ✅ | 🚀 | ✅ | 🚀 | | EC72 | Perform an SQL query inside a loop | Servers are optimized to process multiple selections, insertions, or changes in a single query or transaction. consume CPU cycles, RAM, and bandwidth unnecessarily. | [cnumr best practices (3rd edition) BP_072](https://github.com/cnumr/best-practices/blob/main/chapters/BP_072_fr.md) | ✅ | ✅ | 🚀 | ✅ | 🚀 | | EC74 | Write SELECT * FROM | The database server must resolve the fields based on the schema. If you are familiar with the diagram, it is strongly recommended to name the fields. | [cnumr best practices (3rd edition) BP_074 (no longer exists in edition 4)](https://www.greenit.fr/2019/05/07/ecoconception-web-les-115-bonnes-pratiques-3eme-edition/) | ✅ | ✅ | 🚀 | ✅ | 🚀 | -| EC1 | Calling a Spring repository inside a loop | The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous energy consumption. | | ✅ | 🚫 | 🚫 | 🚫 | 🚫 | +| EC1 | Calling a Spring repository inside a loop or a stream | The use of Spring repository in a loop or a stream induces unnecessary calculations by the CPU and therefore superfluous energy consumption. | | ✅ | 🚫 | 🚫 | 🚫 | 🚫 | | EC3 | Getting the size of the collection in the loop | When iterating over any collection, fetch the size of the collection in advance to avoid fetching it on each iteration, this saves CPU cycles, and therefore consumes less power. | | ✅ | ✅ | 🚀 | 🚫 | 🚀 | | EC2 | Multiple if-else statement | Using too many conditional if-else statements will impact performance since JVM will have to compare the conditions. Prefer using a switch statement instead of multiple if-else if possible, or refactor your code to reduce conditonnal statements on the same variable. Switch statement has a performance advantage over if – else. | | ✅ | ✅ | 🚀 | 🚧 | 🚀 | | EC76 | Usage of static collections | Avoid usage of static collections. If you want to use static collections make them final and create for example a singleton if needed containing the collections. The static fields are more complicated for the Garbage Collector to manage and can lead to memory leaks. | | ✅ | 🚫 | 🚫 | 🚫 | 🚫 | diff --git a/ecocode-rules-specifications/src/main/rules/EC1/EC1.json b/ecocode-rules-specifications/src/main/rules/EC1/EC1.json index 5b583dcfe..4e87a898c 100644 --- a/ecocode-rules-specifications/src/main/rules/EC1/EC1.json +++ b/ecocode-rules-specifications/src/main/rules/EC1/EC1.json @@ -1,5 +1,5 @@ { - "title": "Avoid Spring repository call in loop", + "title": "Avoid Spring repository call in loop or stream operations like peek, forEach, forEachOrdered, map", "type": "CODE_SMELL", "status": "ready", "remediation": { diff --git a/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc b/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc index 91ae3f7f0..072092082 100644 --- a/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc +++ b/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc @@ -1,4 +1,5 @@ -The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous energy consumption. +The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous +Also, the use of Spring repository in a stream operation like "peek, forEach, forEachOrdered, map" induces unnecessary multiple access to the database instead of single batch call. ## Noncompliant Code Example @@ -15,9 +16,25 @@ for (Integer id: ids) { } ``` +```java +List employees = new ArrayList<>(); +Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); +stream.forEach(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant + if (employee.isPresent()) { + employees.add(employee.get()); + } +}); +``` + ## Compliant Solution ```java private final List ids = Arrays.asList(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); List employees = employeeRepository.findAllById(ids); ``` + +```java +private final List ids = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10).toList(); +List employees = employeeRepository.findAllById(ids); +``` diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/JavaCheckRegistrar.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/JavaCheckRegistrar.java index b66e81cfa..f972a2023 100644 --- a/java-plugin/src/main/java/fr/greencodeinitiative/java/JavaCheckRegistrar.java +++ b/java-plugin/src/main/java/fr/greencodeinitiative/java/JavaCheckRegistrar.java @@ -28,7 +28,7 @@ import fr.greencodeinitiative.java.checks.AvoidRegexPatternNotStatic; import fr.greencodeinitiative.java.checks.AvoidSQLRequestInLoop; import fr.greencodeinitiative.java.checks.AvoidSetConstantInBatchUpdate; -import fr.greencodeinitiative.java.checks.AvoidSpringRepositoryCallInLoopCheck; +import fr.greencodeinitiative.java.checks.AvoidSpringRepositoryCallInLoopOrStreamCheck; import fr.greencodeinitiative.java.checks.AvoidStatementForDMLQueries; import fr.greencodeinitiative.java.checks.AvoidUsageOfStaticCollections; import fr.greencodeinitiative.java.checks.AvoidUsingGlobalVariablesCheck; @@ -60,7 +60,7 @@ public class JavaCheckRegistrar implements CheckRegistrar { AvoidRegexPatternNotStatic.class, NoFunctionCallWhenDeclaringForLoop.class, AvoidStatementForDMLQueries.class, - AvoidSpringRepositoryCallInLoopCheck.class, + AvoidSpringRepositoryCallInLoopOrStreamCheck.class, AvoidSQLRequestInLoop.class, AvoidFullSQLRequest.class, UseCorrectForLoop.class, diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheck.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheck.java deleted file mode 100644 index e95d75e98..000000000 --- a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheck.java +++ /dev/null @@ -1,66 +0,0 @@ -/* - * ecoCode - Java language - Provides rules to reduce the environmental footprint of your Java programs - * Copyright © 2023 Green Code Initiative (https://www.ecocode.io) - * - * 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 . - */ -package fr.greencodeinitiative.java.checks; - -import java.util.Arrays; -import java.util.List; - -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.tree.BaseTreeVisitor; -import org.sonar.plugins.java.api.tree.MethodInvocationTree; -import org.sonar.plugins.java.api.tree.Tree; -import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey; - -@Rule(key = "EC1") -@DeprecatedRuleKey(repositoryKey = "greencodeinitiative-java", ruleKey = "GRC1") -public class AvoidSpringRepositoryCallInLoopCheck extends IssuableSubscriptionVisitor { - - protected static final String RULE_MESSAGE = "Avoid Spring repository call in loop"; - - private static final String SPRING_REPOSITORY = "org.springframework.data.repository.Repository"; - - private static final MethodMatchers REPOSITORY_METHOD = - MethodMatchers.create().ofSubTypes(SPRING_REPOSITORY).anyName().withAnyParameters() - .build(); - - private final AvoidSpringRepositoryCallInLoopCheckVisitor visitorInFile = new AvoidSpringRepositoryCallInLoopCheckVisitor(); - - @Override - public List nodesToVisit() { - return Arrays.asList(Tree.Kind.FOR_EACH_STATEMENT, Tree.Kind.FOR_STATEMENT, Tree.Kind.WHILE_STATEMENT); - } - - @Override - public void visitNode(Tree tree) { - tree.accept(visitorInFile); - } - - private class AvoidSpringRepositoryCallInLoopCheckVisitor extends BaseTreeVisitor { - @Override - public void visitMethodInvocation(MethodInvocationTree tree) { - if (REPOSITORY_METHOD.matches(tree)) { - reportIssue(tree, RULE_MESSAGE); - } else { - super.visitMethodInvocation(tree); - } - } - - } -} diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java new file mode 100644 index 000000000..3d7bc8e7b --- /dev/null +++ b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java @@ -0,0 +1,115 @@ +/* + * ecoCode - Java language - Provides rules to reduce the environmental footprint of your Java programs + * Copyright © 2023 Green Code Initiative (https://www.ecocode.io) + * + * 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 . + */ +package fr.greencodeinitiative.java.checks; + +import java.util.Arrays; +import java.util.List; + +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.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.LambdaExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey; + +@Rule(key = "EC1") +@DeprecatedRuleKey(repositoryKey = "greencodeinitiative-java", ruleKey = "GRC1") +public class AvoidSpringRepositoryCallInLoopOrStreamCheck extends IssuableSubscriptionVisitor { + + protected static final String RULE_MESSAGE = "Avoid Spring repository call in loop or stream"; + + private static final String BASE_STREAM = "java.util.stream.BaseStream"; + private static final String SPRING_REPOSITORY = "org.springframework.data.repository.Repository"; + + private static final MethodMatchers SPRING_REPOSITORY_METHOD = + MethodMatchers + .create() + .ofSubTypes(SPRING_REPOSITORY) + .anyName() + .withAnyParameters() + .build(); + + private static final MethodMatchers STREAM_FOREACH_METHOD = + MethodMatchers + .create() + .ofSubTypes(BASE_STREAM) + .names("forEach", "forEachOrdered", "map", "peek") + .withAnyParameters() + .build(); + + private final AvoidSpringRepositoryCallInLoopCheckVisitor visitorInFile = new AvoidSpringRepositoryCallInLoopCheckVisitor(); + private final StreamVisitor streamVisitor = new StreamVisitor(); + + @Override + public List nodesToVisit() { + return Arrays.asList( + Tree.Kind.FOR_EACH_STATEMENT, // loop + Tree.Kind.FOR_STATEMENT, // loop + Tree.Kind.WHILE_STATEMENT, // loop + Tree.Kind.DO_STATEMENT, // loop + Tree.Kind.METHOD_INVOCATION // stream + ); + } + + @Override + public void visitNode(Tree tree) { + if (tree.is(Tree.Kind.METHOD_INVOCATION)) { // stream process + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) tree; + if (STREAM_FOREACH_METHOD.matches(methodInvocationTree)) { + tree.accept(streamVisitor); + } + } else { // loops process + tree.accept(visitorInFile); + } + } + + private class AvoidSpringRepositoryCallInLoopCheckVisitor extends BaseTreeVisitor { + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + if (SPRING_REPOSITORY_METHOD.matches(tree)) { + reportIssue(tree, RULE_MESSAGE); + } else { + super.visitMethodInvocation(tree); + } + } + + } + + private class StreamVisitor extends BaseTreeVisitor { + private final LambdaVisitor lambdaVisitor = new LambdaVisitor(); + + @Override + public void visitLambdaExpression(LambdaExpressionTree tree) { + tree.accept(lambdaVisitor); + } + + } + + private class LambdaVisitor extends BaseTreeVisitor { + + @Override + public void visitMethodInvocation(MethodInvocationTree tree) { + if (SPRING_REPOSITORY_METHOD.matches(tree)) { + reportIssue(tree, RULE_MESSAGE); + } + } + + } +} diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java index d2ae74e8a..349f97f58 100644 --- a/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java @@ -30,7 +30,7 @@ public class AvoidRepositoryCallInLoopCheck { public List smellGetAllEmployeesByIds(List ids) { List employees = new ArrayList<>(); for (Integer id : ids) { - Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop}} + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} if (employee.isPresent()) { employees.add(employee.get()); } diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java new file mode 100644 index 000000000..a5cc4360f --- /dev/null +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java @@ -0,0 +1,94 @@ +package fr.greencodeinitiative.java.checks; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.data.jpa.repository.JpaRepository; + +import java.util.*; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; + +public class AvoidSpringRepositoryCallInStreamCheck { + + @Autowired + private EmployeeRepository employeeRepository; + +// public void smellGetAllEmployeesByIdsForEach() { +// List employees = new ArrayList<>(); +// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); +// stream.forEach(id -> { +// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} +// if (employee.isPresent()) { +// employees.add(employee.get()); +// } +// }); +// } +// +// public void smellGetAllEmployeesByIdsForEachOrdered() { +// List employees = new ArrayList<>(); +// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); +// stream.forEachOrdered(id -> { +// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} +// if (employee.isPresent()) { +// employees.add(employee.get()); +// } +// }); +// } +// +// public void smellGetAllEmployeesByIdsMap() { +// List employees = new ArrayList<>(); +// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); +// stream.map(id -> { +// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} +// if (employee.isPresent()) { +// employees.add(employee.get()); +// } +// }); +// } +// +// public void smellGetAllEmployeesByIdsPeek() { +// List employees = new ArrayList<>(); +// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); +// stream.peek(id -> { +// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} +// if (employee.isPresent()) { +// employees.add(employee.get()); +// } +// }); +// } +// +// public List smellGetAllEmployeesByIds(List ids) { +// return ids +// .stream() +// .map(element -> { +// return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} +// }) +// .collect(Collectors.toList()); +// } +// + public List smellGetAllEmployeesByIds(List ids) { + Stream stream = ids.stream(); + return stream.map(element -> { + return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} + }) + .collect(Collectors.toList()); + } + +// public List smellGetAllEmployeesByIds(List ids) { +// Stream stream = ids.stream(); +// return stream.map(element -> { +// return employeeRepository.findById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} +// }) +// .collect(Collectors.toList()); +// } + +// public List smellGetAllEmployeesByIdsWithoutStream(List ids) { +// return employeeRepository.findAllById(ids); // Compliant +// } + + public class Employee { + } + + public interface EmployeeRepository extends JpaRepository { + } +} \ No newline at end of file diff --git a/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java b/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java index db1383ae5..c38b4c09b 100644 --- a/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java +++ b/java-plugin/src/test/java/fr/greencodeinitiative/java/JavaCheckRegistrarTest.java @@ -33,6 +33,7 @@ void checkNumberRules() { assertThat(context.checkClasses()).hasSize(19); assertThat(context.testCheckClasses()).isEmpty(); + } } diff --git a/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheckTest.java b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheckTest.java index 2ea1887bd..2a855e0c3 100644 --- a/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheckTest.java +++ b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopCheckTest.java @@ -27,7 +27,7 @@ class AvoidSpringRepositoryCallInLoopCheckTest { void test() { CheckVerifier.newVerifier() .onFile("src/test/files/AvoidSpringRepositoryCallInLoopCheck.java") - .withCheck(new AvoidSpringRepositoryCallInLoopCheck()) + .withCheck(new AvoidSpringRepositoryCallInLoopOrStreamCheck()) .withClassPath(FilesUtils.getClassPath("target/test-jars")) .verifyIssues(); } diff --git a/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInStreamCheckTest.java b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInStreamCheckTest.java new file mode 100644 index 000000000..e9fd4f354 --- /dev/null +++ b/java-plugin/src/test/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInStreamCheckTest.java @@ -0,0 +1,35 @@ +/* + * ecoCode - Java language - Provides rules to reduce the environmental footprint of your Java programs + * Copyright © 2023 Green Code Initiative (https://www.ecocode.io) + * + * 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 . + */ +package fr.greencodeinitiative.java.checks; + +import fr.greencodeinitiative.java.utils.FilesUtils; +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +class AvoidSpringRepositoryCallInStreamCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile("src/test/files/AvoidSpringRepositoryCallInStreamCheck.java") + .withCheck(new AvoidSpringRepositoryCallInLoopOrStreamCheck()) + .withClassPath(FilesUtils.getClassPath("target/test-jars")) + .verifyIssues(); + } + +} From af90da094277e55cf4e0306d800d2a3ca1affa7e Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Tue, 28 Nov 2023 22:13:59 +0100 Subject: [PATCH 2/7] [ISSUE 112] correct TU header --- .../AvoidSpringRepositoryCallInStreamCheck.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java index a5cc4360f..e5f29fc15 100644 --- a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java @@ -1,3 +1,20 @@ +/* + * ecoCode - Java language - Provides rules to reduce the environmental footprint of your Java programs + * Copyright © 2023 Green Code Initiative (https://www.ecocode.io) + * + * 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 . + */ package fr.greencodeinitiative.java.checks; import org.springframework.beans.factory.annotation.Autowired; From 0e6b473ad2498be4762ab8ae4363849e57b42662 Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Fri, 1 Dec 2023 20:38:07 +0100 Subject: [PATCH 3/7] [ISSUE 112] improve stream methods detection + TUs --- ...ringRepositoryCallInLoopOrStreamCheck.java | 21 ++- ...voidSpringRepositoryCallInStreamCheck.java | 144 ++++++++++-------- 2 files changed, 94 insertions(+), 71 deletions(-) diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java index 3d7bc8e7b..a03d3af1e 100644 --- a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java +++ b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java @@ -23,10 +23,7 @@ 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.tree.BaseTreeVisitor; -import org.sonar.plugins.java.api.tree.LambdaExpressionTree; -import org.sonar.plugins.java.api.tree.MethodInvocationTree; -import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.*; import org.sonarsource.analyzer.commons.annotations.DeprecatedRuleKey; @Rule(key = "EC1") @@ -57,6 +54,8 @@ public class AvoidSpringRepositoryCallInLoopOrStreamCheck extends IssuableSubscr private final AvoidSpringRepositoryCallInLoopCheckVisitor visitorInFile = new AvoidSpringRepositoryCallInLoopCheckVisitor(); private final StreamVisitor streamVisitor = new StreamVisitor(); + private final AncestorMethodVisitor ancestorMethodVisitor = new AncestorMethodVisitor(); + @Override public List nodesToVisit() { return Arrays.asList( @@ -93,23 +92,31 @@ public void visitMethodInvocation(MethodInvocationTree tree) { } private class StreamVisitor extends BaseTreeVisitor { - private final LambdaVisitor lambdaVisitor = new LambdaVisitor(); @Override public void visitLambdaExpression(LambdaExpressionTree tree) { - tree.accept(lambdaVisitor); + tree.accept(ancestorMethodVisitor); } } - private class LambdaVisitor extends BaseTreeVisitor { + private class AncestorMethodVisitor extends BaseTreeVisitor { @Override public void visitMethodInvocation(MethodInvocationTree tree) { if (SPRING_REPOSITORY_METHOD.matches(tree)) { reportIssue(tree, RULE_MESSAGE); + } else { + if (tree.methodSelect().is(Tree.Kind.MEMBER_SELECT)) { + MemberSelectExpressionTree memberSelectTree = (MemberSelectExpressionTree) tree.methodSelect(); + if ( memberSelectTree.expression().is(Tree.Kind.METHOD_INVOCATION)) { + MethodInvocationTree methodInvocationTree = (MethodInvocationTree) memberSelectTree.expression(); + methodInvocationTree.accept(ancestorMethodVisitor); + } + } } } } + } diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java index e5f29fc15..7c1e8f5a8 100644 --- a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java @@ -30,59 +30,59 @@ public class AvoidSpringRepositoryCallInStreamCheck { @Autowired private EmployeeRepository employeeRepository; -// public void smellGetAllEmployeesByIdsForEach() { -// List employees = new ArrayList<>(); -// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); -// stream.forEach(id -> { -// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} -// if (employee.isPresent()) { -// employees.add(employee.get()); -// } -// }); -// } -// -// public void smellGetAllEmployeesByIdsForEachOrdered() { -// List employees = new ArrayList<>(); -// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); -// stream.forEachOrdered(id -> { -// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} -// if (employee.isPresent()) { -// employees.add(employee.get()); -// } -// }); -// } -// -// public void smellGetAllEmployeesByIdsMap() { -// List employees = new ArrayList<>(); -// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); -// stream.map(id -> { -// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} -// if (employee.isPresent()) { -// employees.add(employee.get()); -// } -// }); -// } -// -// public void smellGetAllEmployeesByIdsPeek() { -// List employees = new ArrayList<>(); -// Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); -// stream.peek(id -> { -// Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} -// if (employee.isPresent()) { -// employees.add(employee.get()); -// } -// }); -// } -// -// public List smellGetAllEmployeesByIds(List ids) { -// return ids -// .stream() -// .map(element -> { -// return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} -// }) -// .collect(Collectors.toList()); -// } -// + public void smellGetAllEmployeesByIdsForEach() { + List employees = new ArrayList<>(); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + stream.forEach(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }); + } + + public void smellGetAllEmployeesByIdsForEachOrdered() { + List employees = new ArrayList<>(); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + stream.forEachOrdered(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }); + } + + public void smellGetAllEmployeesByIdsMap() { + List employees = new ArrayList<>(); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + stream.map(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }); + } + + public void smellGetAllEmployeesByIdsPeek() { + List employees = new ArrayList<>(); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + stream.peek(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }); + } + + public List smellGetAllEmployeesByIds(List ids) { + return ids + .stream() + .map(element -> { + return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} + }) + .collect(Collectors.toList()); + } + public List smellGetAllEmployeesByIds(List ids) { Stream stream = ids.stream(); return stream.map(element -> { @@ -91,17 +91,33 @@ public List smellGetAllEmployeesByIds(List ids) { .collect(Collectors.toList()); } -// public List smellGetAllEmployeesByIds(List ids) { -// Stream stream = ids.stream(); -// return stream.map(element -> { -// return employeeRepository.findById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} -// }) -// .collect(Collectors.toList()); -// } - -// public List smellGetAllEmployeesByIdsWithoutStream(List ids) { -// return employeeRepository.findAllById(ids); // Compliant -// } + public List smellGetAllEmployeesByIds(List ids) { + Stream stream = ids.stream(); + return stream.map(element -> { + return employeeRepository.findById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} + }) + .collect(Collectors.toList()); + } + + public List smellGetAllEmployeesByIdsWithoutStream(List ids) { + return employeeRepository.findAllById(ids); // Compliant + } + + public List smellDeleteEmployeeById(List ids) { + Stream stream = ids.stream(); + return stream.map(element -> { + return employeeRepository.deleteById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} + }) + .collect(Collectors.toList()); + } + + public List smellGetAllEmployeesByIdsWithSeveralMethods(List ids) { + Stream stream = ids.stream(); + return stream.map(element -> { + return employeeRepository.findById(element).orElse(null).anotherMethod().anotherOne();// Noncompliant {{Avoid Spring repository call in loop or stream}} + }) + .collect(Collectors.toList()); + } public class Employee { } From 365dbac33a13844f0a63b71d17547c57d0bfb567 Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Fri, 1 Dec 2023 20:45:29 +0100 Subject: [PATCH 4/7] [ISSUE 112] improve code documentation --- .../checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java index a03d3af1e..3222799ca 100644 --- a/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java +++ b/java-plugin/src/main/java/fr/greencodeinitiative/java/checks/AvoidSpringRepositoryCallInLoopOrStreamCheck.java @@ -74,7 +74,7 @@ public void visitNode(Tree tree) { if (STREAM_FOREACH_METHOD.matches(methodInvocationTree)) { tree.accept(streamVisitor); } - } else { // loops process + } else { // loop process tree.accept(visitorInFile); } } @@ -104,9 +104,10 @@ private class AncestorMethodVisitor extends BaseTreeVisitor { @Override public void visitMethodInvocation(MethodInvocationTree tree) { + // if the method is a spring repository method, report an issue if (SPRING_REPOSITORY_METHOD.matches(tree)) { reportIssue(tree, RULE_MESSAGE); - } else { + } else { // else, check if the method is a method invocation and check recursively if (tree.methodSelect().is(Tree.Kind.MEMBER_SELECT)) { MemberSelectExpressionTree memberSelectTree = (MemberSelectExpressionTree) tree.methodSelect(); if ( memberSelectTree.expression().is(Tree.Kind.METHOD_INVOCATION)) { From 0f3049f39ea74f466c1a88101b44e4177ffc239e Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Fri, 1 Dec 2023 22:12:55 +0100 Subject: [PATCH 5/7] [ISSUE 112] update use case test files to correct other errors --- .../AvoidSpringRepositoryCallInLoopCheck.java | 15 +++- ...voidSpringRepositoryCallInStreamCheck.java | 76 +++++++++++-------- 2 files changed, 56 insertions(+), 35 deletions(-) diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java index 349f97f58..a18b484fd 100644 --- a/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInLoopCheck.java @@ -21,9 +21,8 @@ import org.springframework.data.jpa.repository.JpaRepository; import java.util.*; -import java.util.stream.Collectors; -public class AvoidRepositoryCallInLoopCheck { +public class AvoidSpringRepositoryCallInLoopCheck { @Autowired private EmployeeRepository employeeRepository; @@ -39,10 +38,20 @@ public List smellGetAllEmployeesByIds(List ids) { } public class Employee { + private Integer id; + private String name; + + public Employee(Integer id, String name) { + this.id = id; + this.name = name; + } + + public Integer getId() { return id; } + public String getName() { return name; } } public interface EmployeeRepository extends JpaRepository { } - + } \ No newline at end of file diff --git a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java index 7c1e8f5a8..35dc70ce7 100644 --- a/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java +++ b/java-plugin/src/test/files/AvoidSpringRepositoryCallInStreamCheck.java @@ -32,7 +32,7 @@ public class AvoidSpringRepositoryCallInStreamCheck { public void smellGetAllEmployeesByIdsForEach() { List employees = new ArrayList<>(); - Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); stream.forEach(id -> { Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} if (employee.isPresent()) { @@ -43,7 +43,7 @@ public void smellGetAllEmployeesByIdsForEach() { public void smellGetAllEmployeesByIdsForEachOrdered() { List employees = new ArrayList<>(); - Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); stream.forEachOrdered(id -> { Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} if (employee.isPresent()) { @@ -52,48 +52,47 @@ public void smellGetAllEmployeesByIdsForEachOrdered() { }); } - public void smellGetAllEmployeesByIdsMap() { + public List smellGetAllEmployeesByIdsMap() { List employees = new ArrayList<>(); - Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); - stream.map(id -> { - Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} - if (employee.isPresent()) { - employees.add(employee.get()); - } - }); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + return stream.map(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }) + .collect(Collectors.toList()); } - public void smellGetAllEmployeesByIdsPeek() { + public List smellGetAllEmployeesByIdsPeek() { List employees = new ArrayList<>(); - Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); - stream.peek(id -> { - Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} - if (employee.isPresent()) { - employees.add(employee.get()); - } - }); + Stream stream = Stream.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + return stream.peek(id -> { + Optional employee = employeeRepository.findById(id); // Noncompliant {{Avoid Spring repository call in loop or stream}} + if (employee.isPresent()) { + employees.add(employee.get()); + } + }) + .collect(Collectors.toList()); } - public List smellGetAllEmployeesByIds(List ids) { + public List smellGetAllEmployeesByIdsWithOptional(List ids) { + List employees = new ArrayList<>(); return ids .stream() .map(element -> { - return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} - }) - .collect(Collectors.toList()); - } - - public List smellGetAllEmployeesByIds(List ids) { - Stream stream = ids.stream(); - return stream.map(element -> { - return employeeRepository.findById(element).orElse(null);// Noncompliant {{Avoid Spring repository call in loop or stream}} + Employee empl = new Employee(); + employees.add(empl); + return employeeRepository.findById(element).orElse(empl);// Noncompliant {{Avoid Spring repository call in loop or stream}} }) .collect(Collectors.toList()); } public List smellGetAllEmployeesByIds(List ids) { - Stream stream = ids.stream(); + Stream stream = ids.stream(); return stream.map(element -> { + Employee empl = new Employee(); + employees.add(empl); return employeeRepository.findById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} }) .collect(Collectors.toList()); @@ -104,22 +103,35 @@ public List smellGetAllEmployeesByIdsWithoutStream(List ids) } public List smellDeleteEmployeeById(List ids) { - Stream stream = ids.stream(); + Stream stream = ids.stream(); return stream.map(element -> { + Employee empl = new Employee(); + employees.add(empl); return employeeRepository.deleteById(element);// Noncompliant {{Avoid Spring repository call in loop or stream}} }) .collect(Collectors.toList()); } public List smellGetAllEmployeesByIdsWithSeveralMethods(List ids) { - Stream stream = ids.stream(); + Stream stream = ids.stream(); return stream.map(element -> { - return employeeRepository.findById(element).orElse(null).anotherMethod().anotherOne();// Noncompliant {{Avoid Spring repository call in loop or stream}} + Employee empl = new Employee(); + return employeeRepository.findById(element).orElse(empl).anotherMethod().anotherOne();// Noncompliant {{Avoid Spring repository call in loop or stream}} }) .collect(Collectors.toList()); } public class Employee { + private Integer id; + private String name; + + public Employee(Integer id, String name) { + this.id = id; + this.name = name; + } + + public Integer getId() { return id; } + public String getName() { return name; } } public interface EmployeeRepository extends JpaRepository { From 5b9029ca74f0409a6ee102e70573780e8dade7ce Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Mon, 4 Dec 2023 09:57:55 +0100 Subject: [PATCH 6/7] Update ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc Co-authored-by: Maxime Malgorn <9255967+utarwyn@users.noreply.github.com> --- .../src/main/rules/EC1/java/EC1.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc b/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc index 072092082..524e8d0c2 100644 --- a/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc +++ b/ecocode-rules-specifications/src/main/rules/EC1/java/EC1.asciidoc @@ -1,4 +1,4 @@ -The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous +The use of Spring repository in a loop induces unnecessary calculations by the CPU and therefore superfluous energy consumption. Also, the use of Spring repository in a stream operation like "peek, forEach, forEachOrdered, map" induces unnecessary multiple access to the database instead of single batch call. ## Noncompliant Code Example From d1a93d3bdce2b062772024cfcab67ae9e9de00f1 Mon Sep 17 00:00:00 2001 From: David DE CARVALHO Date: Mon, 4 Dec 2023 09:58:54 +0100 Subject: [PATCH 7/7] Update ecocode-rules-specifications/src/main/rules/EC1/EC1.json Co-authored-by: Maxime Malgorn <9255967+utarwyn@users.noreply.github.com> --- ecocode-rules-specifications/src/main/rules/EC1/EC1.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ecocode-rules-specifications/src/main/rules/EC1/EC1.json b/ecocode-rules-specifications/src/main/rules/EC1/EC1.json index 4e87a898c..826bb1dda 100644 --- a/ecocode-rules-specifications/src/main/rules/EC1/EC1.json +++ b/ecocode-rules-specifications/src/main/rules/EC1/EC1.json @@ -1,5 +1,5 @@ { - "title": "Avoid Spring repository call in loop or stream operations like peek, forEach, forEachOrdered, map", + "title": "Avoid Spring repository call in loop or stream operations", "type": "CODE_SMELL", "status": "ready", "remediation": {