SONARJAVA-4563 S6804 @Value annotation should inject property or Sp…

…EL expression (#4493)

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

@@ -180,15 +180,15 @@ 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)
      *
      * 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();
   }

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
   },
   {

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 {}

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,

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<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.CLASS, Tree.Kind.ANNOTATION_TYPE);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    ClassTree cls = (ClassTree) tree;
+
+    List<AnnotationTree> fieldsAnnotations = cls.members()
+      .stream()
+      .filter(m -> m.is(Tree.Kind.VARIABLE))
+      .flatMap(field -> ((VariableTree) field).modifiers().annotations().stream())
+      .collect(Collectors.toList());
+
+    List<AnnotationTree> 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("}\"");
+  }
+
+}

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

@@ -0,0 +1,60 @@
+<p>This rule reports when the Spring <code>@Value</code> annotation injects a simple value that does not contain an expression.</p>
+<h2>Why is this an issue?</h2>
+<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
+method based on the Spring context after it has been established.</p>
+<p>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.</p>
+<p>This not only implies the redundant use of <code>@Value</code>, but could also indicate an error where the expression indicators (<code>#</code>,
+<code>$</code>) were omitted by mistake.</p>
+<h3>Exceptions</h3>
+<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
+the method is called.</p>
+<h2>How to fix it</h2>
+<ul>
+  <li> If a property is to be injected, use <code>${propertyName}</code> instead of <code>propertyName</code>. </li>
+  <li> If a SpEL expression is to be evaluated, use <code>#{expression}</code> instead of <code>expression</code>. </li>
+  <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
+  initialization statement. </li>
+</ul>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+@Value("catalog.name") // Noncompliant, this will not inject the property
+String catalog;
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+@Value("${catalog.name}") // Compliant
+String catalog;
+</pre>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="2" data-diff-type="noncompliant">
+@Value("book.topics[0]") // Noncompliant, this will not evaluate the expression
+Topic topic;
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="2" data-diff-type="compliant">
+@Value("#{book.topics[0]}") // Compliant
+Topic topic;
+</pre>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="3" data-diff-type="noncompliant">
+@Value("Hello, world!") // Noncompliant, this use of @Value is redundant
+String greeting;
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="3" data-diff-type="compliant">
+String greeting = "Hello, world!"; // Compliant
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> <a href="https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/beans/factory/annotation/Value.html">Spring
+  Framework API - Annotation Interface Value</a> </li>
+</ul>
+<h3>Articles &amp; blog posts</h3>
+<ul>
+  <li> <a href="https://www.baeldung.com/spring-value-annotation">Baeldung - A Quick Guide to Spring @Value</a> </li>
+  <li> <a href="https://www.digitalocean.com/community/tutorials/spring-value-annotation">DigitalOcean - Spring @Value Annotation</a> </li>
+</ul>
+

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"
+  }
+}

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",

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();
+  }
+
+}