What to return if item is not present?

Question

Given this code:

@GetMapping("/data/{id}")
public Data retrieveDataById(@PathVariable Long id) {

    Optional<Data> data = dataService.findById(id);

    if(data.isEmpty()){
        return null;
    }
    else {
        return data.get();
    }

}

The primary issue I think is that null is being returned if the item is not found.

So, instead of

    if(data.isEmpty()){
        return null;
    }

I plan to change it to:

    if(data.isEmpty()){
        ResponseEntity.ok("No records found for id"+id);
    }

Is this correct or are there other changes I should consider ?

Update:

The change results in the return type changing from Data to ResponseEntity. Should I add data.get(); onto the ResponseEntity if the data is present ? Changing

Answer 1

It would be better to wrap the response when returning from Controllers

@GetMapping("/{id}")
    public ResponseEntity<RoleDto> findById(@PathVariable String id) {
        return ResponseEntity.ok(roleService.findById(id));
    }

For the service layer, returning custom exception with a related error message would be more flexible when dealing with the situation later.

public RoleDto findById(String id) {
        Optional<Role> role = roleRepository.findById(id);
        return role.map(roleMapper::toDto).orElseThrow(() -> new ResourceNotFoundException(id, "Role not found"));
    }

Custom exception can be treated according to end-user requirement by implementing @ControllerAdvice, Controller level @ExceptionHandler or ResponseStatusException from Spring 5

The change results in the return type changing from Data to ResponseEntity. Should I add data.get(); onto the ResponseEntity if the data is present ? Changing

Yes, you can. Normally you can do this operation in the service layer and return DTO to the relevant controller.

Answer 2

Optional gives you ability to do some operations with its value in a stream-like way. If you develop rest endpoints in Spring you could either return ResponseEntity as Joop Eggen mentioned or you could create @ControllerAdivce and throw exception that it would catch and send a proper http status. In terms of scalability, the second approach is better, because you can use it with various controllers.

So the first case:

@GetMapping("/data/{id}")
public ResponseEntityy<?> retrieveDataById(@PathVariable Long id) {
    return dataService.findById(id)
             .map(value -> new ResponseEntity<>(value, HttpStatus.OK))
             .orElse(ResponseEntity.notFound().build());
}

The second case:

@GetMapping("/data/{id}")
public Data retrieveDataById(@PathVariable Long id) {
    return dataService.findById(id).orElseThrow(() -> new YourCustomRuntimeException());
}

@ControllerAdvice
class ControllerExceptionHandler {
    @ResponseStatus(HttpStatus.NOT_FOUND) 
    @ExceptionHandler(YourCustomRuntimeException.class)
    public ResponseEntity elementNotFound() {
        return ResponseEntity.notFound().build();
    }
}