[romainbrenguier]

Summary

Fixes a false positive of S8989 (methods with @Transactional should specify rollback behavior for checked exceptions): when all checked exceptions are thrown before any save is made, no write can be rolled back, so the rollback configuration is irrelevant.

The check now uses the method's control flow graph:

• Write operations: method invocations / constructor calls whose name (or a lambda argument's call) has a transactional prefix (save, delete, update, persist, merge, flush, insert).
• Checked throw points: throw statements of a checked (or unknown) type, and invocations/constructors that declare checked (or unknown) exceptions, or whose symbol is unknown.
• The issue is skipped only if the method has at least one write and at least one throw point, and no throw point is reachable from a write (including a write that itself throws, and exceptional edges to catch blocks).

Methods without a body, without detected writes, or without any detected throw point keep the previous behavior.

Tests

• Added compliant/noncompliant cases to TransactionalMethodCheckedExceptionCheckSample.java (throws before writes, writes before throws, loops, lambdas, try/catch rethrow, unchecked throws).
• Added unknown type / unknown method cases to the non-compiling sample.

🤖 Generated with Claude Code

Agent workflow

Tool link: https://github.com/SonarSource/languages-experimental-tooling/tree/romain/my-tickets/personal/romain-brenguier
PR created using uv run create_with_claude.py SONARJAVA-7243.txt

Addressed review comments in pr_report_6401.md using uv run address_reviews.py pr_report_6401.md

---

Summary by Gitar

• Persistence API writes:
  • Treats calls on EntityManager, Query, JdbcOperations and repositories as writes unless the name is a read
• Try-with-resources:
  • Treats implicit close() of resources declaring checked exceptions as throw points
• Method references:
  • Detects transactional references such as repository::save as write operations
• Unknown symbols:
  • Treats unresolved methods and unknown thrown types as checked throw points
• Tests:
  • Adds compliant and noncompliant samples for loops, lambdas, try-with-resources and persistence writes

_{This will update automatically on new commits.}

Addressed review comments in pr_report_6401.md using uv run address_reviews.py pr_report_6401.md

[hashicorp-vault-sonar-prod]

SONARJAVA-7243

[romainbrenguier]

/pvf S8989

[hashicorp-vault-sonar-prod]

📊 Performance Validation complete
🆕 0 new · 🗑️ 1 lost issues
View dashboard

[nathsou]

Approving: the CFG-based approach removes the reported false positive. A few gaps where the new suppression now hides true positives; each case below was reproduced with a CheckVerifier probe and is reported on master. The underlying asymmetry is that throw points are over-approximated (unknown methods count as throwing) while writes are under-approximated (names and a list of persistence types).

Also, the S8989 RSPEC has an explicit == Exceptions list (NOT_SUPPORTED, readOnly = true). The new suppression should probably be documented there in a companion RSPEC PR, as was done for SONARJAVA-7238 and SONARJAVA-7239.

[nathsou]

Method references are not seen as writes: the visitor only checks method invocations. This is suppressed, although the first forEach may have saved items when validate throws:

list.forEach(repository::save);
Checker.validate(list); // throws IOException
repository.save(list);

[romainbrenguier]

Fixed in a5d6027: method references whose method is a potential write (e.g. repository::save) are now considered as writes. Added a Noncompliant sample for this case.

[nathsou]

Design question: writes made through calls that don't match a prefix or a persistence type are invisible here, so this is suppressed too:

service.placeOrder(o); // writes, joins the same transaction
Checker.validate(o);   // throws IOException
repository.save(o);

Similarly, line 413 treats unresolved methods as "not a write", while mayThrowCheckedException treats them as "may throw". Would it make sense to be conservative on the write side as well (e.g. count unresolved calls and calls to project code as potential writes), or is this trade-off intended?

[romainbrenguier]

The current version risks FNs but being conservative on the write side would mean potential FPs. So this is intended that way.

[nathsou]

This only covers writes nested inside the try-with-resources. A resource closed after the write is missed when its creation does not throw:

if (o == null) {
  throw new IOException();
}
repository.save(o);
try (Res r = new Res()) { // Res.close() throws IOException
  r.hashCode();
}

Treating a resource whose close() may throw a checked exception as a throw point itself (in isCheckedThrowPoint) would cover both cases and replace this parent walk.

[romainbrenguier]

Fixed in a5d6027: a resource whose close() may throw a checked exception is now a throw point itself, which covers writes made before the try. The parent walk is kept for writes nested in the body, since the CFG places the resource declaration before the body and not after it. Added Noncompliant and Compliant samples.

[romainbrenguier]

@nathsou Regarding the RSPEC == Exceptions list: the RSPEC lives in a separate repository, so documenting this new exception will be done in a companion RSPEC PR rather than here: https://github.com/SonarSource/rspec/pull/8406

[gitar-bot]

Code Review ✅ Approved 3 closed / 3 findings

🟡 Medium risk · The CFG-based S8989 exception/write analysis could misclassify transaction paths, suppressing needed rollback warnings or producing false positives.

Improves S8989 to skip reporting when all checked exceptions are thrown before any write, using the method's control flow graph to detect writes and throw points. Resolved issues include implicit close() in try-with-resources not being counted as a throw point, false negatives for writes not matching the name prefixes, and writes in catch/finally of try-with-resources being wrongly treated as pre-close. No open issues remain.

✅ 3 closed

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

📄 java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java:316-330 📄 java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java:349-363
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.

✅ Edge Case: Writes not matching the name prefixes before a throw now cause false negatives

📄 java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java:323-336 📄 java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java:370-377
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.

✅ Edge Case: Writes in catch/finally of try-with-resources wrongly treated as pre-close

📄 java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java:424-438
isInTryWithResourcesClosingWithCheckedException walks up every ancestor of the write, and returns true as soon as it reaches a TryStatementTree with a resource whose close() may throw a checked exception. The ancestor can be reached from inside the try body, but also from a CatchTree or a finally block, since both are children of the TryStatementTree. Resources are closed before catch and finally blocks run, so a write there can never be followed by the implicit close(). The method's own javadoc says close happens "after the body".

Example: try (BufferedReader r = Files.newBufferedReader(p)) { r.readLine(); } catch (IOException e) { repository.save(log); } has no checked throw point after the write. It is still reported, so the false positive this PR targets is kept for that pattern.

Fix: only return true when the write is inside tryStatement.block() or tryStatement.resourceList(). That means tracking the child you came from while walking up.

Verification Rules 2/2 met Repos 5 inspected · PRs 1 inspected

Functional validation No results

Rules 2 passed 1 not applicable

✅ Internal artifact registries
✅ Repository license compliance

Broader impact Repos 5 inspected PRs 1 inspected

Cross-repo 5 repositories inspected 0 open impacts

SonarSource/sonarqube Inspected
SonarSource/sonarlint-intellij Inspected
SonarSource/sonarlint-vscode Inspected
SonarSource/sonar-java-symbolic-execution Inspected
SonarSource/sonar-lits Inspected

Cross-PR 1 PR inspected 0 open impacts

PR 6416: SONARJAVA-7252 Decouple the Java frontend from Eclipse JDT core Inspected

Auto-approval Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
91.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube