SonarQube warning: A "NullPointerException" could be thrown; "getBody()" can return null

Question

ResponseEntity<CommonResponseDTO> commonResponseDto = testClient.getCategoryById(sampleEntity.getCategoryId());   

if(Objects.nonNull(commonResponseDto) && Objects.nonNull(commonResponseDto.getBody()) && Objects.nonNull(commonResponseDto.getBody().getName())){

SuperDataDto superDataCategoryDto =  SuperDataDto.builder().id(sampleEntity.getCategoryId()).name(commonResponseDto.getBody().getName()).build();

In the above code SonarQube complains that

A "NullPointerException" could be thrown; "getBody()" can return null. on 

.name(commonResponseDto.getBody().getName())

I am even checking the nonNull for commonResponseDto.getBody().getName()). Am I doing anything wrong?

Answer 1

SonarQube is right.

if(Objects.nonNull(commonResponseDto)
  && Objects.nonNull(commonResponseDto.getBody())
  && Objects.nonNull(commonResponseDto.getBody().getName())) {
SuperDataDto superDataCategoryDto =  SuperDataDto.builder().id(sampleEntity.getCategoryId()).name(commonResponseDto.getBody().getName()).build();

Makes no guarantee about the return value of getBody() of consecutive calls. Here's an implementation that would easily make your condition throw:

private int calls = 0;

public Body getBody() {
  ++calls;
  if (calls % 2 == 0) return null;
  return new Body(...);
}

The first check is non-null, calling it a second time will return null and your code throws an exception. Don't rely on methods to return the same reference from multiple calls, if you require a single value, be explicit about it and store it in a local variable:

if (Objects.nonNull(commonResponseDto)) {
  final CommonResponseDTO body = commonResponseDto.getBody();
  if (Objects.nonNull(body)) {
    final String name = body.getName();
    if (Objects.nonNull(name)) {
      SuperDataDto superDataCategoryDto = SuperDataDto.builder()
          .id(sampleEntity.getCategoryId())
          .name(name)
          .build();

Note that Objects#nonNull is mainly intended to be used as a method reference in lambda expressions and your code could be clearer if you simply wrote obj != null.

But this becomes ugly quickly. You can increase the clarity of your code by moving it to a small helper function:

private static String nameOfResponse(
    final ResponseEntity<CommonResponseDTO> response) {
  if (response == null) return null;
  final CommonResponseDTO body = commonResponseDto.getBody(); 
  if (body == null) return null;
  return body.getName();
}

// ...
final String name = nameOfResponse(commonResponseDto);
if (name != null) {
  final SuperDataDto superDataCategoryDto = SuperDataDto.builder()
      .id(sampleEntity.getCategoryId())
      .name(name)
      .build();
}

---

An alternative with a little bit of overhead is wrapping your object in an Optional and then transforming this optional value:

Optional.ofNullable(commonResponseDto)
    .map(CommonResponseDTO::getBody)
    .map(Body::getName())
    .ifPresent(name -> {
      SuperDataDto superDataCategoryDto = SuperDataDto.builder()
          .id(sampleEntity.getCategoryId())
          .name(name)
          .build();
    });

But please note that it is discouraged to use Optional for flow control. They should only be used as return type of methods to signal the absence of a value.