From 8233634bb133c6345e6b15011542dd6b6e023e67 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 15 Aug 2023 16:23:06 -0700 Subject: [PATCH] improvement: properly set path into error instead of on error messages TBD: this may break some people's tests, and so may need to be reverted or released as part of 3.0 --- lib/ash/embeddable_type.ex | 21 ++----------- lib/ash/error/error.ex | 2 +- lib/ash/type/helpers.ex | 36 ++++++++++----------- lib/ash/type/type.ex | 7 +++-- test/embedded_resource_test.exs | 39 ++++++++++++----------- test/type/keyword_test.exs | 27 ++++++++-------- test/type/map_test.exs | 27 ++++++++-------- test/type/union_test.exs | 56 +++++++++++++++++++++++++++++++-- 8 files changed, 128 insertions(+), 87 deletions(-) diff --git a/lib/ash/embeddable_type.ex b/lib/ash/embeddable_type.ex index e9d6be47..2e97c84d 100644 --- a/lib/ash/embeddable_type.ex +++ b/lib/ash/embeddable_type.ex @@ -111,20 +111,17 @@ defmodule Ash.EmbeddableType do vars |> Keyword.put(:field, field) |> Keyword.put(:message, message) - |> add_index() end defp do_handle_errors(%{message: message, vars: vars, field: field}) do vars |> Keyword.put(:message, message) |> Keyword.put(:field, field) - |> add_index() end defp do_handle_errors(%{message: message, vars: vars}) do vars |> Keyword.put(:message, message) - |> add_index() end defp do_handle_errors(%{field: field} = exception) do @@ -143,20 +140,6 @@ defmodule Ash.EmbeddableType do [message: "Something went wrong"] end - defp add_index(opts) do - opts - # cond do - # opts[:index] && opts[:field] -> - # Keyword.put(opts, :field, "#{opts[:field]}[#{opts[:index]}]") - - # opts[:index] -> - # Keyword.put(opts, :field, "[#{opts[:index]}]") - - # true -> - # opts - # end - end - defmacro single_embed_implementation do # credo:disable-for-next-line Credo.Check.Refactor.LongQuoteBlocks quote location: :keep do @@ -625,7 +608,9 @@ defmodule Ash.EmbeddableType do error |> Ash.EmbeddableType.handle_errors() |> Enum.map(fn keyword -> - Keyword.put(keyword, :index, index) + keyword + |> Keyword.put(:index, index) + |> Keyword.update(:path, [index], &[index | &1]) end) {:halt, {:error, errors}} diff --git a/lib/ash/error/error.ex b/lib/ash/error/error.ex index 09e916a8..af31558e 100644 --- a/lib/ash/error/error.ex +++ b/lib/ash/error/error.ex @@ -519,7 +519,7 @@ defmodule Ash.Error do class_error -> breadcrumb(class_error.error_context) <> - "* #{Exception.message(class_error)}" + "* #{Exception.message(class_error)}" <> path(class_error) end) end end) diff --git a/lib/ash/type/helpers.ex b/lib/ash/type/helpers.ex index 237ceb70..da6798d0 100644 --- a/lib/ash/type/helpers.ex +++ b/lib/ash/type/helpers.ex @@ -71,7 +71,7 @@ defmodule Ash.Type.Helpers do keyword |> Keyword.put( :message, - add_index(keyword[:message], keyword) + keyword[:message] ) |> Keyword.put(:field, attribute.name) ] @@ -79,10 +79,12 @@ defmodule Ash.Type.Helpers do fields -> Enum.map( fields, - &Keyword.merge(message, - field: attribute.name, - message: add_index(add_field(keyword[:message], "#{&1}"), keyword) - ) + fn field -> + keyword + |> Keyword.put(:field, field) + |> Keyword.delete(:fields) + |> Keyword.update(:path, [attribute.name], &[attribute.name | &1]) + end ) end @@ -90,24 +92,18 @@ defmodule Ash.Type.Helpers do [[field: attribute.name, message: message]] value when is_exception(value) -> - value - |> Ash.Error.to_ash_error() - |> Map.put(:field, attribute.name) + exception = + value + |> Ash.Error.to_ash_error() + + if Map.get(exception, :field) || Map.get(exception, :fields) do + Ash.Error.set_path(exception, attribute.name) + else + Map.put(exception, :field, attribute.name) + end _ -> [[field: attribute.name]] end end - - defp add_field(message, field) do - "at field #{field} " <> (message || "") - end - - defp add_index(message, opts) do - if opts[:index] do - "at index #{opts[:index]} " <> (message || "") - else - message - end - end end diff --git a/lib/ash/type/type.ex b/lib/ash/type/type.ex index bf4a7024..1c0ddff6 100644 --- a/lib/ash/type/type.ex +++ b/lib/ash/type/type.ex @@ -446,7 +446,8 @@ defmodule Ash.Type do |> Enum.reduce_while({:ok, []}, fn {item, index}, {:ok, casted} -> case cast_input(type, item, single_constraints) do :error -> - {:halt, {:error, message: "invalid value at %{index}", index: index}} + {:halt, + {:error, message: "invalid value at %{index}", index: index, path: [index]}} {:error, keyword} -> errors = @@ -458,7 +459,9 @@ defmodule Ash.Type do [message: message, index: index, path: [index]] keyword -> - Keyword.merge(keyword, index: index, path: [index]) + keyword + |> Keyword.put(:index, index) + |> Keyword.update(:path, [index], &[index | &1]) end) {:halt, {:error, errors}} diff --git a/test/embedded_resource_test.exs b/test/embedded_resource_test.exs index 00f777a2..348b4100 100644 --- a/test/embedded_resource_test.exs +++ b/test/embedded_resource_test.exs @@ -237,7 +237,7 @@ defmodule Ash.Test.Changeset.EmbeddedResourceTest do test "embedded resources run validations on create" do msg = - ~r/Invalid value provided for profile: at field last_name exactly 2 of first_name,last_name must be present/ + ~r/Invalid value provided for last_name: exactly 2 of first_name,last_name must be present/ assert_raise Ash.Error.Invalid, msg, @@ -272,7 +272,7 @@ defmodule Ash.Test.Changeset.EmbeddedResourceTest do input = %{counter: author.profile.counter - 1} assert_raise Ash.Error.Invalid, - ~r/Invalid value provided for profile: at field counter must be increasing/, + ~r/Invalid value provided for counter: must be increasing/, fn -> Changeset.for_update( author, @@ -384,7 +384,7 @@ defmodule Ash.Test.Changeset.EmbeddedResourceTest do |> Api.create!() assert_raise Ash.Error.Invalid, - ~r/Invalid value provided for tags: at index 0 at field score must be present/, + ~r/Invalid value provided for score: must be present/, fn -> Changeset.for_update( author, @@ -399,7 +399,7 @@ defmodule Ash.Test.Changeset.EmbeddedResourceTest do end assert_raise Ash.Error.Invalid, - ~r/Invalid value provided for tags: at index 0 at field score must be absent/, + ~r/Invalid value provided for score: must be absent/, fn -> Changeset.for_update( author, @@ -428,20 +428,23 @@ defmodule Ash.Test.Changeset.EmbeddedResourceTest do ) |> Api.create!() - assert_raise Ash.Error.Invalid, - ~r/Invalid value provided for tags_with_id: at index 0 at field score must be increasing/, - fn -> - Changeset.for_update( - author, - :update, - %{ - tags_with_id: [ - %{id: tag.id, score: 1} - ] - } - ) - |> Api.update!() - end + exception = + assert_raise Ash.Error.Invalid, + ~r/Invalid value provided for score: must be increasing/, + fn -> + Changeset.for_update( + author, + :update, + %{ + tags_with_id: [ + %{id: tag.id, score: 1} + ] + } + ) + |> Api.update!() + end + + assert Enum.at(exception.errors, 0).path == [:tags_with_id, 0] Changeset.for_update( author, diff --git a/test/type/keyword_test.exs b/test/type/keyword_test.exs index c16bd79e..e97991d3 100644 --- a/test/type/keyword_test.exs +++ b/test/type/keyword_test.exs @@ -115,15 +115,15 @@ defmodule Type.KeywordTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field foo field must be present", + field: :foo, + message: "field must be present", private_vars: nil, value: [bar: 1], changeset: nil, query: nil, error_context: [], - vars: [field: :metadata, message: "at field foo field must be present"], - path: [] + vars: [field: :foo, message: "field must be present", path: [:metadata]], + path: [:metadata] } ] = changeset.errors end @@ -139,18 +139,19 @@ defmodule Type.KeywordTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field bar must be more than or equal to %{min}", + field: :bar, + message: "must be more than or equal to %{min}", private_vars: nil, value: [foo: "hello", bar: -1], changeset: nil, query: nil, error_context: [], vars: [ - field: :metadata, - message: "at field bar must be more than or equal to %{min}" + field: :bar, + message: "must be more than or equal to %{min}", + path: [:metadata] ], - path: [] + path: [:metadata] } ] = changeset.errors end @@ -192,15 +193,15 @@ defmodule Type.KeywordTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field foo value must not be nil", + field: :foo, + message: "value must not be nil", private_vars: nil, value: [foo: "", bar: "2"], changeset: nil, query: nil, error_context: [], - vars: [field: :metadata, message: "at field foo value must not be nil"], - path: [] + vars: [field: :foo, message: "value must not be nil", path: [:metadata]], + path: [:metadata] } ] = changeset.errors end diff --git a/test/type/map_test.exs b/test/type/map_test.exs index 9f6a0135..104758d6 100644 --- a/test/type/map_test.exs +++ b/test/type/map_test.exs @@ -117,15 +117,15 @@ defmodule Type.MapTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field foo field must be present", + field: :foo, + message: "field must be present", private_vars: nil, value: %{bar: 1}, changeset: nil, query: nil, error_context: [], - vars: [field: :metadata, message: "at field foo field must be present"], - path: [] + vars: [field: :foo, message: "field must be present", path: [:metadata]], + path: [:metadata] } ] = changeset.errors end @@ -141,18 +141,19 @@ defmodule Type.MapTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field bar must be more than or equal to %{min}", + field: :bar, + message: "must be more than or equal to %{min}", private_vars: nil, value: %{bar: -1, foo: "hello"}, changeset: nil, query: nil, error_context: [], vars: [ - field: :metadata, - message: "at field bar must be more than or equal to %{min}" + field: :bar, + message: "must be more than or equal to %{min}", + path: [:metadata] ], - path: [] + path: [:metadata] } ] = changeset.errors end @@ -194,15 +195,15 @@ defmodule Type.MapTest do assert [ %Ash.Error.Changes.InvalidAttribute{ - field: :metadata, - message: "at field foo value must not be nil", + field: :foo, + message: "value must not be nil", private_vars: nil, value: %{:bar => "2", "foo" => ""}, changeset: nil, query: nil, error_context: [], - vars: [field: :metadata, message: "at field foo value must not be nil"], - path: [] + vars: [field: :foo, message: "value must not be nil", path: [:metadata]], + path: [:metadata] } ] = changeset.errors end diff --git a/test/type/union_test.exs b/test/type/union_test.exs index 80839c1a..33f73d70 100644 --- a/test/type/union_test.exs +++ b/test/type/union_test.exs @@ -28,6 +28,39 @@ defmodule Ash.Test.Type.UnionTest do end end + defmodule Example do + use Ash.Resource, data_layer: Ash.DataLayer.Ets + + ets do + private? true + end + + actions do + defaults [:create, :read, :update, :destroy] + end + + attributes do + uuid_primary_key :id + + attribute :things, {:array, :union} do + constraints items: [ + types: [ + foo: [ + type: Foo, + tag: :type, + tag_value: :foo + ], + bar: [ + type: Bar, + tag: :type, + tag_value: :bar + ] + ] + ] + end + end + end + test "it handles simple types" do constraints = [ types: [ @@ -107,7 +140,26 @@ defmodule Ash.Test.Type.UnionTest do assert {:ok, [%Ash.Union{value: %{type: "foo", foo: "foo"}, type: :foo}]} = Ash.Type.cast_input(type, [%{type: :foo, foo: "foo"}], constraints) - # assert {:ok, [%Ash.Union{value: %{type: "foo", foo: "foo"}, type: :foo}]} = - Ash.Type.cast_input(type, [%{type: :foo, foo: "bar"}], constraints) |> IO.inspect() + assert {:error, + [ + error + ]} = + Ash.Type.cast_input(type, [%{type: :foo, foo: "bar"}], constraints) + + for {key, val} <- [ + message: "must match the pattern %{regex}", + field: :foo, + regex: "~r/foo/", + index: 0, + path: [0] + ] do + assert error[key] == val + end + end + + test "it handles paths on a resource" do + Example + |> Ash.Changeset.for_create(:create, %{things: [%{type: :foo, foo: "bar"}]}) + |> Ash.Test.AnyApi.create() end end