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 38969a8f6b9..e3d51ae9503 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(80); + softly.assertThat(rulesSilenced).hasSize(81); /** * 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: 3513"); + softly.assertThat(differences).isEqualTo("Issues differences: 3579"); 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 9ea0cf67e17..5781ba50077 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 @@ -2900,7 +2900,7 @@ { "ruleKey": "S6813", "hasTruePositives": true, - "falseNegatives": 33, + "falseNegatives": 55, "falsePositives": 0 }, { @@ -2918,7 +2918,7 @@ { "ruleKey": "S6818", "hasTruePositives": false, - "falseNegatives": 4, + "falseNegatives": 7, "falsePositives": 0 }, { @@ -2939,6 +2939,12 @@ "falseNegatives": 6, "falsePositives": 0 }, + { + "ruleKey": "S6832", + "hasTruePositives": false, + "falseNegatives": 41, + "falsePositives": 0 + }, { "ruleKey": "S6833", "hasTruePositives": false, diff --git a/java-checks-test-sources/src/main/java/checks/spring/NonSingletonAutowiredInSingletonCheckSample.java b/java-checks-test-sources/src/main/java/checks/spring/NonSingletonAutowiredInSingletonCheckSample.java new file mode 100644 index 00000000000..9881901bb61 --- /dev/null +++ b/java-checks-test-sources/src/main/java/checks/spring/NonSingletonAutowiredInSingletonCheckSample.java @@ -0,0 +1,220 @@ +package checks.spring; + +import javax.inject.Inject; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Scope; +import org.springframework.context.annotation.ScopedProxyMode; +import org.springframework.stereotype.Component; +import org.springframework.web.context.WebApplicationContext; + +import static org.springframework.context.annotation.ScopedProxyMode.TARGET_CLASS; + +public class NonSingletonAutowiredInSingletonCheckSample { + + private static final String SINGLETON_SCOPE = "singleton"; + private static final String PROTOTYPE_SCOPE = "prototype"; + private static final String CUSTOM_SCOPE = "custom"; + + @Component + @Scope(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = TARGET_CLASS) + public class RequestBean1 { } + + @Scope("prototype") + public class PrototypeBean1 { } + + @Scope(value = "prototype") + public class PrototypeBean2 { } + + + @Scope(value = PROTOTYPE_SCOPE, scopeName = "prototype", proxyMode = ScopedProxyMode.TARGET_CLASS) + public class PrototypeBean3 { } + + + public class SingletonBean { + @Autowired + private RequestBean1 requestBean1; // Noncompliant, [[sc=13;ec=25]] {{Don't auto-wire this non-Singleton bean into a Singleton bean (autowired field).}} + @Autowired + private PrototypeBean1 prototypeBean1; // Noncompliant + @Autowired + private PrototypeBean2 prototypeBean2; // Noncompliant + @Autowired + private PrototypeBean3 prototypeBean3; // Noncompliant + + @Autowired // Noncompliant@+1 + public SingletonBean(RequestBean1 requestBean1) { // Noncompliant, [[sc=26;ec=38]] {{Don't auto-wire this non-Singleton bean into a Singleton bean (autowired constructor).}} + } + + @Autowired // Noncompliant@+1 + public SingletonBean(PrototypeBean2 prototypeBean2) { // Noncompliant + } + + @Autowired + public SingletonBean(RequestBean1 requestBean1, // Noncompliant + PrototypeBean1 prototypeBean1, // Noncompliant + PrototypeBean2 prototypeBean2, // Noncompliant + PrototypeBean3 prototypeBean3) { // Noncompliant + } + + @Autowired + public void setRequestBean1(RequestBean1 requestBean1) { // Noncompliant, [[sc=33;ec=45]] {{Don't auto-wire this non-Singleton bean into a Singleton bean (autowired setter method).}} + this.requestBean1 = requestBean1; + } + + public void setPrototypeBean1(@Autowired PrototypeBean1 prototypeBean1) { // Noncompliant, [[sc=46;ec=60]] {{Don't auto-wire this non-Singleton bean into a Singleton bean (autowired parameter).}} + this.prototypeBean1 = prototypeBean1; + } + + @Autowired + private void setPrototypeBean2(@Autowired PrototypeBean2 prototypeBean2) { // Compliant + this.prototypeBean2 = prototypeBean2; + } + + public void method(@Autowired RequestBean1 requestBean1) { // Compliant, not a setter or constructor + // ... + } + } + + @Scope(value = "singleton") + public class SingletonBean2 { + @Autowired + private SingletonBean singletonBean; // Compliant, since scope is non-Singleton + @Autowired + private RequestBean1 requestBean1; // Noncompliant + @Autowired + private PrototypeBean1 prototypeBean1; // Noncompliant + + @Autowired + private SingletonBean2(@Autowired SingletonBean singletonBean) {// Compliant, since scope is non-Singleton + this.singletonBean = singletonBean; + } + + @Autowired // Noncompliant@+1 + public SingletonBean2(RequestBean1 requestBean1) { // Noncompliant + this.requestBean1 = requestBean1; + } + } + + @Scope(scopeName = "singleton") + public class SingletonBean3 { + @Autowired + private SingletonBean singletonBean; // Compliant, since scope is non-Singleton + @Autowired + private RequestBean1 requestBean1; // Noncompliant + @Autowired + private PrototypeBean1 prototypeBean1; // Noncompliant + + // Noncompliant@+2 Autowired constructor + @Autowired // Noncompliant@+1 Autowired constructor parameter + public SingletonBean3(@Autowired RequestBean1 requestBean1) { // Noncompliant, single parameter constructor + } + } + + @Scope(proxyMode = TARGET_CLASS) + public class SingletonBean4 { + @Autowired + private SingletonBean singletonBean; // Compliant, since scope is non-Singleton + @Autowired + private RequestBean1 requestBean1; // Noncompliant + @Autowired + private PrototypeBean1 prototypeBean1; // Noncompliant + } + + @Scope(value = SINGLETON_SCOPE, scopeName = "singleton") + public class SingletonBean5 { + @Autowired + private SingletonBean singletonBean; // Compliant, since scope is non-Singleton + @Autowired + private RequestBean1 requestBean1; // Noncompliant + @Autowired + private PrototypeBean1 prototypeBean1; // Noncompliant + } + + @Scope("singleton") + public class SingletonBean6 { + @Inject + private SingletonBean5 singletonBean5; // Compliant, since scope is non-Singleton + @Inject + private RequestBean1 requestBean1; // Noncompliant + private PrototypeBean1 prototypeBean1; + private PrototypeBean2 prototypeBean2; + + public SingletonBean6(PrototypeBean1 prototypeBean1) { // Noncompliant + this.prototypeBean1 = prototypeBean1; + } + + @Inject + public SingletonBean6(PrototypeBean1 prototypeBean1, // Noncompliant + PrototypeBean2 prototypeBean2) { // Noncompliant + this.prototypeBean1 = prototypeBean1; + this.prototypeBean2 = prototypeBean2; + } + + @Inject + public void setPrototypeBean2(PrototypeBean2 prototypeBean2) { // Noncompliant + this.prototypeBean2 = prototypeBean2; + } + } + + public class SingletonBean7 { + @jakarta.inject.Inject + private SingletonBean5 singletonBean5; // Compliant, since scope is non-Singleton + @jakarta.inject.Inject + private RequestBean1 requestBean1; // Noncompliant + private PrototypeBean1 prototypeBean1; + private PrototypeBean2 prototypeBean2; + + public SingletonBean7(PrototypeBean1 prototypeBean1) { // Noncompliant + this.prototypeBean1 = prototypeBean1; + } + + @jakarta.inject.Inject + public SingletonBean7(PrototypeBean1 prototypeBean1, // Noncompliant + PrototypeBean2 prototypeBean2) { // Noncompliant + this.prototypeBean1 = prototypeBean1; + this.prototypeBean2 = prototypeBean2; + } + + @jakarta.inject.Inject + public void setPrototypeBean2(PrototypeBean2 prototypeBean2) { // Noncompliant + this.prototypeBean2 = prototypeBean2; + } + } + + @Scope(value = CUSTOM_SCOPE, scopeName = "custom", proxyMode = TARGET_CLASS) + public class CustomBean { + @Autowired + private SingletonBean singletonBean; // Compliant, since scope is non-Singleton + @Autowired + private SingletonBean5 singletonBean5; // Compliant, since scope is non-Singleton + @Autowired + public PrototypeBean1 prototypeBean1; // Compliant, since scope is non-Singleton + } + + public class SingletonBean11 { + @Autowired + private CustomBean customBean; // Noncompliant + + @Autowired // Noncompliant@+1 + public SingletonBean11(CustomBean customBean) { // Noncompliant + this.customBean = customBean; + } + + @Autowired + public void setCustomBean(CustomBean customBean) { // Noncompliant + this.customBean = customBean; + } + + @Autowired + public void method(CustomBean customBean) { // Compliant, not a setter or constructor + // ... + } + + public void method2(@Autowired CustomBean customBean) { // Compliant, not a setter or constructor + // ... + } + } + + @Autowired + public @interface customAutowired { + } +} 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 45bcc0f01dc..3866ea1bdc6 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 @@ -145,6 +145,7 @@ import org.sonar.java.checks.spring.ControllerWithSessionAttributesCheck; import org.sonar.java.checks.spring.FieldDependencyInjectionCheck; import org.sonar.java.checks.spring.ModelAttributeNamingConventionForSpELCheck; +import org.sonar.java.checks.spring.NonSingletonAutowiredInSingletonCheck; import org.sonar.java.checks.spring.OptionalRestParametersShouldBeObjectsCheck; import org.sonar.java.checks.spring.PersistentEntityUsedAsRequestParameterCheck; import org.sonar.java.checks.spring.RequestMappingMethodPublicCheck; @@ -516,6 +517,7 @@ public final class CheckList { NestedTernaryOperatorsCheck.class, NioFileDeleteCheck.class, NoCheckstyleTagPresenceCheck.class, + NonSingletonAutowiredInSingletonCheck.class, NoPmdTagPresenceCheck.class, NoSonarCheck.class, NonSerializableWriteCheck.class, diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java index 6e4bb97a332..3ebfbeef66a 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java @@ -80,10 +80,19 @@ public static boolean isHashCodeMethod(MethodTree m) { return isPublic(m) && !isStatic(m) && hasHashCodeSignature; } + public static boolean isSetterMethod(MethodTree m) { + boolean publicNonStaticVoid = isPublic(m) && !isStatic(m) && returnsPrimitive(m, "void"); + return publicNonStaticVoid && isNamePrefix(m, "set") && m.parameters().size() == 1; + } + private static boolean isNamed(MethodTree m, String name) { return name.equals(m.simpleName().name()); } + private static boolean isNamePrefix(MethodTree m, String prefix) { + return m.simpleName().name().startsWith(prefix); + } + private static boolean isStatic(MethodTree m) { return ModifiersUtils.hasModifier(m.modifiers(), Modifier.STATIC); } @@ -135,7 +144,7 @@ public static Optional subsequentMethodInvocation(Tree tre @VisibleForTesting static boolean hasKind(@Nullable Tree tree, Tree.Kind kind) { - return tree != null && tree.kind() == kind; + return tree != null && tree.kind() == kind; } public static class MethodInvocationCollector extends BaseTreeVisitor { diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheck.java new file mode 100644 index 00000000000..46add2cf111 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheck.java @@ -0,0 +1,213 @@ +/* + * 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.Optional; +import java.util.Set; +import javax.annotation.Nullable; +import org.sonar.check.Rule; +import org.sonar.java.checks.helpers.MethodTreeUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AnnotationTree; +import org.sonar.plugins.java.api.tree.Arguments; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.LiteralTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S6832") +public class NonSingletonAutowiredInSingletonCheck extends IssuableSubscriptionVisitor { + private static final String SCOPED_ANNOTATION = "org.springframework.context.annotation.Scope"; + private static final String AUTOWIRED_ANNOTATION = "org.springframework.beans.factory.annotation.Autowired"; + private static final String JAVAX_INJECT_ANNOTATION = "javax.inject.Inject"; + private static final String JAKARTA_INJECT_ANNOTATION = "jakarta.inject.Inject"; + private static final Set AUTO_WIRING_ANNOTATIONS = Set.of(AUTOWIRED_ANNOTATION, JAVAX_INJECT_ANNOTATION, JAKARTA_INJECT_ANNOTATION); + private static final Set SINGLETON_LITERALS = Set.of("singleton", "\"singleton\""); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.ANNOTATION, Tree.Kind.CONSTRUCTOR); + } + + /** + * This rule reports an issue when a Singleton Bean auto-wires a non-Singleton Bean via constructor, field or method parameter. + */ + @Override + public void visitNode(Tree tree) { + if (tree.is(Tree.Kind.ANNOTATION)) { + analyzeAnnotation((AnnotationTree) tree); + } + + if (tree.is(Tree.Kind.CONSTRUCTOR)) { + analyzeSingleArgumentConstructor((MethodTree) tree); + } + } + + private void analyzeAnnotation(AnnotationTree annotationTree) { + if (!isAutoWiringAnnotation(annotationTree)) { + return; + } + + var annotatedSymbol = Optional.ofNullable(annotationTree.parent()).map(Tree::parent).orElse(null); + if (annotatedSymbol == null) { + return; + } + + if (annotatedSymbol.is(Tree.Kind.VARIABLE)) { + analyzeAnnotatedFieldOrParameter((VariableTree) annotatedSymbol); + + } else if (annotatedSymbol.is(Tree.Kind.METHOD)) { + analyzeAnnotatedSetter((MethodTree) annotatedSymbol); + + } else if (annotatedSymbol.is(Tree.Kind.CONSTRUCTOR)) { + analyzeAnnotatedConstructor((MethodTree) annotatedSymbol); + } + } + + private void analyzeAnnotatedFieldOrParameter(VariableTree annotatedVar) { + String injectionType; + + if (isClassField(annotatedVar)) { + injectionType = "autowired field"; + } else if (isSetterParameter(annotatedVar) || isConstructorParameter(annotatedVar)) { + injectionType = "autowired parameter"; + } else { + injectionType = null; + } + + if (injectionType != null) { + getEnclosingClass(annotatedVar.symbol().enclosingClass()) + .ifPresent(enclosingClassTree -> reportIfNonSingletonInSingleton(enclosingClassTree, annotatedVar, injectionType)); + } + } + + private void analyzeAnnotatedSetter(MethodTree annotatedMethod) { + if (MethodTreeUtils.isSetterMethod(annotatedMethod)) { + getEnclosingClass(annotatedMethod.symbol().enclosingClass()) + .ifPresent(enclosingClassTree -> annotatedMethod.parameters() + .forEach(variableTree -> reportIfNonSingletonInSingleton(enclosingClassTree, variableTree, "autowired setter method"))); + } + } + + private void analyzeAnnotatedConstructor(MethodTree annotatedConstructor) { + getEnclosingClass(annotatedConstructor.symbol().enclosingClass()) + .ifPresent(enclosingClassTree -> annotatedConstructor.parameters() + .forEach(variableTree -> reportIfNonSingletonInSingleton(enclosingClassTree, variableTree, "autowired constructor"))); + } + + private void analyzeSingleArgumentConstructor(MethodTree constructorTree) { + if (constructorTree.parameters().size() == 1) { + var constructorParameter = constructorTree.parameters().get(0); + getEnclosingClass(constructorTree.symbol().enclosingClass()) + .ifPresent(enclosingClassTree -> reportIfNonSingletonInSingleton(enclosingClassTree, constructorParameter, "single argument constructor")); + } + } + + private static boolean isClassField(VariableTree variableTree) { + return Optional.ofNullable(variableTree.parent()) + .filter(parent -> parent.is(Tree.Kind.CLASS)) + .isPresent(); + } + + private static boolean isSetterParameter(VariableTree variableTree) { + return Optional.ofNullable(variableTree.parent()) + .filter(parent -> parent.is(Tree.Kind.METHOD)) + .map(MethodTree.class::cast) + .filter(MethodTreeUtils::isSetterMethod) + .isPresent(); + } + + private static boolean isConstructorParameter(VariableTree variableTree) { + return Optional.ofNullable(variableTree.parent()) + .filter(parent -> parent.is(Tree.Kind.CONSTRUCTOR)) + .isPresent(); + } + + private static Optional getEnclosingClass(@Nullable Symbol.TypeSymbol enclosingClassSymbol) { + return Optional.ofNullable(enclosingClassSymbol).map(Symbol::declaration).map(ClassTree.class::cast); + } + + private void reportIfNonSingletonInSingleton(ClassTree enclosingClassTree, VariableTree variableTree, String injectionType) { + if (isSingletonBean(enclosingClassTree) && hasTypeNotSingletonBean(variableTree)) { + reportIssue(variableTree.type(), "Don't auto-wire this non-Singleton bean into a Singleton bean (" + injectionType + ")."); + } + } + + private static boolean hasTypeNotSingletonBean(VariableTree variableTree) { + var typeSymbolDeclaration = variableTree.type().symbolType().symbol().declaration(); + return typeSymbolDeclaration != null && hasNotSingletonScopeAnnotation(typeSymbolDeclaration.modifiers().annotations()); + } + + private static boolean isAutoWiringAnnotation(AnnotationTree annotationTree) { + return AUTO_WIRING_ANNOTATIONS.contains(annotationTree.symbolType().fullyQualifiedName()); + } + + private static boolean isSingletonBean(ClassTree classTree) { + return !hasNotSingletonScopeAnnotation(classTree.modifiers().annotations()); + } + + private static boolean hasNotSingletonScopeAnnotation(List annotations) { + // Only classes annotated with @Scope, having a value different from "singleton", are considered as non-Singleton + return annotations.stream().anyMatch(NonSingletonAutowiredInSingletonCheck::isNotSingletonScopeAnnotation); + } + + private static boolean isNotSingletonScopeAnnotation(AnnotationTree annotationTree) { + return annotationTree.symbolType().is(SCOPED_ANNOTATION) + && (isNotSingletonLiteralValue(annotationTree.arguments()) || isNotSingletonAssignmentValue(annotationTree.arguments())); + } + + private static boolean isNotSingletonLiteralValue(Arguments arguments) { + return arguments.size() == 1 + && arguments.get(0).is(Tree.Kind.STRING_LITERAL) + && isNotSingletonLiteral(((LiteralTree) arguments.get(0)).value()); + } + + private static boolean isNotSingletonAssignmentValue(Arguments arguments) { + return arguments + .stream() + .filter(argument -> argument.is(Tree.Kind.ASSIGNMENT)) + .map(AssignmentExpressionTree.class::cast) + .anyMatch(NonSingletonAutowiredInSingletonCheck::isNotAssignmentToSingletonValue); + } + + private static boolean isNotAssignmentToSingletonValue(AssignmentExpressionTree assignmentExpressionTree) { + var expression = assignmentExpressionTree.expression(); + + if (expression.is(Tree.Kind.STRING_LITERAL)) { + return isNotSingletonLiteral(((LiteralTree) expression).value()); + } + + var variable = (IdentifierTree) assignmentExpressionTree.variable(); + + return ("value".equals(variable.name()) || "scopeName".equals(variable.name())) + && (expression.is(Tree.Kind.MEMBER_SELECT) && isNotSingletonLiteral(((MemberSelectExpressionTree) expression).identifier().name())); + } + + private static boolean isNotSingletonLiteral(String value) { + return SINGLETON_LITERALS.stream().noneMatch(singletonLiteral -> singletonLiteral.equalsIgnoreCase(value)); + } + +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.html new file mode 100644 index 00000000000..772748b5dff --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.html @@ -0,0 +1,141 @@ +

In Spring the scope of a bean defines the lifecycle and visibility of that bean in the Spring container. There are six scopes:

+
    +
  • Singleton: default, one instance per Spring container
    • +
  • Prototype: a new instance per bean request
    • +
  • Request: a new instance per HTTP request
    • +
  • Session: a new instance per HTTP session
    • +
  • Application: a new instance per ServletContext
    • +
  • Websocket: a new instance per Websocket session
    • +
+

The last four scopes mentioned, request, session, application and websocket, are only available in a web-aware application.

+

Why is this an issue?

+

In Spring, singleton beans and their dependencies are initialized when the application context is created.

+

If a Singleton bean depends on a bean with a shorter-lived scope (like Request or Session beans), it retains +the same instance of that bean, even when new instances are created for each Request or Session. This mismatch can cause unexpected behavior and bugs, +as the Singleton bean doesn’t interact correctly with the new instances of the shorter-lived bean.

+

This rule raises an issue when non-singleton beans are injected into a singleton bean.

+

What is the potential impact?

+

When a Singleton bean has a dependency on a bean with a shorter-lived scope, it can lead to the following issues:

+
    +
  • Data inconsistency: any state change in the shorter-lived bean will not be reflected in the Singleton bean.
    • +
  • Incorrect behavior: using the same instance of the shorter-lived bean, when a new instance is supposed to be created for each + new request or session.
    • +
  • Memory leaks: preventing garbage collection of a shorter-lived bean that allocates a significant amount of data over time. +
    • +
+

How to fix it

+

Inject a shorter-lived bean into a Singleton bean using ApplicationContext, Factories or +Providers.

+

Code examples

+

Noncompliant code example

+

When a Singleton bean auto-wires a Request bean, the dependency is resolved at instantiation time and thus the same +instance is used for each HTTP request.

+
+@Component
+@Scope(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)
+public class RequestBean {
+    //...
+}
+
+public class SingletonBean {
+    @Autowired
+    private final RequestBean requestBean; // Noncompliant, the same instance of RequestBean is used for each HTTP request.
+
+    public RequestBean getRequestBean() {
+        return requestBean;
+    }
+}
+
+

Compliant solution

+

Instead, use a ObjectFactory<RequestBean>, ObjectProvider<RequestBean>, or +Provider<RequestBean> as injection point (as for JSR-330).

+

Such a dependency is resolved at runtime, allowing for actual injection of a new instance of the shorter-lived bean on each HTTP request.

+
+@Component
+@Scope(value = WebApplicationContext.SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS)
+public class RequestBean {
+    //...
+}
+
+public class SingletonBean {
+    private final ObjectFactory<RequestBean> requestBeanFactory;
+
+    @Autowired
+    public SingletonBean(ObjectFactory<RequestBean> requestBeanFactory) {
+        this.requestBeanFactory = requestBeanFactory;
+    }
+
+    public RequestBean getRequestBean() {
+        return requestBeanFactory.getObject();
+    }
+}
+
+

Noncompliant code example

+

When a Singleton bean auto-wires a Prototype bean, the dependency is resolved at instantiation time and thus the same +instance is used for each bean request.

+
+@Component
+@Scope("prototype")
+public class PrototypeBean {
+    public Object execute() {
+      //...
+    }
+}
+
+public class SingletonBean {
+    private PrototypeBean prototypeBean;
+
+    @Autowired
+    public SingletonBean(PrototypeBean prototypeBean) { // Noncompliant, the same instance of PrototypeBean is used for each bean request.
+      this.prototypeBean = prototypeBean;
+    }
+
+    public Object process() {
+        return prototypeBean.execute();
+    }
+}
+
+

Compliant solution

+

Using the ApplicationContext to retrieve a new instance of a Prototype bean on each bean request.

+
+@Component
+@Scope("prototype")
+public class PrototypeBean {
+    public Object execute() {
+      //...
+    }
+}
+
+public class SingletonBean implements ApplicationContextAware {
+    private ApplicationContext applicationContext;
+
+    @Autowired
+    public SingletonBean(ApplicationContext applicationContext) {
+      this.applicationContext = applicationContext;
+    }
+
+    public Object process() {
+        PrototypeBean prototypeBean = createPrototypeBean();
+        return prototypeBean.execute();
+    }
+
+    protected PrototypeBean createPrototypeBean() {
+        return this.applicationContext.getBean("prototypeBean", PrototypeBean.class);
+    }
+}
+
+

Resources

+

Documentation

+ +

Articles & blog posts

+ + diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.json new file mode 100644 index 00000000000..de2eef99efd --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/java/S6832.json @@ -0,0 +1,23 @@ +{ + "title": "Non-singleton Spring beans should not be injected into singleton beans", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "spring" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6832", + "sqKey": "S6832", + "scope": "All", + "quickfix": "infeasible", + "code": { + "impacts": { + "RELIABILITY": "MEDIUM" + }, + "attribute": "LOGICAL" + } +} 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 ee0d9241bc0..e5feeefa1a4 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 @@ -491,6 +491,7 @@ "S6829", "S6830", "S6831", + "S6832", "S6833", "S6837" ] diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/MethodTreeUtilsTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/MethodTreeUtilsTest.java index 2e6c49e07d6..3bca057b801 100644 --- a/java-checks/src/test/java/org/sonar/java/checks/helpers/MethodTreeUtilsTest.java +++ b/java-checks/src/test/java/org/sonar/java/checks/helpers/MethodTreeUtilsTest.java @@ -72,6 +72,17 @@ void is_hashcode_method() { assertFalse(MethodTreeUtils.isHashCodeMethod(parseMethod("class A { public int hashCode(int a){} }"))); } + @Test + void is_setter_method() { + assertTrue(MethodTreeUtils.isSetterMethod(parseMethod("class A { public void setFoo(String foo){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public static void setFoo(String foo){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public int setFoo(String foo){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public void setFoo(String foo, int a){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public void setFoo(){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public void foo(){} }"))); + assertFalse(MethodTreeUtils.isSetterMethod(parseMethod("class A { public void foo(String foo){} }"))); + } + @Test void consecutive_and_subsequent_method_invocation() { List methodInvocationList = new ArrayList<>(); diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheckTest.java new file mode 100644 index 00000000000..175e362d4d1 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/NonSingletonAutowiredInSingletonCheckTest.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.TestUtils; + +import static org.junit.jupiter.api.Assertions.*; +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; + +class NonSingletonAutowiredInSingletonCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(TestUtils.mainCodeSourcesPath("checks/spring/NonSingletonAutowiredInSingletonCheckSample.java")) + .withCheck(new NonSingletonAutowiredInSingletonCheck()) + .verifyIssues(); + } + + @Test + void test_non_semantics() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/spring/NonSingletonAutowiredInSingletonCheckSample.java")) + .withCheck(new NonSingletonAutowiredInSingletonCheck()) + .withoutSemantic() + .verifyNoIssues(); + } +}