From 24d1bd03c4328ac0b759d526fe53b06dbc0aa665 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Sun, 19 Apr 2020 23:15:52 -0400 Subject: [PATCH] WIP --- README.md | 1 + lib/ash/actions/create.ex | 14 ---- lib/ash/actions/destroy.ex | 38 +++++----- lib/ash/actions/read.ex | 12 ---- lib/ash/actions/relationships/create.ex | 43 ------------ .../actions/relationships/relationships.ex | 5 -- lib/ash/actions/side_load.ex | 69 ++++++++++++------- lib/ash/api/interface.ex | 1 + lib/ash/data_layer/ets.ex | 3 + lib/ash/filter/filter.ex | 15 ++-- lib/ash/filter/inspect.ex | 4 +- .../resource/relationships/relationships.ex | 2 +- test/actions/destroy_test.exs | 2 +- .../relationships/many_to_many_test.exs | 16 ++++- 14 files changed, 91 insertions(+), 134 deletions(-) delete mode 100644 lib/ash/actions/relationships/create.ex delete mode 100644 lib/ash/actions/relationships/relationships.ex diff --git a/README.md b/README.md index 5ea327b7..738a3d47 100644 --- a/README.md +++ b/README.md @@ -170,3 +170,4 @@ end - check if preparations have been done on a superset filter of a request and, if so, use it - without transactions, we can't ensure that all changes are rolled back in the case that relationship updates are included. Don't think there is really anything to do about that, but something worth considering. - perhaps have auth steps express which fields need to be present, so we can avoid loading things unnecessarily +- lift `or` filters over the same field equaling a value into a single `in` filter, for performance (potentially) diff --git a/lib/ash/actions/create.ex b/lib/ash/actions/create.ex index 64bc1e1b..80a5c1f5 100644 --- a/lib/ash/actions/create.ex +++ b/lib/ash/actions/create.ex @@ -3,21 +3,7 @@ defmodule Ash.Actions.Create do alias Ash.Actions.{Attributes, Relationships, SideLoad} require Logger - @spec run(Ash.api(), Ash.resource(), Ash.action(), Ash.params()) :: - {:ok, Ash.record()} | {:error, Ecto.Changeset.t()} | {:error, Ash.error()} def run(api, resource, action, params) do - transaction_result = - Ash.DataLayer.transact(resource, fn -> - do_run(api, resource, action, params) - end) - - case transaction_result do - {:ok, value} -> value - {:error, error} -> {:error, error} - end - end - - defp do_run(api, resource, action, params) do attributes = Keyword.get(params, :attributes, %{}) side_loads = Keyword.get(params, :side_load, []) side_load_filter = Keyword.get(params, :side_load_filter) diff --git a/lib/ash/actions/destroy.ex b/lib/ash/actions/destroy.ex index c78c279f..7125ed0d 100644 --- a/lib/ash/actions/destroy.ex +++ b/lib/ash/actions/destroy.ex @@ -4,18 +4,6 @@ defmodule Ash.Actions.Destroy do @spec run(Ash.api(), Ash.record(), Ash.action(), Ash.params()) :: {:ok, Ash.record()} | {:error, Ecto.Changeset.t()} | {:error, Ash.error()} def run(api, %resource{} = record, action, params) do - transaction_result = - Ash.DataLayer.transact(resource, fn -> - do_authorized(api, params, action, record) - end) - - case transaction_result do - {:ok, value} -> value - {:error, error} -> {:error, error} - end - end - - defp do_authorized(api, params, action, %resource{} = record) do auth_request = Ash.Engine.Request.new( resource: resource, @@ -24,7 +12,7 @@ defmodule Ash.Actions.Destroy do strict_access: false, path: [:data], data: - Ash.Engine.Request.UnresolvedField.data([], fn _request, _ -> + Ash.Engine.Request.UnresolvedField.data([], fn _ -> case Ash.data_layer(resource).destroy(record) do :ok -> {:ok, record} {:error, error} -> {:error, error} @@ -34,15 +22,21 @@ defmodule Ash.Actions.Destroy do resolve_when_fetch_only?: true ) - if params[:authorization] do - Engine.run( - [auth_request], - api, - user: params[:authorization][:user], - log_final_report?: params[:authorization][:log_final_report?] - ) - else - Engine.run([auth_request], api, fetch_only?: true) + result = + if params[:authorization] do + Engine.run( + [auth_request], + api, + user: params[:authorization][:user], + log_final_report?: params[:authorization][:log_final_report?] + ) + else + Engine.run([auth_request], api, fetch_only?: true) + end + + case result do + %{errors: errors} when errors == %{} -> :ok + %{errors: errors} -> {:error, errors} end end end diff --git a/lib/ash/actions/read.ex b/lib/ash/actions/read.ex index 83200939..3d7e2d8d 100644 --- a/lib/ash/actions/read.ex +++ b/lib/ash/actions/read.ex @@ -5,18 +5,6 @@ defmodule Ash.Actions.Read do require Logger def run(api, resource, action, params) do - transaction_result = - Ash.DataLayer.transact(resource, fn -> - do_run(api, resource, action, params) - end) - - case transaction_result do - {:ok, value} -> value - {:error, error} -> {:error, error} - end - end - - defp do_run(api, resource, action, params) do filter = Keyword.get(params, :filter, []) sort = Keyword.get(params, :sort, []) side_loads = Keyword.get(params, :side_load, []) diff --git a/lib/ash/actions/relationships/create.ex b/lib/ash/actions/relationships/create.ex deleted file mode 100644 index 84d6d3d8..00000000 --- a/lib/ash/actions/relationships/create.ex +++ /dev/null @@ -1,43 +0,0 @@ -# defmodule Ash.Actions.Relationships.Create do -# alias Ash.Actions.Relationships.Change - -# def changeset(changeset, api, relationships) do -# relationship_changes = relationship_changes(relationships) - -# changeset -# end - -# defp relationship_changes(relationships) do -# Enum.into(relationships, %{}, fn {key, value} -> -# {key, Change.from(value, :create)} -# end) -# end - -# # def changeset(changeset, api, relationships) do -# # if relationships == %{} do -# # changeset -# # else -# # dependencies = Map.get(changeset, :__changes_depend_on__, []) - -# # Ash.Engine.Request.UnresolvedField.field(dependencies, fn data -> -# # new_changeset = -# # data -# # |> Map.get(:relationships, %{}) -# # |> Enum.reduce(changeset, fn {relationship, relationship_data}, changeset -> -# # relationship = Ash.relationship(changeset.data.__struct__, relationship) - -# # relationship_data = -# # relationship_data -# # |> Enum.into(%{}, fn {key, value} -> -# # {key, value.data} -# # end) -# # |> Map.put_new(:current, []) - -# # add_relationship_to_changeset(changeset, api, relationship, relationship_data) -# # end) - -# # {:ok, new_changeset} -# # end) -# # end -# # end -# end diff --git a/lib/ash/actions/relationships/relationships.ex b/lib/ash/actions/relationships/relationships.ex deleted file mode 100644 index 2c125383..00000000 --- a/lib/ash/actions/relationships/relationships.ex +++ /dev/null @@ -1,5 +0,0 @@ -# defmodule Ash.Actions.Relationships do -# defmodule Change do -# defstruct [:add, :remove, :current] -# end -# end diff --git a/lib/ash/actions/side_load.ex b/lib/ash/actions/side_load.ex index 9571ac08..e5ce0a19 100644 --- a/lib/ash/actions/side_load.ex +++ b/lib/ash/actions/side_load.ex @@ -91,16 +91,18 @@ defmodule Ash.Actions.SideLoad do end) |> Enum.reduce(data, fn {key, %{data: value}}, data -> last_relationship = last_relationship!(resource, key) + lead_path = :lists.droplast(key) case last_relationship do %{type: :many_to_many, name: name} -> # TODO: If we sort the relationships as we do them (doing the join assoc first) # then we can just use those linked assocs (maybe) join_association = String.to_existing_atom(to_string(name) <> "_join_assoc") - join_path = :lists.droplast(key) ++ [join_association] + + join_path = lead_path ++ [join_association] join_data = Map.get(includes, join_path, []) - map_or_update(data, fn record -> + map_or_update(data, lead_path, fn record -> source_value = Map.get(record, last_relationship.source_field) join_values = @@ -125,7 +127,7 @@ defmodule Ash.Actions.SideLoad do %{cardinality: :many} -> values = Enum.group_by(value, &Map.get(&1, last_relationship.destination_field)) - map_or_update(data, fn record -> + map_or_update(data, lead_path, fn record -> source_key = Map.get(record, last_relationship.source_field) related_records = Map.get(values, source_key, []) Map.put(record, last_relationship.name, related_records) @@ -137,7 +139,7 @@ defmodule Ash.Actions.SideLoad do {Map.get(item, last_relationship.destination_field), item} end) - map_or_update(data, fn record -> + map_or_update(data, lead_path, fn record -> source_key = Map.get(record, last_relationship.source_field) related_record = Map.get(values, source_key) Map.put(record, last_relationship.name, related_record) @@ -156,12 +158,18 @@ defmodule Ash.Actions.SideLoad do data end - defp map_or_update(record, func) when not is_list(record), do: func.(record) + defp map_or_update(record, [], func) when not is_list(record), do: func.(record) - defp map_or_update(records, func) do + defp map_or_update(records, [], func) do Enum.map(records, func) end + defp map_or_update(records, [path | tail], func) do + map_or_update(records, [], fn record -> + Map.update!(record, path, &map_or_update(&1, tail, func)) + end) + end + defp last_relationship!(resource, [last]) do Ash.relationship(resource, last) || raise "Assumption Failed" end @@ -177,7 +185,7 @@ defmodule Ash.Actions.SideLoad do {:rel, Ash.relationship(resource, key)}, nested_path <- path ++ [relationship], {:ok, requests} <- - requests(api, relationship.destination, further, filters, nested_path) do + requests(api, relationship.destination, further, filters, root_filter, nested_path) do default_read = Ash.primary_action(relationship.destination, :read) || raise "Must set default read for #{inspect(resource)}" @@ -211,7 +219,7 @@ defmodule Ash.Actions.SideLoad do path: [:include, Enum.map(nested_path, &Map.get(&1, :name))], resolve_when_fetch_only?: true, filter: - side_load_filter2( + side_load_filter( relationship, Map.get(filters || %{}, source, []), nested_path, @@ -237,6 +245,7 @@ defmodule Ash.Actions.SideLoad do # or for doing many to many joins, but can be slower. # If the relationship is already loaded, we should consider doing an in-memory filtering # Right now, we just use the original query + with {:ok, filter} <- true_side_load_filter( relationship, @@ -275,7 +284,7 @@ defmodule Ash.Actions.SideLoad do strict_access?: root_filter not in [:create, :update], resolve_when_fetch_only?: true, filter: - side_load_filter2( + side_load_filter( Ash.relationship(resource, join_relationship.name), [], nested_path, @@ -325,7 +334,7 @@ defmodule Ash.Actions.SideLoad do end end - defp side_load_filter2( + defp side_load_filter( %{reverse_relationship: nil, type: :many_to_many} = relationship, _request_filter, _prior_path, @@ -338,7 +347,7 @@ defmodule Ash.Actions.SideLoad do end) end - defp side_load_filter2( + defp side_load_filter( relationship, request_filter, prior_path, @@ -372,13 +381,13 @@ defmodule Ash.Actions.SideLoad do {:ok, reverse_path} -> Ash.Filter.parse( relationship.destination, - put_nested_relationship(request_filter, reverse_path, root_filter) + put_nested_relationship(request_filter, reverse_path, root_filter, false) ) end end) end - defp side_load_filter2( + defp side_load_filter( relationship, request_filter, prior_path, @@ -416,7 +425,7 @@ defmodule Ash.Actions.SideLoad do {:ok, reverse_path} -> Ash.Filter.parse( relationship.destination, - put_nested_relationship(request_filter, reverse_path, root_filter) + put_nested_relationship(request_filter, reverse_path, root_filter, false) ) :error -> @@ -450,17 +459,22 @@ defmodule Ash.Actions.SideLoad do Map.get(data, :data) path -> - Map.get(data, [:include, Enum.reverse(path)]) + path_names = path |> Enum.reverse() |> Enum.map(& &1.name) + + data + |> Map.get(:include, %{}) + |> Map.get(path_names, %{}) end - values = get_fields(source_data.data, pkey) + related_data = Map.get(source_data || %{}, :data, []) cond do reverse_relationship -> + values = get_fields(related_data, pkey) {:ok, put_nested_relationship(filter, [reverse_relationship], values)} true -> - ids = Enum.map(source_data.data, &Map.get(&1, relationship.source_field)) + ids = Enum.map(related_data, &Map.get(&1, relationship.source_field)) filter_value = case ids do @@ -588,30 +602,33 @@ defmodule Ash.Actions.SideLoad do # |> get_field(name, rest) # end - defp put_nested_relationship(_, _, []), do: [__impossible__: true] - defp put_nested_relationship(_, _, nil), do: [__impossible__: true] + defp put_nested_relationship(request_filter, path, value, records? \\ true) + defp put_nested_relationship(_, _, [], true), do: [__impossible__: true] + defp put_nested_relationship(_, _, nil, true), do: [__impossible__: true] + defp put_nested_relationship(_, _, [], false), do: [] + defp put_nested_relationship(_, _, nil, false), do: [] - defp put_nested_relationship(request_filter, path, value) when not is_list(value) do - put_nested_relationship(request_filter, path, [value]) + defp put_nested_relationship(request_filter, path, value, records?) when not is_list(value) do + put_nested_relationship(request_filter, path, [value], records?) end - defp put_nested_relationship(request_filter, [rel | rest], values) do + defp put_nested_relationship(request_filter, [rel | rest], values, records?) do [ - {rel, put_nested_relationship(request_filter, rest, values)} + {rel, put_nested_relationship(request_filter, rest, values, records?)} ] end - defp put_nested_relationship(request_filter, [], [[{field, _}] | _] = keys) do + defp put_nested_relationship(request_filter, [], [[{field, _}] | _] = keys, _) do add_relationship_id_filter(request_filter, field, Enum.map(keys, &elem(&1, 1))) end - defp put_nested_relationship(request_filter, [], [values]) do + defp put_nested_relationship(request_filter, [], [values], _) do Enum.reduce(values, request_filter, fn {field, value}, filter -> add_relationship_id_filter(filter, field, [value]) end) end - defp put_nested_relationship(request_filter, [], values) do + defp put_nested_relationship(request_filter, [], values, _) do Keyword.update(request_filter, :or, values, &Kernel.++(&1, values)) end diff --git a/lib/ash/api/interface.ex b/lib/ash/api/interface.ex index ded35021..543f8ef1 100644 --- a/lib/ash/api/interface.ex +++ b/lib/ash/api/interface.ex @@ -467,6 +467,7 @@ defmodule Ash.Api.Interface do end end + defp unwrap_or_raise!(:ok), do: :ok defp unwrap_or_raise!({:ok, result}), do: result defp unwrap_or_raise!({:error, error}) when is_bitstring(error) do diff --git a/lib/ash/data_layer/ets.ex b/lib/ash/data_layer/ets.ex index ea718588..1a9619f9 100644 --- a/lib/ash/data_layer/ets.ex +++ b/lib/ash/data_layer/ets.ex @@ -69,6 +69,9 @@ defmodule Ash.DataLayer.Ets do {:ok, %{query | sort: sort}} end + @impl true + def run_query(%Query{filter: %Ash.Filter{impossible?: true}}, _), do: {:ok, []} + @impl true def run_query( %Query{resource: resource, filter: filter, offset: offset, limit: limit, sort: sort}, diff --git a/lib/ash/filter/filter.ex b/lib/ash/filter/filter.ex index cb589a92..8542d067 100644 --- a/lib/ash/filter/filter.ex +++ b/lib/ash/filter/filter.ex @@ -9,7 +9,7 @@ defmodule Ash.Filter do requests: [], path: [], errors: [], - impossible: false + impossible?: false ] alias Ash.Engine.Request @@ -23,7 +23,7 @@ defmodule Ash.Filter do attributes: Keyword.t(), relationships: Map.t(), path: list(atom), - impossible: boolean, + impossible?: boolean, errors: list(String.t()), requests: list(Ash.Engine.Request.t()) } @@ -258,7 +258,7 @@ defmodule Ash.Filter do # TODO: We should probably include some kind of filter that *makes* it immediately impossible # that way, if the data layer doesn't check impossibility they will run the simpler query, # like for each pkey field say `[field: [in: []]]` - %{filter | impossible: true} + %{filter | impossible?: true} else filter end @@ -293,14 +293,14 @@ defmodule Ash.Filter do defp lift_impossibility(filter) do with_related_impossibility = - if Enum.any?(filter.relationships || %{}, fn {_, val} -> Map.get(val, :impossible) end) do - Map.put(filter, :impossible, true) + if Enum.any?(filter.relationships || %{}, fn {_, val} -> Map.get(val, :impossible?) end) do + Map.put(filter, :impossible?, true) else filter end Map.update!(with_related_impossibility, :ors, fn ors -> - Enum.reject(ors, &Map.get(&1, :impossible)) + Enum.reject(ors, &Map.get(&1, :impossible?)) end) end @@ -407,6 +407,9 @@ defmodule Ash.Filter do Enum.reduce(filter_statement, filter, fn {key, value}, filter -> cond do + key == :__impossible__ && value == true -> + %{filter | impossible?: true} + key in [:or, :and, :not] -> new_filter = add_expression_level_boolean_filter(filter, resource, key, value) diff --git a/lib/ash/filter/inspect.ex b/lib/ash/filter/inspect.ex index 18dd73ac..04ac09f8 100644 --- a/lib/ash/filter/inspect.ex +++ b/lib/ash/filter/inspect.ex @@ -70,7 +70,7 @@ defimpl Inspect, for: Ash.Filter do ors: ors, relationships: relationships, attributes: attributes, - impossible: impossible + impossible?: impossible }, opts ) @@ -89,7 +89,7 @@ defimpl Inspect, for: Ash.Filter do end end - def inspect(%{impossible: impossible} = filter, opts) do + def inspect(%{impossible?: impossible} = filter, opts) do rels = filter |> Map.get(:relationships) diff --git a/lib/ash/resource/relationships/relationships.ex b/lib/ash/resource/relationships/relationships.ex index dd8bee05..61979806 100644 --- a/lib/ash/resource/relationships/relationships.ex +++ b/lib/ash/resource/relationships/relationships.ex @@ -243,7 +243,7 @@ defmodule Ash.Resource.Relationships do has_many_name, unquote(config)[:through], destination_field: unquote(config)[:source_field_on_join_table], - source_field: unquote(config)[:source_field] + source_field: unquote(config)[:source_field] || :id ) with {:many_to_many, {:ok, many_to_many}} <- {:many_to_many, many_to_many}, diff --git a/test/actions/destroy_test.exs b/test/actions/destroy_test.exs index fd52852c..a8499ca1 100644 --- a/test/actions/destroy_test.exs +++ b/test/actions/destroy_test.exs @@ -82,7 +82,7 @@ defmodule Ash.Test.Actions.DestroyTest do test "allows destroying a record" do post = Api.create!(Post, attributes: %{title: "foo", contents: "bar"}) - assert Api.destroy!(post) == post + assert Api.destroy!(post) == :ok refute Api.get!(Post, post.id) end diff --git a/test/resource/relationships/many_to_many_test.exs b/test/resource/relationships/many_to_many_test.exs index 2466f05d..19d26569 100644 --- a/test/resource/relationships/many_to_many_test.exs +++ b/test/resource/relationships/many_to_many_test.exs @@ -12,7 +12,7 @@ defmodule Ash.Test.Resource.Relationships.ManyToManyTest do end describe "representation" do - test "it creates a relationship" do + test "it creates a relationship and a join relationship" do defposts do relationships do many_to_many :foobars, Foobar, through: SomeResource @@ -20,16 +20,28 @@ defmodule Ash.Test.Resource.Relationships.ManyToManyTest do end assert [ + %Ash.Resource.Relationships.HasMany{ + cardinality: :many, + destination: SomeResource, + destination_field: :posts_id, + name: :foobars_join_assoc, + source: Ash.Test.Resource.Relationships.ManyToManyTest.Post, + source_field: :id, + type: :has_many, + write_rules: [] + }, %Ash.Resource.Relationships.ManyToMany{ cardinality: :many, destination: Foobar, destination_field: :id, destination_field_on_join_table: :foobars_id, name: :foobars, + source: Ash.Test.Resource.Relationships.ManyToManyTest.Post, source_field: :id, source_field_on_join_table: :posts_id, through: SomeResource, - type: :many_to_many + type: :many_to_many, + write_rules: [] } ] = Ash.relationships(Post) end