jerryshao commented on PR #12784: URL: https://github.com/apache/gravitino/pull/12784#issuecomment-5507808547
Thanks for the thorough review and the repro steps — you were right, and I confirmed it directly. I instrumented `JsonErrorHandler.handle()` and Jetty's `ErrorPageErrorHandler` with debug logging and compared the two scenarios: - Directory-based dev `WebAppContext` (your `web/web/dist` repro): `ErrorPageErrorHandler.getErrorPage(...)` runs, `JsonErrorHandler.handle()` never does. Confirms the fix was a no-op there, exactly as you found. - A real packaged `.war` deployment: the reverse — `JsonErrorHandler` runs directly, `ErrorPageErrorHandler` never does. So the original fix "worked" there, but only because of how that specific `WebAppContext` construction path happens to resolve the error handler, not because of anything the fix controls. Either way, relying on Jetty's context-vs-server `ErrorHandler` precedence isn't something we should build on, so I went with your suggested approach instead: `ParamExceptionMapper` on `org.glassfish.jersey.server.ParamException` (thrown by Jersey itself when a typed `@PathParam`/`@QueryParam`/etc. fails to convert, using its own status per param type) and `NotFoundExceptionMapper` on `javax.ws.rs.NotFoundException` (no matching resource at all), both registered the same way as `JsonParseExceptionMapper`. Since a mapper returns a `Response` with an entity, Jersey never calls `sendError`, so no Jetty `ErrorHandler` — context or server level — is ever involved. Verified `JsonErrorHandlerIT`'s 3 cases pass with the Web UI enabled via the exact directory-based repro you described, plus manually against a real packaged distribution with the real `web`/`web-v2` WAR files. On your other points: - Malformed-parameter type is now `PathParamException` (or `QueryParamException`, etc., from `ParamException.getParameterType()`/exception class) rather than a generic `NotFoundException`, and the message includes the parameter name and underlying cause, e.g. `Invalid value for PathParam parameter 'version': For input string: "abc"`. - Went with keeping Jersey's own status per param type (404 for `@PathParam`/`@QueryParam`/`@MatrixParam`, 400 for `@HeaderParam`/`@CookieParam`/`@FormParam`) rather than normalizing everything to 400, since that's the existing JAX-RS-mandated behavior for this scenario and changing it felt like a separate discussion. - The dead 401 branch, the `internalError` catch-all, and the stray `setShowStacks` no longer apply since the whole Jetty `ErrorHandler` approach is gone along with `JsonErrorHandler`. - Iceberg (`/iceberg/*`) and Lance (`/lance/*`) are still out of scope for this PR — they run their own `JettyServer` instances and would need the equivalent mappers registered separately. Let me know if `javax.ws.rs.NotFoundException` being caught broadly is a concern — I checked and nothing in `server` currently throws it intentionally expecting different handling, but flagging it since you raised 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]
