From d45a9dbbfa91ec300a11df883450ed15e02f7f8b Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Sat, 5 Jun 2021 18:11:09 -0400 Subject: [PATCH] improvement: support calculation sorts --- lib/ash/actions/read.ex | 55 +++++++---- lib/ash/actions/sort.ex | 158 +++++++++++++++++++++++++++++-- lib/ash/data_layer/ets.ex | 2 + lib/ash/filter/filter.ex | 2 +- lib/ash/query/query.ex | 2 +- lib/ash/sort/sort.ex | 18 ++-- test/actions/create_test.exs | 5 + test/actions/read_test.exs | 13 ++- test/actions/update_test.exs | 2 + test/ash/data_layer/ets_test.exs | 58 +++++++----- test/calculation_test.exs | 20 ++++ test/filter/filter_test.exs | 7 ++ test/support/helpers.ex | 27 ++++++ 13 files changed, 300 insertions(+), 69 deletions(-) create mode 100644 test/support/helpers.ex diff --git a/lib/ash/actions/read.ex b/lib/ash/actions/read.ex index 377d125a..e227a94e 100644 --- a/lib/ash/actions/read.ex +++ b/lib/ash/actions/read.ex @@ -293,8 +293,19 @@ defmodule Ash.Actions.Read do query.filter ) do {:ok, filter} -> - {:ok, %{query | filter: filter}, - %{requests: load_requests, notifications: before_notifications}} + case Ash.Actions.Sort.process( + query.resource, + query.sort, + query.aggregates, + query.context + ) do + {:ok, sort} -> + {:ok, %{query | filter: filter, sort: sort}, + %{requests: load_requests, notifications: before_notifications}} + + {:error, error} -> + {:error, error} + end {:error, error} -> {:error, error} @@ -942,26 +953,30 @@ defmodule Ash.Actions.Read do defp add_calculations(data_layer_query, query, calculations_to_add) do Enum.reduce_while(calculations_to_add, {:ok, data_layer_query}, fn calculation, {:ok, data_layer_query} -> - expression = calculation.module.expression(calculation.opts, calculation.context) + if Ash.DataLayer.data_layer_can?(query.resource, :expression_calculation) do + expression = calculation.module.expression(calculation.opts, calculation.context) - with {:ok, expression} <- - Ash.Filter.hydrate_refs(expression, %{ - resource: query.resource, - aggregates: query.aggregates, - calculations: query.calculations, - public?: false - }), - {:ok, query} <- - Ash.DataLayer.add_calculation( - data_layer_query, - calculation, - expression, - query.resource - ) do - {:cont, {:ok, query}} + with {:ok, expression} <- + Ash.Filter.hydrate_refs(expression, %{ + resource: query.resource, + aggregates: query.aggregates, + calculations: query.calculations, + public?: false + }), + {:ok, query} <- + Ash.DataLayer.add_calculation( + data_layer_query, + calculation, + expression, + query.resource + ) do + {:cont, {:ok, query}} + else + other -> + {:halt, other} + end else - other -> - {:halt, other} + {:halt, {:error, "Expression calculations are not supported"}} end end) end diff --git a/lib/ash/actions/sort.ex b/lib/ash/actions/sort.ex index bafed3d3..798c4254 100644 --- a/lib/ash/actions/sort.ex +++ b/lib/ash/actions/sort.ex @@ -9,11 +9,60 @@ defmodule Ash.Actions.Sort do @sort_orders [:asc, :desc, :asc_nils_first, :asc_nils_last, :desc_nils_first, :desc_nils_last] - def process(_resource, empty, _aggregates) when empty in [nil, []], do: {:ok, []} + def process(_resource, empty, _aggregates, context \\ %{}) - def process(resource, sort, aggregates) when is_list(sort) do + def process(_resource, empty, _aggregates, _context) when empty in [nil, []], do: {:ok, []} + + def process(resource, sort, aggregates, context) when is_list(sort) do sort + |> Enum.map(fn {key, val} -> + if !is_atom(val) do + {key, {:asc, val}} + else + {key, val} + end + end) |> Enum.reduce({[], []}, fn + {field, {inner_order, _} = order}, {sorts, errors} when inner_order in @sort_orders -> + case Ash.Resource.Info.calculation(resource, field) do + nil -> + {sorts, + [ + "Cannot provide context to a non-calculation field while sorting" + | errors + ]} + + calc -> + {module, opts} = calc.calculation + + if :erlang.function_exported(module, :expression, 2) do + if Ash.DataLayer.data_layer_can?(resource, :expression_calculation_sort) do + calculation_sort( + field, + calc, + module, + opts, + calc.type, + order, + sorts, + errors, + context + ) + else + {sorts, ["Datalayer cannot sort on calculations"]} + end + else + {sorts, ["Calculations cannot be sorted on unless they define an expression"]} + end + end + + {%Ash.Query.Calculation{} = calc, order}, {sorts, errors} -> + if order in @sort_orders do + {sorts ++ [{calc, order}], errors} + else + {sorts, [InvalidSortOrder.exception(order: order) | errors]} + end + {field, order}, {sorts, errors} when order in @sort_orders -> attribute = Ash.Resource.Info.attribute(resource, field) @@ -21,6 +70,29 @@ defmodule Ash.Actions.Sort do Map.has_key?(aggregates, field) -> aggregate_sort(aggregates, field, order, resource, sorts, errors) + calc = Ash.Resource.Info.calculation(resource, field) -> + {module, opts} = calc.calculation + + if :erlang.function_exported(module, :expression, 2) do + if Ash.DataLayer.data_layer_can?(resource, :expression_calculation_sort) do + calculation_sort( + field, + calc, + module, + opts, + calc.type, + order, + sorts, + errors, + context + ) + else + {sorts, ["Datalayer cannot sort on calculations"]} + end + else + {sorts, ["Calculations cannot be sorted on unless they define an expression"]} + end + !attribute -> {sorts, [NoSuchAttribute.exception(attribute: field) | errors]} @@ -78,25 +150,93 @@ defmodule Ash.Actions.Sort do ) do {sorts ++ [{field, order}], errors} else - {sorts, AggregatesNotSupported.exception(resource: resource, feature: "sorting")} + {sorts, [AggregatesNotSupported.exception(resource: resource, feature: "sorting") | errors]} + end + end + + defp calculation_sort(field, calc, module, opts, type, order, sorts, errors, context) do + {order, calc_context} = + case order do + order when is_atom(order) -> + {order, %{}} + + {order, value} when is_list(value) -> + {order, Map.new(value)} + + {order, value} when is_map(value) -> + {order, value} + + other -> + {other, %{}} + end + + with {:ok, input} <- Ash.Query.validate_calculation_arguments(calc, calc_context), + {:ok, calc} <- + Ash.Query.Calculation.new( + field, + module, + opts, + type, + Map.put(input, :context, context) + ) do + {sorts ++ [{calc, order}], errors} + else + {:error, error} -> + {sorts, [error | errors]} end end def runtime_sort(results, empty) when empty in [nil, []], do: results - def runtime_sort(results, [{field, direction}]) do - sort_by(results, &Map.get(&1, field), direction) + def runtime_sort([%resource{} | _] = results, [{field, direction}]) do + sort_by(results, &resolve_field(&1, field, resource), direction) end - def runtime_sort(results, [{field, direction} | rest]) do + def runtime_sort([%resource{} | _] = results, [{field, direction} | rest]) do results - |> Enum.group_by(&Map.get(&1, field)) + |> Enum.group_by(&resolve_field(&1, field, resource)) |> sort_by(fn {key, _value} -> key end, direction) |> Enum.flat_map(fn {_, records} -> runtime_sort(records, rest) end) end + defp resolve_field(record, %Ash.Query.Calculation{} = calc, resource) do + cond do + :erlang.function_exported(calc.module, :calculate, 3) -> + calc.module.calculate([record], calc.opts, calc.context) + + :erlang.function_exported(calc.module, :expression, 2) -> + expression = calc.module.expression(calc.opts, calc.context) + + case Ash.Filter.hydrate_refs(expression, %{ + resource: resource, + aggregates: %{}, + calculations: %{}, + public?: false + }) do + {:ok, expression} -> + case Ash.Filter.Runtime.do_match(record, expression) do + {:ok, value} -> + {:ok, value} + + _ -> + nil + end + + _ -> + nil + end + + true -> + nil + end + end + + defp resolve_field(record, field, _resource) do + Map.get(record, field) + end + # :asc/:desc added to elixir in 1.10. sort_by and to_sort_by_fun copied from core defp sort_by(enumerable, mapper, sorter) do enumerable @@ -180,6 +320,10 @@ defmodule Ash.Actions.Sort do end end + defp to_sort_by_fun({direction, _input}) do + to_sort_by_fun(direction) + end + defp to_sort_by_fun(module) when is_atom(module), do: &(module.compare(elem(&1, 1), elem(&2, 1)) != :gt) diff --git a/lib/ash/data_layer/ets.ex b/lib/ash/data_layer/ets.ex index c6b91228..7c811b7c 100644 --- a/lib/ash/data_layer/ets.ex +++ b/lib/ash/data_layer/ets.ex @@ -58,6 +58,8 @@ defmodule Ash.DataLayer.Ets do end def can?(_, :composite_primary_key), do: true + def can?(_, :expression_calculation), do: true + def can?(_, :expression_calculation_sort), do: true def can?(_, :multitenancy), do: true def can?(_, :upsert), do: true def can?(_, :create), do: true diff --git a/lib/ash/filter/filter.ex b/lib/ash/filter/filter.ex index 76d3da82..6d5307e7 100644 --- a/lib/ash/filter/filter.ex +++ b/lib/ash/filter/filter.ex @@ -2167,7 +2167,7 @@ defmodule Ash.Filter do defp add_calculation_expression(context, nested_statement, field, module, expression) do if Ash.DataLayer.data_layer_can?(context.resource, :expression_calculation) && - :erlang.function_exported(module, :expression, 1) do + :erlang.function_exported(module, :expression, 2) do case parse_predicates(nested_statement, Map.get(context.calculations, field), context) do {:ok, nested_statement} -> {:ok, BooleanExpression.optimized_new(:and, expression, nested_statement)} diff --git a/lib/ash/query/query.ex b/lib/ash/query/query.ex index bef7518a..f10a5f3d 100644 --- a/lib/ash/query/query.ex +++ b/lib/ash/query/query.ex @@ -1674,7 +1674,7 @@ defmodule Ash.Query do end defp validate_sort(%{resource: resource, sort: sort} = query) do - case Sort.process(resource, sort, query.aggregates) do + case Sort.process(resource, sort, query.aggregates, query.context) do {:ok, new_sort} -> %{query | sort: new_sort} {:error, error} -> add_error(query, :sort, error) end diff --git a/lib/ash/sort/sort.ex b/lib/ash/sort/sort.ex index 72109352..25a1287a 100644 --- a/lib/ash/sort/sort.ex +++ b/lib/ash/sort/sort.ex @@ -124,18 +124,12 @@ defmodule Ash.Sort do end defp get_field(resource, field) do - case Ash.Resource.Info.public_attribute(resource, field) do - %{name: name} -> - name - - nil -> - case Ash.Resource.Info.public_attribute(resource, field) do - %{name: name} -> - name - - nil -> - nil - end + with nil <- Ash.Resource.Info.public_attribute(resource, field), + nil <- Ash.Resource.Info.public_aggregate(resource, field), + nil <- Ash.Resource.Info.public_calculation(resource, field) do + nil + else + %{name: name} -> name end end diff --git a/test/actions/create_test.exs b/test/actions/create_test.exs index ae6bb766..162bbc58 100644 --- a/test/actions/create_test.exs +++ b/test/actions/create_test.exs @@ -3,6 +3,7 @@ defmodule Ash.Test.Actions.CreateTest do use ExUnit.Case, async: true import Ash.Changeset + import Ash.Test.Helpers defmodule Authorized do @moduledoc false @@ -465,24 +466,28 @@ defmodule Ash.Test.Actions.CreateTest do |> new() |> change_attribute(:title, "title2") |> Api.create!() + |> clear_meta() post3 = Post |> new() |> change_attribute(:title, "title3") |> Api.create!() + |> clear_meta() post = Post |> new(%{title: "cannot_be_missing"}) |> replace_relationship(:related_posts, [post2, post3]) |> Api.create!() + |> clear_meta() assert Enum.sort(post.related_posts) == Enum.sort([ Api.get!(Post, post2.id), Api.get!(Post, post3.id) ]) + |> clear_meta() end end diff --git a/test/actions/read_test.exs b/test/actions/read_test.exs index 32cd1141..f1732346 100644 --- a/test/actions/read_test.exs +++ b/test/actions/read_test.exs @@ -3,6 +3,7 @@ defmodule Ash.Test.Actions.ReadTest do use ExUnit.Case, async: true import Ash.Changeset + import Ash.Test.Helpers require Ash.Query @@ -91,7 +92,7 @@ defmodule Ash.Test.Actions.ReadTest do test "it returns a matching record", %{post: post} do assert {:ok, fetched_post} = Api.get(Post, post.id) - assert fetched_post == post + assert clear_meta(fetched_post) == post end test "it returns nil when there is no matching record" do @@ -101,7 +102,7 @@ defmodule Ash.Test.Actions.ReadTest do test "it uses identities if they exist", %{post: post} do assert {:ok, fetched_post} = Api.get(Post, uuid: post.uuid) - assert fetched_post == post + assert clear_meta(fetched_post) == post end test "raises an error when the first argument is not a module" do @@ -140,7 +141,7 @@ defmodule Ash.Test.Actions.ReadTest do end test "it returns a matching record", %{post: post} do - assert ^post = Api.get!(Post, post.id) + assert ^post = clear_meta(Api.get!(Post, post.id)) end test "raises an error when the first argument is not a module", %{post: post} do @@ -392,6 +393,7 @@ defmodule Ash.Test.Actions.ReadTest do Post |> Ash.Query.filter(title == ^post1.title) |> Api.read() + |> clear_meta() end test "a filter returns multiple records if they match", %{post1: post1, post2: post2} do @@ -399,6 +401,7 @@ defmodule Ash.Test.Actions.ReadTest do Post |> Ash.Query.filter(contents == "yeet") |> Api.read() + |> clear_meta() assert post1 in results assert post2 in results @@ -495,6 +498,7 @@ defmodule Ash.Test.Actions.ReadTest do Post |> Ash.Query.sort(title: :asc) |> Api.read() + |> clear_meta() end test "a sort will sor rows accordingly when descending", %{ @@ -505,6 +509,7 @@ defmodule Ash.Test.Actions.ReadTest do Post |> Ash.Query.sort(title: :desc) |> Api.read() + |> clear_meta() end test "a nested sort sorts accordingly", %{post1: post1, post2: post2} do @@ -512,11 +517,13 @@ defmodule Ash.Test.Actions.ReadTest do Post |> new(%{title: "abc", contents: "xyz"}) |> Api.create!() + |> clear_meta() assert {:ok, [^post1, ^middle_post, ^post2]} = Post |> Ash.Query.sort(title: :asc, contents: :asc) |> Api.read() + |> clear_meta() end end end diff --git a/test/actions/update_test.exs b/test/actions/update_test.exs index 50c229a2..813fef25 100644 --- a/test/actions/update_test.exs +++ b/test/actions/update_test.exs @@ -3,6 +3,7 @@ defmodule Ash.Test.Actions.UpdateTest do use ExUnit.Case, async: true import Ash.Changeset + import Ash.Test.Helpers defmodule Authorized do @moduledoc false @@ -372,6 +373,7 @@ defmodule Ash.Test.Actions.UpdateTest do Api.get!(Post, post2.id), Api.get!(Post, post3.id) ]) + |> clear_meta() end test "it updates any join fields" do diff --git a/test/ash/data_layer/ets_test.exs b/test/ash/data_layer/ets_test.exs index b4cdde81..f4e47d85 100644 --- a/test/ash/data_layer/ets_test.exs +++ b/test/ash/data_layer/ets_test.exs @@ -1,6 +1,8 @@ defmodule Ash.DataLayer.EtsTest do use ExUnit.Case, async: false + import Ash.Test.Helpers + alias Ash.DataLayer.Ets, as: EtsDataLayer alias Ash.DataLayer.Ets.Query @@ -116,7 +118,7 @@ defmodule Ash.DataLayer.EtsTest do |> Ash.Query.new() |> Ash.Query.sort(:name) - assert [^joe, ^matthew, ^mike, ^zachary] = EtsApiTest.read!(query) + assert [^joe, ^matthew, ^mike, ^zachary] = clear_meta(EtsApiTest.read!(query)) end test "limit" do @@ -131,7 +133,7 @@ defmodule Ash.DataLayer.EtsTest do |> Ash.Query.sort(:name) |> Ash.Query.limit(2) - assert [^joe, ^matthew] = EtsApiTest.read!(query) + assert [^joe, ^matthew] = clear_meta(EtsApiTest.read!(query)) end test "offset" do @@ -146,7 +148,7 @@ defmodule Ash.DataLayer.EtsTest do |> Ash.Query.sort(:name) |> Ash.Query.offset(1) - assert [^matthew, ^mike, ^zachary] = EtsApiTest.read!(query) + assert [^matthew, ^mike, ^zachary] = clear_meta(EtsApiTest.read!(query)) end describe "filter" do @@ -159,55 +161,61 @@ defmodule Ash.DataLayer.EtsTest do end test "values", %{zachary: zachary, matthew: matthew, joe: joe} do - assert [^zachary] = filter_users(name: "Zachary") - assert [^joe] = filter_users(name: "Joe") - assert [^matthew] = filter_users(age: 9) + assert [^zachary] = clear_meta(filter_users(name: "Zachary")) + assert [^joe] = clear_meta(filter_users(name: "Joe")) + assert [^matthew] = clear_meta(filter_users(age: 9)) end test "or, in, eq", %{mike: mike, zachary: zachary, joe: joe} do assert [^joe, ^mike, ^zachary] = - filter_users( - or: [ - [name: [in: ["Zachary", "Mike"]]], - [age: [eq: 11]] - ] + clear_meta( + filter_users( + or: [ + [name: [in: ["Zachary", "Mike"]]], + [age: [eq: 11]] + ] + ) ) end test "and, in, eq", %{mike: mike} do assert [^mike] = - filter_users( - and: [ - [name: [in: ["Zachary", "Mike"]]], - [age: [eq: 37]] - ] + clear_meta( + filter_users( + and: [ + [name: [in: ["Zachary", "Mike"]]], + [age: [eq: 37]] + ] + ) ) end test "and, in, not", %{zachary: zachary} do assert [^zachary] = - filter_users( - and: [ - [name: [in: ["Zachary", "Mike"]]], - [not: [age: 37]] - ] + clear_meta( + filter_users( + and: [ + [name: [in: ["Zachary", "Mike"]]], + [not: [age: 37]] + ] + ) ) end test "gt", %{mike: mike, joe: joe} do - assert [^joe, ^mike] = filter_users(age: [gt: 10]) + assert [^joe, ^mike] = clear_meta(filter_users(age: [gt: 10])) end test "lt", %{zachary: zachary, matthew: matthew} do - assert [^matthew, ^zachary] = filter_users(age: [lt: 10]) + assert [^matthew, ^zachary] = clear_meta(filter_users(age: [lt: 10])) end test "boolean", %{zachary: zachary, matthew: matthew} do - assert [^matthew, ^zachary] = filter_users(and: [true, age: [lt: 10]]) + assert [^matthew, ^zachary] = clear_meta(filter_users(and: [true, age: [lt: 10]])) end test "is_nil", %{zachary: zachary, matthew: matthew, joe: joe} do - assert [^joe, ^matthew, ^zachary] = filter_users(title: [is_nil: true]) + assert [^joe, ^matthew, ^zachary] = clear_meta(filter_users(title: [is_nil: true])) end end diff --git a/test/calculation_test.exs b/test/calculation_test.exs index b31b009a..ef22e47d 100644 --- a/test/calculation_test.exs +++ b/test/calculation_test.exs @@ -140,6 +140,26 @@ defmodule Ash.Test.CalculationTest do assert full_names == ["brian cranston", "zach daniel"] end + test "expression based calculations can be sorted on" do + full_names = + User + |> Ash.Query.load(:expr_full_name) + |> Ash.Query.sort(:expr_full_name) + |> Api.read!() + |> Enum.map(& &1.expr_full_name) + + assert full_names == ["brian cranston", "zach daniel"] + + full_names = + User + |> Ash.Query.load(:expr_full_name) + |> Ash.Query.sort(expr_full_name: :desc) + |> Api.read!() + |> Enum.map(& &1.expr_full_name) + + assert full_names == ["zach daniel", "brian cranston"] + end + test "the `if` calculation resolves the first expr when true, and the second when false" do User |> Ash.Changeset.new(%{first_name: "bob"}) diff --git a/test/filter/filter_test.exs b/test/filter/filter_test.exs index 7963d406..8c7f6778 100644 --- a/test/filter/filter_test.exs +++ b/test/filter/filter_test.exs @@ -3,6 +3,7 @@ defmodule Ash.Test.Filter.FilterTest do use ExUnit.Case, async: true import Ash.Changeset + import Ash.Test.Helpers alias Ash.Filter @@ -256,6 +257,7 @@ defmodule Ash.Test.Filter.FilterTest do Post |> Ash.Query.filter(title == ^post1.title) |> Api.read!() + |> clear_meta() end test "multiple filter field matches", %{post1: post1} do @@ -263,6 +265,7 @@ defmodule Ash.Test.Filter.FilterTest do Post |> Ash.Query.filter(title == ^post1.title and contents == ^post1.contents) |> Api.read!() + |> clear_meta() end test "no field matches" do @@ -290,12 +293,14 @@ defmodule Ash.Test.Filter.FilterTest do Post |> Ash.Query.filter(points < 2) |> Api.read!() + |> clear_meta() assert [^post1, ^post2] = Post |> Ash.Query.filter(points < 3) |> Ash.Query.sort(points: :asc) |> Api.read!() + |> clear_meta() end test "greater than works", %{ @@ -306,12 +311,14 @@ defmodule Ash.Test.Filter.FilterTest do Post |> Ash.Query.filter(points > 1) |> Api.read!() + |> clear_meta() assert [^post1, ^post2] = Post |> Ash.Query.filter(points > 0) |> Ash.Query.sort(points: :asc) |> Api.read!() + |> clear_meta() end end diff --git a/test/support/helpers.ex b/test/support/helpers.ex new file mode 100644 index 00000000..056d2947 --- /dev/null +++ b/test/support/helpers.ex @@ -0,0 +1,27 @@ +defmodule Ash.Test.Helpers do + @moduledoc false + + def clear_meta({:ok, record}) do + {:ok, clear_meta(record)} + end + + def clear_meta({:error, error}), do: {:error, error} + + def clear_meta(value) when is_list(value) do + Enum.map(value, &clear_meta/1) + end + + def clear_meta(%Ash.Page.Offset{results: results} = page) do + %{page | results: Enum.map(results, &clear_meta/1)} + end + + def clear_meta(%Ash.Page.Keyset{results: results} = page) do + %{page | results: Enum.map(results, &clear_meta/1)} + end + + def clear_meta(%{__metadata__: _} = record) do + Map.put(record, :__metadata__, %{}) + end + + def clear_meta(other), do: other +end