fix: don't raise a backwards incompatible error message on certian changeset functions

now we warn, instead

fix: properly apply managed relationships on manual actions

fix: properly pass `resource` option in filter policies
This commit is contained in:
Zach Daniel 2022-11-29 14:23:49 -05:00
parent cc72783b08
commit f0b0accd10
12 changed files with 112 additions and 29 deletions

View file

@ -347,6 +347,11 @@ defmodule Ash.Actions.Create do
authorize?: authorize?, authorize?: authorize?,
api: changeset.api api: changeset.api
}) })
|> manage_relationships(api, changeset,
actor: actor,
authorize?: authorize?,
upsert?: upsert?
)
action.manual? -> action.manual? ->
{:ok, nil} {:ok, nil}
@ -485,6 +490,21 @@ defmodule Ash.Actions.Create do
{:ok, nil, %{notifications: []}} {:ok, nil, %{notifications: []}}
end end
defp manage_relationships(
{:ok, created, %{notifications: notifications}},
api,
changeset,
engine_opts
) do
case manage_relationships({:ok, created}, api, changeset, engine_opts) do
{:ok, created, info} ->
{:ok, created, Map.update(info, :notifications, notifications, &(&1 ++ notifications))}
other ->
other
end
end
defp manage_relationships({:ok, created}, api, changeset, engine_opts) do defp manage_relationships({:ok, created}, api, changeset, engine_opts) do
with {:ok, loaded} <- with {:ok, loaded} <-
Ash.Actions.ManagedRelationships.load(api, created, changeset, engine_opts), Ash.Actions.ManagedRelationships.load(api, created, changeset, engine_opts),

View file

@ -1206,9 +1206,6 @@ defmodule Ash.Actions.Read do
{query, notifications} {query, notifications}
end end
end) end)
|> then(fn {query, notifications} ->
{Ash.Query.put_context(query, :private, %{in_before_action?: false}), notifications}
end)
end end
defp run_after_action(query, results) do defp run_after_action(query, results) do

View file

@ -375,6 +375,10 @@ defmodule Ash.Actions.Update do
authorize?: authorize?, authorize?: authorize?,
api: changeset.api api: changeset.api
}) })
|> manage_relationships(api, changeset,
actor: actor,
authorize?: authorize?
)
action.manual? -> action.manual? ->
{:ok, changeset.data, %{notifications: []}} {:ok, changeset.data, %{notifications: []}}
@ -462,6 +466,21 @@ defmodule Ash.Actions.Update do
[authorization_request, commit_request] [authorization_request, commit_request]
end end
defp manage_relationships(
{:ok, updated, %{notifications: notifications}},
api,
changeset,
engine_opts
) do
case manage_relationships({:ok, updated}, api, changeset, engine_opts) do
{:ok, updated, info} ->
{:ok, updated, Map.update(info, :notifications, notifications, &(&1 ++ notifications))}
other ->
other
end
end
defp manage_relationships({:ok, updated}, api, changeset, engine_opts) do defp manage_relationships({:ok, updated}, api, changeset, engine_opts) do
with {:ok, loaded} <- with {:ok, loaded} <-
Ash.Actions.ManagedRelationships.load(api, updated, changeset, engine_opts), Ash.Actions.ManagedRelationships.load(api, updated, changeset, engine_opts),

View file

