Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,21 @@ public static ErrorResponse internalError(String message) {
* @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));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,29 @@

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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ public static <T> T doAs(Principal principal, PrivilegedExceptionAction<T> actio
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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will be the exception when propogating to the client side?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current behavior is the server will crash.

}
}

Expand Down
124 changes: 124 additions & 0 deletions core/src/test/java/org/apache/gravitino/utils/TestPrincipalUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -52,4 +63,117 @@ public void testThread() throws Exception {
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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
// 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);

Expand Down Expand Up @@ -121,8 +121,14 @@ public static Exception convertToIcebergException(Exception e) {
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);
}

Expand All @@ -131,7 +137,7 @@ public static Response toRESTResponse(Throwable ex) {
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: {}",
Expand Down
Original file line number Diff line number Diff line change
@@ -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());
}
}
}
}
Loading
Loading