This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 26db660441216ab1cd115257933629cad952130a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Thu Sep 1 00:31:34 2022 +0200 [LOG4J2-3585] Fix throwable logging An inverted condition prevented throwables to be added as additional arguments. --- .../java/org/apache/logging/slf4j/Log4jLogger.java | 11 +++--- .../java/org/apache/logging/slf4j/LoggerTest.java | 44 +++++++++++++++++++++- .../src/test/resources/log4j-test1.xml | 2 + .../java/org/apache/logging/slf4j/Log4jLogger.java | 10 ++--- .../java/org/apache/logging/slf4j/LoggerTest.java | 42 ++++++++++++++++++++- .../src/test/resources/log4j-test1.xml | 2 + 6 files changed, 98 insertions(+), 13 deletions(-) diff --git a/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java b/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java index 1fa808065d..9b970d4a68 100644 --- a/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java +++ b/log4j-slf4j-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java @@ -355,7 +355,7 @@ public class Log4jLogger implements LocationAwareLogger, Serializable { } @Override - public void log(final Marker marker, final String fqcn, final int level, final String message, final Object[] params, Throwable throwable) { + public void log(final Marker marker, final String fqcn, final int level, final String message, final Object[] params, final Throwable throwable) { final Level log4jLevel = getLevel(level); final org.apache.logging.log4j.Marker log4jMarker = getMarker(marker); @@ -363,17 +363,18 @@ public class Log4jLogger implements LocationAwareLogger, Serializable { return; } final Message msg; + final Throwable actualThrowable; if (CONVERTER != null && eventLogger && marker != null && marker.contains(EVENT_MARKER)) { msg = CONVERTER.convertEvent(message, params, throwable); + actualThrowable = throwable != null ? throwable : msg.getThrowable(); } else if (params == null) { msg = new SimpleMessage(message); + actualThrowable = throwable; } else { msg = new ParameterizedMessage(message, params, throwable); - if (throwable != null) { - throwable = msg.getThrowable(); - } + actualThrowable = throwable != null ? throwable : msg.getThrowable(); } - logger.logMessage(fqcn, log4jLevel, log4jMarker, msg, throwable); + logger.logMessage(fqcn, log4jLevel, log4jMarker, msg, actualThrowable); } private static org.apache.logging.log4j.Marker getMarker(final Marker marker) { diff --git a/log4j-slf4j-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java b/log4j-slf4j-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java index 935131dde4..2239dc05df 100644 --- a/log4j-slf4j-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java +++ b/log4j-slf4j-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java @@ -23,8 +23,9 @@ import static org.junit.Assert.assertTrue; import java.util.List; import java.util.Locale; -import org.apache.logging.log4j.core.test.junit.LoggerContextRule; +import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.core.test.appender.ListAppender; +import org.apache.logging.log4j.core.test.junit.LoggerContextRule; import org.apache.logging.log4j.util.Strings; import org.junit.After; import org.junit.Before; @@ -162,9 +163,38 @@ public class LoggerTest { verify("EventLogger", "o.a.l.s.LoggerTest Transfer [Audit@18060 Amount=\"200.00\" FromAccount=\"123457\" ToAccount=\"123456\"] Transfer Complete" + Strings.LINE_SEPARATOR); } - private void verify(final String name, final String expected) { + @Test + public void testThrowable() { + final Throwable expected = new RuntimeException(); + logger.debug("Hello {}", expected); + verifyThrowable(expected); + logger.debug("Hello {}", (Object) expected); + verifyThrowable(null); + logger.debug("Hello", expected); + verifyThrowable(expected); + logger.debug("Hello {}! {}", "World!", expected); + verifyThrowable(null); + logger.debug("Hello {}!", "World!", expected); + verifyThrowable(expected); + final LocationAwareLogger lal = (LocationAwareLogger) logger; + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", null, expected); + verifyThrowable(expected); + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", new Object[] { expected }, + null); + verifyThrowable(null); + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", + new Object[] { "World!", expected }, null); + verifyThrowable(expected); + } + + private ListAppender getAppenderByName(final String name) { final ListAppender listApp = ctx.getListAppender(name); assertNotNull("Missing Appender", listApp); + return listApp; + } + + private void verify(final String name, final String expected) { + final ListAppender listApp = getAppenderByName(name); final List<String> events = listApp.getMessages(); assertTrue("Incorrect number of messages. Expected 1 Actual " + events.size(), events.size()== 1); final String actual = events.get(0); @@ -172,11 +202,21 @@ public class LoggerTest { listApp.clear(); } + private void verifyThrowable(final Throwable expected) { + final ListAppender listApp = getAppenderByName("UnformattedList"); + final List<LogEvent> events = listApp.getEvents(); + assertEquals("Incorrect number of messages", 1, events.size()); + final LogEvent actual = events.get(0); + assertEquals("Incorrect throwable.", expected, actual.getThrown()); + listApp.clear(); + } + @Before @After public void cleanup() { MDC.clear(); ctx.getListAppender("List").clear(); + ctx.getListAppender("UnformattedList").clear(); ctx.getListAppender("EventLogger").clear(); } } diff --git a/log4j-slf4j-impl/src/test/resources/log4j-test1.xml b/log4j-slf4j-impl/src/test/resources/log4j-test1.xml index a64bdfa905..1ba09ca893 100644 --- a/log4j-slf4j-impl/src/test/resources/log4j-test1.xml +++ b/log4j-slf4j-impl/src/test/resources/log4j-test1.xml @@ -20,6 +20,7 @@ <List name="List"> <PatternLayout pattern="%C{1.} %m MDC%X%n%ex{0}"/> </List> + <List name="UnformattedList" /> <SLF4J name="SLF4J"/> </Appenders> @@ -34,6 +35,7 @@ <Root level="trace"> <AppenderRef ref="List"/> + <AppenderRef ref="UnformattedList"/> </Root> </Loggers> diff --git a/log4j-slf4j20-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java b/log4j-slf4j20-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java index 10ad49ca7e..fd42f361fc 100644 --- a/log4j-slf4j20-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java +++ b/log4j-slf4j20-impl/src/main/java/org/apache/logging/slf4j/Log4jLogger.java @@ -349,7 +349,7 @@ public class Log4jLogger implements LocationAwareLogger, Serializable { } @Override - public void log(final Marker marker, final String fqcn, final int level, final String message, final Object[] params, Throwable throwable) { + public void log(final Marker marker, final String fqcn, final int level, final String message, final Object[] params, final Throwable throwable) { final Level log4jLevel = getLevel(level); final org.apache.logging.log4j.Marker log4jMarker = getMarker(marker); @@ -357,15 +357,15 @@ public class Log4jLogger implements LocationAwareLogger, Serializable { return; } final Message msg; + final Throwable actualThrowable; if (params == null) { msg = new SimpleMessage(message); + actualThrowable = throwable; } else { msg = new ParameterizedMessage(message, params, throwable); - if (throwable != null) { - throwable = msg.getThrowable(); - } + actualThrowable = throwable != null ? throwable : msg.getThrowable(); } - logger.logMessage(fqcn, log4jLevel, log4jMarker, msg, throwable); + logger.logMessage(fqcn, log4jLevel, log4jMarker, msg, actualThrowable); } private org.apache.logging.log4j.Marker getMarker(final Marker marker) { diff --git a/log4j-slf4j20-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java b/log4j-slf4j20-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java index 7c8e71379f..1701bbcbc7 100644 --- a/log4j-slf4j20-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java +++ b/log4j-slf4j20-impl/src/test/java/org/apache/logging/slf4j/LoggerTest.java @@ -22,6 +22,7 @@ import static org.junit.Assert.assertTrue; import java.util.List; +import org.apache.logging.log4j.core.LogEvent; import org.apache.logging.log4j.junit.LoggerContextRule; import org.apache.logging.log4j.test.appender.ListAppender; import org.apache.logging.log4j.util.Strings; @@ -142,9 +143,38 @@ public class LoggerTest { verify("List", "o.a.l.s.LoggerTest Hello, Log4j {} MDC{}" + Strings.LINE_SEPARATOR); } - private void verify(final String name, final String expected) { + @Test + public void testThrowable() { + final Throwable expected = new RuntimeException(); + logger.debug("Hello {}", expected); + verifyThrowable(expected); + logger.debug("Hello {}", (Object) expected); + verifyThrowable(null); + logger.debug("Hello", expected); + verifyThrowable(expected); + logger.debug("Hello {}! {}", "World!", expected); + verifyThrowable(null); + logger.debug("Hello {}!", "World!", expected); + verifyThrowable(expected); + final LocationAwareLogger lal = (LocationAwareLogger) logger; + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", null, expected); + verifyThrowable(expected); + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", new Object[] { expected }, + null); + verifyThrowable(null); + lal.log(null, LoggerTest.class.getName(), LocationAwareLogger.DEBUG_INT, "Hello {}", + new Object[] { "World!", expected }, null); + verifyThrowable(expected); + } + + private ListAppender getAppenderByName(final String name) { final ListAppender listApp = ctx.getListAppender(name); assertNotNull("Missing Appender", listApp); + return listApp; + } + + private void verify(final String name, final String expected) { + final ListAppender listApp = getAppenderByName(name); final List<String> events = listApp.getMessages(); assertTrue("Incorrect number of messages. Expected 1 Actual " + events.size(), events.size()== 1); final String actual = events.get(0); @@ -152,10 +182,20 @@ public class LoggerTest { listApp.clear(); } + private void verifyThrowable(final Throwable expected) { + final ListAppender listApp = getAppenderByName("UnformattedList"); + final List<LogEvent> events = listApp.getEvents(); + assertEquals("Incorrect number of messages", 1, events.size()); + final LogEvent actual = events.get(0); + assertEquals("Incorrect throwable.", expected, actual.getThrown()); + listApp.clear(); + } + @Before @After public void cleanup() { MDC.clear(); ctx.getListAppender("List").clear(); + ctx.getListAppender("UnformattedList").clear(); } } diff --git a/log4j-slf4j20-impl/src/test/resources/log4j-test1.xml b/log4j-slf4j20-impl/src/test/resources/log4j-test1.xml index 07a2be6316..2dee2c1153 100644 --- a/log4j-slf4j20-impl/src/test/resources/log4j-test1.xml +++ b/log4j-slf4j20-impl/src/test/resources/log4j-test1.xml @@ -17,6 +17,7 @@ <List name="List"> <PatternLayout pattern="%C{1.} %m MDC%X%n%ex{0}"/> </List> + <List name="UnformattedList" /> <SLF4J name="SLF4J"/> </Appenders> @@ -27,6 +28,7 @@ <Root level="trace"> <AppenderRef ref="List"/> + <AppenderRef ref="UnformattedList"/> </Root> </Loggers>
