Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue #2380: Log messages with insufficient parameters should not throw exception. #2393

Conversation

SeasonPanPan
Copy link
Contributor

Fix issue #2380: Log messages with insufficient parameters should not throw exception.

In this PR, I modified the class org.apache.logging.log4j.message.ParameterFormatter.
I deleted the codes about checking placeholderCount and argCount, if they weren't equal, it would throw an IllegalArgumentException.

At the same time, I also modified the corresponding test cases.

Close #2380

@ppkarwasz Please review the codes when you have time.

@ppkarwasz ppkarwasz self-assigned this Mar 19, 2024
Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see several tests being replaced by single-shot examples, please don't. Instead use a @ParameterizedTest and provide several examples providing sufficient coverage. I also would like to see examples where arguments contain Throwables to match against the cases @ppkarwasz stated.

@SeasonPanPan
Copy link
Contributor Author

@vy @ppkarwasz Done, and added some test cases with the following scenarios:

 * 1. The placeholders are greater than the count of arguments. 
 * 2. The placeholders are less than the count of arguments.  
 * 3. The arguments contains an exception, and the placeholder is greater than normal arguments. 
 * 4. The arguments contains an exception, and the placeholder is less than the arguments. 
 All of these should logged in status logger with WARN level.

But this case with no warn log:

  • The arguments contains an exception, and the placeholder count is equal to normal arguments.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

It almost looks good to me, except we need to maintain Log4j semantics (which is the same as Logback < 1.1.0 semantics) for classifying a trailing Throwable as part of the arguments or as "extra" argument.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the usage of args.length, it looks good to me.

@SeasonPanPan
Copy link
Contributor Author

Apart from the usage of args.length, it looks good to me.

Done. Because the argCount might be 0 in ReusableMessage, I made a non-zero judgment in advance.

@ppkarwasz ppkarwasz merged commit 4c24359 into apache:2.x Mar 25, 2024
9 checks passed
@ppkarwasz
Copy link
Contributor

@SeasonPanPan

Thank you for your contribution.

@vy vy added this to the 2.24.0 milestone Mar 25, 2024
@vy vy added bug Incorrect, unexpected, or unintended behavior of existing code api Affects the public API labels Mar 25, 2024
@ppkarwasz ppkarwasz removed this from the 2.24.0 milestone Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Affects the public API bug Incorrect, unexpected, or unintended behavior of existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log messages with partially missing parameters
3 participants