This is an automated email from the ASF dual-hosted git repository. github-actions[bot] pushed a commit to branch cherry-pick-7478ab48-to-branch-1.3 in repository https://gitbox.apache.org/repos/asf/gravitino.git
commit bc86ffe90593a6ada10cb5043cd30254a6deda84 Author: roryqi <[email protected]> AuthorDate: Thu Sep 10 09:39:52 2026 +0800 [#12975] fix(core): Preserve errors thrown by PrincipalUtils.doAs (#12976) ### What changes were proposed in this pull request? Rethrow the original `Error` from `PrincipalUtils.doAs` after logging it, instead of wrapping it in a `RuntimeException`. Add a unit test to verify that the original error instance is preserved. ### Why are the changes needed? Wrapping fatal JVM errors such as `OutOfMemoryError` in a `RuntimeException` changes their classification and prevents callers and error handlers from recognizing the actual failure. Fix: #12975 ### Does this PR introduce _any_ user-facing change? Yes. Errors thrown by privileged actions retain their original type instead of being exposed as `RuntimeException`. ### How was this patch tested? Ran: `./gradlew :core:spotlessApply :core:test --tests org.apache.gravitino.utils.TestPrincipalUtils` # Conflicts: # lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java # server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java --- .../gravitino/dto/responses/ErrorResponse.java | 18 ++- .../gravitino/json/TestResponseJsonSerDe.java | 23 ++++ .../org/apache/gravitino/utils/PrincipalUtils.java | 7 +- .../apache/gravitino/utils/TestPrincipalUtils.java | 124 +++++++++++++++++++++ .../iceberg/service/IcebergExceptionMapper.java | 12 +- .../iceberg/service/TestIcebergErrorHandling.java | 122 ++++++++++++++++++++ .../service/TestIcebergExceptionMapper.java | 32 +++++- .../apache/gravitino/lance/LanceRESTService.java | 9 ++ .../lance/service/LanceExceptionMapper.java | 8 +- .../lance/service/TestLanceExceptionMapper.java | 90 +++++++++++++++ .../apache/gravitino/server/GravitinoServer.java | 2 + .../server/web/mapper/ErrorExceptionMapper.java | 47 ++++++++ .../web/mapper/TestErrorExceptionMapper.java | 52 +++++++++ .../server/web/rest/TestFilesetOperations.java | 45 ++++++-- 14 files changed, 564 insertions(+), 27 deletions(-) diff --git a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java index a47d4cbb80..3927a743b6 100644 --- a/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java +++ b/common/src/main/java/org/apache/gravitino/dto/responses/ErrorResponse.java @@ -203,11 +203,21 @@ public class ErrorResponse extends BaseResponse { * @return The new instance. */ public static ErrorResponse internalError(String message, Throwable throwable) { + return internalError(RuntimeException.class.getSimpleName(), message, throwable); + } + + /** + * Creates an internal error response with an explicit error type. + * + * @param type The type of the error. + * @param message The message of the error. + * @param throwable The throwable that caused the error, if available. + * @return The new error response. + */ + public static ErrorResponse internalError( + String type, String message, @Nullable Throwable throwable) { return new ErrorResponse( - ErrorConstants.INTERNAL_ERROR_CODE, - RuntimeException.class.getSimpleName(), - message, - getStackTrace(throwable)); + ErrorConstants.INTERNAL_ERROR_CODE, type, message, getStackTrace(throwable)); } /** diff --git a/common/src/test/java/org/apache/gravitino/json/TestResponseJsonSerDe.java b/common/src/test/java/org/apache/gravitino/json/TestResponseJsonSerDe.java index 9729fbf3d6..1bbf5baea6 100644 --- a/common/src/test/java/org/apache/gravitino/json/TestResponseJsonSerDe.java +++ b/common/src/test/java/org/apache/gravitino/json/TestResponseJsonSerDe.java @@ -35,6 +35,29 @@ import org.junit.jupiter.api.Test; public class TestResponseJsonSerDe { + /** + * Checks that custom internal error types and causes survive JSON serialization. + * + * @throws JsonProcessingException If serialization fails. + */ + @Test + public void testInternalErrorRetainsTypeAndCause() throws JsonProcessingException { + Error error = new NoClassDefFoundError("catalog class"); + error.initCause(new ClassNotFoundException("missing dependency")); + ErrorResponse response = + ErrorResponse.internalError("NoClassDefFoundError", "Server error", error); + String json = JsonUtils.objectMapper().writeValueAsString(response); + ErrorResponse restored = JsonUtils.objectMapper().readValue(json, ErrorResponse.class); + Assertions.assertEquals(response, restored); + Assertions.assertEquals("NoClassDefFoundError", restored.getType()); + Assertions.assertTrue( + String.join("\n", restored.getStack()) + .contains("Caused by: java.lang.ClassNotFoundException: missing dependency")); + Assertions.assertEquals( + "RuntimeException", ErrorResponse.internalError("existing behavior", error).getType()); + Assertions.assertNull(ErrorResponse.internalError("Error", "No stack", null).getStack()); + } + @Test public void testBaseResponseSerDe() throws JsonProcessingException { BaseResponse response = new BaseResponse(); diff --git a/core/src/main/java/org/apache/gravitino/utils/PrincipalUtils.java b/core/src/main/java/org/apache/gravitino/utils/PrincipalUtils.java index bd3210c5e7..a324964f11 100644 --- a/core/src/main/java/org/apache/gravitino/utils/PrincipalUtils.java +++ b/core/src/main/java/org/apache/gravitino/utils/PrincipalUtils.java @@ -52,12 +52,13 @@ public class PrincipalUtils { subject.getPrincipals().add(principal); return Subject.doAs(subject, action); } catch (PrivilegedActionException pae) { + LOG.error("doAs method encountered an exception", pae); Throwable cause = pae.getCause(); Throwables.propagateIfPossible(cause, Exception.class); throw new RuntimeException("doAs method encountered an unexpected exception", pae); - } catch (Error t) { - LOG.warn("doAs method encountered an unexpected error", t); - throw new RuntimeException("doAs method encountered an unexpected exception", t); + } catch (Error error) { + LOG.error("doAs method encountered an unexpected error", error); + throw error; } } diff --git a/core/src/test/java/org/apache/gravitino/utils/TestPrincipalUtils.java b/core/src/test/java/org/apache/gravitino/utils/TestPrincipalUtils.java index a2a285737d..d05d51842b 100644 --- a/core/src/test/java/org/apache/gravitino/utils/TestPrincipalUtils.java +++ b/core/src/test/java/org/apache/gravitino/utils/TestPrincipalUtils.java @@ -19,9 +19,20 @@ package org.apache.gravitino.utils; +import java.security.PrivilegedActionException; +import java.util.ArrayList; +import java.util.List; import org.apache.gravitino.UserPrincipal; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.core.Appender; +import org.apache.logging.log4j.core.LogEvent; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.config.Configuration; +import org.apache.logging.log4j.core.config.LoggerConfig; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import org.mockito.Mockito; public class TestPrincipalUtils { @@ -52,4 +63,117 @@ public class TestPrincipalUtils { return null; }); } + + @Test + public void testErrorIsPropagated() { + UserPrincipal principal = new UserPrincipal("testErrorIsPropagated"); + AssertionError error = new AssertionError("test error"); + + AssertionError thrown = + Assertions.assertThrows( + AssertionError.class, + () -> + PrincipalUtils.doAs( + principal, + () -> { + throw error; + })); + + Assertions.assertSame(error, thrown); + } + + /** Checks that checked exceptions retain their identity and cause. */ + @Test + public void testCheckedExceptionIsPropagated() { + Exception cause = new Exception("root cause"); + Exception exception = new Exception("checked failure", cause); + Exception thrown = + Assertions.assertThrows( + Exception.class, + () -> + PrincipalUtils.doAs( + new UserPrincipal("test"), + () -> { + throw exception; + })); + Assertions.assertSame(exception, thrown); + Assertions.assertSame(cause, thrown.getCause()); + } + + /** Checks that runtime exceptions retain their identity and cause. */ + @Test + public void testRuntimeExceptionIsPropagated() { + Exception cause = new Exception("root cause"); + RuntimeException exception = new IllegalArgumentException("invalid argument", cause); + RuntimeException thrown = + Assertions.assertThrows( + RuntimeException.class, + () -> + PrincipalUtils.doAs( + new UserPrincipal("test"), + () -> { + throw exception; + })); + Assertions.assertSame(exception, thrown); + Assertions.assertSame(cause, thrown.getCause()); + } + + /** Checks that caught failures are logged at ERROR with the throwable. */ + @Test + public void testFailuresAreLoggedWithThrowable() { + LoggerContext context = + (LoggerContext) LogManager.getContext(PrincipalUtils.class.getClassLoader(), false); + Configuration configuration = context.getConfiguration(); + String loggerName = PrincipalUtils.class.getName(); + LoggerConfig previousConfig = configuration.getLoggers().get(loggerName); + Appender appender = Mockito.mock(Appender.class); + Mockito.when(appender.getName()).thenReturn("principalUtilsCapture"); + Mockito.when(appender.isStarted()).thenReturn(true); + List<LogEvent> events = new ArrayList<>(); + Mockito.doAnswer( + invocation -> { + events.add(((LogEvent) invocation.getArgument(0)).toImmutable()); + return null; + }) + .when(appender) + .append(Mockito.any(LogEvent.class)); + LoggerConfig loggerConfig = new LoggerConfig(loggerName, Level.ERROR, false); + loggerConfig.addAppender(appender, Level.ERROR, null); + configuration.removeLogger(loggerName); + configuration.addLogger(loggerName, loggerConfig); + context.updateLoggers(); + try { + Error error = new AssertionError("request error"); + Assertions.assertThrows( + Error.class, + () -> + PrincipalUtils.doAs( + new UserPrincipal("test"), + () -> { + throw error; + })); + Exception exception = new Exception("checked failure", new Exception("root cause")); + Assertions.assertThrows( + Exception.class, + () -> + PrincipalUtils.doAs( + new UserPrincipal("test"), + () -> { + throw exception; + })); + Assertions.assertEquals(2, events.size()); + Assertions.assertEquals(Level.ERROR, events.get(0).getLevel()); + Assertions.assertSame(error, events.get(0).getThrown()); + Assertions.assertEquals(Level.ERROR, events.get(1).getLevel()); + Throwable logged = events.get(1).getThrown(); + Assertions.assertInstanceOf(PrivilegedActionException.class, logged); + Assertions.assertSame(exception, logged.getCause()); + } finally { + configuration.removeLogger(loggerName); + if (previousConfig != null) { + configuration.addLogger(loggerName, previousConfig); + } + context.updateLoggers(); + } + } } 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 daaf1db3aa..e7c9cb9546 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 @@ -50,7 +50,7 @@ import org.slf4j.LoggerFactory; // Referred from Apache Iceberg's EXCEPTION_ERROR_CODES implementation // core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java @Provider -public class IcebergExceptionMapper implements ExceptionMapper<Exception> { +public class IcebergExceptionMapper implements ExceptionMapper<Throwable> { private static final Logger LOG = LoggerFactory.getLogger(IcebergExceptionMapper.class); @@ -120,8 +120,14 @@ public class IcebergExceptionMapper implements ExceptionMapper<Exception> { return new ServiceFailureException("%s", message); } + /** + * Maps an uncaught throwable to an Iceberg REST error response. + * + * @param ex the failure raised while processing the request + * @return the error response, defaulting to HTTP 500 for unmapped failures + */ @Override - public Response toResponse(Exception ex) { + public Response toResponse(Throwable ex) { return toRESTResponse(ex); } @@ -130,7 +136,7 @@ public class IcebergExceptionMapper implements ExceptionMapper<Exception> { EXCEPTION_ERROR_CODES.getOrDefault( ex.getClass(), Status.INTERNAL_SERVER_ERROR.getStatusCode()); if (status == Status.INTERNAL_SERVER_ERROR.getStatusCode()) { - LOG.warn("Iceberg REST server unexpected exception:", ex); + LOG.error("Iceberg REST server unexpected failure:", ex); } else { LOG.info( "Iceberg REST server error maybe caused by user request, response http status: {}, exception: {}, exception message: {}", diff --git a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergErrorHandling.java b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergErrorHandling.java new file mode 100644 index 0000000000..4c8ab70eb7 --- /dev/null +++ b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergErrorHandling.java @@ -0,0 +1,122 @@ +/* + * 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 java.io.IOException; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.PathParam; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import org.apache.gravitino.UserPrincipal; +import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.iceberg.rest.responses.ErrorResponse; +import org.glassfish.jersey.jackson.JacksonFeature; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.JerseyTest; +import org.glassfish.jersey.test.TestProperties; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** Tests Iceberg HTTP responses for errors raised on the request path. */ +public class TestIcebergErrorHandling extends JerseyTest { + + /** Simulates direct failures and failures inside the REST doAs boundary. */ + @Path("failure") + public static class FailureResource { + + /** + * Executes a request with an optional failure. + * + * @param mode whether to fail directly, within doAs, or return a successful response + * @return the request response + */ + @GET + @Path("{mode}") + public Response request(@PathParam("mode") String mode) { + if ("healthy".equals(mode)) { + return Response.ok().build(); + } + Error failure = new NoClassDefFoundError("catalog class"); + failure.initCause(new ClassNotFoundException("missing dependency")); + if ("direct".equals(mode)) { + throw failure; + } + try { + return PrincipalUtils.doAs( + new UserPrincipal("test"), + () -> { + throw failure; + }); + } catch (Exception e) { + return IcebergExceptionMapper.toRESTResponse(e); + } + } + } + + /** + * Registers the same error and JSON mappers as the Iceberg REST service. + * + * @return the test application + */ + @Override + protected Application configure() { + try { + forceSet( + TestProperties.CONTAINER_PORT, String.valueOf(RESTUtils.findAvailablePort(2000, 3000))); + } catch (IOException e) { + throw new RuntimeException(e); + } + return new ResourceConfig() + .register(FailureResource.class) + .register(IcebergExceptionMapper.class) + .register(IcebergObjectMapperProvider.class) + .register(JacksonFeature.class); + } + + /** + * Verifies diagnostics survive both request paths and later requests can still succeed. + * + * @throws IOException if the JSON response cannot be parsed + */ + @Test + public void testErrorResponsesAndSubsequentRequests() throws IOException { + for (String mode : new String[] {"direct", "do-as"}) { + try (Response response = + target("failure/" + mode).request(MediaType.APPLICATION_JSON_TYPE).get()) { + Assertions.assertEquals(500, response.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, response.getMediaType()); + ErrorResponse error = + IcebergObjectMapper.getInstance() + .readValue(response.readEntity(String.class), ErrorResponse.class); + Assertions.assertEquals(500, error.code()); + Assertions.assertEquals("NoClassDefFoundError", error.type()); + Assertions.assertEquals("catalog class", error.message()); + Assertions.assertTrue( + String.join("\n", error.stack()) + .contains("Caused by: java.lang.ClassNotFoundException: missing dependency")); + } + try (Response response = target("failure/healthy").request().get()) { + Assertions.assertEquals(200, response.getStatus()); + } + } + } +} diff --git a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java index eef610f6b0..354f4d26bd 100644 --- a/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java +++ b/iceberg/iceberg-rest-server/src/test/java/org/apache/gravitino/iceberg/service/TestIcebergExceptionMapper.java @@ -33,17 +33,13 @@ import org.apache.iceberg.exceptions.NotAuthorizedException; import org.apache.iceberg.exceptions.ServiceUnavailableException; import org.apache.iceberg.exceptions.UnprocessableEntityException; import org.apache.iceberg.exceptions.ValidationException; +import org.apache.iceberg.rest.responses.ErrorResponse; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; public class TestIcebergExceptionMapper { private final IcebergExceptionMapper icebergExceptionMapper = new IcebergExceptionMapper(); - private void checkExceptionStatus(Exception exception, int statusCode) { - Response response = icebergExceptionMapper.toResponse(exception); - Assertions.assertEquals(statusCode, response.getStatus()); - } - @Test public void testIcebergExceptionMapper() { checkExceptionStatus(new IllegalArgumentException(""), 400); @@ -65,4 +61,30 @@ public class TestIcebergExceptionMapper { checkExceptionStatus(new ServiceUnavailableException(""), 503); checkExceptionStatus(new RuntimeException(), 500); } + + /** Checks that errors retain their type and nested causes in the response. */ + @Test + public void testErrorsRetainTypeAndCause() { + for (Error error : + new Error[] { + new OutOfMemoryError("Metaspace"), new StackOverflowError(), + new NoClassDefFoundError("catalog class"), new AssertionError("assertion") + }) { + error.initCause(new IllegalStateException("root cause")); + try (Response response = icebergExceptionMapper.toResponse(error)) { + Assertions.assertEquals(500, response.getStatus()); + ErrorResponse entity = (ErrorResponse) response.getEntity(); + Assertions.assertEquals(error.getClass().getSimpleName(), entity.type()); + Assertions.assertEquals(error.getMessage(), entity.message()); + Assertions.assertTrue( + String.join("\n", entity.stack()) + .contains("Caused by: java.lang.IllegalStateException: root cause")); + } + } + } + + private void checkExceptionStatus(Exception exception, int statusCode) { + Response response = icebergExceptionMapper.toResponse(exception); + Assertions.assertEquals(statusCode, response.getStatus()); + } } diff --git a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java index 98f86aaae5..a97bc96620 100644 --- a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java +++ b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/LanceRESTService.java @@ -30,6 +30,14 @@ import org.apache.gravitino.auxiliary.GravitinoAuxiliaryService; import org.apache.gravitino.lance.common.config.LanceConfig; import org.apache.gravitino.lance.common.ops.LanceNamespaceBackend; import org.apache.gravitino.lance.common.ops.NamespaceWrapper; +<<<<<<< HEAD +======= +import org.apache.gravitino.lance.service.LanceExceptionMapper; +import org.apache.gravitino.lance.service.LanceHealthCheckPathMatcher; +import org.apache.gravitino.lance.service.LanceServiceIdentityFilter; +import org.apache.gravitino.lance.service.authorization.LanceAuthorizationMetadataFilter; +import org.apache.gravitino.lance.service.authorization.LanceRESTAuthInterceptionService; +>>>>>>> 7478ab48e ([#12975] fix(core): Preserve errors thrown by PrincipalUtils.doAs (#12976)) import org.apache.gravitino.listener.EventBus; import org.apache.gravitino.listener.api.event.EventSource; import org.apache.gravitino.metrics.MetricsSystem; @@ -80,6 +88,7 @@ public class LanceRESTService implements GravitinoAuxiliaryService { ResourceConfig resourceConfig = new ResourceConfig(); resourceConfig.register(JacksonFeature.class); resourceConfig.packages(LANCE_REST_SPEC_PACKAGE); + resourceConfig.register(LanceExceptionMapper.class); resourceConfig.register( new AbstractBinder() { @Override diff --git a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java index 2078b75fe6..47d0b7521a 100644 --- a/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java +++ b/lance/lance-rest-server/src/main/java/org/apache/gravitino/lance/service/LanceExceptionMapper.java @@ -42,11 +42,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; @Provider -public class LanceExceptionMapper implements ExceptionMapper<Exception> { +public class LanceExceptionMapper implements ExceptionMapper<Throwable> { private static final Logger LOG = LoggerFactory.getLogger(LanceExceptionMapper.class); - public static Response toRESTResponse(String instance, Exception ex) { + public static Response toRESTResponse(String instance, Throwable ex) { LanceNamespaceException lanceException = ex instanceof LanceNamespaceException ? (LanceNamespaceException) ex @@ -61,11 +61,11 @@ public class LanceExceptionMapper implements ExceptionMapper<Exception> { } @Override - public Response toResponse(Exception ex) { + public Response toResponse(Throwable ex) { return toRESTResponse("", ex); } - private static LanceNamespaceException toLanceNamespaceException(String instance, Exception ex) { + private static LanceNamespaceException toLanceNamespaceException(String instance, Throwable ex) { if (ex instanceof NoSuchTableException) { return new TableNotFoundException(ex.getMessage(), getStackTrace(ex), instance); diff --git a/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java new file mode 100644 index 0000000000..fd3d98ded6 --- /dev/null +++ b/lance/lance-rest-server/src/test/java/org/apache/gravitino/lance/service/TestLanceExceptionMapper.java @@ -0,0 +1,90 @@ +/* + * 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.lance.service; + +import java.io.IOException; +import javax.ws.rs.GET; +import javax.ws.rs.Path; +import javax.ws.rs.core.Application; +import javax.ws.rs.core.MediaType; +import javax.ws.rs.core.Response; +import org.apache.gravitino.rest.RESTUtils; +import org.glassfish.jersey.jackson.JacksonFeature; +import org.glassfish.jersey.server.ResourceConfig; +import org.glassfish.jersey.test.JerseyTest; +import org.glassfish.jersey.test.TestProperties; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.lance.namespace.model.ErrorResponse; + +/** Tests for {@link LanceExceptionMapper}. */ +public class TestLanceExceptionMapper extends JerseyTest { + + /** A resource that raises an error outside the operation-level exception handlers. */ + @Path("error") + public static class ErrorResource { + + /** + * Raises an assertion error. + * + * @return never returns normally + */ + @GET + public String fail() { + AssertionError error = new AssertionError("assertion failure"); + error.initCause(new IllegalStateException("root cause")); + throw error; + } + } + + /** + * Configures the test resource and Lance exception mapper. + * + * @return the test application + */ + @Override + protected Application configure() { + try { + forceSet( + TestProperties.CONTAINER_PORT, String.valueOf(RESTUtils.findAvailablePort(2000, 3000))); + } catch (IOException e) { + throw new RuntimeException(e); + } + return new ResourceConfig() + .register(ErrorResource.class) + .register(LanceExceptionMapper.class) + .register(JacksonFeature.class); + } + + /** Verifies that an uncaught error is converted to a Lance internal error response. */ + @Test + public void testErrorResponse() { + try (Response response = target("error").request(MediaType.APPLICATION_JSON_TYPE).get()) { + Assertions.assertEquals( + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), response.getStatus()); + ErrorResponse entity = response.readEntity(ErrorResponse.class); + Assertions.assertEquals("assertion failure", entity.getError()); + Assertions.assertEquals("", entity.getInstance()); + Assertions.assertTrue( + entity.getDetail().contains("java.lang.AssertionError: assertion failure")); + Assertions.assertTrue( + entity.getDetail().contains("Caused by: java.lang.IllegalStateException: root cause")); + } + } +} 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 f2ef1e77a8..3db96fed27 100644 --- a/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java +++ b/server/src/main/java/org/apache/gravitino/server/GravitinoServer.java @@ -62,6 +62,7 @@ import org.apache.gravitino.server.web.RequestContextFilter; import org.apache.gravitino.server.web.VersioningFilter; import org.apache.gravitino.server.web.filter.AccessControlNotAllowedFilter; import org.apache.gravitino.server.web.filter.GravitinoInterceptionService; +import org.apache.gravitino.server.web.mapper.ErrorExceptionMapper; import org.apache.gravitino.server.web.mapper.JsonMappingExceptionMapper; import org.apache.gravitino.server.web.mapper.JsonParseExceptionMapper; import org.apache.gravitino.server.web.mapper.JsonProcessingExceptionMapper; @@ -181,6 +182,7 @@ public class GravitinoServer extends ResourceConfig { } }); register(JsonProcessingExceptionMapper.class); + register(ErrorExceptionMapper.class); register(JsonParseExceptionMapper.class); register(JsonMappingExceptionMapper.class); register(ParamExceptionMapper.class); diff --git a/server/src/main/java/org/apache/gravitino/server/web/mapper/ErrorExceptionMapper.java b/server/src/main/java/org/apache/gravitino/server/web/mapper/ErrorExceptionMapper.java new file mode 100644 index 0000000000..892cc8f699 --- /dev/null +++ b/server/src/main/java/org/apache/gravitino/server/web/mapper/ErrorExceptionMapper.java @@ -0,0 +1,47 @@ +/* + * 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.MediaType; +import javax.ws.rs.core.Response; +import javax.ws.rs.ext.ExceptionMapper; +import org.apache.gravitino.dto.responses.ErrorResponse; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Reports errors on the request path as server errors without deciding process lifetime. */ +public class ErrorExceptionMapper implements ExceptionMapper<Error> { + private static final Logger LOG = LoggerFactory.getLogger(ErrorExceptionMapper.class); + + /** + * Returns a server error response retaining the original error type and complete stack trace. + * + * @param error The error raised while processing the request. + * @return The internal server error response. + */ + @Override + public Response toResponse(Error error) { + String message = "Server error while processing request: " + error; + LOG.error(message, error); + return Response.status(Response.Status.INTERNAL_SERVER_ERROR) + .entity(ErrorResponse.internalError(error.getClass().getSimpleName(), message, error)) + .type(MediaType.APPLICATION_JSON_TYPE) + .build(); + } +} diff --git a/server/src/test/java/org/apache/gravitino/server/web/mapper/TestErrorExceptionMapper.java b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestErrorExceptionMapper.java new file mode 100644 index 0000000000..78d264db46 --- /dev/null +++ b/server/src/test/java/org/apache/gravitino/server/web/mapper/TestErrorExceptionMapper.java @@ -0,0 +1,52 @@ +/* + * 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.MediaType; +import javax.ws.rs.core.Response; +import org.apache.gravitino.dto.responses.ErrorResponse; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +/** Tests server error responses for errors raised on the request path. */ +public class TestErrorExceptionMapper { + /** Checks that error types and nested causes survive response construction. */ + @Test + public void testErrorsRetainTypeAndCause() { + for (Error error : + new Error[] { + new OutOfMemoryError("Metaspace"), new StackOverflowError(), + new NoClassDefFoundError("catalog class"), new AssertionError("assertion") + }) { + error.initCause(new IllegalStateException("root cause")); + try (Response response = new ErrorExceptionMapper().toResponse(error)) { + Assertions.assertEquals(500, response.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, response.getMediaType()); + ErrorResponse entity = (ErrorResponse) response.getEntity(); + Assertions.assertEquals(error.getClass().getSimpleName(), entity.getType()); + Assertions.assertEquals( + "Server error while processing request: " + error, entity.getMessage()); + String stack = String.join("\n", entity.getStack()); + Assertions.assertTrue(stack.contains(error.toString())); + Assertions.assertTrue( + stack.contains("Caused by: java.lang.IllegalStateException: root cause")); + } + } + } +} diff --git a/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java b/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java index 343a778a77..702921eef1 100644 --- a/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java +++ b/server/src/test/java/org/apache/gravitino/server/web/rest/TestFilesetOperations.java @@ -70,6 +70,7 @@ import org.apache.gravitino.file.Fileset; import org.apache.gravitino.file.FilesetChange; import org.apache.gravitino.lock.LockManager; import org.apache.gravitino.rest.RESTUtils; +import org.apache.gravitino.server.web.mapper.ErrorExceptionMapper; import org.glassfish.jersey.internal.inject.AbstractBinder; import org.glassfish.jersey.server.ResourceConfig; import org.glassfish.jersey.test.TestProperties; @@ -121,6 +122,7 @@ public class TestFilesetOperations extends BaseOperationsTest { ResourceConfig resourceConfig = new ResourceConfig(); resourceConfig.register(FilesetOperations.class); + resourceConfig.register(ErrorExceptionMapper.class); resourceConfig.register( new AbstractBinder() { @Override @@ -392,20 +394,47 @@ public class TestFilesetOperations extends BaseOperationsTest { Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResp3.getCode()); Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResp3.getType()); - // Test throw Error - doThrow(new Error("mock error")) + // A request error must retain its diagnostics without becoming an operation failure. + Error error = new NoClassDefFoundError("mock catalog class"); + error.initCause(new ClassNotFoundException("missing catalog dependency")); + Mockito.doThrow(error) + .doReturn(fileset) .when(dispatcher) +<<<<<<< HEAD .createMultipleLocationFileset(any(), any(), any(), any(), any()); Response resp4 = +======= + .createMultipleLocationFileset(any(), any(), any(), any(), any(), any(), any()); + try (Response errorResponse = +>>>>>>> 7478ab48e ([#12975] fix(core): Preserve errors thrown by PrincipalUtils.doAs (#12976)) target(filesetPath(metalake, catalog, schema)) .request(MediaType.APPLICATION_JSON_TYPE) .accept("application/vnd.gravitino.v1+json") - .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE)); - Assertions.assertEquals( - Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), resp4.getStatus()); - ErrorResponse errorResp4 = resp4.readEntity(ErrorResponse.class); - Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, errorResp4.getCode()); - Assertions.assertEquals(RuntimeException.class.getSimpleName(), errorResp4.getType()); + .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE))) { + Assertions.assertEquals(500, errorResponse.getStatus()); + Assertions.assertEquals(MediaType.APPLICATION_JSON_TYPE, errorResponse.getMediaType()); + ErrorResponse entity = errorResponse.readEntity(ErrorResponse.class); + Assertions.assertEquals(ErrorConstants.INTERNAL_ERROR_CODE, entity.getCode()); + Assertions.assertEquals("NoClassDefFoundError", entity.getType()); + Assertions.assertEquals( + "Server error while processing request: java.lang.NoClassDefFoundError: mock catalog class", + entity.getMessage()); + String stack = String.join("\n", entity.getStack()); + Assertions.assertTrue(stack.contains("java.lang.NoClassDefFoundError: mock catalog class")); + Assertions.assertTrue( + stack.contains( + "Caused by: java.lang.ClassNotFoundException: missing catalog dependency")); + } + + try (Response recoveredResponse = + target(filesetPath(metalake, catalog, schema)) + .request(MediaType.APPLICATION_JSON_TYPE) + .accept("application/vnd.gravitino.v1+json") + .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE))) { + Assertions.assertEquals(200, recoveredResponse.getStatus()); + Assertions.assertEquals( + "fileset1", recoveredResponse.readEntity(FilesetResponse.class).getFileset().name()); + } } @Test
