From 6b43b30d0ca29ed290b72def5dfa7ee74ac95c24 Mon Sep 17 00:00:00 2001 From: Angelo Buono Date: Thu, 2 Nov 2023 15:56:56 +0100 Subject: [PATCH] SONARJAVA-4678: mplement S6831: @Qualifier should not be used on @Bean methods (#4518) --- .../java/org/sonar/java/it/AutoScanTest.java | 4 +- .../autoscan/autoscan-diff-by-rules.json | 14 +- ...voidQualifierOnBeanMethodsCheckSample.java | 76 +++++++++++ .../java/org/sonar/java/checks/CheckList.java | 2 + .../AvoidQualifierOnBeanMethodsCheck.java | 121 ++++++++++++++++++ .../org/sonar/l10n/java/rules/java/S6831.html | 82 ++++++++++++ .../org/sonar/l10n/java/rules/java/S6831.json | 24 ++++ .../java/rules/java/Sonar_way_profile.json | 1 + .../AvoidQualifierOnBeanMethodsCheckTest.java | 47 +++++++ 9 files changed, 365 insertions(+), 6 deletions(-) create mode 100644 java-checks-test-sources/src/main/java/checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheck.java create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.html create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.json create mode 100644 java-checks/src/test/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheckTest.java diff --git a/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java b/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java index d481e2f0232..dce4771dc97 100644 --- a/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java +++ b/its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java @@ -180,7 +180,7 @@ public void javaCheckTestSources() throws Exception { softly.assertThat(newTotal).isEqualTo(knownTotal); softly.assertThat(rulesCausingFPs).hasSize(6); softly.assertThat(rulesNotReporting).hasSize(7); - softly.assertThat(rulesSilenced).hasSize(77); + softly.assertThat(rulesSilenced).hasSize(78); /** * 4. Check total number of differences (FPs + FNs) @@ -188,7 +188,7 @@ public void javaCheckTestSources() throws Exception { * No differences would mean that we find the same issues with and without the bytecode and libraries */ String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences")); - softly.assertThat(differences).isEqualTo("Issues differences: 3483"); + softly.assertThat(differences).isEqualTo("Issues differences: 3489"); softly.assertAll(); } diff --git a/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json b/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json index 18c7d593d0b..cd487842983 100644 --- a/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json +++ b/its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json @@ -2880,15 +2880,15 @@ "falsePositives": 0 }, { - "ruleKey": "S6809", + "ruleKey": "S6806", "hasTruePositives": false, - "falseNegatives": 94, + "falseNegatives": 10, "falsePositives": 0 }, { - "ruleKey": "S6806", + "ruleKey": "S6809", "hasTruePositives": false, - "falseNegatives": 10, + "falseNegatives": 94, "falsePositives": 0 }, { @@ -2927,6 +2927,12 @@ "falseNegatives": 5, "falsePositives": 0 }, + { + "ruleKey": "S6831", + "hasTruePositives": false, + "falseNegatives": 6, + "falsePositives": 0 + }, { "ruleKey": "S6837", "hasTruePositives": false, diff --git a/java-checks-test-sources/src/main/java/checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java b/java-checks-test-sources/src/main/java/checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java new file mode 100644 index 00000000000..76b4e33d2bb --- /dev/null +++ b/java-checks-test-sources/src/main/java/checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java @@ -0,0 +1,76 @@ +package checks.spring; + +import org.springframework.beans.factory.annotation.Qualifier; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.stereotype.Component; + +public class AvoidQualifierOnBeanMethodsCheckSample { + + private static final String FOO = "foo"; + private static final String CAPITALIZED_FOO = "Foo"; + + @Configuration + class Configuration1 { + @Bean + @Qualifier("foo") // Noncompliant, [[sc=5;ec=22;quickfixes=qf1]] {{Remove this redundant "@Qualifier" annotation and rely on the @Bean method.}} + // fix@qf1 {{Remove "@Qualifier"}} + // edit@qf1 [[sc=5;ec=22]] {{}} + public String foo() { + return "foo"; + } + + @Bean + @Qualifier(value = "bar") // Noncompliant + public String bar() { + return "bar"; + } + + @Bean // Compliant + public String foobar() { + return "foobar"; + } + } + + @Component + class Component1 { + @Bean("foo") + @Qualifier(CAPITALIZED_FOO) // Noncompliant + public String foo() { + return "foo"; + } + + @Bean(name = "bar") + @Qualifier(value = "Bar") // Noncompliant + public String bar() { + return "bar"; + } + + @Bean("foobar") // Compliant + public String foobar() { + return "foobar"; + } + } + + class Class1 { + @Bean("foo") + @Qualifier(FOO) // Noncompliant + public String foo() { + return "foo"; + } + + @Bean(name = "bar") + @Qualifier // Noncompliant, [[sc=5;ec=15;quickfixes=qf3]] {{Remove this redundant "@Qualifier" annotation and rely on the @Bean method.}} + // fix@qf3 {{Remove "@Qualifier"}} + // edit@qf3 [[sc=5;ec=15]] {{}} + public String bar() { + return "bar"; + } + + @Bean("foobar") // Compliant + public String foobar() { + return "foobar"; + } + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java index 7a1860e4b26..e34859eab81 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/CheckList.java +++ b/java-checks/src/main/java/org/sonar/java/checks/CheckList.java @@ -140,6 +140,7 @@ import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck; import org.sonar.java.checks.spring.AutowiredOnConstructorWhenMultipleConstructorsCheck; import org.sonar.java.checks.spring.AutowiredOnMultipleConstructorsCheck; +import org.sonar.java.checks.spring.AvoidQualifierOnBeanMethodsCheck; import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck; import org.sonar.java.checks.spring.FieldDependencyInjectionCheck; import org.sonar.java.checks.spring.ModelAttributeNamingConventionForSpELCheck; @@ -290,6 +291,7 @@ public final class CheckList { AuthorizationsStrongDecisionsCheck.class, AutowiredOnConstructorWhenMultipleConstructorsCheck.class, AutowiredOnMultipleConstructorsCheck.class, + AvoidQualifierOnBeanMethodsCheck.class, AwsConsumerBuilderUsageCheck.class, AwsCredentialsShouldBeSetExplicitlyCheck.class, AwsLambdaSyncCallCheck.class, diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheck.java new file mode 100644 index 00000000000..2122f6d0f35 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheck.java @@ -0,0 +1,121 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import java.util.LinkedList; +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.QuickFixHelper; +import org.sonar.java.model.expression.AssignmentExpressionTreeImpl; +import org.sonar.java.model.expression.LiteralTreeImpl; +import org.sonar.java.reporting.JavaQuickFix; +import org.sonar.java.reporting.JavaTextEdit; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.Arguments; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S6831") +public class AvoidQualifierOnBeanMethodsCheck extends IssuableSubscriptionVisitor { + private static final String BEAN_ANNOTATION = "org.springframework.context.annotation.Bean"; + private static final String QUALIFIER_ANNOTATION = "org.springframework.beans.factory.annotation.Qualifier"; + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.METHOD); + } + + /** + * This rule reports an issue when @Bean methods are annotated with @Qualifier. + */ + @Override + public void visitNode(Tree tree) { + var methodTree = (MethodTree) tree; + + var beanAnnotation = getAnnotation(methodTree, BEAN_ANNOTATION); + var qualifierAnnotation = getAnnotation(methodTree, QUALIFIER_ANNOTATION); + + if (beanAnnotation != null && qualifierAnnotation != null) { + QuickFixHelper.newIssue(context) + .forRule(this) + .onTree(qualifierAnnotation) + .withMessage("Remove this redundant \"@Qualifier\" annotation and rely on the @Bean method.") + .withQuickFixes(() -> getQuickFix(methodTree, qualifierAnnotation)) + .report(); + } + } + + private static AnnotationTree getAnnotation(MethodTree methodTree, String annotation) { + return methodTree.modifiers() + .annotations() + .stream() + .filter(annotationTree -> annotationTree.symbolType().is(annotation)) + .findFirst() + .orElse(null); + } + + private static List getQuickFix(MethodTree methodTree, AnnotationTree qualifierAnnotation) { + List quickFixes = new LinkedList<>(); + + // quick fix only for @Qualifier annotations without arguments or with argument that matches the method name + if (isFixable(methodTree, qualifierAnnotation)) { + var quickFix = JavaQuickFix.newQuickFix("Remove \"@Qualifier\"") + .addTextEdit(JavaTextEdit.removeTree(qualifierAnnotation)) + .build(); + quickFixes.add(quickFix); + } + + return quickFixes; + } + + private static boolean isFixable(MethodTree methodTree, AnnotationTree qualifierAnnotation) { + var arguments = qualifierAnnotation.arguments(); + + // @Qualifier annotation without argument can be always removed + if (arguments.isEmpty()) { + return true; + } + + // @Qualifier that matches the method name is redundant and can be removed + var methodName = methodTree.simpleName().name(); + return getQualifierAnnotationValue(arguments).equals(methodName); + } + + private static String getQualifierAnnotationValue(Arguments arguments) { + var argument = arguments.get(0); + String qualifierAnnotationValue; + + if (argument.is(Tree.Kind.ASSIGNMENT)) { + qualifierAnnotationValue = ((LiteralTreeImpl) ((AssignmentExpressionTreeImpl) argument).expression()).value(); + } else if (argument.is(Tree.Kind.STRING_LITERAL)) { + qualifierAnnotationValue = ((LiteralTreeImpl) argument).token().text(); + } else { + // case when argument is an identifier: don't suggest a quick fix + qualifierAnnotationValue = ""; + } + + return removeQuotes(qualifierAnnotationValue); + } + + private static String removeQuotes(String value) { + return value.replace("\"", ""); + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.html new file mode 100644 index 00000000000..a6532465541 --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.html @@ -0,0 +1,82 @@ +

Why is this an issue?

+

In Spring Framework, the @Qualifier annotation is typically used to disambiguate between multiple beans of the same type when +auto-wiring dependencies. It is not necessary to use @Qualifier when defining a bean using the @Bean annotation because the +bean’s name can be explicitly specified using the name attribute or derived from the method name. Using @Qualifier on +@Bean methods can lead to confusion and redundancy. Beans should be named appropriately using either the name attribute of +the @Bean annotation or the method name itself.

+

Noncompliant code example

+
+@Configuration
+public class MyConfiguration {
+  @Bean
+  @Qualifier("myService")
+  public MyService myService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean
+  @Qualifier("betterService")
+  public MyService aBetterService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean
+  @Qualifier("evenBetterService")
+  public MyService anEvenBetterService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean
+  @Qualifier("differentService")
+  public MyBean aDifferentService() {
+    // ...
+    return new MyBean();
+  }
+}
+
+

Compliant solution

+
+@Configuration
+public class MyConfiguration {
+  @Bean
+  public MyService myService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean(name="betterService")
+  public MyService aBetterService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean(name="evenBetterService")
+  public MyService anEvenBetterService() {
+    // ...
+    return new MyService();
+  }
+
+  @Bean(name="differentService")
+  public MyBean aDifferentService() {
+    // ...
+    return new MyBean();
+  }
+}
+
+

Resources

+

Documentation

+ +

Articles & blog posts

+ + diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.json new file mode 100644 index 00000000000..d19f9272c1a --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.json @@ -0,0 +1,24 @@ +{ + "title": "\"@Qualifier\" should not be used on \"@Bean\" methods", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "spring" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6831", + "sqKey": "S6831", + "scope": "Main", + "quickfix": "targeted", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM", + "RELIABILITY": "MEDIUM" + }, + "attribute": "DISTINCT" + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json index b9e5113aa1f..1c104525029 100644 --- a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json @@ -489,6 +489,7 @@ "S6817", "S6818", "S6829", + "S6831", "S6837" ] } diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheckTest.java new file mode 100644 index 00000000000..ad3527bf871 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheckTest.java @@ -0,0 +1,47 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser 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 + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; +import org.sonar.java.checks.verifier.internal.InternalCheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class AvoidQualifierOnBeanMethodsCheckTest { + + @Test + void test() { + ((InternalCheckVerifier) CheckVerifier.newVerifier()) + .onFile(mainCodeSourcesPath("checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java")) + .withCheck(new AvoidQualifierOnBeanMethodsCheck()) + .withQuickFixes() + .verifyIssues(); + } + + @Test + void test_no_semantics() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java")) + .withCheck(new AvoidQualifierOnBeanMethodsCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +}