-
Notifications
You must be signed in to change notification settings - Fork 725
SONARJAVA-6491: Implement S8911 - Methods annotated with @Startup should be non-static, non-producer, and parameter-free #5694
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
93ac858
6899a76
b1ba82b
a1ecdd2
f13ddc5
5862393
7a912e3
46ab3e7
e72e160
683a419
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S6813", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 65, | ||
| "falseNegatives": 66, | ||
| "falsePositives": 0 | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8911", | ||
| "hasTruePositives": true, | ||
| "falseNegatives": 1, | ||
| "falsePositives": 0 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| package checks; | ||
|
|
||
| import io.quarkus.runtime.Startup; | ||
| import jakarta.enterprise.context.ApplicationScoped; | ||
| import jakarta.enterprise.inject.Produces; | ||
| import jakarta.inject.Inject; | ||
|
|
||
| @ApplicationScoped | ||
| public class StartupAnnotationCheckSample { | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| static void staticMethod() { // Noncompliant {{"@Startup" annotation should not be applied to static methods}} | ||
| // ^^^^^^^^^^^^ | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| void withParameters(String config) { // Noncompliant {{"@Startup" annotation should only be applied to no-arg methods}} | ||
| // ^^^^^^^^^^^^^^ | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| @Produces | ||
| String producer() { // Noncompliant {{"@Startup" annotation should not be applied to producer methods}} | ||
| // ^^^^^^^^ | ||
| return "config"; | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| static void staticWithParams(String arg) { // Noncompliant {{"@Startup" annotation should not be applied to static methods}} | ||
| // ^^^^^^^^^^^^^^^^ | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| @Produces | ||
| static String staticProducer() { // Noncompliant {{"@Startup" annotation should not be applied to static methods}} | ||
| // ^^^^^^^^^^^^^^ | ||
| return "value"; | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| @Produces | ||
| Integer producerWithParams(String input) { // Noncompliant {{"@Startup" annotation should not be applied to producer methods}} | ||
| // ^^^^^^^^^^^^^^^^^^ | ||
| return 42; | ||
| } | ||
|
|
||
| @Startup | ||
| // ^^^^^^^ > | ||
| @Produces | ||
| static Double allViolations(String arg1, int arg2) { // Noncompliant {{"@Startup" annotation should not be applied to static methods}} | ||
| // ^^^^^^^^^^^^^ | ||
| return 3.14; | ||
| } | ||
|
|
||
| @Startup | ||
| void initialize() { | ||
| } | ||
|
|
||
| @Inject | ||
| DatabaseService db; | ||
|
|
||
| @Startup | ||
| void onStart() { | ||
| db.connect(); | ||
| } | ||
|
|
||
| static void staticHelper() { | ||
| } | ||
|
|
||
| void setupWithParams(String config, int port) { | ||
| } | ||
|
|
||
| @Produces | ||
| String createConfig() { | ||
| return "config"; | ||
| } | ||
|
|
||
| @Startup | ||
| private void privateInit() { | ||
| } | ||
|
|
||
| static class DatabaseService { | ||
| void connect() { | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package io.quarkus.runtime; | ||
|
|
||
| import java.lang.annotation.ElementType; | ||
| import java.lang.annotation.Retention; | ||
| import java.lang.annotation.RetentionPolicy; | ||
| import java.lang.annotation.Target; | ||
|
|
||
| @Target({ElementType.TYPE, ElementType.METHOD}) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| public @interface Startup { | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,94 @@ | ||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||
| * SonarQube Java | ||||||||||||||||||||||||
| * Copyright (C) SonarSource Sàrl | ||||||||||||||||||||||||
| * mailto:info AT sonarsource DOT com | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * You can redistribute and/or modify this program under the terms of | ||||||||||||||||||||||||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * 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 Sonar Source-Available License for more details. | ||||||||||||||||||||||||
| * | ||||||||||||||||||||||||
| * You should have received a copy of the Sonar Source-Available License | ||||||||||||||||||||||||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
| package org.sonar.java.checks; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||
| import org.sonar.check.Rule; | ||||||||||||||||||||||||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||||||||||||||||||||||||
| import org.sonar.plugins.java.api.JavaFileScannerContext; | ||||||||||||||||||||||||
| import org.sonar.plugins.java.api.tree.AnnotationTree; | ||||||||||||||||||||||||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||||||||||||||||||||||||
| import org.sonar.plugins.java.api.tree.Tree; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Rule(key = "S8911") | ||||||||||||||||||||||||
| public class StartupAnnotationCheck extends IssuableSubscriptionVisitor { | ||||||||||||||||||||||||
| private static final String STARTUP_FQN = "io.quarkus.runtime.Startup"; | ||||||||||||||||||||||||
| private static final String PRODUCES_FQN = "jakarta.enterprise.inject.Produces"; | ||||||||||||||||||||||||
|
Comment on lines
+29
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually we prefer matchers
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if it's possible, I never did it for annotations. Optional : let's give it a try with an agent
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After exploring the codebase, the current pattern of using
The current implementation follows pattern #1, which is appropriate here since we need to create secondary locations pointing to the specific
Given that your follow-up comment indicates this is optional and you're uncertain if matchers work for annotations, I believe the current implementation is correct as-is.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After searching the codebase, there doesn't appear to be a built-in annotation matcher utility (like Creating an annotation matcher would be a larger architectural addition. Since this is marked optional, I'm keeping the current approach which follows the established pattern in the codebase. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||
| public List<Tree.Kind> nodesToVisit() { | ||||||||||||||||||||||||
| return List.of(Tree.Kind.METHOD); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||
| public void visitNode(Tree tree) { | ||||||||||||||||||||||||
| MethodTree methodTree = (MethodTree) tree; | ||||||||||||||||||||||||
| var startupAnnotations = getStartupAnnotations(methodTree); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (startupAnnotations.isEmpty()) { | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| var secondaryLocations = startupAnnotations.stream() | ||||||||||||||||||||||||
| .map(annotation -> | ||||||||||||||||||||||||
| new JavaFileScannerContext.Location("Triggered by this annotation", annotation.annotationType()) | ||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||
| .toList(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // Only report one issue per method, prioritizing: static > producer > parameters | ||||||||||||||||||||||||
| if (methodTree.symbol().isStatic()) { | ||||||||||||||||||||||||
| reportIssue( | ||||||||||||||||||||||||
| methodTree.simpleName(), | ||||||||||||||||||||||||
| "\"@Startup\" annotation should not be applied to static methods", | ||||||||||||||||||||||||
| secondaryLocations, | ||||||||||||||||||||||||
| null); | ||||||||||||||||||||||||
|
Comment on lines
+54
to
+58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| var producesAnnotations = getProducesAnnotations(methodTree); | ||||||||||||||||||||||||
| if (!producesAnnotations.isEmpty()) { | ||||||||||||||||||||||||
| reportIssue( | ||||||||||||||||||||||||
| methodTree.simpleName(), | ||||||||||||||||||||||||
| "\"@Startup\" annotation should not be applied to producer methods", | ||||||||||||||||||||||||
| secondaryLocations, | ||||||||||||||||||||||||
| null | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| if (!methodTree.parameters().isEmpty()) { | ||||||||||||||||||||||||
| reportIssue( | ||||||||||||||||||||||||
| methodTree.simpleName(), | ||||||||||||||||||||||||
| "\"@Startup\" annotation should only be applied to no-arg methods", | ||||||||||||||||||||||||
| secondaryLocations, | ||||||||||||||||||||||||
| null | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private static List<AnnotationTree> getStartupAnnotations(MethodTree methodTree) { | ||||||||||||||||||||||||
| return methodTree.modifiers().annotations().stream() | ||||||||||||||||||||||||
| .filter(annotation -> annotation.annotationType().symbolType().is(STARTUP_FQN)) | ||||||||||||||||||||||||
| .toList(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| private static List<AnnotationTree> getProducesAnnotations(MethodTree methodTree) { | ||||||||||||||||||||||||
| return methodTree.modifiers().annotations().stream() | ||||||||||||||||||||||||
| .filter(annotation -> annotation.annotationType().symbolType().is(PRODUCES_FQN)) | ||||||||||||||||||||||||
| .toList(); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * 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 Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.sonar.java.checks.verifier.CheckVerifier; | ||
|
|
||
| import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; | ||
|
|
||
| class StartupAnnotationCheckTest { | ||
| private static final String SAMPLE_FILE = mainCodeSourcesPath("checks/StartupAnnotationCheckSample.java"); | ||
| private static final StartupAnnotationCheck CHECK = new StartupAnnotationCheck(); | ||
|
|
||
| @Test | ||
| void test() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(SAMPLE_FILE) | ||
| .withCheck(CHECK) | ||
| .verifyIssues(); | ||
| } | ||
|
|
||
| @Test | ||
| void testWithoutSemantic() { | ||
| CheckVerifier.newVerifier() | ||
| .onFile(SAMPLE_FILE) | ||
| .withCheck(CHECK) | ||
| .withoutSemantic() | ||
| .verifyNoIssues(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| <p>This is an issue when a method designated for automatic invocation at application startup is declared as a class-level method rather than an | ||
| instance method, is configured to produce managed objects, or requires input arguments. Such methods will not be properly invoked during application | ||
| startup.</p> | ||
| <p>In Java, this specifically refers to methods annotated with <code>@Startup</code>.</p> | ||
| <h2>Why is this an issue?</h2> | ||
| <p>Annotations or attributes that mark methods for execution during application startup in dependency injection frameworks have specific requirements | ||
| for how these initialization methods must be declared.</p> | ||
| <p>When you mark a method for startup execution, the dependency injection framework generates initialization hooks that trigger the method when the | ||
| application starts. For this mechanism to work correctly, the method must meet three requirements:</p> | ||
| <ul> | ||
| <li> <strong>Non-static</strong>: The method must be an instance method. Dependency injection frameworks work by creating managed component | ||
| instances and invoking methods on those instances. A static method belongs to the class itself, not to an instance, so the framework cannot properly | ||
| manage its lifecycle or invoke it as part of component initialization. </li> | ||
| <li> <strong>Non-producer</strong>: The method cannot be a producer method (one that creates and provides instances for other components to | ||
| consume). Producer methods are designed to create and provide component instances, not to perform initialization logic. Combining startup execution | ||
| markers with producer functionality creates a conflict in the intended purpose of the method. </li> | ||
| <li> <strong>No arguments</strong>: The method must not accept any parameters. The framework invokes these methods automatically at startup and has | ||
| no mechanism to provide argument values. If a method requires parameters, the framework cannot call it. </li> | ||
| </ul> | ||
| <h3>What is the potential impact?</h3> | ||
| <p>When methods designated for startup execution don’t follow the required signature, the initialization logic will not execute at application | ||
| startup. This can lead to:</p> | ||
| <ul> | ||
| <li> Missing configuration or setup steps that are essential for the application to function correctly </li> | ||
| <li> Uninitialized resources or connections that other parts of the application depend on </li> | ||
| <li> Runtime errors when the application tries to use components that were supposed to be initialized at startup </li> | ||
| <li> Silent failures where the method is simply not called, making the problem difficult to diagnose </li> | ||
| </ul> | ||
| <h2>How to fix it</h2> | ||
| <p>Remove the <code>static</code> modifier from the method to make it an instance method. Ensure the method has no parameters and is not annotated | ||
| with <code>@Produces</code>.</p> | ||
| <h3>Code examples</h3> | ||
| <h4>Noncompliant code example</h4> | ||
| <pre data-diff-id="1" data-diff-type="noncompliant"> | ||
| @ApplicationScoped | ||
| public class EagerAppBean { | ||
| @Startup | ||
| static void init() { // Noncompliant: static method | ||
| doSomeCoolInit(); | ||
| } | ||
| } | ||
| </pre> | ||
| <h4>Compliant solution</h4> | ||
| <pre data-diff-id="1" data-diff-type="compliant"> | ||
| @ApplicationScoped | ||
| public class EagerAppBean { | ||
| @Startup | ||
| void init() { // Compliant: non-static, no arguments | ||
| doSomeCoolInit(); | ||
| } | ||
| } | ||
| </pre> | ||
| <h2>Resources</h2> | ||
| <h3>Documentation</h3> | ||
| <ul> | ||
| <li> Quarkus - Application Initialization and Termination - <a href="https://quarkus.io/guides/lifecycle">Official Quarkus guide on using @Startup | ||
| and other lifecycle annotations</a> </li> | ||
| <li> Jakarta CDI Specification - <a href="https://jakarta.ee/specifications/cdi/">The specification for Contexts and Dependency Injection in Jakarta | ||
| EE</a> </li> | ||
| </ul> | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| { | ||
| "title": "Methods annotated with \"@Startup\" should be non-static, non-producer, and parameter-free", | ||
| "type": "BUG", | ||
| "status": "ready", | ||
| "remediation": { | ||
| "func": "Constant\/Issue", | ||
| "constantCost": "5min" | ||
| }, | ||
| "tags": [ | ||
| "injection", | ||
| "lifecycle", | ||
| "quarkus" | ||
| ], | ||
| "defaultSeverity": "Critical", | ||
| "ruleSpecification": "RSPEC-8911", | ||
| "sqKey": "S8911", | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
| "scope": "Main", | ||
| "quickfix": "unknown", | ||
| "code": { | ||
| "impacts": { | ||
| "RELIABILITY": "HIGH" | ||
| }, | ||
| "attribute": "LOGICAL" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -532,6 +532,7 @@ | |
| "S8715", | ||
| "S8745", | ||
| "S8786", | ||
| "S8911", | ||
| "S8924" | ||
| ] | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.