SONARJAVA-6756: Fix FPs in S8989 for NOT_SUPPORTED and readOnly transactions - #5923
SONARJAVA-6756: Fix FPs in S8989 for NOT_SUPPORTED and readOnly transactions#5923romainbrenguier wants to merge 2 commits into
Conversation
…ctions Do not raise S8989 when @transactional specifies propagation NOT_SUPPORTED or NEVER (no transaction is created), or when readOnly = true (no writes can occur), since rollback configuration is meaningless in these cases. 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}
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Edge Case: readOnly=true suppression may hide genuine rollback gaps
readOnly = true is only a hint to the transaction manager/driver; it does not guarantee that no writes occur (e.g. native SQL, some JPA flush scenarios still write). A checked exception thrown during such a write would not trigger rollback without a rollbackFor attribute, so fully suppressing S8989 for readOnly could introduce a false negative. This matches the PR's FP-reduction intent, but consider limiting suppression or documenting the tradeoff.
Was this helpful? React with 👍 / 👎
…ckedExceptionCheck Add test samples to cover uncovered lines: - Static-imported propagation constant (IDENTIFIER case in resolveEnumConstantName) - Method call as propagation value (default case in resolveEnumConstantName) - Method with non-Transactional annotations in class-level @transactional class (exercises annotation loop iteration in getTransactionalAnnotation and isClassLevelAnnotation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsRefines S8989 to prevent false positives on NOT_SUPPORTED, NEVER, and readOnly transactions with comprehensive test coverage. Consider that readOnly=true suppression may hide genuine rollback gaps since it is only a driver hint. 💡 Edge Case: readOnly=true suppression may hide genuine rollback gaps
🤖 Prompt for agentsOptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
|
2 participants
Summary
@Transactional(propagation = Propagation.NOT_SUPPORTED)orPropagation.NEVERis used, since no transaction is created and rollback configuration is inapplicable@Transactional(readOnly = true)is used, since read-only transactions perform no write operations@Transactionaluses default propagation orreadOnly = falseFixes ~46 out of 50 FPs identified in a 1000-issue sample, reducing the FP rate from 5.0% to ~0.4%.
Test plan
propagation = NOT_SUPPORTED,propagation = NEVER,readOnly = true, and combinationsreadOnly = falseandpropagation = REQUIREDto verify they're still flaggedNOT_SUPPORTEDandreadOnlyexclusions🤖 Generated with Claude Code