SONARJAVA-6465 : Rule S8714 - fix quickfix indexoutofbound / NPE exception

[rombirli]

Despite AssertJ doc pretends fail always has a string argument message, it is not the case in practice : causing a index out of bound exception

---

Summary by Gitar

• Fixed bugs:
  • Resolved IndexOutOfBoundsException in AssertThrowsInsteadOfTryCatchFailCheck by handling fail() calls without arguments.
  • Added safeguard against null method owners during fail() invocation analysis.
  • Corrected Throwable to Throwable.class in expected type resolution.
• Refactored logic:
  • Rebuilt JavaTextEdit generation to handle diverse TryStatementTree configurations, including try-with-resources without catch or finally.
• Dependencies:
  • Updated assertj-core to 3.27.7 and updated test samples accordingly.

_{This will update automatically on new commits.}

[hashicorp-vault-sonar-prod]

SONARJAVA-6465

[rombirli]

I wasn't able to add a unit test for this fix :

• assertj fail without argument doesn't exist
• adding a nonCompilingTest sample doesn't work as it doesnt have semantic and the junit test annotation is not resolved (so no issue raised in the test)

[tomasz-tylenda-sonarsource]

I'll wait for the fix for NPE.

[tomasz-tylenda-sonarsource]

This is still attached to the old ticket, right>

[tomasz-tylenda-sonarsource]

I just realized that NPE is possible here. Let's guard against if just in case.

          var failOwner = failMethodInvocation.methodSymbol().owner();
          if (failOwner == null) {
            return;
          }
          var isAssertJ = failOwner.(...)

[rombirli]

It should not be possible as this point is not reached if owner is null (will not match any fail method in UnitTestUtils), but lets add this guard anyway for safety

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

[tomasz-tylenda-sonarsource]

LGTM

[gitar-bot]

Code Review ✅ Approved 5 resolved / 5 findings

Resolves IndexOutOfBoundsException in the quickfix for Rule S8714 by adding safeguards for null method owners and handling parameterless fail() calls. Refactors try-with-resources generation to ensure valid output and updates AssertJ dependencies.

✅ 5 resolved

✅ Quality: Unused Optional import and wildcard import added

📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:27 📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:31
import java.util.Optional; was added but Optional is not used anywhere in the file. Additionally the explicit org.sonar.plugins.java.api.tree.* imports were collapsed into a wildcard import import org.sonar.plugins.java.api.tree.*;, which conflicts with the codebase convention of explicit single-type imports (and is typically flagged by the project's own rules). Remove the unused Optional import and prefer explicit imports for the tree types actually used.

✅ Bug: Quickfix emits 'Throwable' instead of 'Throwable.class'

📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:188-191 📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java:118 📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:125-133 📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java:113-120 📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:120-124
With the new no-catch try path (e.g. try-with-resources), firstCaughtTypeInTry returns null, so typeClass(null) returns the bare string "Throwable". The quickfix then produces assertThrows(Throwable, () -> ...), which does not compile because assertThrows expects a Class<T> (Throwable.class). The new qf3 test expectation (assertThrows(Throwable, () -> ) encodes this broken output — quickfix tests only compare text and never compile the result, so the invalid code slips through.

Fix: make typeClass return "Throwable.class" for the null case so the generated code compiles.

✅ Bug: Try-with-resources quickfix generates non-compiling Java

📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:130-132 📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:135 📄 java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java:193-196 📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java:112-120
The new branch added for try-with-resources without catch/finally (else branch, lines 130-132) avoids the previous NPE, but the resulting quickfix still produces invalid Java in two ways, both visible in the new qf3 expected output (test lines 117-120):

1. The quickfix only replaces the try keyword (line 135, replaceTree(tryStatement.tryKeyword(), ...)), leaving the resource specification in place. For try (java.io.StringReader _ = new java.io.StringReader("data")) { the fix emits assertThrows(Throwable, () -> (java.io.StringReader _ = new java.io.StringReader("data")) {, which does not compile — the resource declaration is now stranded inside the lambda.

2. typeClass(null) returns the bare string "Throwable" (line 194) instead of "Throwable.class", so the generated assertThrows(Throwable, () -> ...) references a type, not a class literal — also a compile error.

Because the try-with-resources path has no catch, firstCaughtTypeInTry returns null and both defects fire together. The qf3 test asserts this exact broken output (assertThrows(Throwable, () -> and the resource left untouched), so the test passes while the quickfix it validates is uncompilable. Consider either suppressing the quickfix for try-with-resources (it cannot be soundly transformed by keyword replacement) or correctly rewriting the resource block, and fixing typeClass to emit Throwable.class.

✅ Quality: New assertJ fail() test does not verify the quickfix output

📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java:122-128
The new test for org.assertj.core.api.Fail.fail() with no argument (lines 123-128) only asserts that the issue is raised (// Noncompliant); it has no [[quickfixes=...]] annotation and no fix@/edit@ expectations. Since this PR's core purpose is to fix the quickfix IndexOutOfBoundsException that occurred precisely on the no-argument fail() path, the regression test should also assert the resulting quickfix edits so a future regression in the quickfix output (not just the crash) would be caught. Consider adding a [[quickfixes=qfX]] block verifying the produced assertThatCode(() -> ...).isInstanceOf(...) replacement.

✅ Quality: Misleading comment: test uses fail with argument, not without

📄 java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java:133-139
The new catch-block test case (lines 133-139) is prefaced with the comment // assertJ fail without argument, available since assertJ 3.26.0, which appears copy-pasted from the try-block case at line 125. However, the test actually calls org.assertj.core.api.Fail.fail("failed") — i.e. fail with a String argument. The comment therefore mislabels the scenario being tested. Since the whole point of this PR is to fix an IndexOutOfBounds/NPE when fail() is called without arguments, consider either (a) correcting the comment to reflect that this is the catch-block case with an argument, or (b) changing the call to org.assertj.core.api.Fail.fail() to genuinely cover the no-argument case in a catch block (the no-argument scenario is currently only covered in the try block at lines 126-128).

Options

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

Comment with these commands to change:

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

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