lasdf1234 commented on code in PR #14556:
URL: https://github.com/apache/gravitino/pull/14556#discussion_r4454002480
##########
lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java:
##########
@@ -126,6 +188,179 @@ public void testProtocolValidationDetailsArePreserved() {
}
}
+ @ParameterizedTest
+ @ValueSource(ints = {400, 401, 403, 404, 405, 406, 409, 415, 429})
+ void testFrameworkClientStatusAndHeadersArePreserved(int status) {
+ try (Response original =
+ Response.status(status)
+ .header("Allow", "GET")
+ .header("WWW-Authenticate", "Basic realm=\"lance\"")
+ .header("Retry-After", "3")
+ .entity("framework entity")
+ .build();
+ Response response =
+ LanceExceptionMapper.toRESTResponse(
+ "table", new WebApplicationException("request rejected",
original))) {
+ Assertions.assertEquals(status, response.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
response.getMediaType());
+ ErrorResponse error = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals("request rejected", error.getError());
+ Assertions.assertEquals("table", error.getInstance());
+ Assertions.assertEquals("", error.getDetail());
+ Assertions.assertNotEquals(new InternalException("", "", "").getCode(),
error.getCode());
+ Assertions.assertEquals("GET", response.getHeaderString("Allow"));
+ Assertions.assertEquals(
+ "Basic realm=\"lance\"",
response.getHeaderString("WWW-Authenticate"));
+ Assertions.assertEquals("3", response.getHeaderString("Retry-After"));
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "Content-Length", "Content-Encoding", "Content-Language",
"Content-Location",
+ "Content-Range", "Content-Disposition", "Transfer-Encoding", "ETag",
+ "Last-Modified", "Content-MD5", "Digest"
+ })
+ void testFrameworkEntityHeadersAreRemoved(String header) {
+ try (Response original =
+ Response.status(415).header(header,
"original-entity-metadata").entity("old").build();
+ Response response =
+ LanceExceptionMapper.toRESTResponse(
+ "table", new WebApplicationException("request rejected",
original))) {
+ Assertions.assertNull(response.getHeaderString(header));
+ Assertions.assertEquals(415, response.getStatus());
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
response.getMediaType());
+ Assertions.assertEquals(
+ "request rejected", ((ErrorResponse)
response.getEntity()).getError());
+ }
+ }
+
+ @Test
+ void testMappedFrameworkEntityCanBeReadOverHttp() {
+ try (Response response = target("error/framework").request().get()) {
+ Assertions.assertEquals(415, response.getStatus());
+ Assertions.assertNull(response.getHeaderString("Content-Encoding"));
+ Assertions.assertEquals("3", response.getHeaderString("Retry-After"));
+ Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE,
response.getMediaType());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals("request rejected", error.getError());
+ Assertions.assertEquals("table", error.getInstance());
+ }
+ }
+
+ @Test
+ void testFrameworkServerErrorDoesNotExposeDetails() {
+ try (Response response =
+ LanceExceptionMapper.toRESTResponse(
+ "table", new WebApplicationException("private-backend-detail",
503))) {
+ Assertions.assertEquals(503, response.getStatus());
+ ErrorResponse error = (ErrorResponse) response.getEntity();
+ Assertions.assertEquals("Internal server error", error.getError());
+ Assertions.assertEquals("", error.getDetail());
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(
+ strings = {
+ "missing",
+ "v1/table/catalog$schema$table/missing",
+ "v1/namespace/catalog$schema/missing"
+ })
+ void testUnknownRouteUsesInvalidInputEnvelope(String path) {
+ try (Response response =
target(path).request(MediaType.APPLICATION_JSON_TYPE).get()) {
+ Assertions.assertEquals(404, response.getStatus());
+ ErrorResponse error = response.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorCode.INVALID_INPUT.getCode(),
error.getCode());
Review Comment:
**P1** — This test locks in unknown routes as `INVALID_INPUT` + HTTP 404.
That conflicts with the Lance Namespace expectation that unsupported operations
surface as `Unsupported` (code 0 / 406) for connector fallback.
If the product decision is “Jersey NotFound ≡ invalid input”, please
document why #14204-style fallback is out of scope; otherwise prefer asserting
`Unsupported` / 406 for unimplemented Namespace methods.
##########
lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java:
##########
@@ -95,6 +102,53 @@ public Response toResponse(Throwable ex) {
return toRESTResponse("", ex);
}
+ private static Response handleWebApplicationException(
+ String instance, WebApplicationException exception) {
+ int status = exception.getResponse().getStatus();
+ String message = exception.getMessage();
+ if (exception instanceof ParamException parameterException) {
+ // Jersey can use 404 for parameter conversion failures, but these are
invalid input.
+ status = Response.Status.BAD_REQUEST.getStatusCode();
+ message = "Invalid value for parameter '" +
parameterException.getParameterName() + "'";
+ }
+ LanceNamespaceException error;
+ // Framework 404s describe route misses, not an identified missing table
or namespace.
+ // Keep their HTTP status, while using INVALID_INPUT instead of an
entity-not-found body code.
+ if (status >= 400 && status < 500) {
+ error =
+ switch (status) {
+ case 401 -> new UnauthenticatedException(message, "", instance);
+ case 403 -> new PermissionDeniedException(message, "", instance);
+ case 404 -> new InvalidInputException(message, "", instance);
+ case 405, 406, 415 -> new
org.lance.namespace.errors.UnsupportedOperationException(
+ message, "", instance);
+ case 409 -> new ConcurrentModificationException(message, "",
instance);
+ case 429 -> new ThrottlingException(message, "", instance);
+ default -> new InvalidInputException(message, "", instance);
+ };
+ } else {
+ LOG.warn("Lance REST framework failure:", exception);
+ error = new InternalException("Internal server error", "", instance);
+ }
+ // Keep protocol headers such as Allow, WWW-Authenticate and Retry-After,
while replacing any
+ // framework entity with the Lance error envelope. Never expose a
framework stack or 5xx detail.
+ Response.ResponseBuilder response =
Response.fromResponse(exception.getResponse());
+ // The old entity's framing, representation metadata and validators do not
describe our JSON.
+ for (String header :
+ new String[] {
+ "Content-Length", "Content-Encoding", "Content-Language",
"Content-Location",
+ "Content-Range", "Content-Disposition", "Transfer-Encoding", "ETag",
+ "Last-Modified", "Content-MD5", "Digest"
+ }) {
+ response.header(header, null);
+ }
+ return response
+ .status(status)
Review Comment:
**P3** — HTTP status from the framework response is preserved (e.g. 404)
while the body code becomes `INVALID_INPUT`. Backend `IllegalArgumentException`
uses the same body code with HTTP **400** (`toHttpStatus`).
Clients that branch on status (not only `code`) see inconsistent behavior
for the same error class. Worth aligning status with body code, or documenting
the split deliberately.
--
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]
