Resolve the issue of the handleEvent being called twice after handleCommand is called in Actions.

[aiacovella]

In reference to the issue with when handling a command the handle event code gets called twice.

The following details why it occurs:

This happens because after applying a command we apply all the events to the aggregate before sending it to persist but we discard the aggregate state after persisting, we re-apply them without discarding this is done to ensure that no event is persisted if they can’t be handle later
however, we could do it in a more intelligent way we could apply it only once and keep two versions of the aggregate: before and after if we succeed in persisting the events on the store, we drop the old version, we keep the updated one and make the aggregate available to receive the next command.

[pawelkaczor]

this is done to ensure that no event is persisted if they can’t be handle later

Events should be persisted once command is validated (constraints are checked). Or am I missing something?

[trbngr]

This has tripped me up in the past and I've been meaning to open an issue too.

Thanks for remembering for me ;)

[octonato]

@pawelkaczor you are right about the order of persisting. We produce Events by applying a command on the current aggregate instance.

But before we really persisted the events, we must be sure that we can apply the events on the aggregate. Imagine the situation where we add a command handler that produces one event, but we don't add an event handler for it. In such a scenario, we end up with an event on the storage that can NOT be handled.

That's why I was first applying the events to make sure we have event handlers for it. In case it's missing, the command is rejected.

Of course, this is not bulletproof (or foolproof). People can always remove event handlers for events that are already persisted. In which case re-playing events won't work anymore.

[trbngr]

I'd argue that the library shouldn't care if there's an event handler or not (although I do like the warnings).

If the command is valid, it should raise an event, that event should be persisted, and after that, there should be an attempt to apply the raised event.

The most important part of this is persisting the event. That's what our aggregates are for after all.

[pawelkaczor]

@rcavalcanti so, if I understand correctly, the fun-cqrs performs some kind of sanity-check of the Aggregate Root implementation on the runtime, that can result in command rejection

Unfortunately, the sanity-check algorithm may be confusing (the issue #60 has been created) and probably introduces accidental complexity. I would suggest reconsidering its value. I think this kind of quality assurance should not be implemented in the main code, but rather in the tests.

[octonato]

Fair enough.
I will remove it and add it as part of the test kit.

Thanks for the valuable feedback.

[octonato]

Hi all,

There is a fix for this on branch feature/issue_60.

I solved it in a slight different way.

Event handlers are only called once, but the sanity check is still in place.

I keep it because I realised that I had already to paths doing an explicit sanity check and another one that was doing it anyway (let's say by design). I explain...

The Interpreter has three methods:

def onCommand(state: State[Aggregate], cmd: Command): F[Events]
def onEvent(state: State[Aggregate], evt: Event): State[Aggregate]
def applyCommand(state: State[Aggregate], cmd: Command): F[(Events, State[A])]

The applyCommand is a composition of onCommand and onEvent.

The Akka backend was not using the applyCommand, but calling onCommand, persisting the events and than calling onEvent.

The updated aggregate was produced only after the confirmation that all events were persisted.

The InMemoryBackend was already using the applyCommand method. The test kit was already doing what @pawelkaczor suggested.

So, the fix consist in promoting the usage of applyCommand for all possible backends. We basically should think in terms of applyCommand for new incoming commands and onEvent for replaying aggregates.

I'm not merging it yet, because I would like to have some more time to revise the code.

Feel to do a code review as well if you are in the mood. Is much appreciated. ;-)

[pawelkaczor]

@rcavalcanti nice to see the improvement 👍 But, I think your explanation of the fix does not reflect actual changes in the code - I saw that the implementation of the onCommand was significantly changed.

[octonato]

Euh! The main change in onCommand is the removal of call to tryHandleAllEvents. And the fact that it's not private.

Now, whoever implements a backend has call applyCommand.

[octonato]

@pawelkaczor the error in persisteAll is fixed. Unfortunately we can't do it without using an internal counter. Check Lagom for inspiration and they are doing exactly the same.

The only clean way of doing it requires:
persistAll[A](events: immutable.Seq[A])(handler: immutable.Seq[A] ⇒ Unit)
instead of
persistAll[A](events: immutable.Seq[A])(handler: A ⇒ Unit)

[pawelkaczor]

@rcavalcanti Thanks for the explanation.