Don't fail on insufficient parameters in ParameterFormatter (#2337, #…

…2343)
apache · Mar 6, 2024 · 0eb232f · 0eb232f
1 parent 32075af
commit 0eb232f
Show file tree

Hide file tree

Showing 7 changed files with 131 additions and 26 deletions.
diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusLoggerMock.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/UsingStatusLoggerMock.java
@@ -24,6 +24,7 @@
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.parallel.ResourceLock;
 
 /**
  * Shortcut to {@link StatusLoggerMockExtension}.
@@ -32,4 +33,5 @@
 @Target({TYPE, METHOD})
 @Documented
 @ExtendWith({ExtensionContextAnchor.class, StatusLoggerMockExtension.class})
+@ResourceLock("log4j2.StatusLogger")
 public @interface UsingStatusLoggerMock {}
diff --git a/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java b/log4j-api-test/src/main/java/org/apache/logging/log4j/test/junit/package-info.java
@@ -15,7 +15,7 @@
  * limitations under the license.
  */
 @Export
-@Version("2.23.0")
+@Version("2.23.1")
 package org.apache.logging.log4j.test.junit;
 
 import org.osgi.annotation.bundle.Export;

diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java
@@ -17,12 +17,14 @@
 package org.apache.logging.log4j.message;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import org.apache.logging.log4j.message.ParameterFormatter.MessagePatternAnalysis;
+import org.apache.logging.log4j.test.junit.UsingStatusLoggerMock;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
@@ -31,7 +33,8 @@
 /**
  * Tests {@link ParameterFormatter}.
  */
