S1166 blind at exception concatenation

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

I am using SonarQube 7.9.1 with java plugin 3.13.1 and sonarlint in eclipse.

The S1166 issue is correctly raised for the following code

try { .... } catch (Exception e) { String msg = "exception " + e.toString(); log.error(msg); }

But not for the following code

try { .... } catch (Exception e) { String msg = "exception " + e; log.error(msg); }
2

Hello,

I agree that the two examples provided should behave the same way, so we have a problem here.
However, for me, in both cases, the exception is logged, and both are therefore compliant.

Does it make sense this way or am I missing something?

3

When I look at the description of the rule S1166 (Exception handlers should preserve the original exceptions) and the rationnal behind it (When handling a caught exception, the original exception’s message and stack trace should be logged or passed forward.), I am unable to know which case is correct, especially when the explanation mention that the following is incorrect:

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

but consider that the following is correct:

try {
  /* ... */
} catch (Exception e) {
  String message = "Exception raised while authenticating user: " + e.getMessage();
  LOGGER.warn(message); // Compliant - exception message logged with some contextual information
}

even if the “contextual information” could just be “Exception” or " ".

I should also mention that this rule considers that

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

is correct even if using the exception as first parameters could means not to write the stacktrace as implied by the fb-contrib:LO_LOGGER_LOST_EXCEPTION_STACK_TRACE rule ( Correctness - Method incorrectly passes exception as first argument to logger method)

4

I don’t have the exact story behind this exception, I can guess it was introduced as an heuristic, since it is usual to have such code. Of course, you can write simple example that will brake the heuristic, but this choice does not seems too bad to me, it will avoid obvious FP and I don’t see an easy alternative, do you?

Concerning the second part of your message, I agree that it might be misleading, but I don’t think it’s the role of this rule to report this specific case. It’s can be a good idea for a new rule though.

5

My initial problem is the fact that the analyser doesn’t behave the same way when it should be.

As I have understood it, the rule S1166 is about the fact that stacktrace shouldn’t be lost. Using exception as the first argument of a logger doesn’t preserve stack trace. The difference between LOGGER.info(e.getMessage()) (which is indicated as non compliant) and LOGGER.info(e) (which is indicated as compliant) is just about logging the class of the exception.
So in both case, it doesn’t conform to the spirit of the rule S1166