Don't persist an aggregate's pending events when executing a command returns an error

[slashdotdash]

Don't persist an aggregate's pending events when executing a command returns an error.

An Aggregate should enforce domain rules by returning an error tuple instead of its state on failure.

• An aggregate function must return {:ok, aggregate} on success.
• An aggregate function should return {:error, reason} on failure.

The example bank account aggregate returns {:error, :account_already_open} should an attempt be made to open an existing account.

defmodule Commanded.ExampleDomain.BankAccount do
  def open_account(%BankAccount{state: %{is_active?: true}} = account, %OpenAccount{}) do
    {:error, :account_already_open}
  end

  def open_account(%BankAccount{state: %{is_active?: false}} = account, %OpenAccount{account_number: account_number, initial_balance: initial_balance}) when is_number(initial_balance) and initial_balance > 0 do
    account = 
      account
      |> update(%BankAccountOpened{account_number: account_number, initial_balance: initial_balance})

    {:ok, account}
  end
end

The error will be returned to the caller when the command is dispatched.

# open account should initially succeed
:ok = Router.dispatch(%OpenAccount{account_number: "acc1"})

# attempting to open same account should error
{:error, :account_already_open} = Router.dispatch(%OpenAccount{account_number: "acc1"})

Fixes #9.

[andrzejsliwa]

Looks good, reviewed and added some comments about my personal preferences for handling errors more strictly (to let it fail on process level vs defensive style)

[andrzejsliwa]

looks good!

[andrzejsliwa]

I wonder if we could have here strong matching:

{:ok, state} = EventStore.append_to_stream(...)

to fail immediately, otherwise with will suppress error and only logger will be the evidence of problem. I personally prefer matching result for success case only and fail in place where errors occurs. In this case this will stop the process with information about state and reason. This is all about using OTP to have less defensive code and handle only happy cases.

Defensive code with with make sense for me in places where you need to handle user input/output or in business logic, but even then I'm always thinking if handling of not success case can be handled on level of supervision tree instead of defensive code.

[slashdotdash]

Are you suggesting to remove the {:error, :wrong_expected_version} match and only match on :ok. This would crash the aggregate process if persisting the pending events fails due to version mismatch. That would follow the 'let it crash' idiom.

I haven't yet thought too much about retrying command handling on failure. I'd like this to be client configurable (e.g. 5 retries, with exponential back-off). Using GenRetry is one approach I am considering.

[andrzejsliwa]

Are you suggesting to remove the {:error, :wrong_expected_version} match and only match on :ok. This would crash the aggregate process if persisting the pending events fails due to version mismatch. That would follow the 'let it crash' idiom.

yes, exactly. GenRetry is good example, can wrap in function this call
{:ok, state} = EventStore.append_to_stream(...) ...
or whole call of persist_events/2

[andrzejsliwa]

what do you think about returning {:ok, state} | {:error, reason} also from handler?

[slashdotdash]

That's a good suggestion. Being explicit about handlers returning :ok and the aggregate state on success makes sense. I'll look at making this change.