From c920b09277edff9d40293fc9d0880eb6702afe58 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Tue, 18 Jul 2023 14:48:35 -0400 Subject: [PATCH] improvement: support new `distinct` features from ash core --- .tool-versions | 4 +- config/config.exs | 1 + lib/data_layer.ex | 117 ++++++++++++++++++++++++++------- lib/expr.ex | 34 ---------- mix.exs | 2 +- mix.lock | 2 +- test/distinct_test.exs | 46 +++++++++++-- test/support/resources/post.ex | 4 ++ 8 files changed, 146 insertions(+), 64 deletions(-) diff --git a/.tool-versions b/.tool-versions index b95b2c4..44acbf0 100644 --- a/.tool-versions +++ b/.tool-versions @@ -1,2 +1,2 @@ -erlang 25.3 -elixir 1.15.2 +erlang 26.0.2 +elixir 1.15.4 diff --git a/config/config.exs b/config/config.exs index ed20f42..3903c01 100644 --- a/config/config.exs +++ b/config/config.exs @@ -17,6 +17,7 @@ if Mix.env() == :dev do end if Mix.env() == :test do + config :ash, :validate_api_resource_inclusion?, false config :ash, :validate_api_config_inclusion?, false config :ash_postgres, AshPostgres.TestRepo, diff --git a/lib/data_layer.ex b/lib/data_layer.ex index da95738..9bd3bd0 100644 --- a/lib/data_layer.ex +++ b/lib/data_layer.ex @@ -408,7 +408,7 @@ defmodule AshPostgres.DataLayer do } alias Ash.Filter - alias Ash.Query.{BooleanExpression, Not} + alias Ash.Query.{BooleanExpression, Not, Ref} @behaviour Ash.DataLayer @@ -1839,6 +1839,7 @@ defmodule AshPostgres.DataLayer do {:error, distinct_statement} -> distinct_query = query + |> Ecto.Query.exclude(:order_by) |> default_bindings(resource) |> Map.put(:distinct, distinct_statement) @@ -1854,8 +1855,28 @@ defmodule AshPostgres.DataLayer do end end) + joined_query_source = + Enum.reduce( + [ + :join, + :group_by, + :having, + :distinct, + :select, + :combinations, + :with_ctes, + :limit, + :offset, + :lock, + :preload, + :update + ], + query, + &Ecto.Query.exclude(&2, &1) + ) + joined_query = - from(row in query.from.source, + from(row in joined_query_source, join: distinct in subquery(distinct_query), on: ^on ) @@ -1878,23 +1899,28 @@ defmodule AshPostgres.DataLayer do defp get_distinct_statement(query, distinct_on) do sort = query.__ash_bindings__.sort || [] + distinct = + query.distinct || + %Ecto.Query.QueryExpr{ + expr: [], + params: [] + } + if sort == [] do {:ok, default_distinct_statement(query, distinct_on)} else distinct_on - |> Enum.reduce_while({sort, []}, fn - _, {[], _distinct_statement} -> + |> Enum.reduce_while({sort, [], [], Enum.count(distinct.params)}, fn + _, {[], _distinct_statement, _, _count} -> {:halt, :error} - distinct_on, {[order_by | rest_order_by], distinct_statement} -> + distinct_on, {[order_by | rest_order_by], distinct_statement, params, count} -> case order_by do {^distinct_on, order} -> + {distinct_expr, params, count} = distinct_on_expr(query, distinct_on, params, count) + {:cont, - {rest_order_by, - [ - {order, {{:., [], [{:&, [], [0]}, distinct_on]}, [], []}} - | distinct_statement - ]}} + {rest_order_by, [{order, distinct_expr} | distinct_statement], params, count}} _ -> {:halt, :error} @@ -1904,14 +1930,13 @@ defmodule AshPostgres.DataLayer do :error -> {:error, default_distinct_statement(query, distinct_on)} - {_, result} -> - distinct = - query.distinct || - %Ecto.Query.QueryExpr{ - expr: [] - } - - {:ok, %{distinct | expr: distinct.expr ++ Enum.reverse(result)}} + {_, result, params, _} -> + {:ok, + %{ + distinct + | expr: distinct.expr ++ Enum.reverse(result), + params: distinct.params ++ Enum.reverse(params) + }} end end end @@ -1923,12 +1948,60 @@ defmodule AshPostgres.DataLayer do expr: [] } - expr = - Enum.map(distinct_on, fn distinct_on_field -> - {:asc, {{:., [], [{:&, [], [0]}, distinct_on_field]}, [], []}} + {expr, params, _} = + Enum.reduce(distinct_on, {[], [], Enum.count(distinct.params)}, fn + {distinct_on_field, order}, {expr, params, count} -> + {distinct_expr, params, count} = + distinct_on_expr(query, distinct_on_field, params, count) + + {[{order, distinct_expr} | expr], params, count} + + distinct_on_field, {expr, params, count} -> + {distinct_expr, params, count} = + distinct_on_expr(query, distinct_on_field, params, count) + + {[{:asc, distinct_expr} | expr], params, count} end) - %{distinct | expr: distinct.expr ++ expr} + %{ + distinct + | expr: distinct.expr ++ Enum.reverse(expr), + params: distinct.params ++ Enum.reverse(params) + } + end + + defp distinct_on_expr(query, field, params, count) do + resource = query.__ash_bindings__.resource + + ref = + case field do + %Ash.Query.Calculation{} = calc -> + %Ref{attribute: calc, relationship_path: [], resource: resource} + + field -> + %Ref{ + attribute: Ash.Resource.Info.field(resource, field), + relationship_path: [], + resource: resource + } + end + + dynamic = AshPostgres.Expr.dynamic_expr(query, ref, query.__ash_bindings__) + + result = + Ecto.Query.Builder.Dynamic.partially_expand( + :distinct, + query, + dynamic, + params, + count + ) + + expr = elem(result, 0) + new_params = elem(result, 1) + new_count = result |> Tuple.to_list() |> List.last() + + {expr, new_params, new_count} end @impl true diff --git a/lib/expr.ex b/lib/expr.ex index 7dabe7f..d887db2 100644 --- a/lib/expr.ex +++ b/lib/expr.ex @@ -787,40 +787,6 @@ defmodule AshPostgres.Expr do end end - defp do_dynamic_expr( - query, - %Ref{ - attribute: %Ash.Resource.Calculation{calculation: {module, opts}} = calculation - } = ref, - bindings, - embedded?, - type - ) do - calc_type = - AshPostgres.Types.parameterized_type( - calculation.type, - Map.get(calculation, :constraints, []) - ) - - validate_type!(query, calc_type, ref) - - {:ok, query_calc} = - Ash.Query.Calculation.new( - calculation.name, - module, - opts, - calculation.type - ) - - expr = do_dynamic_expr(query, %{ref | attribute: query_calc}, bindings, embedded?, type) - - if calc_type do - Ecto.Query.dynamic(type(^expr, ^calc_type)) - else - expr - end - end - defp do_dynamic_expr( query, %Ref{ diff --git a/mix.exs b/mix.exs index 736b73e..26b2bd3 100644 --- a/mix.exs +++ b/mix.exs @@ -161,7 +161,7 @@ defmodule AshPostgres.MixProject do {:ecto, "~> 3.9"}, {:jason, "~> 1.0"}, {:postgrex, ">= 0.0.0"}, - {:ash, ash_version("~> 2.11 and >= 2.11.9")}, + {:ash, ash_version("~> 2.11 and >= 2.11.11")}, {:git_ops, "~> 2.5", only: [:dev, :test]}, {:ex_doc, "~> 0.22", only: [:dev, :test], runtime: false}, {:ex_check, "~> 0.14", only: [:dev, :test]}, diff --git a/mix.lock b/mix.lock index 68494ba..0fcf38f 100644 --- a/mix.lock +++ b/mix.lock @@ -1,5 +1,5 @@ %{ - "ash": {:hex, :ash, "2.11.9", "8f692fc38895d7eafbad981f3ef2b6dc22af3cabe18a847db10faa750cec0537", [:mix], [{:comparable, "~> 1.0", [hex: :comparable, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:earmark, "~> 1.4", [hex: :earmark, repo: "hexpm", optional: true]}, {:ecto, "~> 3.7", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8.0", [hex: :ets, repo: "hexpm", optional: false]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: false]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:spark, ">= 1.1.20 and < 2.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:stream_data, "~> 0.5.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "e57e54083ead9eb4915b0755d15e335b610e595f1030eb338a3c9287b2241764"}, + "ash": {:hex, :ash, "2.11.11", "84bdbc62f984ea10a9c0e30749ed1c015375657b4f1da32a85a7465d24ec45c3", [:mix], [{:comparable, "~> 1.0", [hex: :comparable, repo: "hexpm", optional: false]}, {:decimal, "~> 2.0", [hex: :decimal, repo: "hexpm", optional: false]}, {:earmark, "~> 1.4", [hex: :earmark, repo: "hexpm", optional: true]}, {:ecto, "~> 3.7", [hex: :ecto, repo: "hexpm", optional: false]}, {:ets, "~> 0.8.0", [hex: :ets, repo: "hexpm", optional: false]}, {:jason, ">= 1.0.0", [hex: :jason, repo: "hexpm", optional: false]}, {:picosat_elixir, "~> 0.2", [hex: :picosat_elixir, repo: "hexpm", optional: false]}, {:plug, ">= 0.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:spark, ">= 1.1.20 and < 2.0.0-0", [hex: :spark, repo: "hexpm", optional: false]}, {:stream_data, "~> 0.5.0", [hex: :stream_data, repo: "hexpm", optional: false]}, {:telemetry, "~> 1.1", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "d7c9399ebe0322e93c785e6f9df8b66006326ea5745834ad3e830e5d4a0549e9"}, "bunt": {:hex, :bunt, "0.2.0", "951c6e801e8b1d2cbe58ebbd3e616a869061ddadcc4863d0a2182541acae9a38", [:mix], [], "hexpm", "7af5c7e09fe1d40f76c8e4f9dd2be7cebd83909f31fee7cd0e9eadc567da8353"}, "certifi": {:hex, :certifi, "2.9.0", "6f2a475689dd47f19fb74334859d460a2dc4e3252a3324bd2111b8f0429e7e21", [:rebar3], [], "hexpm", "266da46bdb06d6c6d35fde799bcb28d36d985d424ad7c08b5bb48f5b5cdd4641"}, "comparable": {:hex, :comparable, "1.0.0", "bb669e91cedd14ae9937053e5bcbc3c52bb2f22422611f43b6e38367d94a495f", [:mix], [{:typable, "~> 0.1", [hex: :typable, repo: "hexpm", optional: false]}], "hexpm", "277c11eeb1cd726e7cd41c6c199e7e52fa16ee6830b45ad4cdc62e51f62eb60c"}, diff --git a/test/distinct_test.exs b/test/distinct_test.exs index 5652e1a..9d54d63 100644 --- a/test/distinct_test.exs +++ b/test/distinct_test.exs @@ -7,19 +7,19 @@ defmodule AshPostgres.DistinctTest do setup do Post - |> Ash.Changeset.new(%{title: "title"}) + |> Ash.Changeset.new(%{title: "title", score: 1}) |> Api.create!() Post - |> Ash.Changeset.new(%{title: "title"}) + |> Ash.Changeset.new(%{title: "title", score: 1}) |> Api.create!() Post - |> Ash.Changeset.new(%{title: "foo"}) + |> Ash.Changeset.new(%{title: "foo", score: 2}) |> Api.create!() Post - |> Ash.Changeset.new(%{title: "foo"}) + |> Ash.Changeset.new(%{title: "foo", score: 2}) |> Api.create!() :ok @@ -77,4 +77,42 @@ defmodule AshPostgres.DistinctTest do assert [_] = results end + + test "distinct can use calculations sort that does not match the distinct using a limit #2" do + results = + Post + |> Ash.Query.distinct(:negative_score) + |> Ash.Query.sort(:negative_score) + |> Ash.Query.load(:negative_score) + |> Api.read!() + + assert [ + %{title: "foo", negative_score: -2}, + %{title: "title", negative_score: -1} + ] = results + + results = + Post + |> Ash.Query.distinct(:negative_score) + |> Ash.Query.sort(negative_score: :desc) + |> Ash.Query.load(:negative_score) + |> Api.read!() + + assert [ + %{title: "title", negative_score: -1}, + %{title: "foo", negative_score: -2} + ] = results + + results = + Post + |> Ash.Query.distinct(:negative_score) + |> Ash.Query.sort(:title) + |> Ash.Query.load(:negative_score) + |> Api.read!() + + assert [ + %{title: "foo", negative_score: -2}, + %{title: "title", negative_score: -1} + ] = results + end end diff --git a/test/support/resources/post.ex b/test/support/resources/post.ex index 3a74e19..2770bd1 100644 --- a/test/support/resources/post.ex +++ b/test/support/resources/post.ex @@ -102,6 +102,10 @@ defmodule AshPostgres.Test.Post do has_many(:comments, AshPostgres.Test.Comment, destination_attribute: :post_id) + has_many :comments_matching_post_title, AshPostgres.Test.Comment do + filter(expr(title == parent_expr(title))) + end + has_many :popular_comments, AshPostgres.Test.Comment do destination_attribute(:post_id) filter(expr(likes > 10))