Skip to content

SONARJAVA-6756: Fix FPs in S8989 for NOT_SUPPORTED and readOnly transactions #5923

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.

Sign up for GitHub

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

Open
romainbrenguier wants to merge 2 commits into
base: master
Choose a base branch
from
Open

SONARJAVA-6756: Fix FPs in S8989 for NOT_SUPPORTED and readOnly transactions #5923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4825097
SONARJAVA-6756 Fix FPs in S8989 for NOT_SUPPORTED and readOnly transa…
romainbrenguier Aug 14, 2026
2a28716
SONARJAVA-6756 Improve test coverage for S8989 TransactionalMethodChe…
romainbrenguier Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...n-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,10 @@ public void unknownException() throws UnresolvedCheckedException { // No issue -
public void knownException() throws java.io.IOException { // Noncompliant [[secondary=12]]
// ^^^^^^^^^^^^^^
}

// Propagation set to an unresolvable expression (not an identifier or member select)
@Transactional(propagation = getPropagation())
public void propagationFromMethodCall() throws java.io.IOException { // Noncompliant [[secondary=18]]
// ^^^^^^^^^^^^^^^^^^^^^^^^^
}
}
99 changes: 86 additions & 13 deletions ...s/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
import java.sql.SQLException;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import static org.springframework.transaction.annotation.Propagation.NOT_SUPPORTED;

import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;