@ -150,9 +150,11 @@ defmodule Ash.Changeset do
changeset = unquote(changeset) changeset = unquote(changeset)
if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do
raise ArgumentError, """ IO.warn("""
Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}. Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}.
In the future, this will become an error.
For safety, we prevent any changes after that point because they will bypass validations or other action logic.. To proceed anyway, For safety, we prevent any changes after that point because they will bypass validations or other action logic.. To proceed anyway,
you can use `#{unquote(alternative)}/#{unquote(arity)}`. However, you should prefer a pattern like the below, which makes you can use `#{unquote(alternative)}/#{unquote(arity)}`. However, you should prefer a pattern like the below, which makes
any custom changes *before* calling the action. any custom changes *before* calling the action.
@ -161,7 +163,7 @@ defmodule Ash.Changeset do
|> Ash.Changeset.new() |> Ash.Changeset.new()
|> Ash.Changeset.#{unquote(function)}(...) |> Ash.Changeset.#{unquote(function)}(...)
|> Ash.Changeset.for_create(...) |> Ash.Changeset.for_create(...)
""" """)
end end
end end
else else
@ -169,9 +171,11 @@ defmodule Ash.Changeset do
changeset = unquote(changeset) changeset = unquote(changeset)
if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do if changeset.__validated_for_action__ && !changeset.context[:private][:in_before_action?] do
raise ArgumentError, """ IO.warn("""
Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}. Changeset has already been validated for action #{inspect(changeset.__validated_for_action__)}.
In the future, this will become an error.
For safety, we prevent any changes using `#{unquote(function)}/#{unquote(arity)}` after that point because they will bypass validations or other action logic. For safety, we prevent any changes using `#{unquote(function)}/#{unquote(arity)}` after that point because they will bypass validations or other action logic.
Instead, you should change or set this value before calling the action, like so: Instead, you should change or set this value before calling the action, like so:
@ -179,7 +183,7 @@ defmodule Ash.Changeset do
|> Ash.Changeset.new() |> Ash.Changeset.new()
|> Ash.Changeset.#{unquote(function)}(...) |> Ash.Changeset.#{unquote(function)}(...)
|> Ash.Changeset.for_create(...) |> Ash.Changeset.for_create(...)
""" """)
end end
end end
end end
@ -1544,8 +1548,6 @@ defmodule Ash.Changeset do
end end
) )
changeset = put_context(changeset, :private, %{in_before_action?: false})
case func.(changeset) do case func.(changeset) do
{:ok, result, instructions} -> {:ok, result, instructions} ->
run_after_actions( run_after_actions(

View file

@ -410,7 +410,7 @@ defmodule Ash.Flow.Executor.AshEngine do
[first | rest] -> [first | rest] ->
# only one of the requests needs to be annotated as touching the resources # only one of the requests needs to be annotated as touching the resources
# the transaction claims to touch, since the transaction is urn over all touched resources # the transaction claims to touch, since the transaction is run over all touched resources
# in all requests # in all requests
[ [
%{ %{

View file

@ -494,7 +494,17 @@ defmodule Ash.Policy.Authorizer do
scenario scenario
|> Enum.map(fn |> Enum.map(fn
{{check_module, check_opts}, true} -> {{check_module, check_opts}, true} ->
result = check_module.auto_filter(authorizer.actor, authorizer, check_opts) result =
try do
check_module.auto_filter(authorizer.actor, authorizer, check_opts)
rescue
e ->
reraise Ash.Error.to_ash_error(e, __STACKTRACE__,
error_context:
"Creating filter for check: #{check_module.describe(check_opts)} on resource: #{authorizer.resource}"
),
__STACKTRACE__
end
if is_nil(result) do if is_nil(result) do
false false
@ -504,10 +514,19 @@ defmodule Ash.Policy.Authorizer do
{{check_module, check_opts}, false} -> {{check_module, check_opts}, false} ->
result = result =
if :erlang.function_exported(check_module, :auto_filter_not, 3) do try do
check_module.auto_filter_not(authorizer.actor, authorizer, check_opts) if :erlang.function_exported(check_module, :auto_filter_not, 3) do
else check_module.auto_filter_not(authorizer.actor, authorizer, check_opts)
[not: check_module.auto_filter(authorizer.actor, authorizer, check_opts)] else
[not: check_module.auto_filter(authorizer.actor, authorizer, check_opts)]
end
rescue
e ->
reraise Ash.Error.to_ash_error(e, __STACKTRACE__,
error_context:
"Creating filter for check: #{check_module.describe(check_opts)} on resource: #{authorizer.resource}"
),
__STACKTRACE__
end end
if is_nil(result) do if is_nil(result) do

View file

@ -62,11 +62,21 @@ defmodule Ash.Policy.Check.RelatesToActorVia do
defp relationship_info(resource, [rel], to_many?) do defp relationship_info(resource, [rel], to_many?) do
rel = Ash.Resource.Info.relationship(resource, rel) rel = Ash.Resource.Info.relationship(resource, rel)
if !rel do
raise "No such relationship #{rel} for #{resource}, required in `relates_to_actor` check"
end
{rel, to_many? || rel.cardinality == :many} {rel, to_many? || rel.cardinality == :many}
end end
defp relationship_info(resource, [rel | rest], to_many?) do defp relationship_info(resource, [rel | rest], to_many?) do
rel = Ash.Resource.Info.relationship(resource, rel) rel = Ash.Resource.Info.relationship(resource, rel)
if !rel do
raise "No such relationship #{rel} for #{resource}, required in `relates_to_actor` check"
end
relationship_info(rel.destination, rest, to_many? || rel.cardinality == :many) relationship_info(rel.destination, rest, to_many? || rel.cardinality == :many)
end end
end end

View file

@ -43,18 +43,27 @@ defmodule Ash.Policy.Checker do
check_module = check.check_module check_module = check.check_module
opts = check.check_opts opts = check.check_opts
case check_module.strict_check(authorizer.actor, authorizer, opts) do try do
{:ok, boolean} when is_boolean(boolean) -> case check_module.strict_check(authorizer.actor, authorizer, opts) do
{:ok, authorizer, Map.put(facts, {check_module, opts}, boolean)} {:ok, boolean} when is_boolean(boolean) ->
{:ok, authorizer, Map.put(facts, {check_module, opts}, boolean)}
{:ok, :unknown} -> {:ok, :unknown} ->
{:ok, authorizer, facts} {:ok, authorizer, facts}
{:error, error} -> {:error, error} ->
{:error, error} {:error, error}
other -> other ->
raise "Invalid return value from strict_check call #{check_module}.strict_check(actor, authorizer, #{inspect(opts)}) - #{inspect(other)}" raise "Invalid return value from strict_check call #{check_module}.strict_check(actor, authorizer, #{inspect(opts)}) - #{inspect(other)}"
end
rescue
e ->
reraise Ash.Error.to_ash_error(e, __STACKTRACE__,
error_context:
"Strict checking: #{check_module.describe(opts)} on resource: #{authorizer.resource}"
),
__STACKTRACE__
end end
end end

View file

@ -30,6 +30,8 @@ defmodule Ash.Policy.FilterCheckWithContext do
end end
def strict_check(nil, authorizer, opts) do def strict_check(nil, authorizer, opts) do
opts = Keyword.put_new(opts, :resource, authorizer.resource)
if Ash.Filter.template_references_actor?(filter(nil, authorizer, opts)) do if Ash.Filter.template_references_actor?(filter(nil, authorizer, opts)) do
{:ok, false} {:ok, false}
else else
@ -153,6 +155,7 @@ defmodule Ash.Policy.FilterCheckWithContext do
end end
def reject(actor, authorizer, opts) do def reject(actor, authorizer, opts) do
opts = Keyword.put_new(opts, :resource, authorizer.resource)
[not: filter(actor, authorizer, opts)] [not: filter(actor, authorizer, opts)]
end end

View file

@ -149,8 +149,8 @@ defmodule Ash.Query do
query = unquote(query) query = unquote(query)
if query.__validated_for_action__ && !query.context[:private][:in_before_action?] do if query.__validated_for_action__ && !query.context[:private][:in_before_action?] do
raise ArgumentError, """ IO.warn("""
Changeset has already been validated for action #{inspect(query.__validated_for_action__)}. Query has already been validated for action #{inspect(query.__validated_for_action__)}.
For safety, we prevent any changes after that point because they will bypass validations or other action logic. For safety, we prevent any changes after that point because they will bypass validations or other action logic.
However, you should prefer a pattern like the below, which makes any custom modifications *before* calling the action. However, you should prefer a pattern like the below, which makes any custom modifications *before* calling the action.
@ -159,7 +159,7 @@ defmodule Ash.Query do
|> Ash.Query.new() |> Ash.Query.new()
|> Ash.Query.#{unquote(function)}(...) |> Ash.Query.#{unquote(function)}(...)
|> Ash.Query.for_read(...) |> Ash.Query.for_read(...)
""" """)
end end
end end
end end

View file

@ -15,7 +15,9 @@ defmodule Ash.Resource.ManualCreate do
opts :: Keyword.t(), opts :: Keyword.t(),
context :: context() context :: context()
) :: ) ::
{:ok, Ash.Resource.record()} | {:error, term} {:ok, Ash.Resource.record()}
| {:ok, Ash.Resource.record(), %{notifications: [Ash.Notifier.Notification.t()]}}
| {:error, term}
defmacro __using__(_) do defmacro __using__(_) do
quote do quote do

View file

@ -15,7 +15,9 @@ defmodule Ash.Resource.ManualUpdate do
opts :: Keyword.t(), opts :: Keyword.t(),
context :: context() context :: context()
) :: ) ::
{:ok, Ash.Resource.record()} | {:error, term} {:ok, Ash.Resource.record()}
| {:ok, Ash.Resource.record(), %{notifications: [Ash.Notifier.Notification.t()]}}
| {:error, term}
defmacro __using__(_) do defmacro __using__(_) do
quote do quote do