2

I'm currently facing an issue about saving on redis inside a switchIfEmpty function. Probably is related to the fact I'm pretty new in Reactive Programming and unfortunately I struggling to find proper examples about it.

However I've been able to solve it but I'm pretty sure there's a better way to do it. Here is what I wrote now :

public Mono<ResponseEntity<BaseResponse<Content>>> getContent(final String contentId, final String contentType){
    return redisRepository.findByKeyAndId(REDIS_KEY_CONTENT, contentId.toString()).cast(Content.class)
               .map(contentDTO -> ResponseEntity.status(HttpStatus.OK.value())
                                                .body(new BaseResponse<>(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase(), contentDTO)))
               //here I have to defer, otherwise It would never wait for the findByKeyAndId 
               .switchIfEmpty(Mono.defer(() -> {
                   Mono<ResponseEntity<BaseResponse<Content>>> responseMono = contentService.getContentByIdAndType(contentId, contentType);
                   
                   //so far I understood I need to consume every stream I have, in order to actually carry out the task otherwise will be there waiting for a consumer.
                   //once I get what I need from the cmsService I need to put it in cache and till now this is the only way I've been able to do it
                   responseMono.filter(response -> response.getStatusCodeValue() == HttpStatus.OK.value())
                               .flatMap(contentResponse -> redisRepository.save(REDIS_KEY_CONTENT, contentId.toString(), contentResponse.getBody().getData()))
                                        .subscribe();
                   //then I return the Object I firstly retrived thru the cmsService
                   return responseMono;
               }
    ));
}

any clue or suggestion of better ways to do it? Thank you in advance for the help!

Toerktumlare
  • 12,548
  • 3
  • 35
  • 54
Maxuel
  • 127
  • 10
  • if understand correct, you want to fetch from redis, if its not found you want to fetch the value from another service, save it in the cache and then build your response and return it. – Toerktumlare Mar 31 '21 at 21:11
  • @Toerktumlare correct! I know the code is messy but yeah in some way it carries out the whole flow. – Maxuel Mar 31 '21 at 21:19

1 Answers1

4

There's a few things that aren't great from a best practice perspective there, and this probably isn't doing exactly what you think it's doing:

  • You're subscribing yourself, which is usually a clear sign something is wrong. Special cases aside, subscribing should generally be left to the framework. This is also the reason you need the Mono.defer() - usually, nothing would happen until the framework subscribed to your publisher at the correct time, whereas you're managing the subscription lifecycle for that publisher yourself.
  • The framework will still subscribe to your inner publisher there, it's just that the Mono that you're returning doesn't do anything with its result. So you'll likely be calling contentService.getContentByIdAndType() twice, not just once - once when you subscribe, and once when the framework subscribes.
  • Subscribing on an inner publisher like this creates a "fire and forget" type model, which means when your reactive method returns you've got no clue if redis has actually saved it yet, which could cause issues if you then come to rely on that result later.
  • Unrelated to the above, but contentId is already a string, you don't need to call toString() on it :-)

Instead, you might consider making use of delayUntil inside your switchIfEmpty() block - this will allow you to save the value to redis if the response code is ok, delay until this has happened, and keep the original value when done. The code might look something like this (difficult to say if this is exactly correct without a complete example, but it should give you the idea):

return redisRepository
        .findByKeyAndId(REDIS_KEY_CONTENT, contentId).cast(Content.class)
        .map(contentDTO -> ResponseEntity.status(HttpStatus.OK.value()).body(new BaseResponse<>(HttpStatus.OK.value(), HttpStatus.OK.getReasonPhrase(), contentDTO)))
        .switchIfEmpty(
                contentService.getContentByIdAndType(contentId, contentType)
                        .delayUntil(response -> response.getStatusCodeValue() == HttpStatus.OK.value() ?
                                redisRepository.save(REDIS_KEY_CONTENT, contentId, contentResponse.getBody().getData()) :
                                Mono.empty())
        );
Michael Berry
  • 70,193
  • 21
  • 157
  • 216
  • 1
    A huge THANK YOU! This has been more than simple answer but such a proper lesson about some reactive programming and what not to do with it. I just tried out and IT WORKS :) and of course I looks much more better and it's definitely readable. Grazie Mille! – Maxuel Mar 31 '21 at 22:06
  • @Maxuel That's the plan! Glad it worked out. You're very welcome. – Michael Berry Mar 31 '21 at 22:14
  • regarding the 4 points you wrote : 1) "subscribing should generally be left to the framework." you mean for instance the RestController which is call by someone else out there (e.g Postman) 2) "calling twice", is it because I'm putting a Mono and then return it without actually do anything? if so how about the filter and flatmap applied to Mono before the return? 3) "you've got no clue if redis has actually saved it yet", I don't get it. what I can tell you It's that redis always saved it (I saw it thru monitor and also the find was able to retrieve it) 4)a messy copy paste xD – Maxuel Mar 31 '21 at 22:16
  • @Maxuel The framework in this case is Webflux, which would subscribe (to kick off the reactive chain) whenever you make a request to it, through Postman or similar. As for your second point - remember that the `Mono` is immutable, hence why we use a reactive chain rather than setters to mutate its state. So because of that, the filter & flatmap only apply to the publisher you're explicitly subscribing to in your example - they won't affect the publisher you're returning. – Michael Berry Mar 31 '21 at 22:21
  • @Maxuel Redis is very quick, so you probably wouldn't have noticed in normal operation - but if there was a problem with it (let's say a network issue meant it took a bit longer than expected one time) then you could end up with a weird, hard to trace down error in production with no logging on failure. (This assumes that your downstream chain relies on it being saved of course, but that's a fair assumption to make - if not now, then future work could rely on that assumption.) – Michael Berry Mar 31 '21 at 22:23
  • 1
    Crystal clear thank you again for the time and patience – Maxuel Mar 31 '21 at 22:32