ppkarwasz commented on code in PR #2393:
URL: https://github.com/apache/logging-log4j2/pull/2393#discussion_r1535235106
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/message/ParameterizedMessageTest.java:
##########
@@ -185,16 +185,73 @@ void testSerializable(final Object arg) {
assertThat(actual.getFormattedMessage()).isEqualTo(expected.getFormattedMessage());
}
+ @Test
+ void formatToWithMoreArgsButNoWarn() {
+ final String pattern = "no warn {} {}";
+ final String expectedMessage = "no warn a b";
+ final Object[] args = {"a", "b", new RuntimeException()};
+ final ParameterizedMessage message = new ParameterizedMessage(pattern,
args);
+ final StringBuilder buffer = new StringBuilder();
+ message.formatTo(buffer);
+ assertThat(buffer.toString()).isEqualTo(expectedMessage);
+
assertThat(message.getThrowable()).isInstanceOf(RuntimeException.class);
+ final List<StatusData> statusDataList =
statusListener.getStatusData().collect(Collectors.toList());
+ assertThat(statusDataList).hasSize(0);
+ }
+
+ @Test
+ void formatWithMoreArgsButNoWarn() {
+ final String pattern = "no warn {} {}";
+ final String expectedMessage = "no warn a b";
+ final Object[] args = {"a", "b", new RuntimeException()};
+ final String message = ParameterizedMessage.format(pattern, args);
+ assertThat(message).isEqualTo(expectedMessage);
+ final List<StatusData> statusDataList =
statusListener.getStatusData().collect(Collectors.toList());
+ assertThat(statusDataList).hasSize(0);
+ }
+
+ /**
+ * In this test cases, constructed the following scenarios: <br>
+ * <p>
+ * 1. The placeholders are greater than the count of arguments. <br>
+ * 2. The placeholders are less than the count of arguments. <br>
+ * 3. The arguments contains an exception, and the placeholder is greater
than normal arguments. <br>
+ * 4. The arguments contains an exception, and the placeholder is less
than the arguments.<br>
+ * All of these should logged in status logger with WARN level.
+ * </p>
+ *
+ * @return streams
+ */
static Stream<Object[]> testCasesForInsufficientFormatArgs() {
- return Stream.of(new Object[] {1, "foo {}"}, new Object[] {2, "bar
{}{}"});
+ return Stream.of(
+ new Object[] {"more {} {}", 2, new Object[] {"a"}, "more a
{}"},
+ new Object[] {"more {} {} {}", 3, new Object[] {"a"}, "more a
{} {}"},
+ new Object[] {"less {}", 1, new Object[] {"a", "b"}, "less a"},
+ new Object[] {"less {} {}", 2, new Object[] {"a", "b", "c"},
"less a b"},
+ new Object[] {
+ "more throwable {} {}",
+ 2,
+ new Object[] {"a", new RuntimeException()},
+ "more throwable a java.lang.RuntimeException"
+ },
Review Comment:
This case should issue no warning: the `RuntimeException` is simply
classified as normal argument and it fills the second placeholder.
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java:
##########
@@ -240,12 +244,14 @@ static void formatMessage(
return;
}
- // Fail if there are insufficient arguments
- if (analysis.placeholderCount > args.length) {
+ // check if there are insufficient arguments that do not include
Throwable arg
+ final int realArgCount = args.length;
+ final int noThrowableArgCount = realArgCount - ((args[realArgCount -
1] instanceof Throwable) ? 1 : 0);
+ if (analysis.placeholderCount != noThrowableArgCount) {
final String message = String.format(
"found %d argument placeholders, but provided %d for
pattern `%s`",
- analysis.placeholderCount, args.length, pattern);
- throw new IllegalArgumentException(message);
+ analysis.placeholderCount, realArgCount, pattern);
+ STATUS_LOGGER.warn(message);
Review Comment:
```suggestion
STATUS_LOGGER.warn("Found {} argument placeholders, but only {}
arguments were provided for pattern `{}`",
analysis.placeholderCount,
realArgCount < analysis.placeholderCount ? realArgCount
: noThrowableArgCount,
pattern);
```
We are bound to give a good example and use parameterized logging in our
code. :wink:
##########
log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java:
##########
@@ -240,12 +244,14 @@ static void formatMessage(
return;
}
- // Fail if there are insufficient arguments
- if (analysis.placeholderCount > args.length) {
+ // check if there are insufficient arguments that do not include
Throwable arg
+ final int realArgCount = args.length;
+ final int noThrowableArgCount = realArgCount - ((args[realArgCount -
1] instanceof Throwable) ? 1 : 0);
+ if (analysis.placeholderCount != noThrowableArgCount) {
Review Comment:
As stated by Ralph in [this
comment](https://github.com/apache/logging-log4j2/issues/2363#issuecomment-1996359179):
>Only when the `Throwable` is "extra" should it be logged as a `Throwable`.
Therefore we should issue a warning if:
- `realArgCount < analysis.placeholderCount`: even if we count the
`Throwable` as "normal" argument, there is no way to fill all the placeholders,
- **or** `analysis.placeholderCount < noThrowableArgCount`: even if we count
the `Throwable` as "extra" argument, there are some arguments left.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
