jerryshao commented on PR #12878: URL: https://github.com/apache/gravitino/pull/12878#issuecomment-5524179656
Thanks — verified each of these against the code before acting. **406 claim / `VersioningFilter`**: confirmed the read. Fixed `VersioningFilter.isUnsupportedVersion` to write the JSON `ErrorResponse` directly instead of calling `sendError`, same pattern `IcebergAuthenticationFilter` already uses for its own pre-Jersey responses. Added a real IT test (`testUnsupportedApiVersionReturnsJsonErrorBody`, `Accept: application/vnd.gravitino.v99+json` against a running server) instead of just the unit-level mapper test. Kept `WebApplicationExceptionMapper`'s `NotAcceptableException` branch as defense-in-depth (it's still correct if Jersey ever threw one directly), but the PR description no longer claims it's what fixes the 406 users actually hit — that's the filter fix now. **`hasEntity()` guard**: went a different direction than the literal suggestion. Returning `exception.getResponse()` as-is when it has an entity would let a non-`ErrorResponse` body leak through, which undercuts the actual goal here (every error under `/api/*` is the same JSON shape). Kept the unconditional overwrite, documented it explicitly in the Javadoc as deliberate, and pointed future callers at `getMessage()` for conveying detail. No code path today sets an entity, so behavior is unchanged either way. **Two more throws in `core/`**: confirmed via grep — `CredentialOperationDispatcher.java:102` and `SecretPropertyOperationDispatcher.java:84`. Listed both in the PR description alongside the two in `server/`. All four are caught locally before reaching Jersey's provider chain, so no impact today, but noted for whoever touches those catch blocks later. **401 vs 403**: left as-is for now — not reachable today since `AuthenticationFilter` throws its own exception type, not a JAX-RS one. Happy to add the branch if you'd rather have it explicit than implicit. **Default message quality**: left as-is; agreed it's not blocking. **Design / precedence**: agreed, kept the catch-all structure, no extra precedence test added since the existing IT already proves it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
