Hacker News Reader

Comment by koenigdavidmj | Hacker News Reader

It was a few months ago, but if I recall correctly, there were two overrides for info (and the other equivalent methods). info(String, String...) would do {} expansion like you mentioned, but info(String) would log the string directly, not doing format expansion on it.

I'm not sure how this interacts with the RCE issue reported here.

EDIT: That's because I was thinking of Slf4j, which has additional smarts here.

View on HN

I'm not aware of that feature, but I guess it would mitigate this issue, since the problematic code:

> logger.info("Data: {}");

would effectively turn into something safe:

> logger.info("{}", "Data: {}");

And the issue would only arise if someone mixes the two patterns:

> logger.info("Data for " + username + ": {}", data);

Overall, I don't like the sound of that feature, since it blurs the line between correct and incorrect use of the logging API. The first argument should always be a constant formatting string.

Why though? It is not typical in Java to use format strings, unless you call String.format explicitly. It's not like C where printf-style APIs are common.

It should work one way or the other, not both. For current logging APIs, the format string is used. It actually turns out that the call using string concatenation corresponds to log4j1 rather than log4j2 (looks like this was an error in the post, though it's being fixed to use log4j2).

I guess aesthetically you could argue either way, but I think the main purpose of the formatting string method is that you can write:

> logger.trace("Updates: {}", longListOfUpdates);

and if trace logging is disabled (which can be done dynamically), it's not going to invoke `longListOfUpdates.toString()`, which is what happens when you perform string concatenation. If it didn't work that way, I suspect people would end up writing extra `logger.isTraceEnabled()` conditions around their logging code.

Reply on news.ycombinator.com