Skip to content

SONARJAVA-7243: Improve S8989 to ignore checked exceptions thrown before any write #6401

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

Draft
romainbrenguier wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

SONARJAVA-7243: Improve S8989 to ignore checked exceptions thrown before any write #6401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 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 @@ -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) {
}
}
}
141 changes: 141 additions & 0 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 @@ -414,4 +414,145 @@ 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<Object> 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<Object> list) throws IOException { // Noncompliant
// ^^^^^^^^^^^^^^^^^^^
for (Object o : list) {
if (o == null) {
throw new IOException("null");
}
repository.save(o);
}
}

public void saveInLambdaBeforeThrow(java.util.List<Object> list) throws IOException { // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^
list.forEach(o -> repository.save(o));
Validator.validate(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 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");
}
}
}
98 changes: 98 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 @@ -16,11 +16,15 @@
*/
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.stream.Collectors;
import org.sonar.check.Rule;
Expand All @@ -34,6 +38,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;
Expand All @@ -50,6 +56,7 @@
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.TypeTree;

Expand Down Expand Up @@ -113,6 +120,10 @@ public void visitNode(Tree tree) {
return;
}

if (allCheckedExceptionsThrownBeforeWrites(method)) {
return;
}

var issueBuilder = QuickFixHelper.newIssue(context)
.forRule(this)
.onTree(method.simpleName());
Expand Down Expand Up @@ -296,6 +307,93 @@ private static boolean looksTransactional(MethodTree method) {
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<Tree> 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)) {
Comment on lines +316 to +330

@gitar-bot gitar-bot Bot Oct 26, 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.

⚠️ Bug: Implicit close() in try-with-resources is not counted as a throw point

The CFG does not model the implicit close() calls of try-with-resources. CFG.java builds only the resource list and says so in a comment: "FIXME only path with no failure constructed for now, (not taking try with resources into consideration)". So hasReachableThrowPoint never sees the close() that runs after the body. Example: try (BufferedReader r = Files.newBufferedReader(p)) { repository.save(o); } in a method declaring throws IOException. newBufferedReader is a checked throw point before the write, and nothing checked is found after save, so the issue is now skipped. But r.close() can throw IOException after the save, and Spring will then commit the write instead of rolling it back. Fix: if a write sits inside a try-with-resources whose resource types have a close() that declares checked exceptions, treat it as having a reachable throw point, or skip the optimisation when the method contains try-with-resources. Add a Noncompliant sample for this case.

Was this helpful? React with 👍 / 👎

return false;
}
}
}
}
return hasWrite && hasThrowPoint;
Comment on lines +323 to +336

@gitar-bot gitar-bot Bot Oct 26, 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: Writes not matching the name prefixes before a throw now cause false negatives

The issue is skipped as soon as every detected write comes after every checked throw point. Writes are detected only by method-name prefix (save, delete, update, ...). Any write that doesn't match a prefix and runs before a checked throw is invisible, and the issue is wrongly suppressed. Example: em.createQuery("UPDATE ...").executeUpdate(); Validator.validate(o); /* throws IOException */ repository.save(o);. Here executeUpdate, jdbcTemplate.batchUpdate and execute don't match a prefix, save does, and validate comes before save, so the check returns true. The executeUpdate write is committed when IOException is thrown. Before this PR the method was always reported. Fix: only suppress when no method invocation of unknown write status (e.g. any non-getter call on a repository, EntityManager or JdbcTemplate receiver) runs before the last throw point, or widen the write detection used for this suppression.

Was this helpful? React with 👍 / 👎

}

private static List<Tree> blockItems(ControlFlowGraph.Block block) {
Tree terminator = block.terminator();
if (terminator == null) {
return block.elements();
}
List<Tree> items = new ArrayList<>(block.elements());
items.add(terminator);
return items;
}

private static boolean hasReachableThrowPoint(ControlFlowGraph.Block start) {
Set<ControlFlowGraph.Block> visited = new HashSet<>();
Deque<ControlFlowGraph.Block> 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<ControlFlowGraph.Block> 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();
tree.accept(visitor);
return visitor.foundTransactionalCall;
}

private static boolean isCheckedThrowPoint(Tree tree) {
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) {
String lowerName = name.toLowerCase(Locale.ROOT);
return TRANSACTIONAL_PREFIXES.stream().anyMatch(prefix -> {
Expand Down
Loading