Sonar complaining about logging and rethrowing the exception

Question

I have the following piece of code in my program and I am running SonarQube 5 for code quality check on it after integrating it with Maven.

However, Sonar is complaining that I should Either log or rethrow this exception.

What am I missing here? Am I not already logging the exception?

 private boolean authenticate(User user) {
        boolean validUser = false;
        int validUserCount = 0;
        try {
            DataSource dataSource = (DataSource) getServletContext().getAttribute("dataSource");
            validUserCount = new MasterDao(dataSource).getValidUserCount(user);
        } catch (SQLException sqle) {
            LOG.error("Exception while validating user credentials for user with username: " + user.getUsername() + " and pwd:" + user.getPwd());
            LOG.error(sqle.getMessage());
        }
        if (validUserCount == 1) {
            validUser = true;
        }
        return validUser;
    }

Solution 1

You should do it this way :

try {
    DataSource dataSource = (DataSource) getServletContext().getAttribute("dataSource");
    validUserCount = new MasterDao(dataSource).getValidUserCount(user);
} catch (SQLException sqle) {
    LOG.error("Exception while validating user credentials for user with username: " +
            user.getUsername() + " and pwd:" + user.getPwd(), sqle);
}

Sonar shouldn't bother you anymore

Solution 2

What sonar is asking you to do, is to persist the entire exception object. You can use something like:

    try {
        ...         
    } catch (Exception e) {
        logger.error("Error", e);
    }

Solution 3

I stumbled across the same issue. I'm not 100% sure if I'm completely right at this point, but basically you should rethrow or log the complete Exception. Whereas e.getMessage() just gives you the detailed message but not the snapshot of the execution stack.

From the Oracle docs (Throwable):

A throwable contains a snapshot of the execution stack of its thread at the time it was created. It can also contain a message string that gives more information about the error. Over time, a throwable can suppress other throwables from being propagated. Finally, the throwable can also contain a cause: another throwable that caused this throwable to be constructed. The recording of this causal information is referred to as the chained exception facility, as the cause can, itself, have a cause, and so on, leading to a "chain" of exceptions, each caused by another.

This means the solution provided by abarre works, because the whole exception object (sqle) is being passed to the logger.

Hope it helps. Cheers.

Solution 4

If you believe that SQLException can be safely ignored, then you can add it to the list of exceptions for squid:S1166 rule.

1. Go to Rule-> Search squid:S1166.
2. Edit exceptions in Quality Profile.
3. Add SQLException to the list.

View more solutions

Comments

[JB Nizet]

Maybe it's complaining that you're logging a message, but not the exception itself, which makes you lose the potentially useful stack trace of the exception. Anyway, you should definitely throw an exception here and signal a problem to the user, rather than doing as if everything went normally and returning the same thing as if the user credentials were incorrect. Logging a password is definitely not a good idea either: major security problem.

[SpaceTrucker]

You are not logging a message and the exception in one statement. Therefore other log entries might be in between both messages in the server log hiding the strong connection of both of these messages. And there might be an exception thrown from the first log statement hiding the information that is contained in the second.

[hc_dev]

Sometimes Sonar has difficulties with dynamic log-messages like here. See the Java-specific issue: SONARJAVA-3029. It was resolved 2019.

[Arun Ramakrishnan]

This is really helpful especially in the case of wrapped exceptions like java.util.concurrent.ExecutionException where I really want the cause and not this exception itself.

[Arun Ramakrishnan]

What if the exception is something like java.util.concurrent.ExecutionException and you really wanted to log the cause only. I see a comment below about ignoring specific exceptions! Thanks!

[John Calcote]

This sonar complaint is too strict in my opinion. There are exceptions you might expect to catch and ignore, for instance, FileNotFoundException might be caught and a message logged indicating the file was not found and execution will continue without it - no one needs the entire stack trace for this scenario. Yet, there's no way to get sonar to shut up about it beyond tagging the catch line with //NOSONAR.

[Romano]

You can also be more granular and annotate your method with @SuppressWarnings("squid:S1166") // [Add some explaination here] This way you can keep the rule for other cases.

[Romano]

I understand this is an example but for the sake of beginner's education such as myself, I would like to add that we should usually not catch Exception but one of its implementations instead.

[Rémi Bantos]

IMO it is too strict too. I have a use case where I need to catch an exception and throw another third party library exception which can only be instantiated with a message, not the root exception. I might report a sonar issue for that specific case actually...

[Kumaresan Perumal]

I want both logging and throw. is there any solution?

[Daniele]

@KumaresanPerumal you can add a throw clause (rollbar.com/guides/java/how-to-throw-exceptions-in-java) after logging it, while still inside the catch block

[hc_dev]

Can you explain "this way" and tell us something about the why ? This would help future readers to understand the reasoning behind SonarLint's issue better.