From 865f9aa253607b3e96aea387dbb23974ac09c66d Mon Sep 17 00:00:00 2001
From: Zach Daniel <zach@zachdaniel.dev>
Date: Mon, 21 Nov 2022 02:51:37 -0500
Subject: [PATCH] improvement: return invalid primary key errors for `Api.get`
 when the input can't be cast

---
 lib/ash/error/invalid/invalid_primary_key.ex |  6 ++-
 lib/ash/filter/filter.ex                     | 56 +++++++++++++++++---
 test/actions/read_test.exs                   |  6 +++
 3 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/lib/ash/error/invalid/invalid_primary_key.ex b/lib/ash/error/invalid/invalid_primary_key.ex
index 971947a0..e4a5db03 100644
--- a/lib/ash/error/invalid/invalid_primary_key.ex
+++ b/lib/ash/error/invalid/invalid_primary_key.ex
@@ -9,8 +9,10 @@ defmodule Ash.Error.Invalid.InvalidPrimaryKey do
 
     def code(_), do: "invalid_primary_key"
 
-    def message(%{resource: resource, value: value}) do
-      "invalid primary key #{inspect(value)} provided for resource #{inspect(resource)}"
+    def message(%{resource: _resource, value: value}) do
+      # TODO: removed the resource so as not to leak it, but it would be nice to show it internally somehow
+      # perhaps an `internal_message` that is only shown on raised exceptions
+      "invalid primary key #{inspect(value)} provided"
     end
   end
 end
diff --git a/lib/ash/filter/filter.ex b/lib/ash/filter/filter.ex
index 90cb02e5..2f4905e4 100644
--- a/lib/ash/filter/filter.ex
+++ b/lib/ash/filter/filter.ex
@@ -409,14 +409,26 @@ defmodule Ash.Filter do
 
     case {primary_key, id} do
       {[field], [{field, value}]} ->
-        {:ok, %{field => value}}
+        case cast_value(resource, field, value, id) do
+          {:ok, value} ->
+            {:ok, %{field => value}}
+
+          {:error, error} ->
+            {:error, error}
+        end
 
       {[field], value} when not keyval? ->
-        {:ok, %{field => value}}
+        case cast_value(resource, field, value, id) do
+          {:ok, value} ->
+            {:ok, %{field => value}}
+
+          {:error, error} ->
+            {:error, error}
+        end
 
       {fields, value} ->
         if keyval? do
-          with :error <- get_keys(value, fields),
+          with :error <- get_keys(value, fields, resource),
                :error <- get_identity_filter(resource, id) do
             {:error, InvalidPrimaryKey.exception(resource: resource, value: id)}
           end
@@ -426,16 +438,30 @@ defmodule Ash.Filter do
     end
   end
 
-  defp get_keys(value, fields) do
+  defp get_keys(value, fields, resource) do
+    original_value = value
+
     Enum.reduce_while(fields, {:ok, %{}}, fn field, {:ok, vals} ->
       case fetch(value, field) do
         {:ok, value} ->
-          {:cont, {:ok, Map.put(vals, field, value)}}
+          case cast_value(resource, field, value, original_value) do
+            {:ok, value} ->
+              {:cont, {:ok, Map.put(vals, field, value)}}
+
+            {:error, error} ->
+              {:halt, {:error, error}}
+          end
 
         :error ->
           case fetch(value, to_string(field)) do
             {:ok, value} ->
-              {:cont, {:ok, Map.put(vals, field, value)}}
+              case cast_value(resource, field, value, original_value) do
+                {:ok, value} ->
+                  {:cont, {:ok, Map.put(vals, field, value)}}
+
+                {:error, error} ->
+                  {:error, error}
+              end
 
             :error ->
               {:halt, :error}
@@ -444,6 +470,22 @@ defmodule Ash.Filter do
     end)
   end
 
+  defp cast_value(resource, field, value, id) do
+    attribute = Ash.Resource.Info.attribute(resource, field)
+
+    if attribute do
+      case Ash.Type.cast_input(attribute.type, value, attribute.constraints) do
+        {:ok, value} ->
+          {:ok, value}
+
+        _ ->
+          {:error, InvalidPrimaryKey.exception(resource: resource, value: id)}
+      end
+    else
+      {:error, InvalidPrimaryKey.exception(resource: resource, value: id)}
+    end
+  end
+
   defp fetch(val, key) when is_map(val), do: Map.fetch(val, key)
   defp fetch(val, key) when is_list(val) and is_atom(key), do: Keyword.fetch(val, key)
   defp fetch(_, _), do: :error
@@ -454,7 +496,7 @@ defmodule Ash.Filter do
     |> Enum.find_value(
       :error,
       fn identity ->
-        case get_keys(id, identity.keys) do
+        case get_keys(id, identity.keys, resource) do
           {:ok, key} ->
             {:ok, key}
 
diff --git a/test/actions/read_test.exs b/test/actions/read_test.exs
index 3ed53c4d..df320abf 100644
--- a/test/actions/read_test.exs
+++ b/test/actions/read_test.exs
@@ -143,6 +143,12 @@ defmodule Ash.Test.Actions.ReadTest do
       assert ^post = strip_metadata(Api.get!(Post, post.id))
     end
 
+    test "it gives an invalid primary key error when invalid input is provided" do
+      assert_raise Ash.Error.Invalid, ~r/invalid primary key "not good"/, fn ->
+        Api.get!(Post, "not good")
+      end
+    end
+
     test "it raises when there is no matching record" do
       res =
         assert_raise Ash.Error.Invalid, fn ->