0

I am using Spring boot and analysing my code with sonarqube, on the below line

Optional<Animal> animal = animalRepository.findById(animalId);
if (animal == null) {
            throw new DeviceNotValidException("Failed to found animal detail",  new String[] { animalId});
        }

i am getting this error

Ensure this "Optional" could never be null and remove this null-check. `

As per my understanding, "Optional is generally used as a return type for methods that might not always have a result to return."

I want to make sure that i dont get a null pointer exception even if "findById()" returns null, so i have used "Optional" here.

Is there anything that I can do to improve my code so that the error disappears?

I read this Checking whether an Optional object isn't Empty and null but it did not help.

James Z
  • 12,209
  • 10
  • 24
  • 44
himanshu chugh
  • 3
  • 2
  • 1
    What is the method signature of `animalRepository.findById(animalId)`? Especially the return type? – cyberbrain Feb 28 '23 at 13:28
  • Hi @cyberbrain This is the signature/declaration of "findById) : public Optional findById(String id); – himanshu chugh Feb 28 '23 at 13:33
  • I doubt that this assignment is the line that triggers the SonarQube warning, but it could be a bug in SonarQube: https://sonarsource.atlassian.net/browse/SONARJAVA-4174 – cyberbrain Feb 28 '23 at 13:34
  • 1
    Doing a `null` check with an optional is useless (or at least pointless). If it can return `null` it is wrong use of `Optional`. – M. Deinum Feb 28 '23 at 13:34
  • 2
    "*...even if `findById()` returns null*" if that method declares to return Optional then it should *never* return `null` but `Optional.empty()`. If it *can* return `null` then it shouldn't be declaring `Optional` as return type. – Pshemo Feb 28 '23 at 13:36
  • @cyberbrain sorry, i added an if condition, which is actually causing the error. please check the question again... – himanshu chugh Feb 28 '23 at 13:36
  • 1
    np, but you have two nice answers now how to work with `Optional` - and if SonarQube still complains, have a look at the bugticket I linked above... – cyberbrain Feb 28 '23 at 13:42
  • 1
    Related and informative: [Checking whether an Optional object isn't Empty and null](https://stackoverflow.com/questions/71614826/checking-whether-an-optional-object-isnt-empty-and-null) – Ole V.V. Feb 28 '23 at 14:10

3 Answers3

4

An optional's application is to reduce null-values.

Checking if the supplied optional is null isn't enough for the compiler to be sure its value isn't.

To check for an Optional's value use either optional.isEmpty(); or optional.isPresent();.

In your case: check for the repository's response as follows:

Optional<Animal> animal = animalRepository.findById(animalId);
if (animal.isEmpty()) {
            throw new DeviceNotValidException("...");
}

Or even simpler:

Animal animal = animalRepository.findById(animalId)
                      .orElseThrow(() -> new DeviceNotValidException("...");

The last approach ensures animal not being null.

Z-100
  • 518
  • 3
  • 19
  • 2
    The last one is the proper use of an optional, abusing it to still do an if check isn't the proper way. – M. Deinum Feb 28 '23 at 13:42
-1

the line if (animal == null) is incorrect for your desired behavior. While the content of the Optional could be null, Optional itself is a wrapper which shouldn't be null.

Replace that line with: if (!animal.isPresent()).

ODDminus1
  • 544
  • 1
  • 3
  • 11
  • Which is wrong use for optional as well, or at least not the one you should be using. What would be correct (or better) is to use `return animal.orElseThrow(() -> new DeviceNotValidException("Failed to found animal detail", new String[] { animalId}) );`. That would be the proper use of an `Optional`. – M. Deinum Feb 28 '23 at 13:36
  • @M.Deinum, sure the code can be simplified further, but the question was regarding the error, not how to subjectively write a cleaner statement. One could also argue that your approach is wrong as it would be cleaner to apply `Optional::orElseThrow` to the first line instead of creating a variable. – ODDminus1 Feb 28 '23 at 13:43
  • That won't work with a lambda. When giving an answer to solve something show the best way. Abusing an optional in an if statement is simply not proper use of Optional. There are several presentations of Brian Goetz on this topic. – M. Deinum Feb 28 '23 at 14:06
  • I wasn't suggesting any lambda anywhere. I was referencing the method .orElseThrow() within Optional to point towards the same solution as Z-100 posted. And that your comment where you're applying the Optional to a variable could also be improved on. My point is that my initial answer was meant to give an understanding to what Optional is, not improve the overall code quality. Of course improvements to the code quality is a good addition, but you are incorrect to say that the answer is wrong. – ODDminus1 Mar 01 '23 at 06:21
  • IMHO it is wrong because it advocates wrong/improper usage of optional. We should advocate proper usages. It wasn't about making code shorter it was about proper usage of `Optional` and thus the `orElseThrows`. If assigning that to a variable first depends on the use-case or improves readability by all means go ahead. – M. Deinum Mar 01 '23 at 07:58
-1

Since you are using Spring Data, the problem you have is checking for an Optional to be null where said Optional is here to reduce null checks.

The proper use, proposed by Z-100, is to use orElseThrow:

Animal animal = animalRepository.findById(animalId)
                      .orElseThrow(() -> new DeviceNotValidException("...");

But since you are using Spring Data, you may also use getReferenceById:

Animal animal = animalRepository.getReferenceById(animalId);

It will generally throw a EntityNotFoundException.

NoDataFound
  • 11,381
  • 33
  • 59
  • `getReferenceById` serves a different purpose and actually returns a proxy and only starts throwing an exception when accessing methods on the object. – M. Deinum Feb 28 '23 at 14:07
  • I fail to see your point: _Returns a reference to the entity with the given identifier. Depending on how the JPA persistence provider is implemented this is very likely to always return an instance and throw an EntityNotFoundException on first access. Some of them will reject invalid identifiers immediately. _ This is basically a call to javax.persistence.EntityManager.getReference(Class, Object) – NoDataFound Feb 28 '23 at 16:20
  • It does exactly that. It gives a lazy proxy of that entity and doesn't do any db access until you start using the entity. This should be used when you want to map a many-to-one to a new object and attach an existing object. This saves the actual query and thus performance. – M. Deinum Mar 01 '23 at 07:57