From a479970e2342d7fd3ec09a452feab3c29413147d Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Mon, 21 Nov 2022 01:37:58 -0500 Subject: [PATCH] improvement: much more readable errors when building loads --- lib/ash/error/error.ex | 4 +- lib/ash/error/exception.ex | 4 +- lib/ash/error/side_load/invalid_query.ex | 13 +++++- lib/ash/query/query.ex | 56 +++++++++--------------- test/actions/load_test.exs | 8 ++++ 5 files changed, 45 insertions(+), 40 deletions(-) diff --git a/lib/ash/error/error.ex b/lib/ash/error/error.ex index b3b3a3fe..c6350708 100644 --- a/lib/ash/error/error.ex +++ b/lib/ash/error/error.ex @@ -169,11 +169,11 @@ defmodule Ash.Error do case stacktrace do %Stacktrace{stacktrace: nil} -> {:current_stacktrace, stacktrace} = Process.info(self(), :current_stacktrace) - %Stacktrace{stacktrace: stacktrace} + %Stacktrace{stacktrace: Enum.drop(stacktrace, 2)} nil -> {:current_stacktrace, stacktrace} = Process.info(self(), :current_stacktrace) - %Stacktrace{stacktrace: stacktrace} + %Stacktrace{stacktrace: Enum.drop(stacktrace, 2)} stacktrace -> %Stacktrace{stacktrace: stacktrace} diff --git a/lib/ash/error/exception.ex b/lib/ash/error/exception.ex index 1b3ce504..b51126eb 100644 --- a/lib/ash/error/exception.ex +++ b/lib/ash/error/exception.ex @@ -37,7 +37,9 @@ defmodule Ash.Error.Exception do case Process.info(self(), :current_stacktrace) do {:current_stacktrace, stacktrace} -> super( - Keyword.put_new(opts, :stacktrace, %Ash.Error.Stacktrace{stacktrace: stacktrace}) + Keyword.put_new(opts, :stacktrace, %Ash.Error.Stacktrace{ + stacktrace: Enum.drop(stacktrace, 2) + }) ) _ -> diff --git a/lib/ash/error/side_load/invalid_query.ex b/lib/ash/error/side_load/invalid_query.ex index 57f37322..eefbc4b7 100644 --- a/lib/ash/error/side_load/invalid_query.ex +++ b/lib/ash/error/side_load/invalid_query.ex @@ -12,7 +12,18 @@ defmodule Ash.Error.Load.InvalidQuery do def class(_), do: :invalid def message(%{query: query, load_path: load_path}) do - "Invalid query: #{inspect(query)} at #{Enum.join(load_path, ".")}" + errors_by_path = + query.errors + |> Enum.group_by(&List.wrap(&1.path)) + |> Enum.sort_by(&Enum.count(elem(&1, 0))) + |> Enum.map(fn {path, error} -> + {List.wrap(load_path) ++ path, error} + end) + + "Invalid query\n" <> + Enum.map_join(errors_by_path, "\n", fn {key, errors} -> + Enum.map_join(errors, "\n", &"* #{inspect(key)} - #{Exception.message(&1)}") + end) end def stacktrace(_), do: nil diff --git a/lib/ash/query/query.ex b/lib/ash/query/query.ex index 09befcf5..388fc79b 100644 --- a/lib/ash/query/query.ex +++ b/lib/ash/query/query.ex @@ -870,7 +870,7 @@ defmodule Ash.Query do end true -> - add_error(query, :load, "Invalid load #{inspect(field)}") + add_error(query, :load, Ash.Error.Query.InvalidLoad.exception(load: field)) end field, query -> @@ -969,7 +969,7 @@ defmodule Ash.Query do end true -> - add_error(query, :load, "Could not load #{inspect(field)}") + add_error(query, :load, Ash.Error.Query.InvalidLoad.exception(load: field)) end end @@ -1577,7 +1577,7 @@ defmodule Ash.Query do %{query | load: new_loads} else {:error, errors} -> - Enum.reduce(errors, query, &add_error(&2, :load, &1)) + Enum.reduce(errors, query, &add_error(&2, &1)) end end @@ -1594,14 +1594,8 @@ defmodule Ash.Query do [] -> [] - _errors -> - [ - {:error, - InvalidQuery.exception( - query: load_query, - load_path: Enum.reverse(path) - )} - ] + errors -> + Enum.map(errors, &Ash.Error.set_path(&1, path)) end end @@ -1610,6 +1604,8 @@ defmodule Ash.Query do end defp do_validate_load(query, loads, path) when is_list(loads) do + query = to_query(query) + loads |> List.wrap() |> Enum.flat_map(fn @@ -1617,44 +1613,32 @@ defmodule Ash.Query do case Ash.Resource.Info.relationship(query.resource, key) do nil -> [ - {:error, - NoSuchRelationship.exception( - resource: query.resource, - relationship: key, - load_path: Enum.reverse(path) - )} + NoSuchRelationship.exception( + resource: query.resource, + relationship: key, + load_path: Enum.reverse(path) + ) ] relationship -> cond do !Ash.Resource.Info.primary_action(relationship.destination, :read) -> [ - {:error, - NoReadAction.exception( - resource: relationship.destination, - when: "loading relationship #{relationship.name}" - )} + NoReadAction.exception( + resource: relationship.destination, + when: "loading relationship #{relationship.name}" + ) ] relationship.type == :many_to_many && !Ash.Resource.Info.primary_action(relationship.through, :read) -> [ - {:error, - NoReadAction.exception( - resource: relationship.destination, - when: "loading relationship #{relationship.name}" - )} + NoReadAction.exception( + resource: relationship.destination, + when: "loading relationship #{relationship.name}" + ) ] - match?(%Ash.Query{}, value) -> - validate_matching_query_and_continue( - value, - query.resource, - key, - path, - relationship - ) - true -> validate_matching_query_and_continue( value, diff --git a/test/actions/load_test.exs b/test/actions/load_test.exs index 246191aa..c9eeb7a3 100644 --- a/test/actions/load_test.exs +++ b/test/actions/load_test.exs @@ -527,6 +527,14 @@ defmodule Ash.Test.Actions.LoadTest do assert Enum.sort([category1.id, category2.id]) == Enum.sort([id1, id2]) end + test "it produces a nice error message on loading invalid loads" do + assert_raise Ash.Error.Invalid, ~r/:non_existent_thing is not a valid load/, fn -> + Post + |> Ash.Query.load(categories: [posts: :non_existent_thing]) + |> Api.read!(authorize?: true) + end + end + test "it allows loading nested many to many relationships" do category1 = Category