From 433fe04e4b8de7f190ace3d8b986bb5f0edd5346 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sun, 7 Sep 2025 23:13:35 +0200 Subject: [PATCH] fix: recognize nested converters in `alwaysWriteExceptions` `PatternParser` was not detecting when a throwable was already rendered inside a *nested* converter (e.g. `%style{%ex}{red}`). Because `handlesThrowable` was missing for those wrappers, the parser assumed no throwable was present and incorrectly auto-appended another `%ex`, causing the exception to be logged twice. This change implements `handlesThrowable` for the relevant nested converters so that `alwaysWriteExceptions` correctly recognizes them and no duplicate stack traces are produced. --- .../log4j/core/pattern/PatternParserTest.java | 51 ++++++++++++++++++- .../pattern/AbstractStyleNameConverter.java | 7 +++ .../pattern/EncodingPatternConverter.java | 7 ++- .../EqualsBaseReplacementConverter.java | 7 +++ .../core/pattern/HighlightConverter.java | 9 ++-- .../pattern/LogEventPatternConverter.java | 11 ++-- .../core/pattern/MaxLengthConverter.java | 7 +++ .../pattern/RegexReplacementConverter.java | 7 +++ .../log4j/core/pattern/StyleConverter.java | 9 ++-- ...VariablesNotEmptyReplacementConverter.java | 7 +++ .../.2.x.x/3920-nested-throwables.xml | 12 +++++ 11 files changed, 112 insertions(+), 22 deletions(-) create mode 100644 src/changelog/.2.x.x/3920-nested-throwables.xml diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/PatternParserTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/PatternParserTest.java index f493e25bb6a..53602d9ea29 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/PatternParserTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/PatternParserTest.java @@ -16,6 +16,7 @@ */ package org.apache.logging.log4j.core.pattern; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -40,6 +41,8 @@ import org.apache.logging.log4j.util.Strings; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; class PatternParserTest { @@ -89,6 +92,50 @@ void defaultPattern() { validateConverter(formatters, 1, "Line Sep"); } + @ParameterizedTest + @ValueSource(strings = {"%m", "%p", "%c", "%d"}) + void testAlwaysWriteExceptions_appendsThrowable(String pattern) { + // With alwaysWriteExceptions=true, parser should auto-append a %throwable + final List formatters = parser.parse(pattern + "%n", true, false, false); + assertThat(formatters).hasSize(3); + assertThat(formatters.get(2).getConverter()).isInstanceOf(ThrowablePatternConverter.class); + + // With alwaysWriteExceptions=false, parser should leave the pattern unchanged + final List formatters2 = parser.parse(pattern + "%n", false, false, false); + assertThat(formatters2).hasSize(2); + assertThat(formatters2.get(1).getConverter()).isNotInstanceOf(ThrowablePatternConverter.class); + } + + @ParameterizedTest + @ValueSource( + strings = { + "%black{%throwable}", + "%blue{%throwable}", + "%cyan{%throwable}", + "%green{%throwable}", + "%magenta{%throwable}", + "%red{%throwable}", + "%white{%throwable}", + "%yellow{%throwable}", + "%encode{%throwable}{JSON}", + "%equals{%throwable}{java.lang.Throwable}{T}", + "%equalsIgnoreCase{%throwable}{java.lang.Throwable}{T}", + "%highlight{%throwable}", + "%maxLen{%throwable}{1024}", + "%replace{%throwable}{\n}{ }", + "%style{%throwable}{red bold}", + "%notEmpty{%throwable}", + }) + void testAlwaysWriteExceptions_recognizesNestedPatterns(String pattern) { + // With alwaysWriteExceptions=true, parser must detect the nested %throwable + // and NOT auto-append another one at the top level + final List formatters = parser.parse(pattern, true, false, false); + + // Only one top-level formatter is expected (the wrapper itself), not a trailing ThrowablePatternConverter + assertThat(formatters).hasSize(1); + assertThat(formatters.get(0).getConverter()).isNotInstanceOf(ThrowablePatternConverter.class); + } + /** * Test the custom pattern */ @@ -116,8 +163,8 @@ void testCustomPattern() { formatter.format(event, buf); } final String str = buf.toString(); - final String expected = "INFO [PatternParserTest :101 ] - Hello, world" + Strings.LINE_SEPARATOR; - assertTrue(str.endsWith(expected), "Expected to end with: " + expected + ". Actual: " + str); + final String expected = "INFO [PatternParserTest :148 ] - Hello, world" + Strings.LINE_SEPARATOR; + assertThat(str).endsWith(expected); } @Test diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/AbstractStyleNameConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/AbstractStyleNameConverter.java index b7b5e350180..98d69acadec 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/AbstractStyleNameConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/AbstractStyleNameConverter.java @@ -376,4 +376,11 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) { toAppendTo.append(AnsiEscape.getDefaultStyle()); } } + + @Override + public boolean handlesThrowable() { + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); + } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EncodingPatternConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EncodingPatternConverter.java index 970517f69ff..2458e2cb708 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EncodingPatternConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EncodingPatternConverter.java @@ -54,10 +54,9 @@ private EncodingPatternConverter(final List formatters, final @Override public boolean handlesThrowable() { - return formatters != null - && formatters.stream() - .map(PatternFormatter::getConverter) - .anyMatch(LogEventPatternConverter::handlesThrowable); + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); } /** diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EqualsBaseReplacementConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EqualsBaseReplacementConverter.java index 904d4981698..e71edb61476 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EqualsBaseReplacementConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EqualsBaseReplacementConverter.java @@ -99,4 +99,11 @@ void parseSubstitution(final LogEvent event, final StringBuilder substitutionBuf substitutionBuffer.append(substitution); } } + + @Override + public boolean handlesThrowable() { + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); + } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/HighlightConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/HighlightConverter.java index 29663b4f5a8..6f72d2f47ea 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/HighlightConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/HighlightConverter.java @@ -265,11 +265,8 @@ String getLevelStyle(final Level level) { @Override public boolean handlesThrowable() { - for (final PatternFormatter formatter : patternFormatters) { - if (formatter.handlesThrowable()) { - return true; - } - } - return false; + return patternFormatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LogEventPatternConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LogEventPatternConverter.java index 4d4bc2cbc75..4c175abdb49 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LogEventPatternConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/LogEventPatternConverter.java @@ -53,13 +53,16 @@ public void format(final Object obj, final StringBuilder output) { } /** - * Normally pattern converters are not meant to handle Exceptions although few pattern converters might. + * Tests whether this pattern converter is renders a {@link Throwable}. + * *

- * By examining the return values for this method, the containing layout will determine whether it handles - * throwables or not. + * The {@link PatternParser} checks this flag when processing the + * {@code alwaysWriteExceptions} option: if no converter in the pattern handles + * throwables, the parser automatically appends a converter to ensure exceptions are still written. *

* - * @return true if this PatternConverter handles throwables + * @return {@code true} if this converter consumes and renders a {@link Throwable}, + * {@code false} otherwise */ public boolean handlesThrowable() { return false; diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/MaxLengthConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/MaxLengthConverter.java index 1a2805c23b6..432cd4f61fd 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/MaxLengthConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/MaxLengthConverter.java @@ -99,4 +99,11 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) { } } } + + @Override + public boolean handlesThrowable() { + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); + } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/RegexReplacementConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/RegexReplacementConverter.java index 4a21a753bb0..27bd86ada7f 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/RegexReplacementConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/RegexReplacementConverter.java @@ -92,4 +92,11 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) { } toAppendTo.append(pattern.matcher(buf.toString()).replaceAll(substitution)); } + + @Override + public boolean handlesThrowable() { + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); + } } diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/StyleConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/StyleConverter.java index 40267f6155c..d6c0d123d8f 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/StyleConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/StyleConverter.java @@ -129,12 +129,9 @@ public void format(final LogEvent event, final StringBuilder toAppendTo) { @Override public boolean handlesThrowable() { - for (final PatternFormatter formatter : patternFormatters) { - if (formatter.handlesThrowable()) { - return true; - } - } - return false; + return patternFormatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); } /** diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/VariablesNotEmptyReplacementConverter.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/VariablesNotEmptyReplacementConverter.java index e5d2ba9ecfe..9929dc5a2da 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/VariablesNotEmptyReplacementConverter.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/VariablesNotEmptyReplacementConverter.java @@ -118,4 +118,11 @@ private static boolean sequenceRegionMatches( } return true; } + + @Override + public boolean handlesThrowable() { + return formatters.stream() + .map(PatternFormatter::getConverter) + .anyMatch(LogEventPatternConverter::handlesThrowable); + } } diff --git a/src/changelog/.2.x.x/3920-nested-throwables.xml b/src/changelog/.2.x.x/3920-nested-throwables.xml new file mode 100644 index 00000000000..a8c9b13cfe3 --- /dev/null +++ b/src/changelog/.2.x.x/3920-nested-throwables.xml @@ -0,0 +1,12 @@ + + + + + Fix detection of throwable converters inside nested patterns when applying `alwaysWriteExceptions`. + +