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 e8b14751c9e..e25c7ece2d8 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 @@ -19,4 +19,31 @@ public void knownException() throws java.io.IOException { // Noncompliant [[seco public void propagationFromMethodCall() throws java.io.IOException { // Noncompliant [[secondary=18]] // ^^^^^^^^^^^^^^^^^^^^^^^^^ } + + @Transactional + public void unknownThrownTypeAfterWrite(Repository repository) throws java.io.IOException { // Noncompliant [[secondary=24]] +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(null); + throw new UnknownException(); + } + + @Transactional + public void unknownMethodAfterWrite(Repository repository) throws java.io.IOException { // Noncompliant [[secondary=31]] +// ^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(null); + unknownMethod(); + } + + @Transactional + public void checkedThrowBeforeWrite(Repository repository) throws java.io.IOException { // Compliant + if (repository == null) { + throw new java.io.IOException(); + } + repository.save(null); + } + + static class Repository { + void save(Object o) { + } + } } 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 0b2b5edc2ad..50760808aad 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 @@ -414,4 +414,203 @@ public void directSaveAndLambda() throws IOException { // Noncompliant [[seconda Runnable r = () -> deleteEntity(null); } } + + static class Repository { + void save(Object o) { + } + + void saveChecked(Object o) throws IOException { + } + } + + static class Validator { + static void validate(Object o) throws IOException { + } + + Validator() throws SQLException { + } + } + + @Transactional + static class ThrowsBeforeWrites { + private final Repository repository = new Repository(); + + public void saveAfterThrow(Object o) throws IOException { // Compliant - all checked exceptions are thrown before any write + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + + public void saveAfterValidation(Object o) throws IOException { // Compliant - validation throwing checked exception happens before any write + Validator.validate(o); + repository.save(o); + } + + public void saveAfterConstructor(Object o) throws SQLException { // Compliant - constructor throwing checked exception happens before any write + new Validator(); + repository.save(o); + } + + public void saveAfterThrowInLoop(java.util.List list) throws IOException { // Compliant + for (Object o : list) { + if (o == null) { + throw new IOException("null"); + } + } + list.forEach(repository::save); + list.forEach(o -> repository.save(o)); + } + + public void saveBeforeThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^ + repository.save(o); + if (o == null) { + throw new IOException("null"); + } + } + + public void saveBeforeValidation(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + Validator.validate(o); + } + + public void saveBeforeConstructor(Object o) throws SQLException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + new Validator(); + } + + public void saveThrowingChecked(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^ + repository.saveChecked(o); + } + + public void saveInLoopWithThrow(java.util.List list) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^ + for (Object o : list) { + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + } + + public void saveInLambdaBeforeThrow(java.util.List list) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^ + list.forEach(o -> repository.save(o)); + Validator.validate(list); + } + + public void saveInMethodReferenceBeforeThrow(java.util.List list) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + list.forEach(repository::save); + Validator.validate(list); + repository.save(list); + } + + public void saveInTryThenThrowInCatch(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + try { + repository.save(o); + } catch (RuntimeException e) { + throw new IOException(e); + } + } + + public void saveThenRethrowUnknownType(Object o, Exception e) throws Exception { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + throw e; + } + + public void saveInTryWithResources(java.nio.file.Path path, Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^ + try (java.io.BufferedReader reader = java.nio.file.Files.newBufferedReader(path)) { + repository.save(o); + } + } + + public void saveInTryWithResourcesNotThrowingOnClose(java.nio.file.Path path, java.util.List list) throws IOException { // Compliant - close() of Stream does not throw checked exceptions + try (java.util.stream.Stream lines = java.nio.file.Files.lines(path)) { + repository.save(lines); + } + } + + public void saveBeforeTryWithResources(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^ + if (o == null) { + throw new IOException(); + } + repository.save(o); + try (java.io.ByteArrayInputStream in = new java.io.ByteArrayInputStream(new byte[0])) { + in.available(); + } + } + + public void saveAfterTryWithResources(Object o) throws IOException { // Compliant - the resource is closed before the write + try (java.io.ByteArrayInputStream in = new java.io.ByteArrayInputStream(new byte[0])) { + in.available(); + } + repository.save(o); + } + + public void executeUpdateBeforeValidation(jakarta.persistence.EntityManager em, Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + em.createQuery("UPDATE Order o SET o.status = 'DONE'").executeUpdate(); + Validator.validate(o); + repository.save(o); + } + + public void batchUpdateBeforeValidation(org.springframework.jdbc.core.JdbcTemplate jdbcTemplate, Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + jdbcTemplate.batchUpdate("UPDATE orders SET status = 'DONE'"); + Validator.validate(o); + repository.save(o); + } + + public void readBeforeValidation(org.springframework.jdbc.core.JdbcTemplate jdbcTemplate, Object o) throws IOException { // Compliant - only reads before the checked exception + jdbcTemplate.queryForList("SELECT * FROM orders"); + Validator.validate(o); + jdbcTemplate.execute("UPDATE orders SET status = 'DONE'"); + } + + public void saveAfterUncheckedThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^ + if (o == null) { + throw new IllegalArgumentException(); + } + repository.save(o); + throw new IllegalStateException(); + } + } + + private final Repository repository = new Repository(); + + @Transactional + public void methodLevelThrowBeforeWrite(Object o) throws IOException { // Compliant - all checked exceptions are thrown before any write + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + + @Transactional + public void methodLevelThrowWithoutWrite(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + if (o == null) { + throw new IOException("null"); + } + o.toString(); + } + + @Transactional + public void methodLevelWriteBeforeThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + if (o == null) { + throw new IOException("null"); + } + } } 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 2af80683774..e7105026e8d 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 @@ -16,13 +16,20 @@ */ package org.sonar.java.checks.spring; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; +import java.util.Deque; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Optional; +import java.util.Set; import java.util.function.Function; +import java.util.function.Predicate; import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.java.model.ExpressionUtils; @@ -34,6 +41,8 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.Version; +import org.sonar.plugins.java.api.cfg.ControlFlowGraph; +import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AnnotationTree; import org.sonar.plugins.java.api.tree.Arguments; @@ -46,12 +55,16 @@ import org.sonar.plugins.java.api.tree.LiteralTree; import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodReferenceTree; import org.sonar.plugins.java.api.tree.MethodTree; import org.sonar.plugins.java.api.tree.NewClassTree; import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.SyntaxToken; +import org.sonar.plugins.java.api.tree.ThrowStatementTree; import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.TryStatementTree; import org.sonar.plugins.java.api.tree.TypeTree; +import org.sonar.plugins.java.api.tree.VariableTree; @Rule(key = "S8989") public class TransactionalMethodCheckedExceptionCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware { @@ -59,6 +72,20 @@ public class TransactionalMethodCheckedExceptionCheck extends IssuableSubscripti private static final List TRANSACTIONAL_PREFIXES = List.of( "save", "delete", "update", "persist", "merge", "flush", "insert"); + private static final List READ_PREFIXES = List.of( + "get", "find", "read", "exists", "count", "is", "has", "query", "select", "load", "create", "contains"); + + private static final List PERSISTENCE_TYPES = List.of( + "javax.persistence.EntityManager", + "jakarta.persistence.EntityManager", + "javax.persistence.Query", + "jakarta.persistence.Query", + "org.hibernate.SharedSessionContract", + "org.springframework.jdbc.core.JdbcOperations", + "org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations", + "org.springframework.data.repository.Repository", + "java.sql.Statement"); + @Override public List nodesToVisit() { return Collections.singletonList(Tree.Kind.METHOD); @@ -113,6 +140,10 @@ public void visitNode(Tree tree) { return; } + if (allCheckedExceptionsThrownBeforeWrites(method)) { + return; + } + var issueBuilder = QuickFixHelper.newIssue(context) .forRule(this) .onTree(method.simpleName()); @@ -291,14 +322,188 @@ private static boolean looksTransactional(MethodTree method) { if (hasTransactionalPrefix(method.simpleName().name())) { return true; } - var visitor = new MethodInvocationVisitor(); + var visitor = new MethodInvocationVisitor(TransactionalMethodCheckedExceptionCheck::hasTransactionalName, ref -> false); method.accept(visitor); return visitor.foundTransactionalCall; } + /** + * Uses the control flow graph of the method to determine whether every point that may throw a checked exception + * is executed before any write operation. In that case, no write can have happened when a checked exception is + * thrown, so the rollback behavior is irrelevant. + * Returns false when the method has no body, no write operation or no point throwing a checked exception. + */ + private static boolean allCheckedExceptionsThrownBeforeWrites(MethodTree method) { + ControlFlowGraph cfg = method.cfg(); + if (cfg == null) { + return false; + } + boolean hasWrite = false; + boolean hasThrowPoint = false; + for (ControlFlowGraph.Block block : cfg.blocks()) { + List items = blockItems(block); + for (int i = 0; i < items.size(); i++) { + hasThrowPoint |= isCheckedThrowPoint(items.get(i)); + if (isWriteOperation(items.get(i))) { + hasWrite = true; + if (items.subList(i, items.size()).stream().anyMatch(TransactionalMethodCheckedExceptionCheck::isCheckedThrowPoint) + || hasReachableThrowPoint(block) + || isInTryWithResourcesClosingWithCheckedException(items.get(i))) { + return false; + } + } + } + } + return hasWrite && hasThrowPoint; + } + + private static List blockItems(ControlFlowGraph.Block block) { + Tree terminator = block.terminator(); + if (terminator == null) { + return block.elements(); + } + List items = new ArrayList<>(block.elements()); + items.add(terminator); + return items; + } + + private static boolean hasReachableThrowPoint(ControlFlowGraph.Block start) { + Set visited = new HashSet<>(); + Deque toVisit = new ArrayDeque<>(); + addSuccessors(start, toVisit); + while (!toVisit.isEmpty()) { + ControlFlowGraph.Block block = toVisit.pop(); + if (visited.add(block)) { + if (blockItems(block).stream().anyMatch(TransactionalMethodCheckedExceptionCheck::isCheckedThrowPoint)) { + return true; + } + addSuccessors(block, toVisit); + } + } + return false; + } + + private static void addSuccessors(ControlFlowGraph.Block block, Deque toVisit) { + toVisit.addAll(block.successors()); + toVisit.addAll(block.exceptions()); + } + + private static boolean isWriteOperation(Tree tree) { + if (!(tree instanceof MethodInvocationTree) && !(tree instanceof NewClassTree)) { + return false; + } + var visitor = new MethodInvocationVisitor( + mit -> isPotentialWrite(mit.methodSymbol(), ExpressionUtils.methodName(mit).name()), + ref -> isPotentialWrite(ref.method().symbol(), ref.method().name())); + tree.accept(visitor); + return visitor.foundTransactionalCall; + } + + private static boolean hasTransactionalName(MethodInvocationTree mit) { + return hasTransactionalPrefix(ExpressionUtils.methodName(mit).name()); + } + + /** + * A method invocation is considered as a potential write when its name starts with a transactional prefix, + * or when it is invoked on a persistence API (EntityManager, JdbcTemplate, repository, ...) and its name + * does not look like a read operation. + */ + private static boolean isPotentialWrite(Symbol symbol, String name) { + if (hasTransactionalPrefix(name)) { + return true; + } + if (symbol.isUnknown()) { + return false; + } + Type ownerType = symbol.owner().type(); + return ownerType != null + && PERSISTENCE_TYPES.stream().anyMatch(ownerType::isSubtypeOf) + && !hasPrefix(name, READ_PREFIXES); + } + + /** + * The control flow graph does not model the implicit close() calls of try-with-resources, which are executed + * after the body and may throw checked exceptions. + */ + private static boolean isInTryWithResourcesClosingWithCheckedException(Tree tree) { + Tree child = tree; + Tree parent = tree.parent(); + while (parent != null && !(parent instanceof MethodTree)) { + if (parent instanceof TryStatementTree tryStatement + && (child == tryStatement.block() || child == tryStatement.resourceList()) + && tryStatement.resourceList().stream().anyMatch(TransactionalMethodCheckedExceptionCheck::closeMayThrowCheckedException)) { + return true; + } + child = parent; + parent = parent.parent(); + } + return false; + } + + /** + * A resource of a try-with-resources is closed after the body of the try. Considering it as a throw point covers + * writes executed before the try statement. + */ + private static boolean isResourceClosingWithCheckedException(Tree tree) { + Tree parent = tree.parent(); + return parent != null + && parent.parent() instanceof TryStatementTree tryStatement + && tryStatement.resourceList() == parent + && closeMayThrowCheckedException(tree); + } + + private static boolean closeMayThrowCheckedException(Tree resource) { + Type type = resource instanceof VariableTree variable ? variable.symbol().type() : ((ExpressionTree) resource).symbolType(); + return findCloseMethod(type).map(TransactionalMethodCheckedExceptionCheck::mayThrowCheckedException).orElse(true); + } + + private static Optional findCloseMethod(@Nullable Type type) { + if (type == null || type.isUnknown()) { + return Optional.empty(); + } + Symbol.TypeSymbol typeSymbol = type.symbol(); + Optional close = typeSymbol.lookupSymbols("close").stream() + .filter(Symbol::isMethodSymbol) + .map(Symbol.MethodSymbol.class::cast) + .filter(m -> m.parameterTypes().isEmpty()) + .findFirst(); + if (close.isPresent()) { + return close; + } + return Stream.concat(Stream.of(typeSymbol.superClass()), typeSymbol.interfaces().stream()) + .map(TransactionalMethodCheckedExceptionCheck::findCloseMethod) + .flatMap(Optional::stream) + .findFirst(); + } + + private static boolean isCheckedThrowPoint(Tree tree) { + if (isResourceClosingWithCheckedException(tree)) { + return true; + } + if (tree instanceof ThrowStatementTree throwStatement) { + Type type = throwStatement.expression().symbolType(); + return type.isUnknown() || isCheckedException(type); + } + if (tree instanceof MethodInvocationTree mit) { + return mayThrowCheckedException(mit.methodSymbol()); + } + if (tree instanceof NewClassTree newClass) { + return mayThrowCheckedException(newClass.methodSymbol()); + } + return false; + } + + private static boolean mayThrowCheckedException(Symbol.MethodSymbol symbol) { + return symbol.isUnknown() || symbol.thrownTypes().stream().anyMatch(type -> type.isUnknown() || isCheckedException(type)); + } + private static boolean hasTransactionalPrefix(String name) { + return hasPrefix(name, TRANSACTIONAL_PREFIXES); + } + + private static boolean hasPrefix(String name, List prefixes) { String lowerName = name.toLowerCase(Locale.ROOT); - return TRANSACTIONAL_PREFIXES.stream().anyMatch(prefix -> { + return prefixes.stream().anyMatch(prefix -> { if (!lowerName.startsWith(prefix)) { return false; } @@ -311,16 +516,29 @@ private static boolean hasTransactionalPrefix(String name) { } private static class MethodInvocationVisitor extends BaseTreeVisitor { + private final Predicate isTransactionalCall; + private final Predicate isTransactionalReference; boolean foundTransactionalCall = false; + MethodInvocationVisitor(Predicate isTransactionalCall, Predicate isTransactionalReference) { + this.isTransactionalCall = isTransactionalCall; + this.isTransactionalReference = isTransactionalReference; + } + + @Override + public void visitMethodReference(MethodReferenceTree tree) { + if (!foundTransactionalCall && isTransactionalReference.test(tree)) { + foundTransactionalCall = true; + return; + } + super.visitMethodReference(tree); + } + @Override public void visitMethodInvocation(MethodInvocationTree tree) { - if (!foundTransactionalCall) { - String calledMethodName = ExpressionUtils.methodName(tree).name(); - if (hasTransactionalPrefix(calledMethodName)) { - foundTransactionalCall = true; - return; - } + if (!foundTransactionalCall && isTransactionalCall.test(tree)) { + foundTransactionalCall = true; + return; } super.visitMethodInvocation(tree); }