yuqi1129 commented on PR #12878:
URL: https://github.com/apache/gravitino/pull/12878#issuecomment-5523700707
I reviewed this and ran a few probes against the branch. The direction is
right and the catch-all mapper is the correct JAX-RS way to do this, but the
"whole family" claim does not hold, and I think one guard is worth adding
before merge.
### The 406 the API actually produces is not covered
`VersioningFilter` calls `resp.sendError(SC_NOT_ACCEPTABLE, "Unsupported
version")` at line 160. That is a servlet filter, it runs before Jersey, so no
`ExceptionMapper` can ever see it. The same filter also rewrites the `Accept`
header at lines 123 and 126, which makes Jersey's own `NotAcceptableException`
effectively unreachable on this API.
So the PR adds a 406 branch plus a unit test for a case the server does not
produce, while the 406 users really hit is still not JSON. What I measured on
this branch:
```
Accept: application/xml -> 200 application/json
(VersioningFilter rewrote it)
Accept: application/vnd.gravitino.v99+json -> 406, no Content-Type, empty
body
POST /api/metalakes, Content-Type: text/plain -> 415 application/json
{"code":1000,...} (this one works)
```
The second line is exactly the shape #12783 complains about: a status code
and nothing else.
Two ways out, either is fine with me:
- drop the 406 branch and its unit test, and remove the 406 claim from the
description, then cover the filter layer separately under #12783; or
- make `VersioningFilter` write the JSON `ErrorResponse` itself instead of
calling `sendError`. It is about 10 lines and `Utils` already has the helpers.
What I do not want is for the description to keep saying 406 is covered.
### Please add a `hasEntity()` guard
The mapper replaces the entity unconditionally, so any
`WebApplicationException` that carries its own body loses it, and no test would
fail. Measured:
```
custom entity before = {"conflict":"my own body"}
custom entity after = ErrorResponse(code=1000, type=RESTException,
message=HTTP 409 Conflict)
```
All three exceptions Jersey raises here have no entity
(`NotAllowedException`, `NotSupportedException`, `NotAcceptableException` all
return `hasEntity() == false`), so this costs nothing today:
```java
if (exception.getResponse().hasEntity()) {
return exception.getResponse();
}
```
One line, no behavior change now, and it removes the whole class of future
silent breakage.
### The compatibility survey missed two throws
The description says the only direct throws are the two in `server/`. There
are two more in `core/`, both reachable from `/api/*`:
- `CredentialOperationDispatcher.java:102`
- `SecretPropertyOperationDispatcher.java:84`
I followed both call paths.
`MetadataObjectCredentialOperations#getCredentials` and
`MetadataObjectSecretOperations#getSecrets` both `catch (Exception e)` and go
through `ExceptionHandlers`, so nothing breaks today. But the conclusion is
right for the wrong reason, and these two show we already model business errors
as JAX-RS exceptions. The day a resource method uses a narrower catch, "this
catalog doesn't support credentials" becomes an HTTP 415 `RESTException`, which
is not what it means. Worth listing them in the description, and it is the
second reason for the guard above.
### 403 is in the table but 401 is not
`toErrorResponse` handles 400/403/405 and sends 401 to `default:` ->
`restError` (code 1000, type `RESTException`), even though
`ErrorConstants.UNAUTHORIZED_CODE` and `ErrorResponse.unauthorized(...)` both
exist. It is not reachable today, `AuthenticationFilter:115` throws our own
`UnauthorizedException` and not a `WebApplicationException`, so there is no
impact right now. But a table with 403 and no 401 is a trap. Either add the
branch or say in a comment why 401 is left out.
### Minor: the default message is just the status line
The 415 body is `{"code":1000,"type":"RESTException","message":"HTTP 415
Unsupported Media Type"}`. Compare `ParamExceptionMapper` in the same package,
which builds `Invalid value for X parameter 'y': ...`. A message equal to the
status line does not add much over the status code. Injecting `@Context
HttpHeaders` and naming the offending `Content-Type`/`Accept` would help. Not a
blocker, up to you.
### On the design
I do not think the structure should change. A catch-all on
`WebApplicationException` is the standard approach; writing one mapper per
subtype would be more code, no benefit, and it would silently miss any new
type. Keep it as is plus the guard.
Also worth saying: the "nearest type wins" claim is already proven end to
end by the IT, since `testMalformedModelVersionReturnsJsonErrorBody` asserts
the type is `PathParamException` and
`testUnknownApiRouteStillReturnsJsonErrorBody` asserts `NotFoundException`, and
neither is taken over by the new mapper. No extra precedence test needed.
### Tests worth adding
1. A 415 case in `JsonErrorHandlerIT`. I verified `POST /api/metalakes` with
`Content-Type: text/plain` returns 415 and JSON. This matters because the unit
test calls `mapper.toResponse()` directly and never serializes anything, and
415/406 are exactly the cases where the response type conflicts with what the
client asked for.
2. A test that a `WebApplicationException` carrying its own entity is passed
through, together with the guard.
3. If the 406 branch stays, a test that makes the server actually return
406. I could not construct one, which is the signal that the branch is dead
code.
### Questions
- Do you consider `VersioningFilter`'s 406 part of #12783? The issue says
"any error resolved before reaching a resource method", which reads like yes,
but the PR scopes it to what Jersey raises.
- The description marks this as a user-facing change. I only checked the
Java client, where an unknown code falls through to `RESTException`, so 405/415
are fine there. Do you know whether the Python client or the Trino connector
special-case these responses today?
--
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]