-public class ParameterFormatterTest {
+@UsingStatusLoggerMock
+class ParameterFormatterTest {
 
     @ParameterizedTest
     @CsvSource({
@@ -49,7 +52,7 @@ public class ParameterFormatterTest {
         "4,0:2:4:10,false,{}{}{}a{]b{}",
         "5,0:2:4:7:10,false,{}{}{}a{}b{}"
     })
-    public void test_pattern_analysis(
+    void test_pattern_analysis(
             final int placeholderCount,
             final String placeholderCharIndicesString,
             final boolean escapedPlaceholderFound,
@@ -65,14 +68,22 @@ public void test_pattern_analysis(
         }
     }
 
+    @ParameterizedTest
+    @CsvSource({"1,foo {}", "2,bar {}{}"})
+    void format_should_fail_on_insufficient_args(final int placeholderCount, final String pattern) {
+        final int argCount = placeholderCount - 1;
+        assertThatThrownBy(() -> ParameterFormatter.format(pattern, new Object[argCount], argCount))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage(
+                        "found %d argument placeholders, but provided %d for pattern `%s`",
+                        placeholderCount, argCount, pattern);
+    }
+
     @ParameterizedTest
     @MethodSource("messageFormattingTestCases")
-    void assertMessageFormatting(
+    void format_should_work(
             final String pattern, final Object[] args, final int argCount, final String expectedFormattedMessage) {
-        MessagePatternAnalysis analysis = ParameterFormatter.analyzePattern(pattern, -1);
-        final StringBuilder buffer = new StringBuilder();
-        ParameterFormatter.formatMessage(buffer, pattern, args, argCount, analysis);
-        String actualFormattedMessage = buffer.toString();
+        final String actualFormattedMessage = ParameterFormatter.format(pattern, args, argCount);
         assertThat(actualFormattedMessage).isEqualTo(expectedFormattedMessage);
     }
 

diff --git a/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java b/log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java
@@ -19,17 +19,31 @@
 import static org.assertj.core.api.Assertions.assertThat;
 
 import java.math.BigDecimal;
+import java.util.List;
+import java.util.function.Supplier;
+import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.status.StatusData;
+import org.apache.logging.log4j.test.ListStatusListener;
 import org.apache.logging.log4j.test.junit.Mutable;
 import org.apache.logging.log4j.test.junit.SerialUtil;
+import org.apache.logging.log4j.test.junit.UsingStatusListener;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
 
-public class ParameterizedMessageTest {
+@UsingStatusListener
+class ParameterizedMessageTest {
+
+    final ListStatusListener statusListener;
+
+    ParameterizedMessageTest(ListStatusListener statusListener) {
+        this.statusListener = statusListener;
+    }
 
     @Test
-    public void testNoArgs() {
+    void testNoArgs() {
         final String testMsg = "Test message {}";
         ParameterizedMessage msg = new ParameterizedMessage(testMsg, (Object[]) null);
         String result = msg.getFormattedMessage();
@@ -41,7 +55,7 @@ public void testNoArgs() {
     }
 
     @Test
-    public void testZeroLength() {
+    void testZeroLength() {
         final String testMsg = "";
         ParameterizedMessage msg = new ParameterizedMessage(testMsg, new Object[] {"arg"});
         String result = msg.getFormattedMessage();
@@ -53,7 +67,7 @@ public void testZeroLength() {
     }
 
     @Test
-    public void testOneCharLength() {
+    void testOneCharLength() {
         final String testMsg = "d";
         ParameterizedMessage msg = new ParameterizedMessage(testMsg, new Object[] {"arg"});
         String result = msg.getFormattedMessage();
@@ -65,71 +79,71 @@ public void testOneCharLength() {
     }
 
     @Test
-    public void testFormat3StringArgs() {
+    void testFormat3StringArgs() {
         final String testMsg = "Test message {}{} {}";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message ab c");
     }
 
     @Test
-    public void testFormatNullArgs() {
+    void testFormatNullArgs() {
         final String testMsg = "Test message {} {} {} {} {} {}";
         final String[] args = {"a", null, "c", null, null, null};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message a null c null null null");
     }
 
     @Test
-    public void testFormatStringArgsIgnoresSuperfluousArgs() {
+    void testFormatStringArgsIgnoresSuperfluousArgs() {
         final String testMsg = "Test message {}{} {}";
         final String[] args = {"a", "b", "c", "unnecessary", "superfluous"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message ab c");
     }
 
     @Test
-    public void testFormatStringArgsWithEscape() {
+    void testFormatStringArgsWithEscape() {
         final String testMsg = "Test message \\{}{} {}";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message {}a b");
     }
 
     @Test
-    public void testFormatStringArgsWithTrailingEscape() {
+    void testFormatStringArgsWithTrailingEscape() {
         final String testMsg = "Test message {}{} {}\\";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message ab c\\");
     }
 
     @Test
-    public void testFormatStringArgsWithTrailingText() {
+    void testFormatStringArgsWithTrailingText() {
         final String testMsg = "Test message {}{} {}Text";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message ab cText");
     }
 
     @Test
-    public void testFormatStringArgsWithTrailingEscapedEscape() {
+    void testFormatStringArgsWithTrailingEscapedEscape() {
         final String testMsg = "Test message {}{} {}\\\\";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message ab c\\");
     }
 
     @Test
-    public void testFormatStringArgsWithEscapedEscape() {
+    void testFormatStringArgsWithEscapedEscape() {
         final String testMsg = "Test message \\\\{}{} {}";
         final String[] args = {"a", "b", "c"};
         final String result = ParameterizedMessage.format(testMsg, args);
         assertThat(result).isEqualTo("Test message \\ab c");
     }
 
     @Test
-    public void testSafeWithMutableParams() { // LOG4J2-763
+    void testSafeWithMutableParams() { // LOG4J2-763
         final String testMsg = "Test message {}";
         final Mutable param = new Mutable().set("abc");
         final ParameterizedMessage msg = new ParameterizedMessage(testMsg, param);
@@ -170,4 +184,51 @@ void testSerializable(final Object arg) {
         assertThat(actual).isInstanceOf(ParameterizedMessage.class);
         assertThat(actual.getFormattedMessage()).isEqualTo(expected.getFormattedMessage());
     }
+
+    static Stream<Object[]> testCasesForInsufficientFormatArgs() {
+        return Stream.of(new Object[] {1, "foo {}"}, new Object[] {2, "bar {}{}"});
+    }
+
+    @ParameterizedTest
+    @MethodSource("testCasesForInsufficientFormatArgs")
+    void formatTo_should_fail_on_insufficient_args(final int placeholderCount, final String pattern) {
+        final int argCount = placeholderCount - 1;
+        verifyFormattingFailureOnInsufficientArgs(placeholderCount, pattern, argCount, () -> {
+            final ParameterizedMessage message = new ParameterizedMessage(pattern, new Object[argCount]);
+            final StringBuilder buffer = new StringBuilder();
+            message.formatTo(buffer);
+            return buffer.toString();
+        });
+    }
+
+    @ParameterizedTest
+    @MethodSource("testCasesForInsufficientFormatArgs")
+    void format_should_fail_on_insufficient_args(final int placeholderCount, final String pattern) {
+        final int argCount = placeholderCount - 1;
+        verifyFormattingFailureOnInsufficientArgs(
+                placeholderCount, pattern, argCount, () -> ParameterizedMessage.format(pattern, new Object[argCount]));
+    }
+
+    private void verifyFormattingFailureOnInsufficientArgs(
+            final int placeholderCount,
+            final String pattern,
+            final int argCount,
+            final Supplier<String> formattedMessageSupplier) {
+
+        // Verify the formatted message
+        final String formattedMessage = formattedMessageSupplier.get();
+        assertThat(formattedMessage).isEqualTo(pattern);
+
+        // Verify the logged failure
+        final List<StatusData> statusDataList = statusListener.getStatusData().collect(Collectors.toList());
+        assertThat(statusDataList).hasSize(1);
+        final StatusData statusData = statusDataList.get(0);
+        assertThat(statusData.getLevel()).isEqualTo(Level.ERROR);
+        assertThat(statusData.getMessage().getFormattedMessage()).isEqualTo("Unable to format msg: %s", pattern);
+        assertThat(statusData.getThrowable())
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage(
+                        "found %d argument placeholders, but provided %d for pattern `%s`",
+                        placeholderCount, argCount, pattern);
+    }
 }
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
@@ -205,11 +205,12 @@ else if (placeholderCount >= placeholderCharIndices.length) {
     }
 
     /**
-     * Format the following pattern using provided arguments.
+     * Format the given pattern using provided arguments.
      *
      * @param pattern a formatting pattern
      * @param args arguments to be formatted
      * @return the formatted message
+     * @throws IllegalArgumentException on invalid input
      */
     static String format(final String pattern, final Object[] args, int argCount) {
         final StringBuilder result = new StringBuilder();
@@ -218,6 +219,14 @@ static String format(final String pattern, final Object[] args, int argCount) {
         return result.toString();
     }
 
+    /**
+     * Format the given pattern using provided arguments into the buffer pointed.
+     *
+     * @param buffer a buffer the formatted output will be written to
+     * @param pattern a formatting pattern
+     * @param args arguments to be formatted
+     * @throws IllegalArgumentException on invalid input
+     */
     static void formatMessage(
             final StringBuilder buffer,
             final String pattern,
@@ -250,7 +259,7 @@ static void formatMessage(
         }
     }
 
-    static void formatMessageContainingNoEscapes(
+    private static void formatMessageContainingNoEscapes(
             final StringBuilder buffer,
             final String pattern,
             final Object[] args,
@@ -271,7 +280,7 @@ static void formatMessageContainingNoEscapes(
         buffer.append(pattern, precedingTextStartIndex, pattern.length());
     }
 
-    static void formatMessageContainingEscapes(
+    private static void formatMessageContainingEscapes(
             final StringBuilder buffer,
             final String pattern,
             final Object[] args,

diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterizedMessage.java
@@ -26,7 +26,9 @@
 import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Objects;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterFormatter.MessagePatternAnalysis;
+import org.apache.logging.log4j.status.StatusLogger;
 import org.apache.logging.log4j.util.Constants;
 import org.apache.logging.log4j.util.StringBuilderFormattable;
 import org.apache.logging.log4j.util.internal.SerializationUtil;
@@ -79,6 +81,8 @@ public class ParameterizedMessage implements Message, StringBuilderFormattable {
 
     private static final long serialVersionUID = -665975803997290697L;
 
+    private static final Logger STATUS_LOGGER = StatusLogger.getLogger();
+
     private static final ThreadLocal<FormatBufferHolder> FORMAT_BUFFER_HOLDER_REF =
             Constants.ENABLE_THREADLOCALS ? ThreadLocal.withInitial(FormatBufferHolder::new) : null;
 
@@ -274,7 +278,12 @@ public void formatTo(final StringBuilder buffer) {
             buffer.append(formattedMessage);
         } else {
             final int argCount = args != null ? args.length : 0;
-            ParameterFormatter.formatMessage(buffer, pattern, args, argCount, patternAnalysis);
+            try {
+                ParameterFormatter.formatMessage(buffer, pattern, args, argCount, patternAnalysis);
+            } catch (final Exception error) {
+                STATUS_LOGGER.error("Unable to format msg: {}", pattern, error);
+                buffer.append(pattern);
+            }
         }
     }
 
@@ -285,7 +294,12 @@ public void formatTo(final StringBuilder buffer) {
      */
     public static String format(final String pattern, final Object[] args) {
         final int argCount = args != null ? args.length : 0;
-        return ParameterFormatter.format(pattern, args, argCount);
+        try {
+            return ParameterFormatter.format(pattern, args, argCount);
+        } catch (final Exception error) {
+            STATUS_LOGGER.error("Unable to format msg: {}", pattern, error);
+            return pattern;
+        }
     }
 
     @Override

diff --git a/src/changelog/.2.x.x/fix_ParameterFormatter_for_insufficient_args.xml b/src/changelog/.2.x.x/fix_ParameterFormatter_for_insufficient_args.xml
@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="http://logging.apache.org/log4j/changelog"
+       xsi:schemaLocation="http://logging.apache.org/log4j/changelog https://logging.apache.org/log4j/changelog-0.1.3.xsd"
+       type="fixed">
+  <issue id="2343" link="https://github.com/apache/logging-log4j2/pull/2343"/>
+  <description format="asciidoc">Fix that parameterized message formatting doesn't throw an exception when there are insufficient number of parameters</description>
+</entry>