This is an automated email from the ASF dual-hosted git repository.
snazy pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new c2e932b70 Fix arg-matching in
ExceptionMapperTest.testFullExceptionIsLogged() (#2531)
c2e932b70 is described below
commit c2e932b7064e7ae5551e192c8dd273de2da9fd0d
Author: Robert Stupp <[email protected]>
AuthorDate: Thu Sep 11 10:14:02 2025 +0200
Fix arg-matching in ExceptionMapperTest.testFullExceptionIsLogged() (#2531)
The above test asserts the the log record passed to
`org.jboss.logmanager.Logger#logRaw(org.jboss.logmanager.ExtLogRecord)`. For
the sake of this test, both the `IcebergJsonProcessingExceptionMapper` and
`IcebergExceptionMapper` have a visible-for-testing function `getLogger()` to
provide a mocked logger. `getLogger()` however is used for "all the logging",
which makes the behavior `verify(JBOSS_LOGGER).logRaw(argThat(...))`
"interesting" - the test fails with #2461 although no relate [...]
But eventually the test failure's caused by using `getLogger()` for all
logging, which may provide the _wrong_ log record and cause the test to fail.
This change renames the `getLogger()` functions to make their special
meaning clear, and adds some clarifying comments in the code.
Also adding some more test cases for the various exception mapper code
paths.
Co-authored-by: Alexandre Dutra <[email protected]>
---
.../service/exception/IcebergExceptionMapper.java | 23 ++++---
.../IcebergJsonProcessingExceptionMapper.java | 12 ++--
.../service/exception/ExceptionMapperTest.java | 70 +++++++++++++++++-----
3 files changed, 76 insertions(+), 29 deletions(-)
diff --git
a/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
b/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
index 6e2dec197..c9d91815d 100644
---
a/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
+++
b/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergExceptionMapper.java
@@ -89,15 +89,16 @@ public class IcebergExceptionMapper implements
ExceptionMapper<RuntimeException>
@Override
public Response toResponse(RuntimeException runtimeException) {
- getLogger().info("Handling runtimeException {}",
runtimeException.getMessage());
+ LOGGER.info("Handling runtimeException {}", runtimeException.getMessage());
int responseCode = mapExceptionToResponseCode(runtimeException);
- getLogger()
- .atLevel(responseCode >= 500 ? Level.INFO : Level.DEBUG)
- .log("Full RuntimeException", runtimeException);
-
- if (responseCode == Response.Status.INTERNAL_SERVER_ERROR.getStatusCode())
{
- getLogger().error("Unhandled exception returning INTERNAL_SERVER_ERROR",
runtimeException);
+ if (responseCode == Status.INTERNAL_SERVER_ERROR.getStatusCode()) {
+ getLoggerForExceptionLogging()
+ .error("Unhandled exception returning INTERNAL_SERVER_ERROR",
runtimeException);
+ } else {
+ getLoggerForExceptionLogging()
+ .atLevel(responseCode > 500 ? Level.INFO : Level.DEBUG)
+ .log("Full RuntimeException", runtimeException);
}
ErrorResponse icebergErrorResponse =
@@ -111,7 +112,7 @@ public class IcebergExceptionMapper implements
ExceptionMapper<RuntimeException>
.entity(icebergErrorResponse)
.type(MediaType.APPLICATION_JSON_TYPE)
.build();
- getLogger().debug("Mapped exception to errorResp: {}", errorResp);
+ LOGGER.debug("Mapped exception to errorResp: {}", errorResp);
return errorResp;
}
@@ -261,8 +262,12 @@ public class IcebergExceptionMapper implements
ExceptionMapper<RuntimeException>
return Optional.of(Status.INTERNAL_SERVER_ERROR.getStatusCode());
}
+ /**
+ * This function is only present for the {@code ExceptionMapperTest} and
must only be used once
+ * during any execution of {@link #toResponse(RuntimeException)}.
+ */
@VisibleForTesting
- Logger getLogger() {
+ Logger getLoggerForExceptionLogging() {
return LOGGER;
}
}
diff --git
a/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergJsonProcessingExceptionMapper.java
b/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergJsonProcessingExceptionMapper.java
index 10130700c..7d9674f90 100644
---
a/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergJsonProcessingExceptionMapper.java
+++
b/runtime/service/src/main/java/org/apache/polaris/service/exception/IcebergJsonProcessingExceptionMapper.java
@@ -61,7 +61,7 @@ public class IcebergJsonProcessingExceptionMapper
if (exception instanceof JsonGenerationException
|| exception instanceof InvalidDefinitionException) {
long id = ThreadLocalRandom.current().nextLong();
- getLogger()
+ getLoggerForExceptionLogging()
.error(String.format(Locale.ROOT, "Error handling a request: %016x",
id), exception);
String message =
String.format(
@@ -77,8 +77,8 @@ public class IcebergJsonProcessingExceptionMapper
/*
* Otherwise, it's those pesky users.
*/
- getLogger().info("Unable to process JSON: {}", exception.getMessage());
- getLogger().debug("Full JsonProcessingException", exception);
+ LOGGER.info("Unable to process JSON: {}", exception.getMessage());
+ getLoggerForExceptionLogging().debug("Full JsonProcessingException",
exception);
String messagePrefix =
switch (exception) {
@@ -99,8 +99,12 @@ public class IcebergJsonProcessingExceptionMapper
.build();
}
+ /**
+ * This function is only present for the {@code ExceptionMapperTest} and
must only be used during
+ * once any execution of {@link #toResponse(JsonProcessingException)}.
+ */
@VisibleForTesting
- Logger getLogger() {
+ Logger getLoggerForExceptionLogging() {
return LOGGER;
}
}
diff --git
a/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java
b/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java
index 7ba2bd3e6..f37d34db5 100644
---
a/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java
+++
b/runtime/service/src/test/java/org/apache/polaris/service/exception/ExceptionMapperTest.java
@@ -18,21 +18,25 @@
*/
package org.apache.polaris.service.exception;
+import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.ArgumentMatchers.argThat;
+import static org.mockito.ArgumentMatchers.assertArg;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
+import com.fasterxml.jackson.core.JsonGenerationException;
import com.fasterxml.jackson.core.JsonLocation;
import com.fasterxml.jackson.core.JsonProcessingException;
+import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.core.Response;
import jakarta.ws.rs.ext.ExceptionMapper;
+import java.io.IOException;
import java.util.Optional;
import java.util.stream.Stream;
+import org.apache.iceberg.exceptions.RuntimeIOException;
import org.apache.polaris.core.exceptions.AlreadyExistsException;
import org.apache.polaris.core.exceptions.CommitConflictException;
-import org.assertj.core.api.Assertions;
import org.jboss.logmanager.Level;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
@@ -67,27 +71,25 @@ public class ExceptionMapperTest {
verify(JBOSS_LOGGER)
.logRaw(
- argThat(
+ assertArg(
record -> {
- if (record.getLevel() != level) {
- return false;
- }
+ assertThat(record.getLevel()).isEqualTo(level);
String message = record.getMessage();
if (message == null) {
- return false;
+ return;
}
Throwable error = record.getThrown();
if (error == null) {
- return false;
+ return;
}
- return message.contains(CAUSE)
- || Optional.ofNullable(error.getCause())
- .map(Throwable::getMessage)
- .orElse("")
- .contains(CAUSE);
+ assertThat(
+ Optional.ofNullable(error.getCause())
+ .map(Throwable::getMessage)
+ .orElse(message))
+ .contains(CAUSE);
}));
}
@@ -95,7 +97,7 @@ public class ExceptionMapperTest {
public void testNamespaceException() {
PolarisExceptionMapper mapper = new PolarisExceptionMapper();
Response response = mapper.toResponse(new CommitConflictException("test"));
- Assertions.assertThat(response.getStatus()).isEqualTo(409);
+ assertThat(response.getStatus()).isEqualTo(409);
}
static Stream<Arguments> testFullExceptionIsLogged() {
@@ -105,21 +107,57 @@ public class ExceptionMapperTest {
Arguments.of(
new IcebergExceptionMapper() {
@Override
- Logger getLogger() {
+ Logger getLoggerForExceptionLogging() {
return new Slf4jLogger(JBOSS_LOGGER);
}
},
new RuntimeException(MESSAGE, new RuntimeException(CAUSE)),
Level.ERROR),
+ Arguments.of(
+ new IcebergExceptionMapper() {
+ @Override
+ Logger getLoggerForExceptionLogging() {
+ return new Slf4jLogger(JBOSS_LOGGER);
+ }
+ },
+ new RuntimeIOException(new IOException(new
RuntimeException(CAUSE)), "%s", MESSAGE),
+ Level.INFO),
+ Arguments.of(
+ new IcebergExceptionMapper() {
+ @Override
+ Logger getLoggerForExceptionLogging() {
+ return new Slf4jLogger(JBOSS_LOGGER);
+ }
+ },
+ new ForbiddenException(MESSAGE, new RuntimeException(CAUSE)),
+ Level.DEBUG),
Arguments.of(
new IcebergJsonProcessingExceptionMapper() {
@Override
- Logger getLogger() {
+ Logger getLoggerForExceptionLogging() {
return new Slf4jLogger(JBOSS_LOGGER);
}
},
new TestJsonProcessingException(MESSAGE, null, new
RuntimeException(CAUSE)),
Level.DEBUG),
+ Arguments.of(
+ new IcebergJsonProcessingExceptionMapper() {
+ @Override
+ Logger getLoggerForExceptionLogging() {
+ return new Slf4jLogger(JBOSS_LOGGER);
+ }
+ },
+ new TestJsonProcessingException(MESSAGE, null, new
RuntimeException(CAUSE)),
+ Level.DEBUG),
+ Arguments.of(
+ new IcebergJsonProcessingExceptionMapper() {
+ @Override
+ Logger getLoggerForExceptionLogging() {
+ return new Slf4jLogger(JBOSS_LOGGER);
+ }
+ },
+ new JsonGenerationException(MESSAGE, new RuntimeException(CAUSE),
null),
+ Level.ERROR),
Arguments.of(
new PolarisExceptionMapper() {
@Override
