[PR] [server] Add meta-test to prevent exception cause loss in catch blocks [fluss]

["XuQianJin-Stars (via GitHub)" <gi...@apache.org>]

XuQianJin-Stars opened a new pull request, #3776:
URL: https://github.com/apache/fluss/pull/3776

   ## Purpose
   
   An earlier PR — `Preserve exception cause when rethrowing catalog exceptions`
   — already fixed the most visible `catch (Exception e) { throw new XxxException(...); }`
   sites in `fluss-server` so that the original cause (`e`) is preserved in the
   exception chain. That change is great for debugging because it lets us trace
   the root cause from server logs without having to grep through stdout.
   
   However, the same pitfall — re-throwing a *new* exception without passing
   the caught one as `cause` — is easy to reintroduce in any new code path,
   because the Java compiler happily accepts a single-argument constructor
   call. Without a guard, future catalog / coordinator / RpcServiceBase
   contributions could quietly regress the diagnostic chain again.
   
   This PR adds a **regression-proof static check** for exactly that pattern
   inside the `fluss-server` module: a `RpcServiceBaseTest#metaTestNoCatchRethrowWithoutCause`
   test that walks every `.java` file under `src/main/java/org/apache/fluss/server`,
   parses `catch (X var)` / `throw new YyyException(...)` sites, and fails the
   build if any `throw new` inside a `catch` block does not reference the
   caught variable in its constructor arguments.
   
   In addition to the guard, this PR performs a one-time, full scan over
   `fluss-server/src/main/java/org/apache/fluss/server/` to confirm that no
   remaining `throw new XxxException(...)` inside a catch block is missing the
   cause — and the scan passes against the current `upstream/main`, so no
   business code is changed in this PR.
   
   Linked issue: N/A (build-time regression guard for catalog / RPC exception
   handling)
   
   ## Brief change log
   
   - **Add** new test class
     `fluss-server/src/test/java/org/apache/fluss/server/RpcServiceBaseTest.java`
     with a single but powerful meta-test:
     `RpcServiceBaseTest#metaTestNoCatchRethrowWithoutCause`.
   - The test walks `src/main/java/org/apache/fluss/server` (constant
     `SERVER_SOURCE_DIR`) via `java.nio.file.Files.walkFileTree` and a
     `SimpleFileVisitor<Path>`, processing every `.java` source file.
   - Per file, a small line-by-line state machine:
     - Maintains a stack of currently active `catch` blocks and their
       brace depth / caught variable name, populated by a
       `CATCH_PATTERN` regex that captures the variable name from any
       `catch (Type varName)` clause.
     - Tracks brace depth to know exactly when a `catch` block ends and
       must be popped from the active set.
     - For every line that matches `THROW_NEW_PATTERN`
       (`throw new XxxException(`), collects the full multi-line statement
       via `collectThrowStatement(...)` (balanced parens scanner).
     - For each active catch context, asserts that the caught variable name
       appears somewhere in the throw statement's text. If not, the
       statement is appended to a `violations` list along with the file
       path, line number, and the offending statement.
   - The test finally calls `assertThat(violations).isEmpty()`, failing the
     build with a clear, actionable message that lists every offending file
     and line.
   - **No production code change** in this PR — the scan against the current
     `upstream/main` returns zero violations, which is itself the proof
     point that the earlier cause-preservation work has held.
   
   ## Tests
   
   - `RpcServiceBaseTest#metaTestNoCatchRethrowWithoutCause` — the only
     test method in the new file. It:
     1. Verifies the source directory exists (early failure with an
        actionable error message if run from the wrong CWD).
     2. Walks the source tree, scanning every `.java` file.
     3. Asserts the `violations` list is empty.
     4. On failure, prints a full list of `file:line: catch(var) -> statement`
        entries in the assertion message so a reviewer can immediately
        navigate to the offender.
   - The test runs in `~0.3s` on a developer laptop (it is pure
     file-IO + regex; no real cluster needed), so the cost of running it
     in CI is negligible.
   - Locally verified to pass against `upstream/main`:
     `mvn test -pl fluss-server -Dtest=RpcServiceBaseTest`
     → `Tests run: 1, Failures: 0, Errors: 0, Skipped: 0`.
   
   ## API and Format
   
   - **Zero production code change.** This PR is purely a regression guard.
   - The new test follows the project's testing conventions:
     - JUnit 5 (`@Test` from `org.junit.jupiter.api`).
     - AssertJ assertions (`assertThat(...).isTrue()`, `.isEmpty()`).
     - No `@Timeout` annotation (relies on the global per-test timeout).
     - Descriptive method name (`metaTestNoCatchRethrowWithoutCause`).
     - Test class name ends in `Test` (unit, not ITCase).
   - `mvn spotless:apply` clean — imports and Javadoc follow
     [AGENTS.md §3 Code Organization](AGENTS.md).
   - Java 8 compatible — only uses `java.nio.file`, `java.util.regex`,
     `java.util.List` and lambdas, all available since Java 8.
   - Commit title `[server] Add meta-test to prevent exception cause loss
     in catch blocks` follows the `<component>` prefix convention.
   - `./mvnw clean install -DskipTests -T 32` passes locally on JDK
     `11.0.23-kona`.
   
   ## Documentation
   
   - The test class Javadoc explains:
     1. *What* the meta-test is scanning for (`catch` blocks that throw
        a new exception without preserving the cause).
     2. *Why* it matters (loss of root-cause information in exception
        chains, which hurts RPC-side debugging).
     3. *How* to fix violations ("pass the caught exception as the second
        argument to the new exception constructor").
   - No website / docs change is needed; the test is the documentation for
     this pattern.
   - A future PR that wants to extend the guard to other modules (e.g.
     `fluss-client`) can simply copy the test class and point
     `SERVER_SOURCE_DIR` at the new module — the scanner is intentionally
     module-agnostic.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@fluss.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org