[Cherry-pick to branch-1.3] [#12783] fix(server): return JSON errors for the whole pre-resource-method WebApplicationException family (#12878) - #12910
Merged
Conversation
…ethod WebApplicationException family (#12878) ### What changes were proposed in this pull request? Follow-up to #12784. That PR registered `ParamExceptionMapper` (typed parameter conversion failures) and `NotFoundExceptionMapper` (unmatched routes), but those only cover two specific cases. Any other error resolved before reaching a resource method was still unmapped and fell through to Jetty's default HTML error page. This PR adds two fixes: - `WebApplicationExceptionMapper`, registered on the common `javax.ws.rs.WebApplicationException` base class as a catch-all for the rest of the JAX-RS family — a wrong HTTP method (`NotAllowedException`, 405), an unsupported `Content-Type` (`NotSupportedException`, 415), etc. JAX-RS always selects the mapper for the nearest type in an exception's class hierarchy, so `ParamExceptionMapper` and `NotFoundExceptionMapper` still take precedence for the cases they already handle. It preserves the original response's headers (e.g. `Allow` on 405) via `Response.fromResponse(...)`, and intentionally never preserves any entity the original response might carry, so every error under `/api/*` stays the same `ErrorResponse` JSON shape. - `VersioningFilter` now writes the JSON `ErrorResponse` directly instead of calling `sendError` when it rejects an unsupported API version. This filter runs *before* Jersey ever sees the request, so no `ExceptionMapper` — including the one above — can ever intercept it; this was the actual 406 case users hit. (Jersey's own `NotAcceptableException` is not reachable through this API today, since `VersioningFilter` normalizes or rejects the version before Jersey runs; `WebApplicationExceptionMapper` still handles it generically as defense-in-depth.) Confirmed no existing code throws a raw `WebApplicationException` subtype expecting to reach the catch-all mapper with custom headers/entity: the two direct throws in `server/` (`NotSupportedException` in `MetadataObjectSecretOperations`/`MetadataObjectCredentialOperations`) and two more in `core/` (`CredentialOperationDispatcher`, `SecretPropertyOperationDispatcher`) are all already caught by a local `try`/`catch` before ever reaching Jersey's provider chain. Fix: #12783 ### Why are the changes needed? Stated generally (per review discussion on #12784 and this PR): any error resolved before reaching a resource method should return the same structured JSON body as the rest of the API. ### Does this PR introduce _any_ user-facing change? Yes, under `/api/*`: - A wrong HTTP method (e.g. `POST /api/version`) now returns a structured JSON `ErrorResponse` — HTTP 405, type `UnsupportedOperationException` — instead of Jetty's default HTML error page. - An unrecognized `Accept` API version (e.g. `application/vnd.gravitino.v99+json`) now returns a structured JSON `ErrorResponse` — HTTP 406, type `IllegalArgumentException` — instead of an empty body with no `Content-Type`. No other endpoint's behavior changes. ### How was this patch tested? - `TestWebApplicationExceptionMapper` (unit test) covers `NotAllowedException` (405, including the preserved `Allow` header), `NotSupportedException` (415), and `NotAcceptableException` (406). - `TestVersioningFilter` updated to assert the new JSON `ErrorResponse` body (status, content type, and parsed `ErrorResponse` fields) instead of the old `sendError` call. - `JsonErrorHandlerIT` (integration test against a real running server) covers a malformed model version, a malformed model version URI, an unmatched `/api/*` route, a wrong HTTP method, and an unsupported API version — all 5 pass. - Ran the full `server` and `server-common` unit test suites; no regressions. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> # Conflicts: # server/src/main/java/org/apache/gravitino/server/web/VersioningFilter.java
github-actions
Bot
added
cherry-pick
Adapts the JSON-error fix from deb1e2d to branch-1.3's older VersioningFilter, which predates the versionFromHeaders/isUnsupportedVersion refactor on main and still inlines version checking in doFilter. Also updates the three TestVersioningFilter assertions that still expected the old sendError call on an unsupported-version response, since branch-1.3 only supports API v1 and those call sites now hit the same JSON-error path.
jerryshao
removed
the
cherry-pick-conflict
jerryshao
changed the title
Author
Code Coverage Report
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cherry-pick Information:
branch-1.3Do not merge until conflict markers are resolved and the
cherry-pick-conflictlabel is removed.Please review and resolve the conflicts before merging.