Is critical SonarLint issue S1166 in my Java code a false positive or not?

Question

SonarLint 1.0.0 for Eclipse flags a critical issue in my code and I can't see why and how to fix it. It really looks like a false positive to me—or am I missing something?

import org.apache.log4j.Logger;

[...]

public final class Foo {

    private static final Logger logger = Logger.getLogger(Foo.class);

    [...]

    public static void foo() {

        MyCommand command = new MyCommand(foo, bar);
        try {
            commandService.executeCommand(command);
        } catch (CommandException e) {
            logger.error("My command execution failed", e);
        }
    }

    [...]

Here's an excerpt of the matching SonarLint rule description:

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

Noncompliant Code Example

// Noncompliant - exception is lost
try { /* ... */ } catch (Exception e) { LOGGER.info("context"); }   

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

// Noncompliant - exception is lost
try { /* ... */ } catch (Exception e) { throw new RuntimeException("context"); }

Compliant Solution

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

try { /* ... */ } catch (Exception e) { throw new RuntimeException(e); }

try {   /* ... */ } catch (RuntimeException e) {
    doSomething();  
    throw e;
} catch (Exception e) {
    // Conversion into unchecked exception is also allowed
    throw new RuntimeException(e);
}

In my opinion, my code qualifies for the first variant of the given compliant solutions, but SonarLint does not accept it.

There was another discussion of Sonar rule S1166 a little while ago, but it's not really about the same issue I have.

Edit: In response to question below: I use log4j for logging. I expanded the code to reflect this.

What Logging-Framework are you using? Maybe SonarLint doesn't know your Framework. — hinneLinks, Dec 01 '15 at 14:12
Thanks for your reply hinneLinks, I use log4j. I also revised the original question to include this information. — anothernode, Dec 01 '15 at 17:06
Not directly relevant, but I am wondering if Sonar finds `LOGGER.error(e)` acceptable, since `LOGGER.info(e)` is compliant. — Miserable Variable, Dec 01 '15 at 17:38
Hi Miserable Variable, I tried that. It doesn't make a difference whether I use `LOGGER.info(e)` or `LOGGER.error(e)`. — anothernode, Dec 02 '15 at 07:56

score 4 · Accepted Answer · answered Dec 01 '15 at 17:15

4

You are, in fact, logging the original exception's message and stack trace; this is an erroneous finding.

It may be that the rule doesn't have specific knowledge of Log4j, but lacking omniscience of all logging libraries, the fact that the exception is passed as a parameter could suffice.

answered Dec 01 '15 at 17:15

erickson

265,237
58
395
493

I think expecting support for log4j is not unreasonable. But in any case if it can't claim "omniscience" then it should be sufficient that the code does *something* with the exception - how can it know otherwise? – robert Feb 17 '16 at 10:43

Is critical SonarLint issue S1166 in my Java code a false positive or not?

Noncompliant Code Example

Compliant Solution

1 Answers1