diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java index 1a2e8bb8f96..e8b14751c9e 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java @@ -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]] +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + } } diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java index aa1e53b4865..cc9bfe1dfa3 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java @@ -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 @@ -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})}} @@ -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)}} @@ -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)}} @@ -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.}} } @@ -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)}} @@ -131,7 +134,7 @@ public void partialConfig() throws SQLException { } @Transactional(value = "txManager") - public void withValueAttribute() throws IOException { // Noncompliant [[secondary=133]] + public void withValueAttribute() throws IOException { // Noncompliant [[secondary=134]] // ^^^^^^^^^^^^^^^^^^ } @@ -139,7 +142,7 @@ public void withValueAttribute() throws IOException { // Noncompliant [[secondar 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.}} } } @@ -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 } @@ -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]] +// ^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java index 85c2cfce102..7ce07f4b0ae 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java @@ -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; @@ -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) @@ -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; + }); + } + + 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;