java:S2259 - A "NullPointerException" could be thrown; "getMessage()" can return null

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

Hi,

It looks like we are getting a false positive (a few) with the S2259 rule.

SonarQube Community Edition Version 9.7.1 (build 62043)
Sonar gradle plugin ‘org.sonarqube’ version ‘3.+’

This example explicitly checks for null however sonar getMessage can return null:

public String s2259Error() throws HttpServerErrorException {
        try {
            throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
        } catch (HttpClientErrorException | HttpServerErrorException e) {
            if ((e.getMessage() != null) && e.getMessage().contains("NOT_FOUND_PROOF")) {// sonar shows the error here
                throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
            }
        }

        return "";
    }

Same for this example:

public String s2259WorseError() throws HttpServerErrorException {
        try {
            throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
        } catch (HttpClientErrorException | HttpServerErrorException e) {
            if (Objects.nonNull(e.getMessage()) && e.getMessage().contains("NOT_FOUND_PROOF")) {// sonar shows the error here
                throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
            }
        }

        return "";
    }

The only way I got it to work was:

public String s2259Works() throws HttpServerErrorException {
        try {
            throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
        } catch (HttpClientErrorException | HttpServerErrorException e) {
            if (Objects.requireNonNull(e.getMessage()).contains("NOT_FOUND_DOCUMENT")) {
                throw new HttpServerErrorException(HttpStatus.BAD_REQUEST);
            }
        }

        return "";
    }

It looks like the first 2 are false positives, can you confirm?

3

Thanks for reporting this false-positive!

Since all the necessary information has been included, we’ve flagged this for attention by an expert. This means that somebody will look at your report, maybe ask some follow-up questions, and try and determine if it’s really a false-positive that should be fied.

This review could be done hours, days, or even weeks from now. If it takes a while – it doesn’t mean your report isn’t important to us, it just means that our teams are already hard-at-work developing new language analysis features, and your report is in the queue.

If you’re using SonarQube or SonarCloud – an issue administrator can always mark an issue as a false-postive in the UI (this also suppresses it in SonarLint when using Connected Mode). The rule can also be disabled in your Quality Profile if it’s particularly noisy.

5

Hey @PaulTransport ,

First, thank you for the feedback. What you are facing here is unfortunately both a limitation from the engine hiding behind the rule and a (sometimes frustrating) feature… That we do not plan to work on at this time.

To summarize what you are facing, here is what I see, abstracted from your examples:

MyObject o = ...
if (o.getSomething() != null && o.getSomething().foo()) {
   ...                       // ^^^^^^^^^^^^^^^^ FP S2259 - NPE
}

Here, we face a dilemma. What guarantees that two consecutive calls to o.getSomething() return the same value each time? What if the state of o change in between calls?

Our engine can not guarantee that o didn’t change, so it assumes that the consecutive calls are responding, potentially, different results. This leads to the False Positive (FP) you experience. You might argue that we are speaking here of an Exception, and that its state is supposed to be immutable, but from the engine perspective (and especially knowing nothing about implementation details), this is much less obvious.

Unfortunately, there is no easy fix to this problem on our end, since there is no easy way to understand what’s the behavior of methods defined outside the file under analysis, or that we are just returning immutable field values.

An easy way to workaround the issue (and, in my taste, a more appropriate way to code), is to make sure the value stays the same, by using an intermediate variable. You can also decide to mark the issue as a False Positive, or Won't Fix.

MyObject o = ...
MyOtherObject something = o.getSomething();
if (something != null && something.foo()) {
   ...                // ^^^^^^^^^ value is guaranteed to be the same, no FP
}

Cheers,
Michael

1 Like
7

Hi,

Thanks for the response, it was clear and concise. We’ll refactor/ accept the action in sonar to work around it.
Cheers,

Paul