java:S2589 False-positive on condition checking

  • What language is this for? JAVA
  • Which rule? java:S2589
  • Why do you believe it’s a false-positive/false-negative? because our “h” variable could be null if we go into “testListFiles” method or “testPrometheeConfigProfiles” method (each one of these two methods can return null in some case) … please see code at bottom
  • SonarQube : 10.3 (build 82913)
  • How can we reproduce the problem? Give us a self-contained snippet of code (formatted text, no screenshots) … see bottom
public Health health() {
        try {
            var gitWrapper = "one value wrappe";
            var h = testListFiles(gitWrapper);
            if (h == null) {
                h = testPrometheeConfigProfiles(gitWrapper);
            }
            if (h != null) {
                return h;
            }
        } catch (Exception e) {
            return new Health("Impossible de trouver le domaine");
        }
        return new Health("OK");
    }

real use case :

Hey @dedece35 ,

Thanks for reporting this use case!

I tried to reproduce on my side based on what you provided, and I suspect that the only reason why you get this issue is that the engine on which is based the rule java:S2589 (our Symbolic Execution engine, aka SE) assumes your method testPrometheeConfigProfiles(String s) can not return null.

The engine is certainly wrong, according to what you say, however, it would help if you could share the content of the method its testPrometheeConfigProfiles(String s). FYI, if the method is located in the same file, the engine jumps into it and infer its behavior based on what it explore. It might however suffer from approximations, leading to what you see.

Can you provide the code for this second method?

  • If you can not, maybe a simplified version of it that still reproduces the issue?

Below are some examples of what could happen and illustrate some of the possible scenarios that could influence the SE engine. If you play with commented lines r = ...(p);, you can make the issue appear or not.

Finally, as a workaround, explicitly marking the method with some Nullability annotations (for instance @javax.annotation.Nullable, from the good old JSR-305) might help the engine understand that null is a possible scenario here (and make it explicit in your code for any method user).

Hope this helps,
Michael

public abstract class MyClass {

  public static class MyException extends Exception {
  }

  public static class Health {
    Health(String s) { }
  }

  MyClass(String s) {
  }

  public Health health(String p) {
    try {
      var r = foo(p);
      if (r == null) {
        /* using any of these methods makes the issue being raised below, as the Symbolic-Execution engine believe only non-null value can be returned */
        r = fooAnnotationNonnull(p);
        // r = fooDetectedNonnull(p);

        /* using any of these methods makes the issue disappear, null is understood as being a possible outcome, or the engine will not risk any assumption */
        // r = fooAnnotationNullable(p);
        // r = fooDetectedNull(p);
        // r = fooTotallyAbstract(p);
      }
      if (r != null) { // Issue
        return r;
      }
    } catch (Exception e) {
      return new Health("NOK");
    }
    return new Health("OK");
  }

  public abstract Health foo(String s) throws MyException;

  /**
   * Not knowing anything
   */
  public abstract Health fooTotallyAbstract(String s) throws MyException;

  /**
   * Marked as returning only Non-null results using the annotation
   */
  @javax.annotation.Nonnull
  public abstract Health fooAnnotationNonnull(String s) throws MyException;

  /**
   * Marked as returning potential null results using the annotation
   */
  @javax.annotation.Nullable
  public abstract Health fooAnnotationNullable(String s) throws MyException;


  /**
   * Detected as being able to return only non-null results
   */
  private static Health fooDetectedNonnull(String s) {
    return new Health(s);
  }

  /**
   * Detected as being able to return some null results
   */
  private static Health fooDetectedNull(String s) {
    if (s.isEmpty()) {
      return null;
    }
    return new Health(s);
  }
}

Hi @Michael

here is the code of the two called methods.
As you can see, there is quite a lot code calling internal services.
As you can see also, null and not null values can be returned in some cases.
These two methods are in the same class of my first method above.

private Health testListFiles(final GitWrapper gitWrapper) {

        String branch = "master";
        if (stBootstrapService.getRepositoryName().contentEquals(gitWrapper.getDomain())) { // cas specifique pour le domain bootstrap 'promethee-config'
            branch = stBootstrapService.getGitBranch();
        }

        try {
            Repository repository = gitWrapper.getGit().getRepository();
            ObjectId sha1 = repository.resolve(branch);
            if (sha1 == null) {
                return Health.down().withDetail(MESSAGE, String.format("La révision '%s' du domaine '%s' n'a pas été trouvée", branch, gitWrapper.getDomain())).build();
            }
            try (RevWalk revWalk = new RevWalk(repository)) {

                RevCommit commit = revWalk.parseCommit(sha1);
                RevTree tree = commit.getTree();
                try (TreeWalk treeWalk = new TreeWalk(repository)) {
                    treeWalk.addTree(tree);
                    treeWalk.setRecursive(true);
                    if (!treeWalk.next()) {
                        return Health.outOfService().withDetail(MESSAGE, String.format("Le domaine '%s' existe mais il est vide", gitWrapper.getDomain())).build();
                    }
                }
            }
        } catch (RevisionSyntaxException | IOException e) {
            return Health.down(e).withDetail(MESSAGE, String.format("Erreur lors de la récupération de fichiers dans le domaine '%s'", gitWrapper.getDomain())).build();
        }
        return null;
    }

    private Health testPrometheeConfigProfiles(final GitWrapper gitWrapper) {

        String branch = PROMETHEE_CONFIG_BRANCH;
        if (stBootstrapService.getRepositoryName().contentEquals(gitWrapper.getDomain())) { // cas specifique pour le domain bootstrap 'promethee-config'
            branch = stBootstrapService.getGitBranch();
        }

        try (InputStream inProfiles = sbConfiguration.getResourceAsStream(gitWrapper, branch, PROMETHEE_PROFILES_FILE)) {
            if (inProfiles != null) {
                ProfileConfig profileConfig = objectMapper.readValue(inProfiles, ProfileConfig.class);
                if (profileConfig.getProfilesByAlias().isEmpty()) {
                    return Health.down().withDetail(MESSAGE, String.format("Erreur lors de la récupération des profiles pour le domaine '%s', la liste des profiles est vide", gitWrapper.getDomain())).build();
                }
            }
        } catch (PrometheeResourceNotFoundException | PrometheeRevisionNotFoundException e) {
            // NO-OP
        } catch (Exception e) {
            return Health.down(e).withDetail(MESSAGE, String.format("Erreur lors de la récupération des profiles pour le domaine '%s', le fichier semble invalide", gitWrapper.getDomain())).build();
        }
        return null;
    }
1 Like