From 917131c21ff30442539a0a6b9c12aca6064939f2 Mon Sep 17 00:00:00 2001 From: Zach Daniel Date: Wed, 17 Aug 2022 12:58:43 -0400 Subject: [PATCH] improvement: handle required but not accepted values better --- lib/ash/actions/create.ex | 81 +++++++++++++++--------- lib/ash/actions/managed_relationships.ex | 16 ++++- lib/ash/changeset/changeset.ex | 17 +++++ test/actions/create_test.exs | 49 ++++++++++++++ 4 files changed, 131 insertions(+), 32 deletions(-) diff --git a/lib/ash/actions/create.ex b/lib/ash/actions/create.ex index 87ed53b2..e25c5e46 100644 --- a/lib/ash/actions/create.ex +++ b/lib/ash/actions/create.ex @@ -293,36 +293,59 @@ defmodule Ash.Actions.Create do if action.manual? do {:ok, nil} else - if upsert? do - resource - |> Ash.DataLayer.upsert(changeset, upsert_keys) - |> add_tenant(changeset) - |> manage_relationships(api, changeset, - actor: actor, - authorize?: authorize?, - upsert?: upsert? - ) - else - resource - |> Ash.DataLayer.create(changeset) - |> add_tenant(changeset) - |> manage_relationships(api, changeset, - actor: actor, - authorize?: authorize?, - upsert?: upsert? - ) - end - |> case do - {:ok, result, notifications} -> - {:ok, result, - Map.update!( - notifications, - :notifications, - &(&1 ++ manage_instructions.notifications) - )} + final_check = + changeset.resource + |> Ash.Resource.Info.attributes() + |> Enum.reject(&(&1.allow_nil? || &1.generated?)) - {:error, error} -> - {:error, error} + changeset = + changeset + |> Ash.Changeset.require_values( + :create, + true, + final_check + ) + + {changeset, _} = + Ash.Actions.ManagedRelationships.validate_required_belongs_to( + {changeset, []}, + false + ) + + if changeset.valid? do + if upsert? do + resource + |> Ash.DataLayer.upsert(changeset, upsert_keys) + |> add_tenant(changeset) + |> manage_relationships(api, changeset, + actor: actor, + authorize?: authorize?, + upsert?: upsert? + ) + else + resource + |> Ash.DataLayer.create(changeset) + |> add_tenant(changeset) + |> manage_relationships(api, changeset, + actor: actor, + authorize?: authorize?, + upsert?: upsert? + ) + end + |> case do + {:ok, result, notifications} -> + {:ok, result, + Map.update!( + notifications, + :notifications, + &(&1 ++ manage_instructions.notifications) + )} + + {:error, error} -> + {:error, error} + end + else + {:error, changeset.errors} end end else diff --git a/lib/ash/actions/managed_relationships.ex b/lib/ash/actions/managed_relationships.ex index a1c0a376..9883ec98 100644 --- a/lib/ash/actions/managed_relationships.ex +++ b/lib/ash/actions/managed_relationships.ex @@ -325,14 +325,24 @@ defmodule Ash.Actions.ManagedRelationships do ) end - defp validate_required_belongs_to({:error, error}), do: {:error, error} + def validate_required_belongs_to(changeset_instructions_or_error, preflight? \\ true) + def validate_required_belongs_to({:error, error}, _), do: {:error, error} - defp validate_required_belongs_to({changeset, instructions}) do + def validate_required_belongs_to({changeset, instructions}, preflight?) do changeset.resource |> Ash.Resource.Info.relationships() |> Enum.filter(&(&1.type == :belongs_to && &1.required?)) |> Enum.reject(fn relationship -> - changeset.context[:private][:error][relationship.name] + errored? = changeset.context[:private][:error][relationship.name] + + not_changing? = + if preflight? do + false + else + !Map.has_key?(changeset.relationships, relationship.name) + end + + errored? || not_changing? end) |> Enum.reduce({changeset, instructions}, fn required_relationship, {changeset, instructions} -> diff --git a/lib/ash/changeset/changeset.ex b/lib/ash/changeset/changeset.ex index 33dc6629..148270f4 100644 --- a/lib/ash/changeset/changeset.ex +++ b/lib/ash/changeset/changeset.ex @@ -1215,6 +1215,19 @@ defmodule Ash.Changeset do # Attributes that are private and/or are the source field of a belongs_to relationship # are typically not set by input, so they aren't required until the actual action # is run. + defp attributes_to_require(resource, %{type: :create, accept: accept}, true) do + resource + |> Ash.Resource.Info.attributes() + |> Enum.reject(&(&1.allow_nil? || &1.generated?)) + |> Enum.filter(&(&1.name in accept)) + end + + defp attributes_to_require(resource, %{type: :create, accept: accept} = action, false) do + resource + |> do_attributes_to_require(action) + |> Enum.filter(&(&1.name in accept)) + end + defp attributes_to_require(resource, _action, true = _private_and_belongs_to?) do resource |> Ash.Resource.Info.attributes() @@ -1222,6 +1235,10 @@ defmodule Ash.Changeset do end defp attributes_to_require(resource, action, false = _private_and_belongs_to?) do + do_attributes_to_require(resource, action) + end + + defp do_attributes_to_require(resource, action) do belongs_to = resource |> Ash.Resource.Info.relationships() diff --git a/test/actions/create_test.exs b/test/actions/create_test.exs index 9fc94466..b78b5871 100644 --- a/test/actions/create_test.exs +++ b/test/actions/create_test.exs @@ -99,6 +99,21 @@ defmodule Ash.Test.Actions.CreateTest do end end + defmodule ManualCreateAuthorWithRequiredId do + @moduledoc false + use Ash.Resource.Change + + def change(changeset, _, _) do + Ash.Changeset.after_action(changeset, fn _, nil -> + {:ok, + Ash.Test.Actions.CreateTest.AuthorWithRequiredId + |> Ash.Changeset.for_create(:create, %{name: "manual"}) + |> Ash.Changeset.force_change_attribute(:id, Ash.UUID.generate()) + |> Ash.Test.Actions.CreateTest.Api.create!()} + end) + end + end + defmodule Author do @moduledoc false use Ash.Resource, data_layer: Ash.DataLayer.Ets @@ -138,6 +153,31 @@ defmodule Ash.Test.Actions.CreateTest do end end + defmodule AuthorWithRequiredId do + @moduledoc false + use Ash.Resource, data_layer: Ash.DataLayer.Ets + + ets do + private?(true) + end + + actions do + defaults [:create, :read, :update, :destroy] + + create :manual_create do + accept [] + manual? true + change ManualCreateAuthorWithRequiredId + end + end + + attributes do + uuid_primary_key :id, generated?: false, default: nil + attribute(:name, :string, allow_nil?: false) + attribute(:bio, :string) + end + end + defmodule PostDefaults do @moduledoc false def garbage2, do: "garbage2" @@ -245,6 +285,7 @@ defmodule Ash.Test.Actions.CreateTest do entries do entry(Author) + entry(AuthorWithRequiredId) entry(Post) entry(Profile) entry(ProfileWithBelongsTo) @@ -406,6 +447,14 @@ defmodule Ash.Test.Actions.CreateTest do assert [%{name: "manual"}] = Api.read!(Author) end + + test "the manual action does not require values that aren't accepted" do + AuthorWithRequiredId + |> Ash.Changeset.for_create(:manual_create) + |> Api.create!() + + assert [%{name: "manual"}] = Api.read!(AuthorWithRequiredId) + end end describe "require_attributes" do