This is an automated email from the ASF dual-hosted git repository.
jerryshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new e4d076acbf [#12783] fix(server): move malformed path-param handling to
a Jersey ExceptionMapper (#12784)
e4d076acbf is described below
commit e4d076acbfa0f66427563f9e782cdb1370a70edd
Author: Jerry Shao <[email protected]>
AuthorDate: Thu Sep 3 16:11:52 2026 +0800
[#12783] fix(server): move malformed path-param handling to a Jersey
ExceptionMapper (#12784)
### What changes were proposed in this pull request?
`{version}` on the model version endpoints (and any other typed
`@PathParam` on
the metadata API) is matched and converted by Jersey before any resource
method runs. When a non-numeric value is supplied, that conversion fails
and
Jersey's `ServletContainer` calls `HttpServletResponse#sendError`
directly,
which falls through to Jetty's default `ErrorHandler` — producing an
HTML
error page instead of a Gravitino `ErrorResponse`.
This PR registers two JAX-RS `ExceptionMapper`s, following the existing
`JsonParseExceptionMapper` pattern:
- `ParamExceptionMapper` maps
`org.glassfish.jersey.server.ParamException`,
which Jersey itself throws when a typed `@PathParam`/`@QueryParam`/etc.
fails to convert. It keeps Jersey's own status per parameter type (404
for
`@PathParam`/`@QueryParam`/`@MatrixParam`, 400 for
`@HeaderParam`/`@CookieParam`/`@FormParam`) and reports the specific
exception type (e.g. `PathParamException`) plus the parameter name and
underlying cause in the message.
- `NotFoundExceptionMapper` maps `javax.ws.rs.NotFoundException`, thrown
when
no resource method matches the request URI at all.
An earlier version of this PR instead installed a Jetty-level
`ErrorHandler`
as a server-wide fallback. Review found that `WebAppContext` (used
whenever
the Web UI is enabled) installs its own `ErrorPageErrorHandler` at the
context level, which Jetty always resolves before the server-level
fallback —
making that approach a no-op in real deployments with the Web UI
enabled. The
`ExceptionMapper` approach avoids this entirely: since a mapper returns
a
`Response` with an entity, Jersey never calls `sendError`, so no Jetty
`ErrorHandler` of either kind is ever involved, regardless of how the
Web
UI's `WebAppContext` was constructed.
Iceberg (`/iceberg/*`) and Lance (`/lance/*`) run their own
`JettyServer`
instances and are out of scope for this PR.
### Why are the changes needed?
Every other error on the metadata API returns a structured body carrying
a
Gravitino error code and type. A caller that receives HTML on one input
shape
and JSON on every other has to special-case the parser, and the response
identifies the servlet container and its version to anyone who sends a
bad
value.
Fix: #12783
### Does this PR introduce _any_ user-facing change?
Yes. A malformed typed path parameter under `/api/*` (e.g.
`GET
/api/metalakes/{ml}/catalogs/{c}/schemas/{s}/models/{m}/versions/abc`)
now returns a structured JSON `ErrorResponse` — HTTP 404, type
`PathParamException`, with a message naming the parameter and the
underlying
conversion error — instead of Jetty's default HTML error page. An
unmatched `/api/*` route returns type `NotFoundException`. No other
endpoint's behavior changes.
### How was this patch tested?
- Added `TestParamExceptionMapper` and `TestNotFoundExceptionMapper`
(unit
tests) covering the exception-to-`ErrorResponse` mapping, including the
404-vs-400 split across parameter annotation types.
- `JsonErrorHandlerIT` (integration test against a real running server)
covers a malformed model version, a malformed model version URI, and an
unmatched `/api/*` route, asserting a JSON `Content-Type` and a
parseable
`ErrorResponse` body. Verified it passes both with the Web UI disabled
and
with it enabled (a directory-based dev `WebAppContext`, and manually
against a real packaged distribution with the `web`/`web-v2` WAR files)
—
the scenario that broke the earlier Jetty-`ErrorHandler`-based approach.
- Ran the full `server` and `server-common` unit test suites; no
regressions.
---------
Co-authored-by: Claude Sonnet 5 <[email protected]>
---
.../integration/test/JsonErrorHandlerIT.java | 97 ++++++++++++++++++++++
.../apache/gravitino/server/GravitinoServer.java | 4 +
.../server/web/mapper/NotFoundExceptionMapper.java | 44 ++++++++++
.../server/web/mapper/ParamExceptionMapper.java | 58 +++++++++++++
.../web/mapper/TestNotFoundExceptionMapper.java | 51 ++++++++++++
.../web/mapper/TestParamExceptionMapper.java | 71 ++++++++++++++++
6 files changed, 325 insertions(+)
diff --git
a/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/JsonErrorHandlerIT.java
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/JsonErrorHandlerIT.java
new file mode 100644
index 0000000000..797f9eda3c
--- /dev/null
+++
b/clients/client-java/src/test/java/org/apache/gravitino/client/integration/test/JsonErrorHandlerIT.java
@@ -0,0 +1,97 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.client.integration.test;
+
+import java.net.URI;
+import java.net.http.HttpClient;
+import java.net.http.HttpRequest;
+import java.net.http.HttpResponse;
+import org.apache.gravitino.dto.responses.ErrorConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.apache.gravitino.integration.test.util.BaseIT;
+import org.apache.gravitino.server.web.ObjectMapperProvider;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Integration test verifying that a malformed typed path parameter on the
metadata API (e.g. a
+ * non-numeric model version) returns the same structured JSON {@link
ErrorResponse} used by every
+ * other error on the API, instead of Jetty's default HTML error page.
+ *
+ * <p>Jersey fails to convert the path segment into the resource method's
typed {@code @PathParam}
+ * before any resource method runs, so the metalake/catalog/schema/model in
the URL need not
+ * actually exist for this failure to occur.
+ */
+public class JsonErrorHandlerIT extends BaseIT {
+
+ private final HttpClient httpClient = HttpClient.newHttpClient();
+
+ @Test
+ public void testMalformedModelVersionReturnsJsonErrorBody() throws Exception
{
+ HttpResponse<String> response =
+
sendGet("/api/metalakes/m/catalogs/c/schemas/s/models/mo/versions/abc");
+
+ Assertions.assertEquals(404, response.statusCode());
+ assertJsonNotFoundBody(response, "PathParamException");
+ }
+
+ @Test
+ public void testMalformedModelVersionUriReturnsJsonErrorBody() throws
Exception {
+ HttpResponse<String> response =
+
sendGet("/api/metalakes/m/catalogs/c/schemas/s/models/mo/versions/abc/uri");
+
+ Assertions.assertEquals(404, response.statusCode());
+ assertJsonNotFoundBody(response, "PathParamException");
+ }
+
+ @Test
+ public void testUnknownApiRouteStillReturnsJsonErrorBody() throws Exception {
+ // A route that Jersey cannot match at all is a different failure (no
@PathParam conversion
+ // is even attempted), but it must be covered by the same fix.
+ HttpResponse<String> response = sendGet("/api/v99/nonexistent/route");
+
+ Assertions.assertEquals(404, response.statusCode());
+ assertJsonNotFoundBody(response, "NotFoundException");
+ }
+
+ private HttpResponse<String> sendGet(String path) throws Exception {
+ HttpRequest request =
+ HttpRequest.newBuilder()
+ .uri(new URI("http://localhost:"; + getGravitinoServerPort() +
path))
+ .GET()
+ .build();
+ return httpClient.send(request, HttpResponse.BodyHandlers.ofString());
+ }
+
+ private void assertJsonNotFoundBody(HttpResponse<String> response, String
expectedType)
+ throws Exception {
+ String contentType =
response.headers().firstValue("Content-Type").orElse("");
+ Assertions.assertTrue(
+ contentType.contains("application/json"),
+ "Expected a JSON error body, got Content-Type: " + contentType);
+ Assertions.assertFalse(
+ response.body().contains("<html"), "Response body must not be Jetty's
HTML error page");
+
+ ErrorResponse errorResponse =
+ ObjectMapperProvider.objectMapper().readValue(response.body(),
ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResponse.getCode());
+ Assertions.assertEquals(expectedType, errorResponse.getType());
+ Assertions.assertFalse(errorResponse.getMessage().isEmpty());
+ }
+}
diff --git
a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
index b12308c228..1450124335 100644
--- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
+++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java
@@ -64,6 +64,8 @@ import
org.apache.gravitino.server.web.filter.GravitinoInterceptionService;
import org.apache.gravitino.server.web.mapper.JsonMappingExceptionMapper;
import org.apache.gravitino.server.web.mapper.JsonParseExceptionMapper;
import org.apache.gravitino.server.web.mapper.JsonProcessingExceptionMapper;
+import org.apache.gravitino.server.web.mapper.NotFoundExceptionMapper;
+import org.apache.gravitino.server.web.mapper.ParamExceptionMapper;
import org.apache.gravitino.server.web.ui.WebUIFilter;
import org.apache.gravitino.stats.StatisticDispatcher;
import org.apache.gravitino.tag.TagDispatcher;
@@ -170,6 +172,8 @@ public class GravitinoServer extends ResourceConfig {
register(JsonProcessingExceptionMapper.class);
register(JsonParseExceptionMapper.class);
register(JsonMappingExceptionMapper.class);
+ register(ParamExceptionMapper.class);
+ register(NotFoundExceptionMapper.class);
register(ObjectMapperProvider.class).register(JacksonFeature.class);
property(CommonProperties.JSON_JACKSON_DISABLED_MODULES,
"DefaultScalaModule");
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/mapper/NotFoundExceptionMapper.java
b/server/src/main/java/org/apache/gravitino/server/web/mapper/NotFoundExceptionMapper.java
new file mode 100644
index 0000000000..f3630619e1
--- /dev/null
+++
b/server/src/main/java/org/apache/gravitino/server/web/mapper/NotFoundExceptionMapper.java
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.server.web.mapper;
+
+import javax.annotation.Priority;
+import javax.ws.rs.NotFoundException;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.server.web.Utils;
+
+/**
+ * NotFoundExceptionMapper returns a structured JSON error body when Jersey
cannot match a request
+ * URI to any resource method at all, instead of letting the servlet container
fall back to Jetty's
+ * default HTML error page.
+ */
+@Priority(1)
+public class NotFoundExceptionMapper implements
ExceptionMapper<NotFoundException> {
+
+ @Override
+ public Response toResponse(NotFoundException exception) {
+ String message =
+ StringUtils.isBlank(exception.getMessage())
+ ? "Requested resource not found"
+ : exception.getMessage();
+ return Utils.notFound(NotFoundException.class.getSimpleName(), message);
+ }
+}
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/mapper/ParamExceptionMapper.java
b/server/src/main/java/org/apache/gravitino/server/web/mapper/ParamExceptionMapper.java
new file mode 100644
index 0000000000..80c578b1e2
--- /dev/null
+++
b/server/src/main/java/org/apache/gravitino/server/web/mapper/ParamExceptionMapper.java
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.server.web.mapper;
+
+import javax.annotation.Nullable;
+import javax.annotation.Priority;
+import javax.ws.rs.core.Response;
+import javax.ws.rs.ext.ExceptionMapper;
+import org.apache.gravitino.server.web.Utils;
+import org.glassfish.jersey.server.ParamException;
+
+/**
+ * ParamExceptionMapper returns a structured JSON error body when Jersey fails
to convert a request
+ * parameter (e.g. a typed {@code @PathParam}) into its declared Java type,
instead of letting the
+ * servlet container fall back to Jetty's default HTML error page.
+ *
+ * <p>{@link ParamException} is thrown by Jersey itself while binding request
parameters, before any
+ * resource method runs, so this mapper is the only place such a conversion
failure can be
+ * intercepted.
+ */
+@Priority(1)
+public class ParamExceptionMapper implements ExceptionMapper<ParamException> {
+
+ @Override
+ public Response toResponse(ParamException exception) {
+ String message =
+ String.format(
+ "Invalid value for %s parameter '%s'%s",
+ exception.getParameterType().getSimpleName(),
+ exception.getParameterName(),
+ causeMessage(exception.getCause()));
+
+ if (exception.getResponse().getStatus() ==
Response.Status.NOT_FOUND.getStatusCode()) {
+ return Utils.notFound(exception.getClass().getSimpleName(), message);
+ }
+ return Utils.illegalArguments(message);
+ }
+
+ private static String causeMessage(@Nullable Throwable cause) {
+ return cause == null || cause.getMessage() == null ? "" : ": " +
cause.getMessage();
+ }
+}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/mapper/TestNotFoundExceptionMapper.java
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestNotFoundExceptionMapper.java
new file mode 100644
index 0000000000..f8fd02f16f
--- /dev/null
+++
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestNotFoundExceptionMapper.java
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.server.web.mapper;
+
+import javax.ws.rs.NotFoundException;
+import javax.ws.rs.core.Response;
+import org.apache.gravitino.dto.responses.ErrorConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestNotFoundExceptionMapper {
+
+ private final NotFoundExceptionMapper mapper = new NotFoundExceptionMapper();
+
+ @Test
+ public void testNotFoundExceptionWithoutMessage() {
+ Response response = mapper.toResponse(new NotFoundException());
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, entity.getCode());
+ Assertions.assertEquals(NotFoundException.class.getSimpleName(),
entity.getType());
+ Assertions.assertFalse(entity.getMessage().isEmpty());
+ }
+
+ @Test
+ public void testNotFoundExceptionWithMessage() {
+ Response response = mapper.toResponse(new NotFoundException("no matching
route"));
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals("no matching route", entity.getMessage());
+ }
+}
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/mapper/TestParamExceptionMapper.java
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestParamExceptionMapper.java
new file mode 100644
index 0000000000..fa97ae8b12
--- /dev/null
+++
b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestParamExceptionMapper.java
@@ -0,0 +1,71 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.gravitino.server.web.mapper;
+
+import javax.ws.rs.core.Response;
+import org.apache.gravitino.dto.responses.ErrorConstants;
+import org.apache.gravitino.dto.responses.ErrorResponse;
+import org.glassfish.jersey.server.ParamException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestParamExceptionMapper {
+
+ private final ParamExceptionMapper mapper = new ParamExceptionMapper();
+
+ @Test
+ public void testPathParamExceptionReturnsNotFound() {
+ NumberFormatException cause = new NumberFormatException("For input string:
\"abc\"");
+ ParamException exception = new ParamException.PathParamException(cause,
"version", null);
+
+ Response response = mapper.toResponse(exception);
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, entity.getCode());
+ Assertions.assertTrue(entity.getMessage().contains("version"));
+ Assertions.assertTrue(entity.getMessage().contains("For input string"));
+ }
+
+ @Test
+ public void testQueryParamExceptionReturnsNotFound() {
+ NumberFormatException cause = new NumberFormatException("For input string:
\"xyz\"");
+ ParamException exception = new ParamException.QueryParamException(cause,
"limit", null);
+
+ Response response = mapper.toResponse(exception);
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE, entity.getCode());
+ Assertions.assertTrue(entity.getMessage().contains("limit"));
+ }
+
+ @Test
+ public void testHeaderParamExceptionReturnsIllegalArguments() {
+ NumberFormatException cause = new NumberFormatException("For input string:
\"nope\"");
+ ParamException exception = new ParamException.HeaderParamException(cause,
"X-Count", null);
+
+ Response response = mapper.toResponse(exception);
+ ErrorResponse entity = (ErrorResponse) response.getEntity();
+
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
response.getStatus());
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
entity.getCode());
+ Assertions.assertTrue(entity.getMessage().contains("X-Count"));
+ }
+}
