Skip to content

Commit 6b43b30

Browse files
author
Angelo Buono
authored
SONARJAVA-4678: mplement S6831: @qualifier should not be used on @bean methods (#4518)
1 parent d6d7e86 commit 6b43b30

9 files changed

Lines changed: 365 additions & 6 deletions

File tree

‎its/ruling/src/test/java/org/sonar/java/it/AutoScanTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -180,15 +180,15 @@ public void javaCheckTestSources() throws Exception {
180180
softly.assertThat(newTotal).isEqualTo(knownTotal);
181181
softly.assertThat(rulesCausingFPs).hasSize(6);
182182
softly.assertThat(rulesNotReporting).hasSize(7);
183-
softly.assertThat(rulesSilenced).hasSize(77);
183+
softly.assertThat(rulesSilenced).hasSize(78);
184184

185185
/**
186186
* 4. Check total number of differences (FPs + FNs)
187187
*
188188
* No differences would mean that we find the same issues with and without the bytecode and libraries
189189
*/
190190
String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences"));
191-
softly.assertThat(differences).isEqualTo("Issues differences: 3483");
191+
softly.assertThat(differences).isEqualTo("Issues differences: 3489");
192192

193193
softly.assertAll();
194194
}

‎its/ruling/src/test/resources/autoscan/autoscan-diff-by-rules.json‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2880,15 +2880,15 @@
28802880
"falsePositives": 0
28812881
},
28822882
{
2883-
"ruleKey": "S6809",
2883+
"ruleKey": "S6806",
28842884
"hasTruePositives": false,
2885-
"falseNegatives": 94,
2885+
"falseNegatives": 10,
28862886
"falsePositives": 0
28872887
},
28882888
{
2889-
"ruleKey": "S6806",
2889+
"ruleKey": "S6809",
28902890
"hasTruePositives": false,
2891-
"falseNegatives": 10,
2891+
"falseNegatives": 94,
28922892
"falsePositives": 0
28932893
},
28942894
{
@@ -2927,6 +2927,12 @@
29272927
"falseNegatives": 5,
29282928
"falsePositives": 0
29292929
},
2930+
{
2931+
"ruleKey": "S6831",
2932+
"hasTruePositives": false,
2933+
"falseNegatives": 6,
2934+
"falsePositives": 0
2935+
},
29302936
{
29312937
"ruleKey": "S6837",
29322938
"hasTruePositives": false,

‎java-checks-test-sources/src/main/java/checks/spring/AvoidQualifierOnBeanMethodsCheckSample.java‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package checks.spring;
2+
3+
import org.springframework.beans.factory.annotation.Qualifier;
4+
import org.springframework.context.annotation.Bean;
5+
import org.springframework.context.annotation.Configuration;
6+
import org.springframework.stereotype.Component;
7+
8+
public class AvoidQualifierOnBeanMethodsCheckSample {
9+
10+
private static final String FOO = "foo";
11+
private static final String CAPITALIZED_FOO = "Foo";
12+
13+
@Configuration
14+
class Configuration1 {
15+
@Bean
16+
@Qualifier("foo") // Noncompliant, [[sc=5;ec=22;quickfixes=qf1]] {{Remove this redundant "@Qualifier" annotation and rely on the @Bean method.}}
17+
// fix@qf1 {{Remove "@Qualifier"}}
18+
// edit@qf1 [[sc=5;ec=22]] {{}}
19+
public String foo() {
20+
return "foo";
21+
}
22+
23+
@Bean
24+
@Qualifier(value = "bar") // Noncompliant
25+
public String bar() {
26+
return "bar";
27+
}
28+
29+
@Bean // Compliant
30+
public String foobar() {
31+
return "foobar";
32+
}
33+
}
34+
35+
@Component
36+
class Component1 {
37+
@Bean("foo")
38+
@Qualifier(CAPITALIZED_FOO) // Noncompliant
39+
public String foo() {
40+
return "foo";
41+
}
42+
43+
@Bean(name = "bar")
44+
@Qualifier(value = "Bar") // Noncompliant
45+
public String bar() {
46+
return "bar";
47+
}
48+
49+
@Bean("foobar") // Compliant
50+
public String foobar() {
51+
return "foobar";
52+
}
53+
}
54+
55+
class Class1 {
56+
@Bean("foo")
57+
@Qualifier(FOO) // Noncompliant
58+
public String foo() {
59+
return "foo";
60+
}
61+
62+
@Bean(name = "bar")
63+
@Qualifier // Noncompliant, [[sc=5;ec=15;quickfixes=qf3]] {{Remove this redundant "@Qualifier" annotation and rely on the @Bean method.}}
64+
// fix@qf3 {{Remove "@Qualifier"}}
65+
// edit@qf3 [[sc=5;ec=15]] {{}}
66+
public String bar() {
67+
return "bar";
68+
}
69+
70+
@Bean("foobar") // Compliant
71+
public String foobar() {
72+
return "foobar";
73+
}
74+
}
75+
76+
}

‎java-checks/src/main/java/org/sonar/java/checks/CheckList.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@
140140
import org.sonar.java.checks.spring.AsyncMethodsReturnTypeCheck;
141141
import org.sonar.java.checks.spring.AutowiredOnConstructorWhenMultipleConstructorsCheck;
142142
import org.sonar.java.checks.spring.AutowiredOnMultipleConstructorsCheck;
143+
import org.sonar.java.checks.spring.AvoidQualifierOnBeanMethodsCheck;
143144
import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck;
144145
import org.sonar.java.checks.spring.FieldDependencyInjectionCheck;
145146
import org.sonar.java.checks.spring.ModelAttributeNamingConventionForSpELCheck;
@@ -290,6 +291,7 @@ public final class CheckList {
290291
AuthorizationsStrongDecisionsCheck.class,
291292
AutowiredOnConstructorWhenMultipleConstructorsCheck.class,
292293
AutowiredOnMultipleConstructorsCheck.class,
294+
AvoidQualifierOnBeanMethodsCheck.class,
293295
AwsConsumerBuilderUsageCheck.class,
294296
AwsCredentialsShouldBeSetExplicitlyCheck.class,
295297
AwsLambdaSyncCallCheck.class,

‎java-checks/src/main/java/org/sonar/java/checks/spring/AvoidQualifierOnBeanMethodsCheck.java‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
/*
2+
* SonarQube Java
3+
* Copyright (C) 2012-2023 SonarSource SA
4+
* mailto:info AT sonarsource DOT com
5+
*
6+
* This program is free software; you can redistribute it and/or
7+
* modify it under the terms of the GNU Lesser General Public
8+
* License as published by the Free Software Foundation; either
9+
* version 3 of the License, or (at your option) any later version.
10+
*
11+
* This program is distributed in the hope that it will be useful,
12+
* but WITHOUT ANY WARRANTY; without even the implied warranty of
13+
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
14+
* Lesser General Public License for more details.
15+
*
16+
* You should have received a copy of the GNU Lesser General Public License
17+
* along with this program; if not, write to the Free Software Foundation,
18+
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
19+
*/
20+
package org.sonar.java.checks.spring;
21+
22+
import java.util.LinkedList;
23+
import java.util.List;
24+
import org.sonar.check.Rule;
25+
import org.sonar.java.checks.helpers.QuickFixHelper;
26+
import org.sonar.java.model.expression.AssignmentExpressionTreeImpl;
27+
import org.sonar.java.model.expression.LiteralTreeImpl;
28+
import org.sonar.java.reporting.JavaQuickFix;
29+
import org.sonar.java.reporting.JavaTextEdit;
30+
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
31+
import org.sonar.plugins.java.api.tree.AnnotationTree;
32+
import org.sonar.plugins.java.api.tree.Arguments;
33+
import org.sonar.plugins.java.api.tree.MethodTree;
34+
import org.sonar.plugins.java.api.tree.Tree;
35+
36+
@Rule(key = "S6831")
37+
public class AvoidQualifierOnBeanMethodsCheck extends IssuableSubscriptionVisitor {
38+
private static final String BEAN_ANNOTATION = "org.springframework.context.annotation.Bean";
39+
private static final String QUALIFIER_ANNOTATION = "org.springframework.beans.factory.annotation.Qualifier";
40+
41+
@Override
42+
public List<Tree.Kind> nodesToVisit() {
43+
return List.of(Tree.Kind.METHOD);
44+
}
45+
46+
/**
47+
* This rule reports an issue when @Bean methods are annotated with @Qualifier.
48+
*/
49+
@Override
50+
public void visitNode(Tree tree) {
51+
var methodTree = (MethodTree) tree;
52+
53+
var beanAnnotation = getAnnotation(methodTree, BEAN_ANNOTATION);
54+
var qualifierAnnotation = getAnnotation(methodTree, QUALIFIER_ANNOTATION);
55+
56+
if (beanAnnotation != null && qualifierAnnotation != null) {
57+
QuickFixHelper.newIssue(context)
58+
.forRule(this)
59+
.onTree(qualifierAnnotation)
60+
.withMessage("Remove this redundant \"@Qualifier\" annotation and rely on the @Bean method.")
61+
.withQuickFixes(() -> getQuickFix(methodTree, qualifierAnnotation))
62+
.report();
63+
}
64+
}
65+
66+
private static AnnotationTree getAnnotation(MethodTree methodTree, String annotation) {
67+
return methodTree.modifiers()
68+
.annotations()
69+
.stream()
70+
.filter(annotationTree -> annotationTree.symbolType().is(annotation))
71+
.findFirst()
72+
.orElse(null);
73+
}
74+
75+
private static List<JavaQuickFix> getQuickFix(MethodTree methodTree, AnnotationTree qualifierAnnotation) {
76+
List<JavaQuickFix> quickFixes = new LinkedList<>();
77+
78+
// quick fix only for @Qualifier annotations without arguments or with argument that matches the method name
79+
if (isFixable(methodTree, qualifierAnnotation)) {
80+
var quickFix = JavaQuickFix.newQuickFix("Remove \"@Qualifier\"")
81+
.addTextEdit(JavaTextEdit.removeTree(qualifierAnnotation))
82+
.build();
83+
quickFixes.add(quickFix);
84+
}
85+
86+
return quickFixes;
87+
}
88+
89+
private static boolean isFixable(MethodTree methodTree, AnnotationTree qualifierAnnotation) {
90+
var arguments = qualifierAnnotation.arguments();
91+
92+
// @Qualifier annotation without argument can be always removed
93+
if (arguments.isEmpty()) {
94+
return true;
95+
}
96+
97+
// @Qualifier that matches the method name is redundant and can be removed
98+
var methodName = methodTree.simpleName().name();
99+
return getQualifierAnnotationValue(arguments).equals(methodName);
100+
}
101+
102+
private static String getQualifierAnnotationValue(Arguments arguments) {
103+
var argument = arguments.get(0);
104+
String qualifierAnnotationValue;
105+
106+
if (argument.is(Tree.Kind.ASSIGNMENT)) {
107+
qualifierAnnotationValue = ((LiteralTreeImpl) ((AssignmentExpressionTreeImpl) argument).expression()).value();
108+
} else if (argument.is(Tree.Kind.STRING_LITERAL)) {
109+
qualifierAnnotationValue = ((LiteralTreeImpl) argument).token().text();
110+
} else {
111+
// case when argument is an identifier: don't suggest a quick fix
112+
qualifierAnnotationValue = "";
113+
}
114+
115+
return removeQuotes(qualifierAnnotationValue);
116+
}
117+
118+
private static String removeQuotes(String value) {
119+
return value.replace("\"", "");
120+
}
121+
}

‎java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.html‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
<h2>Why is this an issue?</h2>
2+
<p>In Spring Framework, the <code>@Qualifier</code> annotation is typically used to disambiguate between multiple beans of the same type when
3+
auto-wiring dependencies. It is not necessary to use <code>@Qualifier</code> when defining a bean using the <code>@Bean</code> annotation because the
4+
bean’s name can be explicitly specified using the <code>name</code> attribute or derived from the method name. Using <code>@Qualifier</code> on
5+
<code>@Bean</code> methods can lead to confusion and redundancy. Beans should be named appropriately using either the <code>name</code> attribute of
6+
the <code>@Bean</code> annotation or the method name itself.</p>
7+
<h3>Noncompliant code example</h3>
8+
<pre data-diff-id="1" data-diff-type="noncompliant">
9+
@Configuration
10+
public class MyConfiguration {
11+
@Bean
12+
@Qualifier("myService")
13+
public MyService myService() {
14+
// ...
15+
return new MyService();
16+
}
17+
18+
@Bean
19+
@Qualifier("betterService")
20+
public MyService aBetterService() {
21+
// ...
22+
return new MyService();
23+
}
24+
25+
@Bean
26+
@Qualifier("evenBetterService")
27+
public MyService anEvenBetterService() {
28+
// ...
29+
return new MyService();
30+
}
31+
32+
@Bean
33+
@Qualifier("differentService")
34+
public MyBean aDifferentService() {
35+
// ...
36+
return new MyBean();
37+
}
38+
}
39+
</pre>
40+
<h3>Compliant solution</h3>
41+
<pre data-diff-id="1" data-diff-type="compliant">
42+
@Configuration
43+
public class MyConfiguration {
44+
@Bean
45+
public MyService myService() {
46+
// ...
47+
return new MyService();
48+
}
49+
50+
@Bean(name="betterService")
51+
public MyService aBetterService() {
52+
// ...
53+
return new MyService();
54+
}
55+
56+
@Bean(name="evenBetterService")
57+
public MyService anEvenBetterService() {
58+
// ...
59+
return new MyService();
60+
}
61+
62+
@Bean(name="differentService")
63+
public MyBean aDifferentService() {
64+
// ...
65+
return new MyBean();
66+
}
67+
}
68+
</pre>
69+
<h2>Resources</h2>
70+
<h3>Documentation</h3>
71+
<ul>
72+
<li> <a href="https://docs.spring.io/spring-framework/reference/core/beans/java/bean-annotation.html">Spring Framework - Using the @Bean
73+
Annotation</a> </li>
74+
<li> <a href="https://docs.spring.io/spring-framework/reference/core/beans/annotation-config/autowired-qualifiers.html">Spring Framework - Using
75+
@Qualifier</a> </li>
76+
</ul>
77+
<h3>Articles &amp; blog posts</h3>
78+
<ul>
79+
<li> <a href="https://www.baeldung.com/spring-qualifier-annotation">Baeldung - Spring @Qualifier Annotation</a> </li>
80+
<li> <a href="https://www.baeldung.com/spring-bean-annotations">Baeldung - Spring Bean Annotations</a> </li>
81+
</ul>
82+

‎java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6831.json‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
{
2+
"title": "\"@Qualifier\" should not be used on \"@Bean\" methods",
3+
"type": "BUG",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"spring"
11+
],
12+
"defaultSeverity": "Major",
13+
"ruleSpecification": "RSPEC-6831",
14+
"sqKey": "S6831",
15+
"scope": "Main",
16+
"quickfix": "targeted",
17+
"code": {
18+
"impacts": {
19+
"MAINTAINABILITY": "MEDIUM",
20+
"RELIABILITY": "MEDIUM"
21+
},
22+
"attribute": "DISTINCT"
23+
}
24+
}

‎java-checks/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,7 @@
489489
"S6817",
490490
"S6818",
491491
"S6829",
492+
"S6831",
492493
"S6837"
493494
]
494495
}

0 commit comments

Comments
 (0)