'"Preconditions" and logging arguments should not require evaluation' - no good solution

Clean Code Report False-positive / False-negative...
1

macOS Ventura 13.3.1
IntelliJ IDEA Ultimate 2022.2.3

The following code generates a Code Smell, java:S2629, that does not, as far as I am aware, have a reasonable solution:

private void logMessage(
  Message message,
  org.slf4j.Logger logger
)
{
  final var builder = new StringBuilder();

  // append a bunch of different information to the builder
  ...
  logger.info(sb.toString()); // <- this generates the code smell
}

Unlike Java logging, as far as I am aware, unless you are using SLF4J 2 with the fluent logging API, there isn’t an option to pass in Supplier<String>/lambda arguments. You can cheat your way around the Code Smell by doing the following:

    final var logMessage = builder.toString();
    logger.warn(logMessage);

but that’s not addressing the underlying concern. Any guidance would be appreciated.

2

Hey there.

What version of SonarLint for IntelliJ are you using?

3

I’m using SonarLint 8.1.0.65508

5

I’m seeing this same issue in a slightly different context where I again don’t know how to work around this issue given SLF4J 1.x API limitations (unless I factor out the error message into a local var, which doesn’t really solve the problem that the code smell is pointing out - performance penalty of computing messages that may not be used/needed based on logging levels):

org.slf4j.LoggerFactory.getLogger(MyClass.class)
  .error(
    "Uncaught exception on thread: " + thread,
    throwable
  );
8

Hi @cssprs,

Thank you for engaging with the community.

As you can see from the rule description:

Similarly, passing concatenated strings into a logging method can also incur a needless performance hit because the concatenation will be performed every time the method is called, whether or not the log level is low enough to show the message.

You can see how to work around this in the code examples sections.

If you need more help, feel free to write again.

All the best,

Irina

9

Irina,

Thank you for the reply. I thoroughly read the lint recommendations before posting. Take this example:

org.slf4j.LoggerFactory.getLogger(MyClass.class)
  .error(
    "Uncaught exception on thread: " + thread,
    throwable
  );

The slf4j Logger error method has the following variants:

public void error(String msg);

public void error(String format, Object arg1);

...

public void error(String msg, Throwable t);

As far as I can tell, there is only one variant that takes an exception as an argument. The following will compile, but the selected method is the public void error(String format, Object arg1, Object arg2) method.

org.slf4j.LoggerFactory.getLogger(MyClass.class)
  .error(
    "Uncaught exception on thread: {}",
    thread,
    throwable
  );

There is apparently a bit of a hack going on underneath the covers in this scenario if you are using SFL4J version 1.6.0 and above where, if the last argument to the logging method is a Throwable, it will not be treated as an argument to the format and will have its stack trace and message printed as if it were magically using some variant of error(String format, Object argument, ... Throwable t). I was not aware of this trick until now. I’m not sure if it’s SonarLint’s responsibility to point out this quirk in SLF4J’s API in the Compliant Solutions section, but it definitely would have helped reduce confusion in my case.

10

Hi @cssprs,

Sorry for the late response.

The Compliant Solutions section already has a similar example, and in my opinion, don’t make sense to add another one.
LOG.error("Unable to open file {0}", csvPath, e);

Since last I wrote, we updated the rule description in the new Learn as You Code format. You can check it out and leave feedback in a separate thread.

All the best,

Irina