SONARJAVA-6892: S8989 Fix FP for class-level @Transactional when no transaction occurs - #6076
SONARJAVA-6892: S8989 Fix FP for class-level @Transactional when no transaction occurs#6076romainbrenguier wants to merge 2 commits into
Conversation
…ansaction occurs When @transactional is at the class level, only raise when the method likely performs a transactional operation: either the method name or a called method name starts with save, delete, update, persist, merge, or flush. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| public void noConfig() throws IOException { // Compliant - method name does not suggest a transactional operation | ||
| } | ||
|
|
||
| public void saveOrder() throws IOException { // Noncompliant [[secondary=108]] |
There was a problem hiding this comment.
⚠️ Bug: New/moved Noncompliant markers carry stale secondary= line numbers
secondary=NNN without a +/- sign is an absolute source line and is strictly asserted (Expectations.relativeValueToInt -> InternalCheckVerifier.validateSecondaryLocations, which fails on any expected line not reported). The markers added or shifted by this PR are mutually inconsistent: lines 113-133 use secondary=108, which is exactly the class-level @Transactional line (108), while the other new/relocated ones keep pre-shift values that point at unrelated lines — line 174 says 155 but its annotation is at 169, line 190 says 169 but the interface annotation is at 186, line 238 says 207 (annotation at 224), line 312 says 290 (annotation at 304), and lines 319/332/340 say 297 (annotation at 317). At least one of these two groups cannot match, so TransactionalMethodCheckedExceptionCheckTest.test will fail on the secondary-location assertion; recompute every touched secondary= against the line of the reporting @Transactional annotation (or switch them to relative -N form so they survive future line shifts).
Point each new marker at the absolute line of the class-level @transactional annotation that is reported as the secondary location.:
public void saveNestedEntity() throws IOException { // Noncompliant [[secondary=169]]
void saveEntity() throws IOException; // Noncompliant [[secondary=186]]
public void savePublicInClassLevel() throws IOException { // Noncompliant [[secondary=224]]
public void deleteWithAnnotations() throws IOException { // Noncompliant [[secondary=304]]
public void processOrder(Object repository) throws IOException { // Noncompliant [[secondary=317]]
public void delegateToFlush() throws IOException { // Noncompliant [[secondary=317]]
public void delegateToDelete(Object entity) throws IOException { // Noncompliant [[secondary=317]]
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| @Rule(key = "S8989") | ||
| public class TransactionalMethodCheckedExceptionCheck extends IssuableSubscriptionVisitor implements DependencyVersionAware { | ||
|
|
||
| private static final List<String> TRANSACTIONAL_PREFIXES = List.of("save", "delete", "update", "persist", "merge", "flush"); |
There was a problem hiding this comment.
💡 Edge Case: Prefix match has no word boundary, so unrelated names match
hasTransactionalPrefix lowercases the whole identifier and tests raw startsWith, so purely non-persistence names such as updateUiLabel, deleteTempFile, mergeSortResults, flushOutputStream, or a call to savedFilterCount() are classified as transactional and still raise, while the equally common persistence verbs (insert, create, remove, store, executeUpdate) never match. Requiring the prefix to be followed by an upper-case character or end-of-name (e.g. save, saveOrder but not savedSearch) and adding the missing verbs would make the heuristic match the intent described in the PR.
Was this helpful? React with 👍 / 👎
Code Review
|
| Auto-apply | Compact | Unblock |
|
|
|
Was this helpful? React with 👍 / 👎 | Gitar
…nsactional prefix matching Fix all secondary= line numbers in test markers to point at the actual @transactional annotation lines. Add word-boundary check to prefix matching so names like "savedSearch", "persisted", and "deletion" are not incorrectly classified as transactional. Add missing persistence verbs: insert, create, remove, store, execute. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
1 participant
Summary
@Transactionalis at the class level, only raise S8989 when the method likely performs a transactional operationsave,delete,update,persist,merge, orflush@Transactionalare unaffected and always raise as beforeTest plan
@Transactionalstill pass@Transactionalwith transactional method names (e.g.,saveOrder,deleteRecord) raises issues@Transactionalwith non-transactional method names (e.g.,noConfig,readData) no longer raises@Transactionalwith transactional method calls (e.g., callingsaveOrder()inside body) raises issuestest_without_semanticandtest_noDependenciestests pass🤖 Generated with Claude Code