This is an automated email from the ASF dual-hosted git repository.
roryqi pushed a commit to branch branch-1.2
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/branch-1.2 by this push:
new ac071bf44e [Cherry-pick to branch-1.2] [#10667] fix(iceberg): Return
JSON error body instead of HTML for pre-JAX-RS errors (#10668) (#10679)
ac071bf44e is described below
commit ac071bf44e18648c1e3e957112f6c045010f2dae
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Mon Apr 6 13:54:03 2026 +0800
[Cherry-pick to branch-1.2] [#10667] fix(iceberg): Return JSON error body
instead of HTML for pre-JAX-RS errors (#10668) (#10679)
**Cherry-pick Information:**
- Original commit: 922009a3f21f60d51f173d62068ab8238d4d806c
- Target branch: `branch-1.2`
- Status: ✅ Clean cherry-pick (no conflicts)
Co-authored-by: akshay thorat <[email protected]>
---
.../org/apache/gravitino/iceberg/RESTService.java | 9 +-
.../service/IcebergAuthenticationFilter.java | 61 ++++++++++
.../iceberg/service/IcebergExceptionMapper.java | 42 +++++++
.../iceberg/service/IcebergRESTUtils.java | 12 ++
.../service/TestIcebergAuthenticationFilter.java | 123 +++++++++++++++++++++
.../authentication/AuthenticationFilter.java | 23 +++-
.../apache/gravitino/server/web/JettyServer.java | 12 +-
7 files changed, 277 insertions(+), 5 deletions(-)
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
index 23f1fcf042..f4bcfedd8b 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/RESTService.java
@@ -27,6 +27,7 @@ import org.apache.gravitino.Configs;
import org.apache.gravitino.GravitinoEnv;
import org.apache.gravitino.auxiliary.GravitinoAuxiliaryService;
import org.apache.gravitino.iceberg.common.IcebergConfig;
+import org.apache.gravitino.iceberg.service.IcebergAuthenticationFilter;
import org.apache.gravitino.iceberg.service.IcebergCatalogWrapperManager;
import org.apache.gravitino.iceberg.service.IcebergExceptionMapper;
import org.apache.gravitino.iceberg.service.IcebergObjectMapperProvider;
@@ -79,7 +80,13 @@ public class RESTService implements
GravitinoAuxiliaryService {
private void initServer(IcebergConfig icebergConfig) {
JettyServerConfig serverConfig =
JettyServerConfig.fromConfig(icebergConfig);
- server = new JettyServer();
+ server =
+ new JettyServer() {
+ @Override
+ protected javax.servlet.Filter createAuthenticationFilter() {
+ return new IcebergAuthenticationFilter();
+ }
+ };
MetricsSystem metricsSystem = GravitinoEnv.getInstance().metricsSystem();
server.initialize(serverConfig, SERVICE_NAME, false /* shouldEnableUI */);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergAuthenticationFilter.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergAuthenticationFilter.java
new file mode 100644
index 0000000000..20209b056e
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergAuthenticationFilter.java
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.service;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.gravitino.server.authentication.AuthenticationFilter;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.eclipse.jetty.http.HttpStatus;
+
+/**
+ * An {@link AuthenticationFilter} subclass for the Iceberg REST server that
produces JSON error
+ * responses conforming to the Iceberg REST API specification.
+ *
+ * <p>When authentication fails, the default {@link AuthenticationFilter}
calls {@code
+ * resp.sendError()} which produces HTML error pages via Jetty's default error
handler. This
+ * subclass overrides the error response to write a proper Iceberg {@link
ErrorResponse} JSON body,
+ * which Iceberg REST clients (e.g., the Java {@code RESTCatalog}) expect.
+ */
+public class IcebergAuthenticationFilter extends AuthenticationFilter {
+
+ private static final ObjectMapper MAPPER = IcebergObjectMapper.getInstance();
+
+ @Override
+ protected void sendAuthErrorResponse(HttpServletResponse response, Exception
exception)
+ throws IOException {
+ Exception icebergException =
IcebergExceptionMapper.convertToIcebergException(exception);
+ int status = IcebergExceptionMapper.getErrorCode(icebergException);
+ String message = icebergException.getMessage();
+ if (StringUtils.isBlank(message)) {
+ message = HttpStatus.getMessage(status);
+ }
+
+ String type = icebergException.getClass().getSimpleName();
+ ErrorResponse errorResponse = IcebergRESTUtils.errorResponse(status, type,
message);
+
+ response.setStatus(status);
+ response.setContentType("application/json");
+ response.setCharacterEncoding(StandardCharsets.UTF_8.name());
+ MAPPER.writeValue(response.getWriter(), errorResponse);
+ }
+}
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
index c1a443b24e..ecc16caca9 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergExceptionMapper.java
@@ -26,7 +26,9 @@ import javax.ws.rs.ext.ExceptionMapper;
import javax.ws.rs.ext.Provider;
import org.apache.gravitino.exceptions.IllegalNameIdentifierException;
import org.apache.gravitino.exceptions.NoSuchCatalogException;
+import org.apache.gravitino.exceptions.UnauthorizedException;
import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.CommitFailedException;
import org.apache.iceberg.exceptions.CommitStateUnknownException;
import org.apache.iceberg.exceptions.ForbiddenException;
@@ -36,6 +38,7 @@ import org.apache.iceberg.exceptions.NoSuchNamespaceException;
import org.apache.iceberg.exceptions.NoSuchTableException;
import org.apache.iceberg.exceptions.NoSuchViewException;
import org.apache.iceberg.exceptions.NotAuthorizedException;
+import org.apache.iceberg.exceptions.ServiceFailureException;
import org.apache.iceberg.exceptions.ServiceUnavailableException;
import org.apache.iceberg.exceptions.UnprocessableEntityException;
import org.apache.iceberg.exceptions.ValidationException;
@@ -56,6 +59,7 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Exception> {
.put(IllegalNameIdentifierException.class, 400)
.put(NamespaceNotEmptyException.class, 400)
.put(NotAuthorizedException.class, 401)
+ .put(UnauthorizedException.class, 401)
.put(org.apache.gravitino.exceptions.ForbiddenException.class, 403)
.put(ForbiddenException.class, 403)
.put(NoSuchNamespaceException.class, 404)
@@ -68,9 +72,47 @@ public class IcebergExceptionMapper implements
ExceptionMapper<Exception> {
.put(CommitFailedException.class, 409)
.put(UnprocessableEntityException.class, 422)
.put(CommitStateUnknownException.class, 500)
+ .put(ServiceFailureException.class, 500)
.put(ServiceUnavailableException.class, 503)
.build();
+ /**
+ * Returns the HTTP status code for the given exception based on the Iceberg
REST spec.
+ *
+ * @param ex the exception
+ * @return the HTTP status code, defaulting to 500 for unmapped exceptions
+ */
+ public static int getErrorCode(Exception ex) {
+ return EXCEPTION_ERROR_CODES.getOrDefault(
+ ex.getClass(), Status.INTERNAL_SERVER_ERROR.getStatusCode());
+ }
+
+ /**
+ * Converts a Gravitino or generic exception to the corresponding Iceberg
REST spec exception.
+ *
+ * @param e the original exception
+ * @return the equivalent Iceberg exception, or the original if already an
Iceberg exception
+ */
+ public static Exception convertToIcebergException(Exception e) {
+ String message = e.getMessage() != null ? e.getMessage() : "";
+ if (e instanceof UnauthorizedException) {
+ return new NotAuthorizedException("%s", message);
+ }
+ if (e instanceof org.apache.gravitino.exceptions.ForbiddenException) {
+ return new ForbiddenException("%s", message);
+ }
+ if (e instanceof IllegalArgumentException
+ || e instanceof IllegalNameIdentifierException
+ || e instanceof ValidationException
+ || e instanceof NamespaceNotEmptyException) {
+ return new BadRequestException("%s", message);
+ }
+ if (EXCEPTION_ERROR_CODES.containsKey(e.getClass())) {
+ return e;
+ }
+ return new ServiceFailureException("%s", message);
+ }
+
@Override
public Response toResponse(Exception ex) {
return toRESTResponse(ex);
diff --git
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
index 84ea56cc37..78ffd7e801 100644
---
a/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
+++
b/iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java
@@ -74,6 +74,18 @@ public class IcebergRESTUtils {
.build();
}
+ /**
+ * Build an Iceberg {@link ErrorResponse} for a given HTTP status code and
message, without an
+ * exception. Used by the Jetty error handler for pre-JAX-RS errors.
+ */
+ public static ErrorResponse errorResponse(int httpStatus, String type,
String message) {
+ return ErrorResponse.builder()
+ .responseCode(httpStatus)
+ .withType(type)
+ .withMessage(message)
+ .build();
+ }
+
public static Instant calculateNewTimestamp(Instant currentTimestamp, int
hours) {
LocalDateTime currentDateTime =
LocalDateTime.ofInstant(currentTimestamp, ZoneId.systemDefault());
diff --git
a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
new file mode 100644
index 0000000000..404ba94827
--- /dev/null
+++
b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergAuthenticationFilter.java
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.service;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import javax.servlet.http.HttpServletResponse;
+import org.apache.gravitino.exceptions.UnauthorizedException;
+import org.apache.iceberg.rest.responses.ErrorResponse;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestIcebergAuthenticationFilter {
+
+ private static final ObjectMapper MAPPER = IcebergObjectMapper.getInstance();
+
+ @Test
+ public void testUnauthorizedErrorReturnsJson() throws Exception {
+ IcebergAuthenticationFilter filter = new IcebergAuthenticationFilter();
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(
+ response, new UnauthorizedException("The provided credentials did not
support"));
+
+ verify(response).setStatus(HttpServletResponse.SC_UNAUTHORIZED);
+ verify(response).setContentType("application/json");
+ verify(response).setCharacterEncoding("UTF-8");
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ErrorResponse errorResponse = MAPPER.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(401, errorResponse.code());
+ Assertions.assertEquals("NotAuthorizedException", errorResponse.type());
+ Assertions.assertEquals("The provided credentials did not support",
errorResponse.message());
+ }
+
+ @Test
+ public void testInternalServerErrorReturnsJson() throws Exception {
+ IcebergAuthenticationFilter filter = new IcebergAuthenticationFilter();
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(response, new RuntimeException("Something
went wrong"));
+
+ verify(response).setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ErrorResponse errorResponse = MAPPER.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(500, errorResponse.code());
+ Assertions.assertEquals("ServiceFailureException", errorResponse.type());
+ Assertions.assertEquals("Something went wrong", errorResponse.message());
+ }
+
+ @Test
+ public void testForbiddenExceptionReturnsJson() throws Exception {
+ IcebergAuthenticationFilter filter = new IcebergAuthenticationFilter();
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(
+ response, new
org.apache.gravitino.exceptions.ForbiddenException("Access denied"));
+
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ErrorResponse errorResponse = MAPPER.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(403, errorResponse.code());
+ Assertions.assertEquals("ForbiddenException", errorResponse.type());
+ Assertions.assertEquals("Access denied", errorResponse.message());
+ }
+
+ @Test
+ public void testNullMessageUsesDefaultStatusMessage() throws Exception {
+ IcebergAuthenticationFilter filter = new IcebergAuthenticationFilter();
+
+ HttpServletResponse response = mock(HttpServletResponse.class);
+ StringWriter stringWriter = new StringWriter();
+ PrintWriter printWriter = new PrintWriter(stringWriter);
+ when(response.getWriter()).thenReturn(printWriter);
+
+ filter.sendAuthErrorResponse(response, new RuntimeException((String)
null));
+
+ printWriter.flush();
+ String json = stringWriter.toString();
+ ErrorResponse errorResponse = MAPPER.readValue(json, ErrorResponse.class);
+ Assertions.assertEquals(500, errorResponse.code());
+ Assertions.assertEquals("ServiceFailureException", errorResponse.type());
+ Assertions.assertEquals("Server Error", errorResponse.message());
+ }
+}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
b/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
index 7df4e68d6a..ae234285cd 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/authentication/AuthenticationFilter.java
@@ -99,13 +99,32 @@ public class AuthenticationFilter implements Filter {
resp.setHeader(AuthConstants.HTTP_CHALLENGE_HEADER, challenge);
}
}
- resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, ue.getMessage());
+ sendAuthErrorResponse(resp, ue);
} catch (Exception e) {
HttpServletResponse resp = (HttpServletResponse) response;
- resp.sendError(HttpServletResponse.SC_UNAUTHORIZED, e.getMessage());
+ sendAuthErrorResponse(resp, e);
}
}
+ /**
+ * Sends an error response when authentication fails. Subclasses can
override this to customize
+ * the error response format (e.g., Iceberg REST server returns JSON error
bodies).
+ *
+ * <p>TODO: Gravitino server should override this method to return a correct
JSON response
+ * following the Gravitino error response spec.
+ *
+ * @param response the HTTP servlet response
+ * @param exception the authentication exception
+ */
+ protected void sendAuthErrorResponse(HttpServletResponse response, Exception
exception)
+ throws IOException {
+ int status =
+ exception instanceof UnauthorizedException
+ ? HttpServletResponse.SC_UNAUTHORIZED
+ : HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+ response.sendError(status, exception.getMessage());
+ }
+
@Override
public void destroy() {}
}
diff --git
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
index e81b4fa9e1..1af9ed8259 100644
---
a/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
+++
b/server-common/src/main/java/org/apache/gravitino/server/web/JettyServer.java
@@ -64,7 +64,7 @@ import org.eclipse.jetty.webapp.WebAppContext;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-public final class JettyServer {
+public class JettyServer {
private static final Logger LOG = LoggerFactory.getLogger(JettyServer.class);
@@ -470,6 +470,14 @@ public final class JettyServer {
servletContextHandler.addFilter(
CorsFilterHolder.create(serverConfig), pathSpec,
EnumSet.allOf(DispatcherType.class));
}
- addFilter(new AuthenticationFilter(), pathSpec);
+ addFilter(createAuthenticationFilter(), pathSpec);
+ }
+
+ /**
+ * Creates the authentication filter for this server. Subclasses can
override this to provide a
+ * custom authentication filter (e.g., one that returns Iceberg-spec JSON
error responses).
+ */
+ protected Filter createAuthenticationFilter() {
+ return new AuthenticationFilter();
}
}
