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

slashdotdash · 2016-09-26T22:36:11Z

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.

…mand succeeds

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 · 2016-09-27T05:35:15Z

lib/commanded/aggregates/aggregate.ex

+    else
+      {:error, :wrong_expected_version} -> retry_command(command, handler, state, retries - 1)
+      {:error, _reason} = reply -> {reply, state}
+    end


looks good!

andrzejsliwa · 2016-09-27T05:49:16Z

lib/commanded/aggregates/aggregate.ex

+      :ok ->
+        # clear pending events after successful store
+        {:ok, %{aggregate_state | pending_events: []}}
+      {:error, :wrong_expected_version} = reply ->


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.

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.

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 · 2016-09-27T05:49:29Z

lib/commanded/aggregates/aggregate.ex

+      {:error, reason} = reply ->
+        Logger.debug(fn -> "failed to handle command due to: #{inspect reason}" end)
+        reply
+      aggregate_state -> {:ok, aggregate_state}


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

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.

Let the aggregate process crash instead. Starting a new process for the aggregate will load its events to restore it to a valid state.

…return `{:ok, aggregate}` or `{:error, reason}` To allow strict pattern matching on success/failure.

slashdotdash added 2 commits September 26, 2016 21:59

Less strict mix dependency version requirements

db7845d

Only persist aggregate pending events and remember its state when com…

b42d889

…mand succeeds

andrzejsliwa approved these changes Sep 27, 2016

View reviewed changes

slashdotdash changed the title ~~Handle aggregate error~~ Don't persist an aggregate's pending events when executing a command returns an error Sep 27, 2016

slashdotdash added 2 commits September 27, 2016 22:22

Remove command retry on event persistence failure

c9fded7

Let the aggregate process crash instead. Starting a new process for the aggregate will load its events to restore it to a valid state.

Aggregate root, command handler and process manager functions should …

eb17ff9

…return `{:ok, aggregate}` or `{:error, reason}` To allow strict pattern matching on success/failure.

slashdotdash merged commit c781128 into master Sep 29, 2016

slashdotdash deleted the feature/handle-aggregate-error branch September 29, 2016 21:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

slashdotdash commented Sep 26, 2016 •

edited

Loading

andrzejsliwa left a comment

andrzejsliwa Sep 27, 2016

andrzejsliwa Sep 27, 2016 •

edited

Loading

slashdotdash Sep 27, 2016 •

edited

Loading

andrzejsliwa Sep 27, 2016 •

edited

Loading

andrzejsliwa Sep 27, 2016

slashdotdash Sep 27, 2016

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

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

Conversation

slashdotdash commented Sep 26, 2016 • edited Loading

andrzejsliwa left a comment

Choose a reason for hiding this comment

andrzejsliwa Sep 27, 2016

Choose a reason for hiding this comment

andrzejsliwa Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

slashdotdash Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

andrzejsliwa Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

andrzejsliwa Sep 27, 2016

Choose a reason for hiding this comment

slashdotdash Sep 27, 2016

Choose a reason for hiding this comment

slashdotdash commented Sep 26, 2016 •

edited

Loading

andrzejsliwa Sep 27, 2016 •

edited

Loading

slashdotdash Sep 27, 2016 •

edited

Loading

andrzejsliwa Sep 27, 2016 •

edited

Loading