Repository: commanded/commanded (elixir)

Issue:
Call error callback function when an event handler `handle/2` returns an invalid value
The valid return values for an event handler's `handle/2` callback function are `:ok` or `{:error, term}`. Returning any other value will cause the handler to crash with a `CaseClauseError`. This can be demonstrated by returning `nil`.

```elixir
defmodule ExampleHandler do
  use Commanded.Event.Handler,
    application: ExampleApp,
    name: __MODULE__

  def handle(_event, _metadata) do
    nil
  end
end
```

Instead of crashing, an error should be logged and the event handler's `error/3` callback should be called to allow the handler to determine what to do (skip the event, retry, or stop). 

Pull request description:
Call event handler's `error/3` callback function when `handle/2` function returns an invalid value
The valid return values for an event handler's `handle/2` callback function are `:ok` or `{:error, term}`. Returning any other value previously caused the handler to crash with a CaseClauseError. This is now caught and will call the handler's `error/3` callback function allowing the handler to decide what to do.

Closes #371.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index d04a6ab..f911cad 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,6 +19,7 @@
 - Ignore `:not_found` error when reseting InMemory event store ([#354](https://github.com/commanded/commanded/pull/354)).
 - Add `router/1` to `locals_without_parens` in Mix format config ([#351](https://github.com/commanded/commanded/pull/351)).
 - Include stacktrace in event handler and process manager `error` callback functions ([#342](https://github.com/commanded/commanded/pull/342)).
+- Call event handler's `error/3` callback function when `handle/2` function returns an invalid value ([#372](https://github.com/commanded/commanded/pull/372)).
 
 ### Bug fixes
 
diff --git a/lib/commanded/event/handler.ex b/lib/commanded/event/handler.ex
index 95445ea..fd91bbd 100644
--- a/lib/commanded/event/handler.ex
+++ b/lib/commanded/event/handler.ex
@@ -635,14 +635,37 @@ defmodule Commanded.Event.Handler do
         confirm_receipt(event, state)
 
       {:error, _reason} = error ->
+        log_event_error(error, event, state)
+
         failure_context = build_failure_context(event, context)
 
         handle_event_error(error, event, failure_context, state)
 
       {:error, reason, stacktrace} ->
+        log_event_error({:error, reason}, event, state)
+
         failure_context = build_failure_context(event, context, stacktrace)
 
         handle_event_error({:error, reason}, event, failure_context, state)
+
+      invalid ->
+        Logger.error(fn ->
+          describe(state) <>
+            " failed to handle event " <>
+            inspect(event, pretty: true) <>
+            ", `handle/2` function returned an invalid value: " <>
+            inspect(invalid, pretty: true) <>
+            ", expected `:ok` or `{:error, term}`"
+        end)
+
+        failure_context = build_failure_context(event, context)
+
+        handle_event_error(
+          {:error, :invalid_return_value},
+          event,
+          failure_context,
+          state
+        )
     end
   end
 
@@ -664,15 +687,13 @@ defmodule Commanded.Event.Handler do
   end
 
   defp build_failure_context(%RecordedEvent{} = failed_event, context, stacktrace \\ nil) do
-    %FailureContext{
-      context: context,
-      stacktrace: stacktrace,
-      metadata: enrich_metadata(failed_event)
-    }
+    metadata = enrich_metadata(failed_event)
+
+    %FailureContext{context: context, metadata: metadata, stacktrace: stacktrace}
   end
 
   defp handle_event_error(
-         {:error, reason} = error,
+         error,
          %RecordedEvent{} = failed_event,
          %FailureContext{} = failure_context,
          %Handler{} = state
@@ -680,14 +701,6 @@ defmodule Commanded.Event.Handler do
     %RecordedEvent{data: data} = failed_event
     %Handler{handler_module: handler_module} = state
 
-    Logger.error(fn ->
-      describe(state) <>
-        " failed to handle event " <>
-        inspect(failed_event, pretty: true) <>
-        " due to: " <>
-        inspect(reason, pretty: true)
-    end)
-
     case handler_module.error(error, data, failure_context) do
       {:retry, context} when is_map(context) ->
         # Retry the failed event
@@ -712,9 +725,9 @@ defmodule Commanded.Event.Handler do
         confirm_receipt(failed_event, state)
 
       {:stop, reason} ->
-        # Stop event handler
         Logger.warn(fn -> describe(state) <> " has requested to stop: #{inspect(reason)}" end)
 
+        # Stop event handler with given reason
         throw({:error, reason})
 
       invalid ->
@@ -727,6 +740,16 @@ defmodule Commanded.Event.Handler do
     end
   end
 
+  defp log_event_error({:error, reason}, %RecordedEvent{} = failed_event, %Handler{} = state) do
+    Logger.error(fn ->
+      describe(state) <>
+        " failed to handle event " <>
+        inspect(failed_event, pretty: true) <>
+        " due to: " <>
+        inspect(reason, pretty: true)
+    end)
+  end
+
   # Confirm receipt of event
   defp confirm_receipt(%RecordedEvent{} = event, %Handler{} = state) do
     %Handler{


Test patch:
diff --git a/test/event/event_handler_error_handling_test.exs b/test/event/event_handler_error_handling_test.exs
index 7276ebd..d426b79 100644
--- a/test/event/event_handler_error_handling_test.exs
+++ b/test/event/event_handler_error_handling_test.exs
@@ -1,9 +1,17 @@
 defmodule Commanded.Event.EventHandlerErrorHandlingTest do
   use ExUnit.Case
 
+  import ExUnit.CaptureLog
+
   alias Commanded.DefaultApp
   alias Commanded.Event.FailureContext
-  alias Commanded.Event.ErrorAggregate.Events.{ErrorEvent, ExceptionEvent}
+
+  alias Commanded.Event.ErrorAggregate.Events.{
+    ErrorEvent,
+    ExceptionEvent,
+    InvalidReturnValueEvent
+  }
+
   alias Commanded.Event.ErrorEventHandler
   alias Commanded.Helpers.EventFactory
 
@@ -12,7 +20,7 @@ defmodule Commanded.Event.EventHandlerErrorHandlingTest do
 
     {:ok, handler} = ErrorEventHandler.start_link()
 
-    Process.unlink(handler)
+    true = Process.unlink(handler)
 
     [
       handler: handler,
@@ -22,8 +30,6 @@ defmodule Commanded.Event.EventHandlerErrorHandlingTest do
 
   describe "event handling exception handling" do
     test "should print the stack trace", %{handler: handler, ref: ref} do
-      import ExUnit.CaptureLog
-
       send_error_message = fn ->
         send_exception_event(handler)
 
@@ -64,6 +70,16 @@ defmodule Commanded.Event.EventHandlerErrorHandlingTest do
     end
   end
 
+  test "should call `error/3` callback function on invalid `handle/2` return value",
+       %{handler: handler, ref: ref} do
+    send_events_to_handler(handler, [
+      %InvalidReturnValueEvent{reply_to: reply_to()}
+    ])
+
+    assert_receive {:error, :invalid_return_value}
+    assert_receive {:DOWN, ^ref, :process, ^handler, :invalid_return_value}
+  end
+
   test "should stop event handler on error by default", %{handler: handler, ref: ref} do
     send_error_event(handler)
 
@@ -72,10 +88,8 @@ defmodule Commanded.Event.EventHandlerErrorHandlingTest do
     refute Process.alive?(handler)
   end
 
-  test "should stop event handler when invalid error response returned", %{
-    handler: handler,
-    ref: ref
-  } do
+  test "should stop event handler when invalid error response returned",
+       %{handler: handler, ref: ref} do
     send_error_event(handler, strategy: "invalid")
 
     assert_receive {:error, :invalid}
@@ -95,10 +109,8 @@ defmodule Commanded.Event.EventHandlerErrorHandlingTest do
     refute Process.alive?(handler)
   end
 
-  test "should retry event handler after delay on error", %{
-    handler: handler,
-    ref: ref
-  } do
+  test "should retry event handler after delay on error",
+       %{handler: handler, ref: ref} do
     send_error_event(handler, strategy: "retry", delay: 10)
 
     assert_receive {:error, :failed, %{failures: 1, delay: 10}}
diff --git a/test/event/support/error/error_aggregate.ex b/test/event/support/error/error_aggregate.ex
index 39cde88..8b8abee 100644
--- a/test/event/support/error/error_aggregate.ex
+++ b/test/event/support/error/error_aggregate.ex
@@ -23,6 +23,11 @@ defmodule Commanded.Event.ErrorAggregate do
       @derive Jason.Encoder
       defstruct [:uuid, :strategy, :delay, :reply_to]
     end
+
+    defmodule InvalidReturnValueEvent do
+      @derive Jason.Encoder
+      defstruct [:uuid, :reply_to]
+    end
   end
 
   alias Commanded.Event.ErrorAggregate
diff --git a/test/event/support/error/error_event_handler.ex b/test/event/support/error/error_event_handler.ex
index 7b960d7..2e32ea9 100644
--- a/test/event/support/error/error_event_handler.ex
+++ b/test/event/support/error/error_event_handler.ex
@@ -6,7 +6,12 @@ defmodule Commanded.Event.ErrorEventHandler do
     name: __MODULE__
 
   alias Commanded.Event.FailureContext
-  alias Commanded.Event.ErrorAggregate.Events.{ErrorEvent, ExceptionEvent}
+
+  alias Commanded.Event.ErrorAggregate.Events.{
+    ErrorEvent,
+    ExceptionEvent,
+    InvalidReturnValueEvent
+  }
 
   # Simulate event handling error reply
   def handle(%ErrorEvent{}, _metadata) do
@@ -18,6 +23,11 @@ defmodule Commanded.Event.ErrorEventHandler do
     raise "exception"
   end
 
+  # Simulate event handling returning an invalid value
+  def handle(%InvalidReturnValueEvent{}, _metadata) do
+    nil
+  end
+
   # Default behaviour is to stop the event handler with the given error reason
   def error({:error, reason}, %ErrorEvent{strategy: "default"} = event, _failure_context) do
     %ErrorEvent{reply_to: reply_to} = event
@@ -74,6 +84,14 @@ defmodule Commanded.Event.ErrorEventHandler do
     {:stop, error}
   end
 
+  def error({:error, error}, %InvalidReturnValueEvent{} = event, _failure_context) do
+    %InvalidReturnValueEvent{reply_to: reply_to} = event
+
+    send_reply(reply_to, {:error, error})
+
+    {:stop, error}
+  end
+
   defp record_failure(context) do
     Map.update(context, :failures, 1, fn failures -> failures + 1 end)
   end