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

(doc) Clarify the usage of parameterized logging in Log4j 2 API page #1918

Closed
ppkarwasz opened this issue Oct 30, 2023 · 7 comments
Closed
Assignees
Labels
documentation Pull requests or issues that affect documentation

Comments

@ppkarwasz
Copy link
Contributor

The Log4j 2 API documentation page could be improved to state that:

  • parameterized logging and string concatenation should not be mixed, e.g.:
    // This is discouraged, but fine:
    logger.info("Hello " + name + "!");
    // This is recommended:
    logger.info("My name is {}.", myName);
    // Mixing the two approaches: a recipe for pattern specifier injection:
    // E.g. if name contains '{}'.
    logger.info("Hello " + name + "! My name is {}.", myName);
  • we could replace the usage of LogManager.getFormatterLogger(Class) with LogManager.getLogger(Class, MessageFormatter), which is more generic.
@ppkarwasz ppkarwasz self-assigned this Oct 31, 2023
@jvz jvz added the documentation Pull requests or issues that affect documentation label Nov 2, 2023
@ppkarwasz ppkarwasz assigned jvz and unassigned ppkarwasz Dec 15, 2023
@ppkarwasz
Copy link
Contributor Author

@jvz, I assigned this to you since it is related to #2100.

@grobmeier grobmeier self-assigned this Dec 15, 2023
@grobmeier
Copy link
Member

Not only @jvz but also general documentation - assigned myself too

@rm5248
Copy link
Contributor

rm5248 commented Dec 15, 2023

// Mixing the two approaches: a recipe for pattern specifier injection:
// E.g. if name contains '{}'.
logger.info("Hello " + name + "! My name is {}.", myName);

It would be good to expand on this a bit more, perhaps something like:

Mixing the two approaches can lead to undesired results. For example, let's say that we prompt the user for their name, and they tell us their name is {}. If our log statement looks like the following:

logger.info("Hello " + name + "! My name is {}.", myName);

That results in a log message of "Hello myName! My name is {}".

An MCVE would probably be appropriate here to show the (likely unintended) behavior.

@vy vy mentioned this issue Apr 26, 2024
5 tasks
@Chealer
Copy link

Chealer commented Apr 27, 2024

* parameterized logging and string concatenation should **not** be mixed, e.g.:
  // This is discouraged, but fine:
  logger.info("Hello " + name + "!");
  // This is recommended:
  logger.info("My name is {}.", myName);
[...]

If this refers to void info(String message, Object p0), then the JavaDoc is misleading. In fact, if message is actually a message template, then the JavaDoc for lots of methods really needs to be fixed.

@jvz
Copy link
Member

jvz commented May 1, 2024

That's more of a problem if you mix parameterized logging and concatenated strings. If there are no parameters given, then no parameterized placeholders will be replaced.

@vy
Copy link
Member

vy commented May 24, 2024

Fixed by several changes implemented for #2535.

@vy vy closed this as completed May 24, 2024
@Chealer
Copy link

Chealer commented May 25, 2024

As far as I can see this fully persists, but I assume @vy meant that a fix is underway in the staging area. The new Don’t use string concatenation section does improve, but is far from being as clear as @ppkarwasz's suggestion.
Note that "using message parameters" is called interpolation: https://en.wikipedia.org/wiki/String_interpolation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests or issues that affect documentation
Projects
None yet
Development

No branches or pull requests

6 participants