Skip to content

Commit e0e27d5

Browse files
erwan-serandourerwan-serandour
authored
SONARJAVA-4563 S6804 @Value annotation should inject property or SpEL expression (#4493)
1 parent 3e95ca4 commit e0e27d5

9 files changed

Lines changed: 262 additions & 3 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(72);
183+
softly.assertThat(rulesSilenced).hasSize(73);
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: 3461");
191+
softly.assertThat(differences).isEqualTo("Issues differences: 3469");
192192

193193
softly.assertAll();
194194
}

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2873,6 +2873,12 @@
28732873
"falseNegatives": 0,
28742874
"falsePositives": 0
28752875
},
2876+
{
2877+
"ruleKey": "S6804",
2878+
"hasTruePositives": false,
2879+
"falseNegatives": 7,
2880+
"falsePositives": 0
2881+
},
28762882
{
28772883
"ruleKey": "S6809",
28782884
"hasTruePositives": false,
@@ -2894,7 +2900,7 @@
28942900
{
28952901
"ruleKey": "S6813",
28962902
"hasTruePositives": true,
2897-
"falseNegatives": 32,
2903+
"falseNegatives": 33,
28982904
"falsePositives": 0
28992905
},
29002906
{

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
package checks.spring;
2+
3+
import org.springframework.beans.factory.annotation.Autowired;
4+
import org.springframework.beans.factory.annotation.Value;
5+
6+
class ValueAnnotationShouldInjectPropertyOrSpELCheckSample {
7+
8+
@Value("catalog.name") // Noncompliant [[sc=3;ec=25]] {{Either replace the "@Value" annotation with a standard field initialization, use "${propertyName}" to inject a property or use "#{expression}" to evaluate a SpEL expression.}}
9+
String catalogA;
10+
11+
@Value("${catalog.name}") // Compliant
12+
String catalogB;
13+
14+
@Value("book.topics[0]") // Noncompliant, this will not evaluate the expression
15+
String topicA;
16+
17+
@Value("#{book.topics[0]}") // Compliant
18+
String topicB;
19+
20+
@Value("Hello, world!") // Noncompliant, this use of @Value is redundant
21+
String greetingA;
22+
23+
String greetingB = "Hello, world!"; // Compliant
24+
25+
@Value("") // Noncompliant
26+
String empty;
27+
28+
public void setValue(@Value("xxx") String x){ // compliant
29+
}
30+
31+
@Value("xxx")
32+
public void setValueA(String x){ // compliant
33+
}
34+
35+
@Value("${a") // Noncompliant
36+
String a;
37+
38+
@Value("#{a") // Noncompliant
39+
String b;
40+
41+
@Autowired
42+
String c;
43+
}
44+
45+
@Value("${myValue.ok}") // Compliant
46+
@interface MyValueOk {}
47+
@Value("myValue.not.ok") // Noncompliant
48+
@interface MyValueNotOk {}

‎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
@@ -157,6 +157,7 @@
157157
import org.sonar.java.checks.spring.SpringSecurityDisableCSRFCheck;
158158
import org.sonar.java.checks.spring.SpringSessionFixationCheck;
159159
import org.sonar.java.checks.spring.TransactionalMethodVisibilityCheck;
160+
import org.sonar.java.checks.spring.ValueAnnotationShouldInjectPropertyOrSpELCheck;
160161
import org.sonar.java.checks.synchronization.DoubleCheckedLockingCheck;
161162
import org.sonar.java.checks.synchronization.SynchronizationOnGetClassCheck;
162163
import org.sonar.java.checks.synchronization.TwoLocksWaitCheck;
@@ -720,6 +721,7 @@ public final class CheckList {
720721
UselessParenthesesCheck.class,
721722
UserEnumerationCheck.class,
722723
UtilityClassWithPublicConstructorCheck.class,
724+
ValueAnnotationShouldInjectPropertyOrSpELCheck.class,
723725
ValueBasedObjectUsedForLockCheck.class,
724726
ValueBasedObjectsShouldNotBeSerializedCheck.class,
725727
VarArgCheck.class,

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

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
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.List;
23+
import java.util.stream.Collectors;
24+
import java.util.stream.Stream;
25+
import org.sonar.check.Rule;
26+
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
27+
import org.sonar.plugins.java.api.tree.AnnotationTree;
28+
import org.sonar.plugins.java.api.tree.ClassTree;
29+
import org.sonar.plugins.java.api.tree.LiteralTree;
30+
import org.sonar.plugins.java.api.tree.Tree;
31+
import org.sonar.plugins.java.api.tree.VariableTree;
32+
33+
@Rule(key = "S6804")
34+
public class ValueAnnotationShouldInjectPropertyOrSpELCheck extends IssuableSubscriptionVisitor {
35+
36+
private static final String SPRING_VALUE = "org.springframework.beans.factory.annotation.Value";
37+
38+
@Override
39+
public List<Tree.Kind> nodesToVisit() {
40+
return List.of(Tree.Kind.CLASS, Tree.Kind.ANNOTATION_TYPE);
41+
}
42+
43+
@Override
44+
public void visitNode(Tree tree) {
45+
ClassTree cls = (ClassTree) tree;
46+
47+
List<AnnotationTree> fieldsAnnotations = cls.members()
48+
.stream()
49+
.filter(m -> m.is(Tree.Kind.VARIABLE))
50+
.flatMap(field -> ((VariableTree) field).modifiers().annotations().stream())
51+
.collect(Collectors.toList());
52+
53+
List<AnnotationTree> interfaceAnnotations = cls.is(Tree.Kind.ANNOTATION_TYPE) ? cls.modifiers().annotations() : List.of();
54+
55+
Stream.concat(fieldsAnnotations.stream(), interfaceAnnotations.stream())
56+
.filter(ValueAnnotationShouldInjectPropertyOrSpELCheck::isSimpleSpringValue)
57+
.forEach(ann -> reportIssue(
58+
ann,
59+
"Either replace the \"@Value\" annotation with a standard field initialization," +
60+
" use \"${propertyName}\" to inject a property " +
61+
"or use \"#{expression}\" to evaluate a SpEL expression."));
62+
}
63+
64+
private static boolean isSimpleSpringValue(AnnotationTree ann) {
65+
if (ann.symbolType().is(SPRING_VALUE)) {
66+
LiteralTree literal = (LiteralTree) ann.arguments().get(0);
67+
String value = literal.value();
68+
return !isPropertyName(value) && !isSpEL(value);
69+
}
70+
return false;
71+
}
72+
73+
private static boolean isPropertyName(String value) {
74+
return value.startsWith("\"${") && value.endsWith("}\"");
75+
}
76+
77+
private static boolean isSpEL(String value) {
78+
return value.startsWith("\"#{") && value.endsWith("}\"");
79+
}
80+
81+
}

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
<p>This rule reports when the Spring <code>@Value</code> annotation injects a simple value that does not contain an expression.</p>
2+
<h2>Why is this an issue?</h2>
3+
<p>The purpose of the <code>@Value</code> annotation in <code>org.springframework.beans.factory.annotation</code> is to inject a value into a field or
4+
method based on the Spring context after it has been established.</p>
5+
<p>If the annotation does not include an expression (either Spring Expression Language or a property injection), the injected value is a simple
6+
constant that does not depend on the Spring context, making the annotation replaceable with a standard field initialization statement.</p>
7+
<p>This not only implies the redundant use of <code>@Value</code>, but could also indicate an error where the expression indicators (<code>#</code>,
8+
<code>$</code>) were omitted by mistake.</p>
9+
<h3>Exceptions</h3>
10+
<p>This rule does not raise an issue if <code>@Value</code> is applied to a method or method argument, because the annotation has the side effect that
11+
the method is called.</p>
12+
<h2>How to fix it</h2>
13+
<ul>
14+
<li> If a property is to be injected, use <code>${propertyName}</code> instead of <code>propertyName</code>. </li>
15+
<li> If a SpEL expression is to be evaluated, use <code>#{expression}</code> instead of <code>expression</code>. </li>
16+
<li> If you intend to initialize a field with a simple value or with an expression that does not depend on the Spring context, use a standard field
17+
initialization statement. </li>
18+
</ul>
19+
<h3>Code examples</h3>
20+
<h4>Noncompliant code example</h4>
21+
<pre data-diff-id="1" data-diff-type="noncompliant">
22+
@Value("catalog.name") // Noncompliant, this will not inject the property
23+
String catalog;
24+
</pre>
25+
<h4>Compliant solution</h4>
26+
<pre data-diff-id="1" data-diff-type="compliant">
27+
@Value("${catalog.name}") // Compliant
28+
String catalog;
29+
</pre>
30+
<h4>Noncompliant code example</h4>
31+
<pre data-diff-id="2" data-diff-type="noncompliant">
32+
@Value("book.topics[0]") // Noncompliant, this will not evaluate the expression
33+
Topic topic;
34+
</pre>
35+
<h4>Compliant solution</h4>
36+
<pre data-diff-id="2" data-diff-type="compliant">
37+
@Value("#{book.topics[0]}") // Compliant
38+
Topic topic;
39+
</pre>
40+
<h4>Noncompliant code example</h4>
41+
<pre data-diff-id="3" data-diff-type="noncompliant">
42+
@Value("Hello, world!") // Noncompliant, this use of @Value is redundant
43+
String greeting;
44+
</pre>
45+
<h4>Compliant solution</h4>
46+
<pre data-diff-id="3" data-diff-type="compliant">
47+
String greeting = "Hello, world!"; // Compliant
48+
</pre>
49+
<h2>Resources</h2>
50+
<h3>Documentation</h3>
51+
<ul>
52+
<li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/beans/factory/annotation/Value.html">Spring
53+
Framework API - Annotation Interface Value</a> </li>
54+
</ul>
55+
<h3>Articles &amp; blog posts</h3>
56+
<ul>
57+
<li> <a href="https://www.baeldung.com/spring-value-annotation">Baeldung - A Quick Guide to Spring @Value</a> </li>
58+
<li> <a href="https://www.digitalocean.com/community/tutorials/spring-value-annotation">DigitalOcean - Spring @Value Annotation</a> </li>
59+
</ul>
60+

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
{
2+
"title": "\"@Value\" annotation should inject property or SpEL expression",
3+
"type": "CODE_SMELL",
4+
"status": "ready",
5+
"remediation": {
6+
"func": "Constant\/Issue",
7+
"constantCost": "5min"
8+
},
9+
"tags": [
10+
"spring"
11+
],
12+
"defaultSeverity": "Major",
13+
"ruleSpecification": "RSPEC-6804",
14+
"sqKey": "S6804",
15+
"scope": "All",
16+
"quickfix": "unknown",
17+
"code": {
18+
"impacts": {
19+
"MAINTAINABILITY": "LOW",
20+
"RELIABILITY": "MEDIUM"
21+
},
22+
"attribute": "CONVENTIONAL"
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
@@ -480,6 +480,7 @@
480480
"S6539",
481481
"S6541",
482482
"S6548",
483+
"S6804",
483484
"S6806",
484485
"S6809",
485486
"S6810",

‎java-checks/src/test/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckTest.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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 org.junit.jupiter.api.Test;
23+
import org.sonar.java.checks.verifier.CheckVerifier;
24+
25+
import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath;
26+
27+
class ValueAnnotationShouldInjectPropertyOrSpELCheckTest {
28+
29+
@Test
30+
void test() {
31+
CheckVerifier.newVerifier()
32+
.onFile(mainCodeSourcesPath("checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java"))
33+
.withCheck(new ValueAnnotationShouldInjectPropertyOrSpELCheck())
34+
.verifyIssues();
35+
}
36+
37+
}

0 commit comments

Comments
 (0)