Recognize nested converters in alwaysWriteExceptions (#3920)

Author: ppkarwasz

log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/PatternParserTest.java

@@ -44,6 +44,7 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 class PatternParserTest {
 
@@ -93,6 +94,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<PatternFormatter> 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<PatternFormatter> 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<PatternFormatter> 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
      */
@@ -103,7 +148,7 @@ void testCustomPattern() {
         final StringMap mdc = ContextDataFactory.createContextData();
         mdc.putValue("loginId", "Fred");
         // The line number of the Throwable definition
-        final int nextLineNumber = 107;
+        final int nextLineNumber = 152; // Adjust this when the code above changes!
         final Throwable t = new Throwable();
         final StackTraceElement[] elements = t.getStackTrace();
         final Log4jLogEvent event = Log4jLogEvent.newBuilder() //

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);
+    }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/EncodingPatternConverter.java

@@ -54,10 +54,9 @@ private EncodingPatternConverter(final List<PatternFormatter> 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);
     }
 
     /**

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);
+    }
 }

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);
     }
 }

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}.
+     *
      * <p>
-     * 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.
      * </p>
      *
-     * @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;

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);
+    }
 }

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);
+    }
 }

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);
     }
 
     /**

log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/VariablesNotEmptyReplacementConverter.java

@@ -119,4 +119,11 @@ private static boolean sequenceRegionMatches(
         }
         return true;
     }
+
+    @Override
+    public boolean handlesThrowable() {
+        return formatters.stream()
+                .map(PatternFormatter::getConverter)
+                .anyMatch(LogEventPatternConverter::handlesThrowable);
+    }
 }