// Composed/meta-annotation for testing
Expand All @@ -19,7 +22,7 @@ class CustomCheckedException extends Exception {}
public class TransactionalMethodCheckedExceptionCheckSample {

@Transactional
public void processOrder(Order order) throws IOException, SQLException { // Noncompliant [[secondary=21;quickfixes=qf1,qf2]]
public void processOrder(Order order) throws IOException, SQLException { // Noncompliant [[secondary=22;quickfixes=qf1,qf2]]
// ^^^^^^^^^^^^
// fix@qf1 {{Add rollbackFor attribute}}
// edit@qf1 [[sl=-1;sc=3;el=-1;ec=17]] {{@Transactional(rollbackFor = {java.io.IOException.class, java.sql.SQLException.class})}}
Expand All @@ -28,19 +31,19 @@ public class TransactionalMethodCheckedExceptionCheckSample {
}

@Transactional
public void importData() throws Exception { // Noncompliant [[secondary=30;quickfixes=qf3]]
public void importData() throws Exception { // Noncompliant [[secondary=31;quickfixes=qf3]]
// ^^^^^^^^^^
// fix@qf3 {{Add rollbackFor = Exception.class}}
// edit@qf3 [[sl=-1;sc=3;el=-1;ec=17]] {{@Transactional(rollbackFor = java.lang.Exception.class)}}
}

@Transactional(timeout = 30)
public void withOtherAttributes() throws SQLException { // Noncompliant [[secondary=37]]
public void withOtherAttributes() throws SQLException { // Noncompliant [[secondary=38]]
// ^^^^^^^^^^^^^^^^^^^
}

@Transactional
public void customException() throws CustomCheckedException { // Noncompliant [[secondary=42;quickfixes=qf7,qf8]]
public void customException() throws CustomCheckedException { // Noncompliant [[secondary=43;quickfixes=qf7,qf8]]
// ^^^^^^^^^^^^^^^
// fix@qf7 {{Add rollbackFor attribute}}
// edit@qf7 [[sl=-1;sc=3;el=-1;ec=17]] {{@Transactional(rollbackFor = checks.spring.CustomCheckedException.class)}}
Expand All @@ -49,7 +52,7 @@ public void withOtherAttributes() throws SQLException { // Noncompliant [[second
}

@Transactional
public void mixedExceptions() throws IOException, RuntimeException { // Noncompliant [[secondary=51;quickfixes=qf9,qf10]]
public void mixedExceptions() throws IOException, RuntimeException { // Noncompliant [[secondary=52;quickfixes=qf9,qf10]]
// ^^^^^^^^^^^^^^^
// fix@qf9 {{Add rollbackFor attribute}}
// edit@qf9 [[sl=-1;sc=3;el=-1;ec=17]] {{@Transactional(rollbackFor = java.io.IOException.class)}}
Expand Down Expand Up @@ -104,7 +107,7 @@ public void inherited() throws IOException {

@Transactional
static class ClassLevelNoConfig {
public void noConfig() throws IOException { // Noncompliant [[secondary=105]]
public void noConfig() throws IOException { // Noncompliant [[secondary=106]]
// ^^^^^^^^ {{Specify rollback behavior for checked exceptions using "rollbackFor" or "noRollbackFor" attributes on the class-level @Transactional.}}
}

Expand All @@ -118,7 +121,7 @@ public void errorNotChecked() throws Error {
}

@org.springframework.transaction.annotation.Transactional
public void fullyQualified() throws IOException { // Noncompliant [[secondary=120;quickfixes=qf13,qf14]]
public void fullyQualified() throws IOException { // Noncompliant [[secondary=121;quickfixes=qf13,qf14]]
// ^^^^^^^^^^^^^^
// fix@qf13 {{Add rollbackFor attribute}}
// edit@qf13 [[sl=-1;sc=3;el=-1;ec=60]] {{@org.springframework.transaction.annotation.Transactional(rollbackFor = java.io.IOException.class)}}
Expand All @@ -131,15 +134,15 @@ public void partialConfig() throws SQLException {
}

@Transactional(value = "txManager")
public void withValueAttribute() throws IOException { // Noncompliant [[secondary=133]]
public void withValueAttribute() throws IOException { // Noncompliant [[secondary=134]]
// ^^^^^^^^^^^^^^^^^^
}

// Test nested structure to ensure parent traversal works
static class OuterClass {
@Transactional
static class InnerClassWithAnnotation {
public void nestedMethod() throws IOException { // Noncompliant [[secondary=140]]
public void nestedMethod() throws IOException { // Noncompliant [[secondary=141]]
// ^^^^^^^^^^^^ {{Specify rollback behavior for checked exceptions using "rollbackFor" or "noRollbackFor" attributes on the class-level @Transactional.}}
}
}
Expand All @@ -153,19 +156,19 @@ public void nestedMethodNoAnnotation() throws IOException {

@Transactional
interface TransactionalInterface {
void interfaceMethod() throws IOException; // Noncompliant [[secondary=154]]
void interfaceMethod() throws IOException; // Noncompliant [[secondary=155]]
// ^^^^^^^^^^^^^^^ {{Specify rollback behavior for checked exceptions using "rollbackFor" or "noRollbackFor" attributes on the class-level @Transactional.}}
}

// Test meta-annotated (composed) annotation
@MyTransactional
public void metaAnnotated() throws IOException { // Noncompliant [[secondary=161]]
public void metaAnnotated() throws IOException { // Noncompliant [[secondary=162]]
// ^^^^^^^^^^^^^
}

// Test annotation with value attribute (transaction manager name)
@Transactional("txManager")
public void valueShorthand() throws IOException { // Noncompliant [[secondary=167]]
public void valueShorthand() throws IOException { // Noncompliant [[secondary=168]]
// ^^^^^^^^^^^^^^
// Has value attribute but no rollback configuration
}
Expand Down Expand Up @@ -198,8 +201,78 @@ protected void protectedInClassLevel() throws IOException { // Compliant - prote
void packagePrivateInClassLevel() throws IOException { // Compliant - package-private methods are not proxied
}

public void publicInClassLevel() throws IOException { // Noncompliant [[secondary=190]]
public void publicInClassLevel() throws IOException { // Noncompliant [[secondary=191]]
// ^^^^^^^^^^^^^^^^^^
}
}

// Compliant: propagation = NOT_SUPPORTED means no transaction is created
@Transactional(propagation = Propagation.NOT_SUPPORTED)
public void notSupported() throws IOException { // Compliant
}

// Compliant: propagation = NEVER means no transaction is created
@Transactional(propagation = Propagation.NEVER)
public void neverPropagation() throws IOException { // Compliant
}

// Compliant: readOnly = true means no writes can occur
@Transactional(readOnly = true)
public void readOnlyTransaction() throws IOException { // Compliant
}

// Compliant: combination of readOnly and NOT_SUPPORTED
@Transactional(readOnly = true, propagation = Propagation.NOT_SUPPORTED)
public void readOnlyAndNotSupported() throws IOException { // Compliant
}

// readOnly = false with default propagation is still noncompliant
@Transactional(readOnly = false)
public void readOnlyFalse() throws IOException { // Noncompliant [[secondary=228]]
// ^^^^^^^^^^^^^
}

// Explicit REQUIRED propagation still needs rollback config
@Transactional(propagation = Propagation.REQUIRED)
public void requiredPropagation() throws IOException { // Noncompliant [[secondary=234]]
// ^^^^^^^^^^^^^^^^^^^
}

// Compliant: NOT_SUPPORTED with other attributes
@Transactional(propagation = Propagation.NOT_SUPPORTED, timeout = 30)
public void notSupportedWithTimeout() throws IOException { // Compliant
}

// Compliant: readOnly with other attributes
@Transactional(readOnly = true, timeout = 30)
public void readOnlyWithTimeout() throws IOException { // Compliant
}

// Compliant: static-imported NOT_SUPPORTED propagation (identifier without member select)
@Transactional(propagation = NOT_SUPPORTED)
public void notSupportedStaticImport() throws IOException { // Compliant
}

@Transactional(propagation = Propagation.NOT_SUPPORTED)
static class ClassLevelNotSupported {
public void methodInNotSupportedClass() throws IOException { // Compliant
}
}

@Transactional(readOnly = true)
static class ClassLevelReadOnly {
public void methodInReadOnlyClass() throws IOException { // Compliant
}
}

// Method with non-Transactional annotations, class-level @Transactional found after iterating class annotations
@Deprecated
@Transactional
static class ClassWithMultipleAnnotations {
@Deprecated
@SuppressWarnings("unused")
public void methodWithOtherAnnotations() throws IOException { // Noncompliant [[secondary=270]]
// ^^^^^^^^^^^^^^^^^^^^^^^^^^
}
}
}
54 changes: 54 additions & 0 deletions .../src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,12 @@
import org.sonar.plugins.java.api.semantic.Type;
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.ExpressionTree;
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.Modifier;
import org.sonar.plugins.java.api.tree.SyntaxToken;
Expand Down Expand Up @@ -83,6 +88,16 @@ public void visitNode(Tree tree) {
return;
}

// No transaction is created with NOT_SUPPORTED or NEVER propagation, so rollback configuration is inapplicable
if (hasNonTransactionalPropagation(transactionalAnnotation)) {
return;
}

// Read-only transactions perform no write operations, making rollback policies irrelevant
if (hasReadOnly(transactionalAnnotation)) {
return;
}

boolean isClassLevel = isClassLevelAnnotation(method, transactionalAnnotation);

var issueBuilder = QuickFixHelper.newIssue(context)
Expand Down Expand Up @@ -195,6 +210,45 @@ private JavaQuickFix createQuickFix(AnnotationTree annotation, String attribute,
}
}

private static boolean hasNonTransactionalPropagation(AnnotationTree annotation) {
return annotation.arguments().stream()
.anyMatch(arg -> {
if (arg.is(Tree.Kind.ASSIGNMENT)) {
var assignment = (AssignmentExpressionTree) arg;
String name = ((IdentifierTree) assignment.variable()).name();
if ("propagation".equals(name)) {
String enumName = resolveEnumConstantName(assignment.expression());
return "NOT_SUPPORTED".equals(enumName) || "NEVER".equals(enumName);
}
}
return false;
});
}

private static boolean hasReadOnly(AnnotationTree annotation) {
return annotation.arguments().stream()
.anyMatch(arg -> {
if (arg.is(Tree.Kind.ASSIGNMENT)) {
var assignment = (AssignmentExpressionTree) arg;
String name = ((IdentifierTree) assignment.variable()).name();
if ("readOnly".equals(name)) {
ExpressionTree expression = assignment.expression();
return expression.is(Tree.Kind.BOOLEAN_LITERAL) && "true".equals(((LiteralTree) expression).value());
}
}
return false;
});
}
Comment on lines +228 to +241

@gitar-bot gitar-bot Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Edge Case: readOnly=true suppression may hide genuine rollback gaps

readOnly = true is only a hint to the transaction manager/driver; it does not guarantee that no writes occur (e.g. native SQL, some JPA flush scenarios still write). A checked exception thrown during such a write would not trigger rollback without a rollbackFor attribute, so fully suppressing S8989 for readOnly could introduce a false negative. This matches the PR's FP-reduction intent, but consider limiting suppression or documenting the tradeoff.

Was this helpful? React with 👍 / 👎


private static String resolveEnumConstantName(ExpressionTree expression) {
if (expression.is(Tree.Kind.MEMBER_SELECT)) {
return ((MemberSelectExpressionTree) expression).identifier().name();
} else if (expression.is(Tree.Kind.IDENTIFIER)) {
return ((IdentifierTree) expression).name();
}
return "";
}

private static boolean isCheckedException(Type type) {
if (type.isUnknown()) {
return false;
Expand Down
Loading