From e0e27d504f271e84f9c660359af5f74e9a4e02ee Mon Sep 17 00:00:00 2001 From: erwan-serandour-sonarsource Date: Thu, 19 Oct 2023 18:07:48 +0200 Subject: [PATCH] SONARJAVA-4563 S6804 `@Value` annotation should inject property or SpEL expression (#4493) --- .../java/org/sonar/java/it/AutoScanTest.java | 4 +- .../autoscan/autoscan-diff-by-rules.json | 8 +- ...ShouldInjectPropertyOrSpELCheckSample.java | 48 +++++++++++ .../java/org/sonar/java/checks/CheckList.java | 2 + ...tationShouldInjectPropertyOrSpELCheck.java | 81 +++++++++++++++++++ .../org/sonar/l10n/java/rules/java/S6804.html | 60 ++++++++++++++ .../org/sonar/l10n/java/rules/java/S6804.json | 24 ++++++ .../java/rules/java/Sonar_way_profile.json | 1 + ...onShouldInjectPropertyOrSpELCheckTest.java | 37 +++++++++ 9 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 java-checks-test-sources/src/main/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java create mode 100644 java-checks/src/main/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheck.java create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.html create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.json create mode 100644 java-checks/src/test/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckTest.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 bfa6e1b65cd..f615623cee1 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(72); + softly.assertThat(rulesSilenced).hasSize(73); /** * 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: 3461"); + softly.assertThat(differences).isEqualTo("Issues differences: 3469"); 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 2d1ac41407e..b8feab636c5 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 @@ -2873,6 +2873,12 @@ "falseNegatives": 0, "falsePositives": 0 }, + { + "ruleKey": "S6804", + "hasTruePositives": false, + "falseNegatives": 7, + "falsePositives": 0 + }, { "ruleKey": "S6809", "hasTruePositives": false, @@ -2894,7 +2900,7 @@ { "ruleKey": "S6813", "hasTruePositives": true, - "falseNegatives": 32, + "falseNegatives": 33, "falsePositives": 0 }, { diff --git a/java-checks-test-sources/src/main/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java b/java-checks-test-sources/src/main/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java new file mode 100644 index 00000000000..63fda6b7407 --- /dev/null +++ b/java-checks-test-sources/src/main/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java @@ -0,0 +1,48 @@ +package checks.spring; + +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; + +class ValueAnnotationShouldInjectPropertyOrSpELCheckSample { + + @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.}} + String catalogA; + + @Value("${catalog.name}") // Compliant + String catalogB; + + @Value("book.topics[0]") // Noncompliant, this will not evaluate the expression + String topicA; + + @Value("#{book.topics[0]}") // Compliant + String topicB; + + @Value("Hello, world!") // Noncompliant, this use of @Value is redundant + String greetingA; + + String greetingB = "Hello, world!"; // Compliant + + @Value("") // Noncompliant + String empty; + + public void setValue(@Value("xxx") String x){ // compliant + } + + @Value("xxx") + public void setValueA(String x){ // compliant + } + + @Value("${a") // Noncompliant + String a; + + @Value("#{a") // Noncompliant + String b; + + @Autowired + String c; +} + +@Value("${myValue.ok}") // Compliant +@interface MyValueOk {} +@Value("myValue.not.ok") // Noncompliant +@interface MyValueNotOk {} 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 968a7416d18..9791ac5ce9b 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 @@ -157,6 +157,7 @@ import org.sonar.java.checks.spring.SpringSecurityDisableCSRFCheck; import org.sonar.java.checks.spring.SpringSessionFixationCheck; import org.sonar.java.checks.spring.TransactionalMethodVisibilityCheck; +import org.sonar.java.checks.spring.ValueAnnotationShouldInjectPropertyOrSpELCheck; import org.sonar.java.checks.synchronization.DoubleCheckedLockingCheck; import org.sonar.java.checks.synchronization.SynchronizationOnGetClassCheck; import org.sonar.java.checks.synchronization.TwoLocksWaitCheck; @@ -720,6 +721,7 @@ public final class CheckList { UselessParenthesesCheck.class, UserEnumerationCheck.class, UtilityClassWithPublicConstructorCheck.class, + ValueAnnotationShouldInjectPropertyOrSpELCheck.class, ValueBasedObjectUsedForLockCheck.class, ValueBasedObjectsShouldNotBeSerializedCheck.class, VarArgCheck.class, diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheck.java new file mode 100644 index 00000000000..82336a383c1 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheck.java @@ -0,0 +1,81 @@ +/* + * 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.List; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.sonar.check.Rule; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.LiteralTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S6804") +public class ValueAnnotationShouldInjectPropertyOrSpELCheck extends IssuableSubscriptionVisitor { + + private static final String SPRING_VALUE = "org.springframework.beans.factory.annotation.Value"; + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS, Tree.Kind.ANNOTATION_TYPE); + } + + @Override + public void visitNode(Tree tree) { + ClassTree cls = (ClassTree) tree; + + List fieldsAnnotations = cls.members() + .stream() + .filter(m -> m.is(Tree.Kind.VARIABLE)) + .flatMap(field -> ((VariableTree) field).modifiers().annotations().stream()) + .collect(Collectors.toList()); + + List interfaceAnnotations = cls.is(Tree.Kind.ANNOTATION_TYPE) ? cls.modifiers().annotations() : List.of(); + + Stream.concat(fieldsAnnotations.stream(), interfaceAnnotations.stream()) + .filter(ValueAnnotationShouldInjectPropertyOrSpELCheck::isSimpleSpringValue) + .forEach(ann -> reportIssue( + ann, + "Either replace the \"@Value\" annotation with a standard field initialization," + + " use \"${propertyName}\" to inject a property " + + "or use \"#{expression}\" to evaluate a SpEL expression.")); + } + + private static boolean isSimpleSpringValue(AnnotationTree ann) { + if (ann.symbolType().is(SPRING_VALUE)) { + LiteralTree literal = (LiteralTree) ann.arguments().get(0); + String value = literal.value(); + return !isPropertyName(value) && !isSpEL(value); + } + return false; + } + + private static boolean isPropertyName(String value) { + return value.startsWith("\"${") && value.endsWith("}\""); + } + + private static boolean isSpEL(String value) { + return value.startsWith("\"#{") && value.endsWith("}\""); + } + +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.html new file mode 100644 index 00000000000..4fceec4f002 --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.html @@ -0,0 +1,60 @@ +

This rule reports when the Spring @Value annotation injects a simple value that does not contain an expression.

+

Why is this an issue?

+

The purpose of the @Value annotation in org.springframework.beans.factory.annotation is to inject a value into a field or +method based on the Spring context after it has been established.

+

If the annotation does not include an expression (either Spring Expression Language or a property injection), the injected value is a simple +constant that does not depend on the Spring context, making the annotation replaceable with a standard field initialization statement.

+

This not only implies the redundant use of @Value, but could also indicate an error where the expression indicators (#, +$) were omitted by mistake.

+

Exceptions

+

This rule does not raise an issue if @Value is applied to a method or method argument, because the annotation has the side effect that +the method is called.

+

How to fix it

+
    +
  • If a property is to be injected, use ${propertyName} instead of propertyName.
    • +
  • If a SpEL expression is to be evaluated, use #{expression} instead of expression.
    • +
  • 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 + initialization statement.
    • +
+

Code examples

+

Noncompliant code example

+
+@Value("catalog.name") // Noncompliant, this will not inject the property
+String catalog;
+
+

Compliant solution

+
+@Value("${catalog.name}") // Compliant
+String catalog;
+
+

Noncompliant code example

+
+@Value("book.topics[0]") // Noncompliant, this will not evaluate the expression
+Topic topic;
+
+

Compliant solution

+
+@Value("#{book.topics[0]}") // Compliant
+Topic topic;
+
+

Noncompliant code example

+
+@Value("Hello, world!") // Noncompliant, this use of @Value is redundant
+String greeting;
+
+

Compliant solution

+
+String greeting = "Hello, world!"; // Compliant
+
+

Resources

+

Documentation

+ +

Articles & blog posts

+ + diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.json new file mode 100644 index 00000000000..7393fdc021a --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6804.json @@ -0,0 +1,24 @@ +{ + "title": "\"@Value\" annotation should inject property or SpEL expression", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "spring" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6804", + "sqKey": "S6804", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "LOW", + "RELIABILITY": "MEDIUM" + }, + "attribute": "CONVENTIONAL" + } +} 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 2be21ba2d0e..cbe9a08e8b4 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 @@ -480,6 +480,7 @@ "S6539", "S6541", "S6548", + "S6804", "S6806", "S6809", "S6810", diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckTest.java new file mode 100644 index 00000000000..624bb25158e --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckTest.java @@ -0,0 +1,37 @@ +/* + * 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 static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class ValueAnnotationShouldInjectPropertyOrSpELCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/ValueAnnotationShouldInjectPropertyOrSpELCheckSample.java")) + .withCheck(new ValueAnnotationShouldInjectPropertyOrSpELCheck()) + .verifyIssues(); + } + +}