From 59ddb5f5af8dbf1cd9c7f492552c279233bef622 Mon Sep 17 00:00:00 2001 From: Robert Ellen Date: Tue, 22 Nov 2022 09:32:25 +1000 Subject: [PATCH] improvement: add error context to error creation / normalisation (#440) --- lib/ash/error/error.ex | 279 +++++++++++++++++++++++++++++++++--- lib/ash/error/exception.ex | 1 + lib/ash/filter/filter.ex | 19 ++- lib/ash/query/query.ex | 16 ++- test/error_test.exs | 201 ++++++++++++++++++++++++++ test/filter/filter_test.exs | 35 +++++ 6 files changed, 524 insertions(+), 27 deletions(-) create mode 100644 test/error_test.exs diff --git a/lib/ash/error/error.ex b/lib/ash/error/error.ex index c6350708..2a589475 100644 --- a/lib/ash/error/error.ex +++ b/lib/ash/error/error.ex @@ -78,6 +78,108 @@ defmodule Ash.Error do !!Ash.ErrorKind.impl_for(value) end + @doc """ + Conforms a term into one of the built-in Ash [Error classes](handle-errors.html#error-classes). + + The provided term would usually be an Ash Error or a list of Ash Errors. + + If the term is: + - a map/struct/Ash Error with a key `:class` having a value `:special`, + - a list with a single map/struct/Ash Error element as above, or + - an `Ash.Error.Invalid` containing such a list in its `:errors` field + + then the term is returned unchanged. + + Example: + ```elixir + + iex(1)> Ash.Error.to_error_class("oops", changeset: Ash.Changeset.new(%Post{}), error_context: "some context") + %Ash.Error.Unknown{ + changeset: #Ash.Changeset< + errors: [ + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "oops", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + } + ], + ... + >, + class: :unknown, + error_context: ["some context"], + errors: [ + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "oops", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + } + ], + stacktrace: #Stacktrace<>, + stacktraces?: true, + vars: [] + } + + ``` + + Example of nested errors: + ```elixir + iex(1)> error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + iex(2)> error2 = Ash.Error.to_ash_error("whoops, again!!", nil, error_context: "some other context") + iex(3)> Ash.Error.to_error_class([error1, error2], error_context: "some higher context") + %Ash.Error.Unknown{ + changeset: nil, + class: :unknown, + error_context: ["some higher context"], + errors: [ + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops!", + error_context: ["some higher context", "some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + }, + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops, again!!", + error_context: ["some higher context", "some other context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + } + ], + path: [], + query: nil, + stacktrace: #Stacktrace<>, + stacktraces?: true, + vars: [] + } + + ``` + + Options: + - `changeset`: a changeset related to the error + - `query`: a query related to the error + - `error_context`: a sting message providing extra context around the error + """ def to_error_class(values, opts \\ []) def to_error_class(%{class: :special} = special, _opts) do @@ -111,51 +213,144 @@ defmodule Ash.Error do end) |> Enum.uniq() - choose_error(values, opts[:changeset] || opts[:query]) + values + |> accumulate_error_context(opts[:error_context]) + |> choose_error(opts[:changeset] || opts[:query]) + |> add_error_context(opts[:error_context]) end end def to_error_class(value, opts) do if ash_error?(value) && value.__struct__ in Keyword.values(@error_modules) do - add_changeset_or_query(value, [value], opts[:changeset] || opts[:query]) + value + |> add_changeset_or_query([value], opts[:changeset] || opts[:query]) + |> Map.put(:error_context, [opts[:error_context] | value.error_context]) else to_error_class([value], opts) end end - def to_ash_error(list, stacktrace \\ nil) + @doc """ + Converts a term into an Ash Error. - def to_ash_error(list, stacktrace) when is_list(list) do + The term could be a simple string, the second element in an `{:error, error}` tuple, an Ash Error, or a list of any of these. + In most cases the returned error is an Ash.Error.Unknown.UnknownError. + + A stacktrace is added to the error, and any existing stacktrace (i.e. when the term is an Ash Error) is preserved. + + `to_ash_error` converts string(s) into UnknownError(s): + ```elixir + iex(1)> Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops!", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + } + + iex(2)> Ash.Error.to_ash_error(["whoops!", "whoops, again!!"], nil, error_context: "some context") + [ + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops!", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + }, + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops, again!!", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [] + } + ] + ``` + + `to_ash_error` can preserve error-like data from a keyword-list and accumulate context if called against an Ash Error: + ```elixir + iex(1)> err = Ash.Error.to_ash_error([vars: [:some_var], message: "whoops!"], nil, error_context: " some context") + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops!", + error_context: ["some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [:some_var] + } + iex(2)> Ash.Error.to_ash_error(err, nil, error_context: "some higher context") + %Ash.Error.Unknown.UnknownError{ + changeset: nil, + class: :unknown, + error: "whoops!", + error_context: ["some higher context", "some context"], + field: nil, + path: [], + query: nil, + stacktrace: #Stacktrace<>, + vars: [:some_var] + } + ``` + + Options: + - `error_context`: a sting message providing extra context around the error + """ + def to_ash_error(list, stacktrace \\ nil, opts \\ []) + + def to_ash_error(list, stacktrace, opts) when is_list(list) do if Keyword.keyword?(list) do - error = - list - |> Keyword.take([:error, :vars]) - |> Keyword.put_new(:error, list[:message]) - |> UnknownError.exception() - - add_stacktrace(error, stacktrace) + list + |> Keyword.take([:error, :vars]) + |> Keyword.put_new(:error, list[:message]) + |> UnknownError.exception() + |> add_stacktrace(stacktrace) + |> add_error_context(opts[:error_context]) else - Enum.map(list, &to_ash_error(&1, stacktrace)) + Enum.map(list, &to_ash_error(&1, stacktrace, opts)) end end - def to_ash_error(error, stacktrace) when is_binary(error) do - add_stacktrace(UnknownError.exception(error: error), stacktrace) + def to_ash_error(error, stacktrace, opts) when is_binary(error) do + [error: error] + |> UnknownError.exception() + |> add_stacktrace(stacktrace) + |> add_error_context(opts[:error_context]) end - def to_ash_error(other, stacktrace) do + def to_ash_error(other, stacktrace, opts) do cond do ash_error?(other) -> - add_stacktrace(other, stacktrace) + other + |> add_stacktrace(stacktrace) + |> add_error_context(opts[:error_context]) is_exception(other) -> - error = UnknownError.exception(error: Exception.format(:error, other)) - - add_stacktrace(error, stacktrace) + [error: Exception.format(:error, other)] + |> UnknownError.exception() + |> add_stacktrace(stacktrace) + |> add_error_context(opts[:error_context]) true -> - error = UnknownError.exception(error: "unknown error: #{inspect(other)}") - add_stacktrace(error, stacktrace) + [error: "unknown error: #{inspect(other)}"] + |> UnknownError.exception() + |> add_stacktrace(stacktrace) + |> add_error_context(opts[:error_context]) end end @@ -182,6 +377,35 @@ defmodule Ash.Error do %{error | stacktrace: stacktrace} end + defp add_error_context(error, error_context) when is_binary(error_context) do + %{error | error_context: [error_context | error.error_context]} + end + + defp add_error_context(error, _) do + error + end + + defp accumulate_error_context(%{errors: [_ | _] = errors} = error, error_context) + when is_binary(error_context) do + updated_errors = accumulate_error_context(errors, error_context) + + %{error | errors: updated_errors} + end + + defp accumulate_error_context(errors, error_context) + when is_list(errors) and is_binary(error_context) do + errors + |> Enum.map(fn err -> + err + |> add_error_context(error_context) + |> accumulate_error_context(error_context) + end) + end + + defp accumulate_error_context(error, _) do + error + end + @doc "A utility to flatten a list, but preserve keyword list elements" def flatten_preserving_keywords(list) do if Keyword.keyword?(list) do @@ -282,7 +506,8 @@ defmodule Ash.Error do "* #{error}" %{stacktrace: %Stacktrace{stacktrace: stacktrace}} = class_error -> - "* #{Exception.message(class_error)}\n" <> + breadcrumb(class_error.error_context) <> + "* #{Exception.message(class_error)}\n" <> path(class_error) <> Enum.map_join(stacktrace, "\n", fn stack_item -> " " <> Exception.format_stacktrace_entry(stack_item) @@ -295,7 +520,8 @@ defmodule Ash.Error do "* #{class_error}" class_error -> - "* #{Exception.message(class_error)}" + breadcrumb(class_error.error_context) <> + "* #{Exception.message(class_error)}" end) end end) @@ -348,4 +574,11 @@ defmodule Ash.Error do defp header(:forbidden), do: "Forbidden" defp header(:framework), do: "Framework Error" defp header(:unknown), do: "Unknown Error" + + defp breadcrumb(nil), do: "" + defp breadcrumb([]), do: "" + + defp breadcrumb(error_context) do + "Context: " <> Enum.join(error_context, " > ") <> "\n" + end end diff --git a/lib/ash/error/exception.ex b/lib/ash/error/exception.ex index b51126eb..16ea6633 100644 --- a/lib/ash/error/exception.ex +++ b/lib/ash/error/exception.ex @@ -13,6 +13,7 @@ defmodule Ash.Error.Exception do [ :changeset, :query, + error_context: [], vars: [], path: [], stacktrace: [], diff --git a/lib/ash/filter/filter.ex b/lib/ash/filter/filter.ex index 2f4905e4..db2303ca 100644 --- a/lib/ash/filter/filter.ex +++ b/lib/ash/filter/filter.ex @@ -236,7 +236,9 @@ defmodule Ash.Filter do filter {:error, error} -> - raise Ash.Error.to_error_class(error) + raise Ash.Error.to_error_class(error, + error_context: parse_error_context(resource, statement, context) + ) end end @@ -929,7 +931,9 @@ defmodule Ash.Filter do value {:error, error} -> - raise Ash.Error.to_ash_error(error) + raise Ash.Error.to_ash_error(error, nil, + error_context: parse_error_context(base.resource, addition, context) + ) end end @@ -966,6 +970,17 @@ defmodule Ash.Filter do end end + defp parse_error_context(%{resource: resource} = _filter, addition, context) do + parse_error_context(resource, addition, context) + end + + defp parse_error_context(resource, addition, context) do + context_str = if context, do: ", given context: #{inspect(context)}", else: "" + + "parsing addition of filter statement: #{inspect(addition)}, to resource: #{inspect(resource)}" <> + context_str + end + @doc """ Returns true if the second argument is a strict subset (always returns the same or less data) of the first """ diff --git a/lib/ash/query/query.ex b/lib/ash/query/query.ex index 388fc79b..83e1bb8c 100644 --- a/lib/ash/query/query.ex +++ b/lib/ash/query/query.ex @@ -911,10 +911,22 @@ defmodule Ash.Query do %{query | aggregates: new_aggregates} else %{errors: errors} -> - add_error(query, :aggregates, Ash.Error.to_ash_error(errors)) + add_error( + query, + :aggregates, + Ash.Error.to_ash_error(errors, nil, + error_context: "Loading aggregate: #{inspect(field)} for query: #{inspect(query)}" + ) + ) {:error, error} -> - add_error(query, :aggregates, Ash.Error.to_ash_error(error)) + add_error( + query, + :aggregates, + Ash.Error.to_ash_error(error, nil, + error_context: "Loading aggregate: #{inspect(field)} for query: #{inspect(query)}" + ) + ) {:can?, false} -> add_error( diff --git a/test/error_test.exs b/test/error_test.exs new file mode 100644 index 00000000..921e97db --- /dev/null +++ b/test/error_test.exs @@ -0,0 +1,201 @@ +defmodule Ash.Test.ErrorTest do + @moduledoc false + use ExUnit.Case, async: true + + defmodule TestError do + use Ash.Error.Exception + def_ash_error([:some_field]) + end + + defmodule TestResource do + use Ash.Resource, data_layer: Ash.DataLayer.Ets + + actions do + defaults [:create, :read, :update, :destroy] + end + + attributes do + uuid_primary_key :id + end + end + + describe "to_error_class" do + test "returns exception if it is a map/struct with class: :special" do + assert Ash.Error.to_error_class(%{class: :special}, []) == %{class: :special} + end + + test "returns exception if it is a map/struct with class: :special wrapped in a list" do + assert Ash.Error.to_error_class([%{class: :special}], []) == %{class: :special} + end + + test "returns exception if it is a map/struct with class: :special wrapped in an Invalid error" do + err = Ash.Error.Invalid.exception(errors: [%{class: :special}]) + + assert Ash.Error.to_error_class(err, []) == %{class: :special} + end + + test "returns chosen error if the value argument is a list of values" do + values = [[a: "a", b: "b"], [c: "c", d: "d"], "foo", "bar"] + + result = Ash.Error.to_error_class(values, []) + + assert match?(%Ash.Error.Unknown{}, result) + + # given the test arrangement, each error in the error class should + # * be an Ash.Error.Unknown.UnknownError + # * have a .class == :unknown + # * have a .error that is a distinct element of the uniq subset of the values provided + + assert same_elements?(Enum.map(result.errors, fn err -> err.error end), values) + + for err <- result.errors do + assert match?(%Ash.Error.Unknown.UnknownError{}, err) + assert err.class == :unknown + end + end + + test "returns chosen error if the value argument is a list of errors" do + err1 = Ash.Error.Unknown.UnknownError.exception(error: :an_error) + err2 = Ash.Error.Invalid.exception(errors: [:more, :errors]) + + result = Ash.Error.to_error_class([err1, err2], []) + + # the parent error will be of the invalid class because it take precedence over unknown + assert match?(%Ash.Error.Invalid{}, result) + + # the parent error's errors field gets prepended to the list of other errors + assert same_elements?(result.errors, [:more, :errors, err1]) + end + + test "has a context field populated when there is a single error" do + test_error = TestError.exception([]) + + err = Ash.Error.to_error_class(test_error, error_context: "some context") + + assert err.error_context == ["some context"] + end + + test "has a context field populated when there is a list of errors" do + test_error1 = TestError.exception(some_field: :a) + test_error2 = TestError.exception(some_field: :b) + + err = Ash.Error.to_error_class([test_error1, test_error2], error_context: "some context") + + assert err.error_context == ["some context"] + end + + test "accumulates error_context field in child errors" do + error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + error2 = Ash.Error.to_ash_error("whoops, again!!", nil, error_context: "some other context") + + error_class = + Ash.Error.to_error_class([error1, error2], error_context: "some higher context") + + child_error_1 = Enum.find(error_class.errors, fn err -> err.error == "whoops!" end) + assert child_error_1.error_context == ["some higher context", "some context"] + + child_error_2 = Enum.find(error_class.errors, fn err -> err.error == "whoops, again!!" end) + assert child_error_2.error_context == ["some higher context", "some other context"] + end + + test "accumulates error_context field in child errors who have no error_context of their own" do + error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + error2 = Ash.Error.to_ash_error("whoops, again!!", nil) + + error_class = + Ash.Error.to_error_class([error1, error2], error_context: "some higher context") + + child_error_1 = Enum.find(error_class.errors, fn err -> err.error == "whoops!" end) + assert child_error_1.error_context == ["some higher context", "some context"] + + child_error_2 = Enum.find(error_class.errors, fn err -> err.error == "whoops, again!!" end) + assert child_error_2.error_context == ["some higher context"] + end + + test "leaves child error contexts unchanged if no error_context field provided" do + error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + error2 = Ash.Error.to_ash_error("whoops, again!!", nil, error_context: "some other context") + + error_class = Ash.Error.to_error_class([error1, error2]) + + child_error_1 = Enum.find(error_class.errors, fn err -> err.error == "whoops!" end) + assert child_error_1.error_context == ["some context"] + + child_error_2 = Enum.find(error_class.errors, fn err -> err.error == "whoops, again!!" end) + assert child_error_2.error_context == ["some other context"] + end + + test "error message contains error context breadcrumbs" do + error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + error2 = Ash.Error.to_ash_error("whoops, again!!", nil, error_context: "some other context") + + error_class = + Ash.Error.to_error_class([error1, error2], error_context: "some higher context") + + error_message = Ash.Error.Unknown.message(error_class) + + assert error_message =~ "Context: some higher context > some context" + assert error_message =~ "Context: some higher context > some other context" + end + + test "error message still renders when there's no error context" do + error1 = Ash.Error.to_ash_error("whoops!") + error2 = Ash.Error.to_ash_error("whoops, again!!") + + error_class = Ash.Error.to_error_class([error1, error2]) + + error_message = Ash.Error.Unknown.message(error_class) + + assert error_message =~ "Unknown Error\n\n* whoops!" + end + + test "has a context field populated in changeset" do + test_error = TestError.exception([]) + + cs = Ash.Changeset.for_create(TestResource, :create) + + err = Ash.Error.to_error_class(test_error, changeset: cs, error_context: "some context") + + assert err.error_context == ["some context"] + + [cs_error] = err.changeset.errors + assert cs_error.error_context == ["some context"] + end + + test "accumulates error_context field in changeset's copy of error hierarchy" do + error1 = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + error2 = Ash.Error.to_ash_error("whoops, again!!", nil, error_context: "some other context") + + cs = Ash.Changeset.for_create(TestResource, :create) + + error_class = + Ash.Error.to_error_class([error1, error2], + changeset: cs, + error_context: "some higher context" + ) + + cs_child_error_1 = + Enum.find(error_class.changeset.errors, fn err -> err.error == "whoops!" end) + + cs_child_error_2 = + Enum.find(error_class.changeset.errors, fn err -> err.error == "whoops, again!!" end) + + assert cs_child_error_1.error_context == ["some higher context", "some context"] + assert cs_child_error_2.error_context == ["some higher context", "some other context"] + end + end + + describe "to_ash_error" do + test "populates error_context field" do + error = Ash.Error.to_ash_error("whoops!", nil, error_context: "some context") + + assert error.error_context == ["some context"] + end + end + + defp same_elements?(xs, ys) when is_list(xs) and is_list(ys) do + Enum.sort(xs) == Enum.sort(ys) + end + + defp same_elements?(_, _), do: false +end diff --git a/test/filter/filter_test.exs b/test/filter/filter_test.exs index eda5d531..fd57d755 100644 --- a/test/filter/filter_test.exs +++ b/test/filter/filter_test.exs @@ -593,6 +593,41 @@ defmodule Ash.Test.Filter.FilterTest do assert Filter.strict_subset_of?(filter, candidate) end + + test "raises an error if the underlying parse returns an error" do + filter = Filter.parse!(Post, points: 1) + + err = + assert_raise(Ash.Error.Query.NoSuchAttributeOrRelationship, fn -> + Filter.add_to_filter!(filter, bad_field: "bad field") + end) + + [error_context] = err.error_context + assert error_context =~ "parsing addition of filter statement: [bad_field: \"bad field\"]" + assert error_context =~ ", to resource: " <> (Post |> Module.split() |> Enum.join(".")) + end + end + + describe "parse!" do + test "raises an error if the statement is invalid" do + filter = Filter.parse!(Post, points: 1) + + err = + assert_raise(Ash.Error.Invalid, fn -> + Filter.parse!(filter, 1) + end) + + [error_context] = err.error_context + assert error_context =~ "parsing addition of filter statement: 1" + assert error_context =~ ", to resource: " <> (Post |> Module.split() |> Enum.join(".")) + + [inner_error] = err.errors + [inner_error_context] = inner_error.error_context + assert inner_error_context =~ "parsing addition of filter statement: 1" + + assert inner_error_context =~ + ", to resource: " <> (Post |> Module.split() |> Enum.join(".")) + end end describe "parse_input" do