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
}
2 Likes

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
1 Like

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

What is the issue in the below condition?
if(Optional.ofNullable(aliMnsId).isPresent()){
aliMnsId.get()
}

I am still getting the same issue. Optional value should only be accessed after calling isPresent();

@Rohit_kalia Your code suggests that aliMnsId is already an Optional<ID>. Your call Optional.ofNullable(aliMnsId) creates an Optional<Optional<ID>>, which (by convention) should never be empty because an Optional should never be set to null (rule java:S2789).
Thus your calls to isPresent() and get() are on different objects.

I agree that this is not a false positive in general, but I get this even when “obj” is a record and hence as immutable as an Optional.

My code is like

if(myRecord.someOptional().isEmpty() 
|| myRecord.someOptional().get().equals(otherValue))

When i would expand this to

if(myRecord.someOptional().isEmpty() 
|| (myRecord.someOptional().isPresent() && myRecord.someOptional().get().equals(otherValue)))

then the isPresent() call would even be marked by the compiler as “unnessecary, because always true”.