java:S3655 Call "Optional#isPresent()" before accessing the value

Template for a good false-positive report,

  • versions used SonarCloud.io (version May 29, 2020) example sonar run here
  • minimal code sample to reproduce in github here

The FALSE positive is:

  • SonarCloud is NOT able to identify “isPresent()” call from a “Chained” method call, as you notice in “test-1”
  • where as
  • It can identify from a NON-chained call as you notice in “test-2”

Please see details of Image capture in this GIST

This is not a false positive. Your code is as follow:

if (obj.getOptional().isPresent()) {
    obj.getOptional().get() // issue
}

There is no guarantee that the first getOptional() method execution returns the same object as the second getOptional() method execution. You have to store the value locally:

Optional<String> value = obj.getOptional()
if (value.isPresent()) {
    value.get() // no issue
}
1 Like

So you are saying, what if “Optional Object” is mutated between the “isPresent” and “get” call, in case of multithreaded app?
Hence chained call is not honored same as - assign to variable and use the same !?

if (obj.getOptional().isPresent()) {
   // execution at this commented line - in case of  multithreaded app.
    obj.getOptional().get() // issue
}

No. Optional class is immutable. As I wrote there is no guarantee that your method returns the same object:

obj.getOptional() == obj.getOptional() // true or false

Why? Two examples:

  1. your method can always return a new Optional object
Optional<String> getOptional() {
    return Optional.ofNullable(valueWhichCouldBeChangedInOtherThread);
}

void setValue(String value) {
    valueWhichCouldBeChangedInOtherThread = value;
}
  1. your method can return a value set by other method
Optional<String> getOptional() {
    return optional;
}

void setOptional(Optional<String> optional) {
    this.optional = optional;
}

When you store the result in variable, then it always keeps the same value:

Optional<String> optional = obj.getOptional()
optional  == optional  // always true

Or avoid the local variable with:

obj.getOptional().ifPresent(value -> {
   // do something with the value
});

Or if you need to return something, use map:

Optional<String> result = obj.getOptional().map(value -> {
   return value.toString().trim(); // use the value to return your result type  
});