yuqi1129 commented on PR #12784:
URL: https://github.com/apache/gravitino/pull/12784#issuecomment-5492939892
I checked this PR and also ran a test myself. The problem it fixes is real,
but the fix does not work when the Web UI is enabled.
### The fix is skipped when the Web UI WAR exists
`JettyServer.initializeWebAppServletContextHandler()` creates the context
with `new WebAppContext()`. In Jetty 9.4.58 this constructor already installs
its own `ErrorPageErrorHandler`:
```
public org.eclipse.jetty.webapp.WebAppContext();
6: new #21 // class org/eclipse/jetty/servlet/ErrorPageErrorHandler
```
And `ErrorHandler.getErrorHandler(server, context)` takes the context
handler first, it only falls back to `server.getBean(ErrorHandler.class)` when
the context has none. `server.addBean(errorHandler)` is exactly that fallback,
and the `/api/*` servlet is registered on the same context. So once the WAR is
there, `JsonErrorHandler` is never called.
How to reproduce on this branch:
```shell
# the path GRAVITINO_WAR points to in embedded mode, see build.gradle.kts
mkdir -p web/web/dist && echo '<html></html>' > web/web/dist/index.html
./gradlew :clients:client-java:cleanTest :clients:client-java:test --tests
'*JsonErrorHandlerIT*'
```
All 3 tests in `JsonErrorHandlerIT` fail with `Content-Type:
text/html;charset=iso-8859-1`. Without `web/web/dist`, all 3 pass.
So:
- In a real deployment the WAR is shipped and `GRAVITINO_HOME` is set, so
users still get the HTML page. The fix is a no-op there.
- `-PtestMode=deploy` sets `GRAVITINO_WAR` to the packaged war, so the new
IT will fail in that CI job.
- In embedded mode the result depends on whether the web UI was built
before, so the new IT is flaky.
`TestJsonErrorHandler` mocks `HttpServletResponse`, so it never goes through
the real Jetty dispatch and cannot catch this.
### Suggestion: do it in a Jersey ExceptionMapper instead
We already have this pattern in `server/web/mapper/`
(`JsonParseExceptionMapper` and friends, registered in `GravitinoServer`). I
tried it locally: revert the Jetty part and register two mappers,
```java
@Priority(1) class ParamExceptionMapper implements
ExceptionMapper<org.glassfish.jersey.server.ParamException>
@Priority(1) class NotFoundExceptionMapper implements
ExceptionMapper<javax.ws.rs.NotFoundException>
```
then the 3 tests of `JsonErrorHandlerIT` pass **with** `web/web/dist`
present. The reason is that a mapper returns a response with an entity, so
Jersey does not call `sendError` at all and no Jetty error handler is involved,
no matter which context type we use. It also needs no new extension point on
`JettyServer`, and it does not touch anything outside `/api/*`.
Two things to decide if you go this way: whether mapping
`javax.ws.rs.NotFoundException` is too wide (need to check no resource code
throws it on purpose), and whether a malformed typed parameter should be 400
instead of Jersey's 404.
### Minor
1. The `default` branch of `JsonErrorHandler.toErrorResponse` returns
`internalError` (code 1002, `RuntimeException`) for every other status. A
413/429/503 under `/api/*` will then have a body saying internal error while
the status code says something else. `restError` looks better here.
2. The 401 branch looks like dead code, `AuthenticationFilter` already
writes a JSON 401 itself. If it is really hit, it makes up a
`NotAuthorizedException` type, which is not a Gravitino error type.
3. `JettyServer.initialize` calls `errorHandler.setShowStacks(true)` right
after `createErrorHandler()`. It does nothing for `JsonErrorHandler` but reads
like it configures it.
4. Iceberg REST (`/iceberg/*`) and Lance (`/lance/*`) run their own
`JettyServer` and still return HTML. The issue says "any other typed path
parameter on the metadata API", so it's better to say in the PR description
that they are out of scope.
5. The anonymous `JettyServer` subclass in the `GravitinoServer` constructor
is a bit different from how we do it elsewhere, e.g. `LanceJettyServer`.
--
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]
