Is my implementation of a simple CRUD service with Spring WebFlux correct?

Question

I am implementing a simple Movie based CRUD API using Spring WebFlux and Reactive Spring Data MongoDB. I want to make sure that my implementation is correct and that I am properly using Flux and Mono to implement the CRUD operations. I also want to make sure that I am properly handling any errors or null values. I am very new to this programming paradigm and Spring WebFlux, so I am not sure about the correctness of the implementation of the Controller and Service layer, I want to make sure I am adhering to Spring WebFlux and Project Reactor best practices.

@Repository
public interface MovieRepository extends ReactiveMongoRepository<Movie, String> {

    Flux<Movie> findByRating(String rating);
}

public interface MovieService {
    Flux<Movie> list();

    Flux<Movie> findByRating(String rating);

    Mono<Movie> update(String id, MovieRequest movieRequest);

    Mono<Movie> create(Mono<MovieRequest> movieRequest);

    Mono<Movie> read(String id);

    Mono<Movie> delete(String id);
}

@Service
public class MovieServiceImpl implements MovieService {

    @Autowired
    private MovieRepository movieRepository;

    @Override
    public Flux<Movie> list(){
        return movieRepository.findAll();
    }


    @Override
    public Flux<Movie> findByRating(final String rating){
        return movieRepository.findByRating(rating);
    }

    @Override
    public Mono<Movie> update(String id, MovieRequest movieRequest) {

       return movieRepository.findOne(id).map(existingMovie -> {

           if(movieRequest.getDescription() != null){
               existingMovie.setDescription(movieRequest.getDescription());
           }
           if(movieRequest.getRating() != null){
               existingMovie.setRating(movieRequest.getRating());
           }
           if(movieRequest.getTitle() != null) {
               existingMovie.setTitle(movieRequest.getTitle());
           }

           return existingMovie;

       }).then(movieRepository::save);
    }

    @Override
    public Mono<Movie> create(Mono<MovieRequest> movieRequest) {

        return movieRequest.map(newMovie -> {

            Movie movie = new Movie();

            if(newMovie.getDescription() != null){
                movie.setDescription(newMovie.getDescription());
            }
            if(newMovie.getRating() != null){
                movie.setRating(newMovie.getRating());
            }
            if(newMovie.getTitle() != null) {
                movie.setTitle(newMovie.getTitle());
            }

            return movie;

        }).then(movieRepository::save);
    }

    @Override
    public Mono<Movie> read(String id) {
        return movieRepository.findOne(id);
    }

    @Override
    public Mono<Movie> delete(String id) {
        Mono<Movie> movie = movieRepository.findOne(id);
        movieRepository.delete(id);
        return movie;
    }
}

@RestController
public class MovieRestController {

    @Autowired
    private MovieService movieService;

    @GetMapping(value = "/movies")
    public Flux<ResponseEntity<Movie>> list() {

        return movieService.list().map(m -> new ResponseEntity<>(m, HttpStatus.OK));
    }

    @GetMapping(value = "/moviesByRating")
    public Flux<ResponseEntity<Movie>> findByRating(
            @RequestParam(value = "rating", required = false) final String rating) {

        return movieService.findByRating(rating)
                .map(m -> new ResponseEntity<>(m, HttpStatus.OK));

    }

    @GetMapping("/movies/{movieId}")
    public Mono<ResponseEntity<Movie>> read(
            @PathVariable("movieId") final String movieId) {

        return movieService.read(movieId)
                .map(m -> new ResponseEntity<>(m, HttpStatus.OK));
    }

    @DeleteMapping("/movies/{movieId}")
    public Mono<ResponseEntity<Movie>> delete(
            @PathVariable("movieId") final String movieId) {

        return movieService.delete(movieId)
                .map(m -> new ResponseEntity<>(m, HttpStatus.OK));
    }

    @PutMapping("/movies/{movieId}")
    public Mono<ResponseEntity<Movie>> update(
            @PathVariable("movieId") final String movieId,
            @RequestBody final MovieRequest movieRequest) {

        return movieService.update(movieId, movieRequest)
                .map(m -> new ResponseEntity<>(m, HttpStatus.OK));

    }

    @PostMapping("/movies")
    public Mono<ResponseEntity<Movie>> create(
            @RequestBody final Mono<MovieRequest> movieRequest) {

        return movieService.create(movieRequest)
                .map(m -> new ResponseEntity<>(m, HttpStatus.OK));

    }

}

Answer 1

This is not necessarily an in-depth review, but I see one issue and one improvement:

The CRUD method for delete in your service should combine operators
Currently you have

@Override
public Mono<Movie> delete(String id) {
    Mono<Movie> movie = movieRepository.findOne(id);
    movieRepository.delete(id);
    return movie;
}

This is problematic because you return the result of findOne rather than the combination of 1) waiting for the DB to find your movie then 2) getting the ack from the DB that it deleted it. The result of delete is never subscribed to, which means that it is never actually executed! Even if you added .subscribe() there, that could mean you execute both calls in parallel and your controller's response time would be tied to the fetch rather than the delete.

So you need to return a Mono that also triggers and represents completion of the delete.

The problem is that delete in the repository returns a Mono<Void>. There is a new Mono.untilOther operator in the upcoming 3.0.6 release of Reactor that will be usable like this:

@Override
public Mono<Movie> delete(String id) {
    return movieRepository.findOne(id)
        .untilOther(movieRepository.delete(id));
}

This will trigger the findOne and hold on emitting its value until the delete has completed.

But for now release is 3.0.5 so in the meantime here is a workaround:

@Override
public Mono<Movie> delete(String id) {
    return movieRepository.findOne(id)
        .flatMap(oldValue -> 
            movieRepository.delete(id)
                           .then(Mono.just(oldValue))
        )
        .singleOrEmpty();
}

The idea here is to first go to the DB to fetch the current value, and one this is done trigger the delete inside a flatMap. Once the delete completes (then(...)), we're still inside the flatmap and the value from findOne has been captured within the flatmap context, so we can re-emit it using Mono.just(...).

Now because of flatMap we are now dealing with a Flux<Movie>, but we know it will not contain more than one value ever, so we can revert to a Mono<Movie> using singleOrEmpty. Note it could be empty if the key doesn't exist in DB.

which leads me to:

Some CrudRepository methods can return an empty Mono/Flux
This you probably want to represent as a 404, especially for the read endpoint. You can use .defaultIfEmpty(ResponseEntity.notFound()) for that.