S1166 does not recognize exception message parameter

Report a False-positive / False-negative
#1

Sonarqube 8.2 with latest java plugin

The rule java:S1166 states that passing e.getMessage to the log message would be complaint, but it’s not recognized here using slf4j logging framework:

    } catch (InvalidPathException e) {
      LOG.error("Configured backup path ({}) is invalid. Backup aborted. {}", e.getInput(), e.getMessage());
      return;
    }
#2

Where does the rule say, that passing only the message is compliant with the rule? Under “Non-compliant code examples” I have:

try {
  /* ... */
} catch (Exception e) {  // Noncompliant - exception is lost (only message is preserved)
  LOGGER.info(e.getMessage());
}

To be compliant with the rule and use a logger you have to pass the exception to your logger:

try {
  /* ... */
} catch (Exception e) {
  LOGGER.info(e);  // exception is logged
}
#3

Please scroll down in rule description:

Furthermore, no issue will be raised if the exception message is logged with additional information, as it shows that the developer added some context to the error message.

#4

I have reported another problem with the S1166 S1166 blind at exception concatenation

I find it really strange to consider that logger.info(e) is valid and not logger.info(e.getMessage()) while the only difference is the class name of the exception in the log and in both case the stacktrace is lost

#5

Yes, you’re right. Somehow I overlooked that part of the exceptions to the rule. However, in my opinion this goes counter to the intent of the rule.

#6

The rule with the exceptions is useful, because otherwise your log is always full of stacktraces which normally should not be needed to determine the cause of the problem.

#7

I can understand that exception to rule is useful.
But i am failling to see how LOGGER.info(e.getMessage()); can be non compliant as the exception is lost while LOGGER.info(e); is considered as compliant as it considers that the exception is better logged and doesn’t violate the issue rationale :

When handling a caught exception, the original exception’s message and stack trace should be logged or passed forward.

It isn’t a compliant solution. It should at least be in the exception part, like the mention

Furthermore, no issue will be raised if the exception message is logged with additional information, as it shows that the developer added some context to the error message.

#8

Hi Michael,

With the code that you provide, the rule S1166 should not raise an issue:

    } catch (InvalidPathException e) {
      LOG.error("Configured backup path ({}) is invalid. Backup aborted. {}", e.getInput(), e.getMessage());
      return;
    }

This is a similar compliant unit test case of the java analyzer: CatchUsesExceptionWithContextCheck.java#L246

So either the rule S1166 has a bug or the java analyzer has not a proper sonar.java.binaries configuration to resolve the org.slf4j.Logger method invocation.

About a potential bug, I failed to reproduce the false-positive with the following code snippet:

class A {
  static org.slf4j.Logger LOG = org.slf4j.LoggerFactory.getLogger("");
  void f() {
    try {
      /* ... */
    } catch (java.nio.file.InvalidPathException e) {
      LOG.error("Configured backup path ({}) is invalid. Backup aborted. {}", e.getInput(), e.getMessage());
      return;
    }
  }
}

Could you provide a more detailed code reproducer? Or could you check your classpath configuration of the java analyzer?

Alban