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;
    }

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. — JB Nizet, Jan 24 '15 at 07:27
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. — SpaceTrucker, Jan 27 '15 at 08:52
Sometimes Sonar has difficulties with _dynamic log-messages_ like here. See the [Java-specific issue: SONARJAVA-3029](https://jira.sonarsource.com/browse/SONARJAVA-3029). It was resolved 2019. — hc_dev, Jan 06 '22 at 20:17

score 41 · Accepted Answer · answered Apr 16 '15 at 09:02

41

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

answered Apr 16 '15 at 09:02

abarre

525
7
8

2

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! – Arun Ramakrishnan Jun 09 '16 at 14:08
4

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. – John Calcote Sep 13 '17 at 23:09
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... – Rémi Bantos Feb 07 '19 at 17:20
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. – hc_dev Jan 06 '22 at 14:16

score 13 · Answer 2 · answered May 17 '16 at 15:50

13

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);
    }

answered May 17 '16 at 15:50

Daniele

1,053
10
17

1

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. – Romano Apr 04 '18 at 19:47
I want both logging and throw. is there any solution? – Kumaresan Perumal Sep 20 '21 at 12:31
@KumaresanPerumal you can add a `throw` clause (https://rollbar.com/guides/java/how-to-throw-exceptions-in-java/) after logging it, while still inside the catch block – Daniele Sep 20 '21 at 16:11

score 5 · Answer 3 · answered Apr 15 '16 at 10:19

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.

score 4 · Answer 4 · answered Sep 04 '15 at 11:25

4

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

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

answered Sep 04 '15 at 11:25

rbitshift

96
5

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 Jun 09 '16 at 14:08
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 Apr 04 '18 at 19:45

Sonar complaining about logging and rethrowing the exception

4 Answers4

Linked

Related