CODEWITHSUNDEEP
javareactive-programmingspring-webfluxproject-reactor
lkatiforis
lkatiforis

Reputation: 6255

Update objects's state in Reactor

Given the following method:

private Mono<UserProfileUpdate> upsertUserIdentifier(UserProfileUpdate profileUpdate, String id){
    return userIdentifierRepository.findUserIdentifier(id)
            .switchIfEmpty(Mono.defer(() -> {
                profileUpdate.setNewUser(true);
                return createProfileIdentifier(profileUpdate.getType(), id);
            }))
            .map(userIdentifier -> {
                profileUpdate.setProfileId(userIdentifier.getProfileId());
                return profileUpdate;
            });
}

switchIfEmpty and map operators mutate the profileUpdate object. Is it safe to mutate in switchIfEmpty operator? Regarding map, if I have understood correctly, this is not safe and object profileUpdate must be immutable, right? eg:

private Mono<UserProfileUpdate> upsertUserIdentifier(UserProfileUpdate profileUpdate, String id){
        return userIdentifierRepository.findUserIdentifier(id)
                .switchIfEmpty(Mono.defer(() -> {
                    profileUpdate.setNewUser(true);
                    return createProfileIdentifier(profileUpdate.getType(), id);
                }))
                .map(userIdentifier -> profileUpdate.withProfileId(userIdentifier.getProfileId()));
    }

Later in the chain, another method mutates the object:

public Mono<UserProfileUpdate> transform(UserProfileUpdate profUpdate) {
        if (profUpdate.isNewUser()) {
            profUpdate.getAttributesToSet().putAll(profUpdate.getAttributesToSim());
        } else if (!profUpdate.getAttributesToSim().isEmpty()) {
            return  userProfileRepository.findUserProfileById(profUpdate.getProfileId())
                    .map(profile -> {
                        profUpdate.getAttributesToSet().putAll(
                                collectMissingAttributes(profUpdate.getAttributesToSim(), profile.getAttributes().keySet()));
                        return profUpdate;
                    });
        }
        return Mono.just(profUpdate);
    }

The above methods are called as follows:

  Mono.just(update)
  .flatMap(update -> upsertUserIdentifier(update, id))
  .flatMap(this::transform)

Upvotes: 3

Views: 1542

Answers (2)

Michael Berry
Michael Berry

Reputation: 72379

There's arguably two factors here - the "dangerous" aspect and the "style" aspect.

Simon has covered the "dangerous" aspect very well; there is one thing I'd add however. Even though your code is "safe" within the method that we can see here (due to the guarantees we have behind a single subscription to an inner flatmap publisher), we still can't absolutely guarantee it's safe in a wider context - we don't know what else has visibility of, or might mutate, your profileUpdate parameter. If it's created, immediately passed into this method only, then read after this method completes, then sure, it's good. If it's created at some point in the past, perhaps passed around to a few methods that may or may not mutate it, passed back, passed into this method, passed into a few other methods... then, well, it might be safe, but it becomes increasingly difficult to analyse and say for certain - and if it's not safe, it becomes just as difficult to track down where that one in a million bug caused by the behaviour might occur.

Now, your code may look nothing like this complex mess I've just described - but with just a few lines changed here and there, or "just doing one more mutation" with this object elsewhere before it's passed in, it could start to get there.

That leads into the "style" aspect.

Personally, I'm very much a fan of keeping everything as part of the reactive chain where at all possible. Even ignoring the potential for bad mutations / side-effects, it becomes much harder to read the code if it's all written in this way - you have to mentally keep track of both the value being passed through the chain, as well as values external to the chain being mutated. With this example it's reasonably trivial, in larger examples it becomes almost unreadable (at least to my brain!)

So with that in mind, if I were reviewing this code I'd strongly prefer UserProfileUpdate to be immutable, then to use something like this instead:

private Mono<UserProfileUpdate> upsertUserIdentifier(UserProfileUpdate profileUpdate, String id){
    return userIdentifierRepository.findUserIdentifier(id)
            .switchIfEmpty(() -> createProfileIdentifier(profileUpdate.withNewUser(true), id))
            .map(userIdentifier -> profileUpdate.withProfileId(userIdentifier.getProfileId()));
}

...note this isn't a drop-in replacement, in particular:

  • createProfileIdentifier() would just take a UserProfileUpdate object and an id, and be expected to return a new UserProfileUpdate object from those parameters;
  • UserProfileUpdate would need to be enhanced with @With (lombok) or a similar implementation to allow it to produce immutable copies of itself with only a single value changed.
  • Other code would likely need to be modified similarly to encapsulate the profileUpdate as part of the reactive chain, and not rely on mutating it.

However, in my opinion at least, the resulting code would be far more robust and readable.

Upvotes: 1

Simon Basl&#233;
Simon Basl&#233;

Reputation: 28351

Nebulous answer, but... it depends!

The danger of mutating an input parameter in the returned Mono or Flux comes from the fact that said Mono or Flux could be subscribed multiple times. In such a case, suddenly you have a shared resource on your hands, and it can lead to puzzling issues.

But if the methods in question are called from a well controlled context, it can be safe.

In your case, flatMap ensures that the inner publishers are subscribed to only once. So as long as you only use these methods inside such flatMaps, they can safely mutate their input parameter (it is kept in the scope of the flatmapping function).

Upvotes: 2

Related Questions