From 094958005051439324419cb355ac7e36d0303db8 Mon Sep 17 00:00:00 2001
From: Jechol Lee <mr.jechol@gmail.com>
Date: Fri, 21 Feb 2025 01:00:20 +0900
Subject: [PATCH] improvement: Generic actions to raise if they don't have
 return type but have an return value (#1805)

---
 lib/ash/actions/action.ex             | 27 +++++++-----
 test/actions/generic_actions_test.exs | 60 +++++++++++++++++++++------
 2 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/lib/ash/actions/action.ex b/lib/ash/actions/action.ex
index 722b4b61c..028b856e1 100644
--- a/lib/ash/actions/action.ex
+++ b/lib/ash/actions/action.ex
@@ -73,7 +73,11 @@ defmodule Ash.Actions.Action do
                         {:ok, nil, []}
 
                       {:ok, result} ->
-                        {:ok, result, []}
+                        if input.action.returns do
+                          {:ok, result, []}
+                        else
+                          raise_invalid_generic_action_return!(input, result)
+                        end
 
                       {:ok, result, notifications} ->
                         {:ok, result, notifications}
@@ -82,7 +86,7 @@ defmodule Ash.Actions.Action do
                         Ash.DataLayer.rollback(resources, error)
 
                       other ->
-                        raise_invalid_manual_action_return!(input, other)
+                        raise_invalid_generic_action_return!(input, other)
                     end
 
                   {:error, error} ->
@@ -144,7 +148,7 @@ defmodule Ash.Actions.Action do
                   if input.action.returns do
                     {:ok, result}
                   else
-                    :ok
+                    raise_invalid_generic_action_return!(input, result)
                   end
 
                 {:ok, result, notifications} ->
@@ -164,7 +168,7 @@ defmodule Ash.Actions.Action do
                   {:error, error}
 
                 other ->
-                  raise_invalid_manual_action_return!(input, other)
+                  raise_invalid_generic_action_return!(input, other)
               end
 
             {:error, error} ->
@@ -204,14 +208,17 @@ defmodule Ash.Actions.Action do
     end
   end
 
-  defp raise_invalid_manual_action_return!(input, other) do
-    raise """
-    Invalid return from generic action #{input.resource}.#{input.action.name}.
+  defp raise_invalid_generic_action_return!(input, other) do
+    ok_or_ok_tuple = if input.action.returns, do: "{:ok, result}", else: ":ok"
+
+    raise Ash.Error.Framework.InvalidReturnType,
+      message: """
+      Invalid return from generic action #{input.resource}.#{input.action.name}.
 
-    Expected {:ok, result} or {:error, error}, got:
+      Expected #{ok_or_ok_tuple} or {:error, error}, got:
 
-    #{inspect(other)}
-    """
+      #{inspect(other)}
+      """
   end
 
   defp authorize(_domain, _actor, %{context: %{private: %{authorize?: false}}}) do
diff --git a/test/actions/generic_actions_test.exs b/test/actions/generic_actions_test.exs
index 32db71c25..4de0211b5 100644
--- a/test/actions/generic_actions_test.exs
+++ b/test/actions/generic_actions_test.exs
@@ -49,12 +49,20 @@ defmodule Ash.Test.Actions.GenericActionsTest do
         run fn _, _ -> :ok end
       end
 
-      action :action_without_return_type do
-        run fn _, _ -> {:ok, [:this, :will, :not, :be, :used]} end
+      action :typed_with_value, :integer do
+        run fn _, _ -> {:ok, 100} end
       end
 
-      action :action_with_return_type, {:array, :atom} do
-        run fn _, _ -> {:ok, [:this, :will, :be, :used]} end
+      action :untyped_without_value do
+        run fn _, _ -> :ok end
+      end
+
+      action :typed_without_value, :integer do
+        run fn _, _ -> :ok end
+      end
+
+      action :untyped_with_value do
+        run fn _, _ -> {:ok, :unexpected} end
       end
     end
 
@@ -74,11 +82,19 @@ defmodule Ash.Test.Actions.GenericActionsTest do
         authorize_if always()
       end
 
-      policy action(:action_without_return_type) do
+      policy action(:typed_with_value) do
+        authorize_if always()
+      end
+
+      policy action(:untyped_without_value) do
         authorize_if always()
       end
 
-      policy action(:action_with_return_type) do
+      policy action(:typed_without_value) do
+        authorize_if always()
+      end
+
+      policy action(:untyped_with_value) do
         authorize_if always()
       end
     end
@@ -125,19 +141,39 @@ defmodule Ash.Test.Actions.GenericActionsTest do
       end
     end
 
-    test "generic actions return :ok if they don't have a return type" do
-      assert :ok =
+    test "generic actions return the value if they have a return type and a return value" do
+      assert 100 =
                Post
-               |> Ash.ActionInput.for_action(:action_without_return_type, %{})
+               |> Ash.ActionInput.for_action(:typed_with_value, %{})
                |> Ash.run_action!()
     end
 
-    test "generic actions return the value if they have a return type" do
-      assert [:this, :will, :be, :used] =
+    test "generic actions return :ok if they don't have a return type and a return value" do
+      assert :ok =
                Post
-               |> Ash.ActionInput.for_action(:action_with_return_type, %{})
+               |> Ash.ActionInput.for_action(:untyped_without_value, %{})
                |> Ash.run_action!()
     end
+
+    test "generic actions raise if they have a return type but don't have a return value" do
+      assert_raise Ash.Error.Framework.InvalidReturnType,
+                   ~r/Expected {:ok, result} or {:error, error}/,
+                   fn ->
+                     Post
+                     |> Ash.ActionInput.for_action(:typed_without_value, %{})
+                     |> Ash.run_action!()
+                   end
+    end
+
+    test "generic actions raise if they don't have a return type but have an return value" do
+      assert_raise Ash.Error.Framework.InvalidReturnType,
+                   ~r/Expected :ok or {:error, error}/,
+                   fn ->
+                     Post
+                     |> Ash.ActionInput.for_action(:untyped_with_value, %{})
+                     |> Ash.run_action!()
+                   end
+    end
   end
 
   describe "authorization" do