fix: load belongs to relationships before managing them

.credo.exs

@@ -125,7 +125,7 @@
         {Credo.Check.Refactor.MatchInCondition, []},
         {Credo.Check.Refactor.NegatedConditionsInUnless, []},
         {Credo.Check.Refactor.NegatedConditionsWithElse, []},
-        {Credo.Check.Refactor.Nesting, [max_nesting: 4]},
+        {Credo.Check.Refactor.Nesting, [max_nesting: 5]},
         {Credo.Check.Refactor.UnlessWithElse, []},
         {Credo.Check.Refactor.WithClauses, []},
 

lib/ash/actions/managed_relationships.ex

@@ -65,93 +65,155 @@ defmodule Ash.Actions.ManagedRelationships do
                                                                {changeset, instructions} ->
       pkeys = pkeys(relationship)
 
-      opts = Ash.Changeset.ManagedRelationshipHelpers.sanitize_opts(relationship, opts)
+      changeset =
-      opts = Keyword.put(opts, :authorize?, engine_opts[:authorize?] && opts[:authorize?])
+        if changeset.action.type == :update do
+          case changeset.api.load(changeset.data, relationship.name, authorize?: opts[:authorize?]) do
+            {:ok, result} ->
+              {:ok, %{changeset | data: result}}
 
-      current_value =
+            {:error, error} ->
-        case Map.get(changeset.data, relationship.name) do
+              {:error, error}
-          %Ash.NotLoaded{} ->
+          end
-            nil
-
-          other ->
-            other
-        end
-
-      input =
-        if is_list(input) do
-          Enum.at(input, 0)
         else
-          input
+          {:ok, changeset}
         end
 
-      match =
+      case changeset do
-        if input do
+        {:ok, changeset} ->
-          find_match(
+          opts = Ash.Changeset.ManagedRelationshipHelpers.sanitize_opts(relationship, opts)
-            List.wrap(current_value),
+          opts = Keyword.put(opts, :authorize?, engine_opts[:authorize?] && opts[:authorize?])
-            input,
-            pkeys,
-            relationship,
-            opts[:on_no_match] == :match
-          )
-        else
-          nil
-        end
 
-      case match do
+          changeset =
-        nil ->
+            if input in [nil, []] && opts[:on_missing] != :ignore do
-          case opts[:on_lookup] do
+              Ash.Changeset.force_change_attribute(changeset, relationship.source_field, nil)
-            _ when is_nil(input) ->
+              |> Ash.Changeset.after_action(fn _changeset, result ->
-              create_belongs_to_record(
+                {:ok, Map.put(result, relationship.name, nil)}
-                changeset,
+              end)
-                instructions,
+            else
-                relationship,
+              changeset
+            end
+
+          current_value =
+            case Map.get(changeset.data, relationship.name) do
+              %Ash.NotLoaded{} ->
+                nil
+
+              other ->
+                other
+            end
+
+          input =
+            if is_list(input) do
+              Enum.at(input, 0)
+            else
+              input
+            end
+
+          match =
+            if input do
+              find_match(
+                List.wrap(current_value),
                 input,
-                actor,
+                pkeys,
-                index,
-                opts
-              )
-
-            :ignore ->
-              create_belongs_to_record(
-                changeset,
-                instructions,
                 relationship,
-                input,
+                opts[:on_no_match] == :match
-                actor,
-                index,
-                opts
               )
+            else
+              nil
+            end
 
-            {_key, _create_or_update, read} ->
+          case match do
-              if is_struct(input) do
+            nil ->
-                changeset =
+              case opts[:on_lookup] do
-                  changeset
+                _ when is_nil(input) ->
-                  |> Ash.Changeset.set_context(%{
+                  create_belongs_to_record(
-                    belongs_to_manage_found: %{relationship.name => %{index => input}}
+                    changeset,
-                  })
+                    instructions,
-                  |> Ash.Changeset.force_change_attribute(
+                    relationship,
-                    relationship.source_field,
+                    input,
-                    Map.get(input, relationship.destination_field)
+                    actor,
+                    index,
+                    opts
                   )
 
-                {:cont, {changeset, instructions}}
+                :ignore ->
-              else
+                  create_belongs_to_record(
-                case Ash.Filter.get_filter(relationship.destination, input) do
+                    changeset,
-                  {:ok, keys} ->
+                    instructions,
-                    relationship.destination
+                    relationship,
-                    |> Ash.Query.for_read(read, input, actor: actor)
+                    input,
-                    |> Ash.Query.filter(^keys)
+                    actor,
-                    |> Ash.Query.do_filter(relationship.filter)
+                    index,
-                    |> Ash.Query.sort(relationship.sort)
+                    opts
-                    |> Ash.Query.set_context(relationship.context)
+                  )
-                    |> Ash.Query.limit(1)
+
-                    |> Ash.Query.set_tenant(changeset.tenant)
+                {_key, _create_or_update, read} ->
-                    |> changeset.api.read_one(
+                  if is_struct(input) do
-                      authorize?: opts[:authorize?],
+                    changeset =
-                      actor: actor
+                      changeset
-                    )
+                      |> Ash.Changeset.set_context(%{
-                    |> case do
+                        private: %{
-                      {:ok, nil} ->
+                          belongs_to_manage_found: %{relationship.name => %{index => input}}
+                        }
+                      })
+                      |> Ash.Changeset.force_change_attribute(
+                        relationship.source_field,
+                        Map.get(input, relationship.destination_field)
+                      )
+
+                    {:cont, {changeset, instructions}}
+                  else
+                    case Ash.Filter.get_filter(relationship.destination, input) do
+                      {:ok, keys} ->
+                        relationship.destination
+                        |> Ash.Query.for_read(read, input, actor: actor)
+                        |> Ash.Query.filter(^keys)
+                        |> Ash.Query.do_filter(relationship.filter)
+                        |> Ash.Query.sort(relationship.sort)
+                        |> Ash.Query.set_context(relationship.context)
+                        |> Ash.Query.limit(1)
+                        |> Ash.Query.set_tenant(changeset.tenant)
+                        |> changeset.api.read_one(
+                          authorize?: opts[:authorize?],
+                          actor: actor
+                        )
+                        |> case do
+                          {:ok, nil} ->
+                            create_belongs_to_record(
+                              changeset,
+                              instructions,
+                              relationship,
+                              input,
+                              actor,
+                              index,
+                              opts
+                            )
+
+                          {:ok, found} ->
+                            changeset =
+                              changeset
+                              |> Ash.Changeset.set_context(%{
+                                private: %{
+                                  belongs_to_manage_found: %{
+                                    relationship.name => %{index => found}
+                                  }
+                                }
+                              })
+                              |> Ash.Changeset.force_change_attribute(
+                                relationship.source_field,
+                                Map.get(found, relationship.destination_field)
+                              )
+
+                            {:cont, {changeset, instructions}}
+
+                          {:error, error} ->
+                            {:halt,
+                             {Ash.Changeset.add_error(changeset, error, [
+                                opts[:meta][:id] || relationship.name
+                              ]), instructions}}
+                        end
+
+                      _ ->
                         create_belongs_to_record(
                           changeset,
                           instructions,
@@ -161,45 +223,16 @@ defmodule Ash.Actions.ManagedRelationships do
                           index,
                           opts
                         )
-
-                      {:ok, found} ->
-                        changeset =
-                          changeset
-                          |> Ash.Changeset.set_context(%{
-                            private: %{
-                              belongs_to_manage_found: %{relationship.name => %{index => found}}
-                            }
-                          })
-                          |> Ash.Changeset.force_change_attribute(
-                            relationship.source_field,
-                            Map.get(found, relationship.destination_field)
-                          )
-
-                        {:cont, {changeset, instructions}}
-
-                      {:error, error} ->
-                        {:halt,
-                         {Ash.Changeset.add_error(changeset, error, [
-                            opts[:meta][:id] || relationship.name
-                          ]), instructions}}
                     end
-
+                  end
-                  _ ->
-                    create_belongs_to_record(
-                      changeset,
-                      instructions,
-                      relationship,
-                      input,
-                      actor,
-                      index,
-                      opts
-                    )
-                end
               end
+
+            _value ->
+              {:cont, {changeset, instructions}}
           end
 
-        _value ->
+        {:error, error} ->
-          {:cont, {changeset, instructions}}
+          {:halt, {:error, error}}
       end
     end)
     |> validate_required_belongs_to()
@@ -243,105 +276,58 @@ defmodule Ash.Actions.ManagedRelationships do
          index,
          opts
        ) do
-    data =
+    if input in [nil, []] do
-      case Map.get(changeset.data, relationship.name) do
+      {:cont, {changeset, instructions}}
-        %Ash.NotLoaded{} ->
+    else
-          changeset.api.load(changeset.data, relationship.name,
+      case opts[:on_no_match] do
-            authorize?: opts[:authorize?],
+        ignore when ignore in [:ignore, :match] ->
-            actor: actor
-          )
-
-        _ ->
-          {:ok, changeset.data}
-      end
-
-    case data do
-      {:ok, data} ->
-        if input in [nil, []] do
           {:cont, {changeset, instructions}}
-        else
-          case opts[:on_no_match] do
-            ignore when ignore in [:ignore, :match] ->
-              {:cont, {changeset, instructions}}
 
-            :error ->
+        :error ->
-              if opts[:on_lookup] != :ignore do
+          if opts[:on_lookup] != :ignore do
-                changeset =
-                  changeset
-                  |> Ash.Changeset.add_error(
-                    NotFound.exception(
-                      primary_key: input,
-                      resource: relationship.destination
-                    ),
-                    [opts[:meta][:id] || relationship.name]
-                  )
-                  |> Ash.Changeset.put_context(:private, %{
-                    error: %{relationship.name => true}
-                  })
-
-                {:halt, {changeset, instructions}}
-              else
-                changeset =
-                  changeset
-                  |> Ash.Changeset.add_error(
-                    InvalidRelationship.exception(
-                      relationship: relationship.name,
-                      message: "Changes would create a new related record"
-                    ),
-                    [opts[:meta][:id] || relationship.name]
-                  )
-                  |> Ash.Changeset.put_context(:private, %{
-                    error: %{relationship.name => true}
-                  })
-
-                {:halt, {changeset, instructions}}
-              end
-
-            {:create, action_name} ->
-              do_create_belongs_to_record(
-                relationship,
-                action_name,
-                input,
-                changeset,
-                actor,
-                opts,
-                instructions,
-                index
-              )
-          end
-        end
-        |> case do
-          {:cont, {changeset, instructions}} ->
             changeset =
-              Ash.Changeset.after_action(changeset, fn _, result ->
+              changeset
-                current_value = Map.get(data, relationship.name)
+              |> Ash.Changeset.add_error(
+                NotFound.exception(
+                  primary_key: input,
+                  resource: relationship.destination
+                ),
+                [opts[:meta][:id] || relationship.name]
+              )
+              |> Ash.Changeset.put_context(:private, %{
+                error: %{relationship.name => true}
+              })
 
-                case delete_unused(
+            {:halt, {changeset, instructions}}
-                       data,
+          else
-                       List.wrap(current_value),
+            changeset =
-                       relationship,
+              changeset
-                       [],
+              |> Ash.Changeset.add_error(
-                       [],
+                InvalidRelationship.exception(
-                       changeset,
+                  relationship: relationship.name,
-                       actor,
+                  message: "Changes would create a new related record"
-                       opts
+                ),
-                     ) do
+                [opts[:meta][:id] || relationship.name]
-                  {:ok, _, new_instructions} ->
+              )
-                    {:ok, result, new_instructions}
+              |> Ash.Changeset.put_context(:private, %{
+                error: %{relationship.name => true}
+              })
 
-                  {:error, error} ->
+            {:halt, {changeset, instructions}}
-                    {:halt, {Ash.Changeset.add_error(changeset, error), instructions}}
+          end
-                end
-              end)
 
-            {:cont, {changeset, instructions}}
+        {:create, action_name} ->
-
+          do_create_belongs_to_record(
-          {:halt, other} ->
+            relationship,
-            {:halt, other}
+            action_name,
-        end
+            input,
-
+            changeset,
-      {:error, error} ->
+            actor,
-        {:halt, {Ash.Changeset.add_error(changeset, error), instructions}}
+            opts,
+            instructions,
+            index
+          )
+      end
     end
   end
 
@@ -375,8 +361,7 @@ defmodule Ash.Actions.ManagedRelationships do
           changeset
           |> Ash.Changeset.set_context(%{
             private: %{
-              belongs_to_manage_created: %{relationship.name => %{index => created}},
+              belongs_to_manage_created: %{relationship.name => %{index => created}}
-              belongs_to_manage_original: Map.get(changeset.data, relationship.name)
             }
           })
           |> Ash.Changeset.force_change_attribute(
@@ -460,7 +445,7 @@ defmodule Ash.Actions.ManagedRelationships do
     opts = Ash.Changeset.ManagedRelationshipHelpers.sanitize_opts(relationship, opts)
     pkeys = pkeys(relationship)
 
-    original_value = original_value(record, changeset, relationship)
+    original_value = original_value(changeset, relationship)
 
     inputs
     |> Enum.with_index()
@@ -532,7 +517,7 @@ defmodule Ash.Actions.ManagedRelationships do
 
     pkeys = [Ash.Resource.Info.primary_key(relationship.destination) | identities]
 
-    original_value = original_value(record, changeset, relationship)
+    original_value = original_value(changeset, relationship)
 
     inputs = List.wrap(inputs)
 
@@ -563,26 +548,22 @@ defmodule Ash.Actions.ManagedRelationships do
     )
     |> case do
       {:ok, new_value, all_notifications, all_used} ->
-        if relationship.type == :belongs_to do
+        case delete_unused(
-          {:ok, record, all_notifications}
+               record,
-        else
+               original_value,
-          case delete_unused(
+               relationship,
-                 record,
+               new_value,
-                 original_value,
+               all_used,
-                 relationship,
+               changeset,
-                 new_value,
+               actor,
-                 all_used,
+               opts
-                 changeset,
+             ) do
-                 actor,
+          {:ok, new_value, notifications} ->
-                 opts
+            {:ok, Map.put(record, relationship.name, Enum.at(List.wrap(new_value), 0)),
-               ) do
+             all_notifications ++ notifications}
-            {:ok, new_value, notifications} ->
-              {:ok, Map.put(record, relationship.name, Enum.at(List.wrap(new_value), 0)),
-               all_notifications ++ notifications}
 
-            {:error, error} ->
+          {:error, error} ->
-              {:error, Ash.Changeset.set_path(error, [opts[:meta][:id] || relationship.name])}
+            {:error, Ash.Changeset.set_path(error, [opts[:meta][:id] || relationship.name])}
-          end
         end
 
       {:error, error} ->
@@ -590,8 +571,8 @@ defmodule Ash.Actions.ManagedRelationships do
     end
   end
 
-  defp original_value(record, _changeset, relationship) do
+  defp original_value(changeset, relationship) do
-    case Map.get(record, relationship.name) do
+    case Map.get(changeset.data, relationship.name) do
       %Ash.NotLoaded{} -> []
       value -> List.wrap(value)
     end
@@ -902,14 +883,14 @@ defmodule Ash.Actions.ManagedRelationships do
 
             case created do
               {:ok, created, notifications} ->
-                {:ok, [created | current_value], notifications, [input]}
+                {:ok, [created | current_value], notifications, [created]}
 
               {:error, error} ->
                 {:error, error}
             end
 
           created ->
-            {:ok, [created | current_value], [], [input]}
+            {:ok, [created | current_value], [], [created]}
         end
 
       {:create, action_name, join_action_name, params} ->
@@ -973,7 +954,8 @@ defmodule Ash.Actions.ManagedRelationships do
             )
             |> case do
               {:ok, _join_row, notifications} ->
-                {:ok, [created | current_value], regular_notifications ++ notifications, [input]}
+                {:ok, [created | current_value], regular_notifications ++ notifications,
+                 [created]}
 
               {:error, error} ->
                 {:error, error}
@@ -1482,34 +1464,16 @@ defmodule Ash.Actions.ManagedRelationships do
   end
 
   defp unrelate_data(
-         source_record,
+         _source_record,
          _record,
-         api,
+         _api,
-         actor,
+         _actor,
-         opts,
+         _opts,
-         action_name,
+         _action_name,
-         tenant,
+         _tenant,
-         %{type: :belongs_to} = relationship,
+         %{type: :belongs_to},
          _changeset
        ) do
-    action_name =
+    {:ok, []}
-      action_name || Ash.Resource.Info.primary_action(relationship.source, :update).name
-
-    source_record
-    |> Ash.Changeset.for_update(action_name, %{},
-      relationships: opts[:relationships] || [],
-      actor: actor
-    )
-    |> Ash.Changeset.force_change_attribute(relationship.source_field, nil)
-    |> Ash.Changeset.set_context(relationship.context)
-    |> Ash.Changeset.set_tenant(tenant)
-    |> api.update(return_notifications?: true, actor: actor, authorize?: opts[:authorize?])
-    |> case do
-      {:ok, _unrelated, notifications} ->
-        {:ok, notifications}
-
-      {:error, error} ->
-        {:error, error}
-    end
   end
 end

test/actions/create_test.exs

@@ -621,6 +621,31 @@ defmodule Ash.Test.Actions.CreateTest do
 
       assert post.author.id == author.id
     end
+
+    test "it clears the relationship if replaced with nil" do
+      author =
+        Author
+        |> new()
+        |> change_attribute(:bio, "best dude")
+        |> Api.create!()
+
+      post =
+        Post
+        |> new()
+        |> change_attribute(:title, "foobar")
+        |> replace_relationship(:author, author)
+        |> Api.create!()
+
+      post =
+        post
+        |> new()
+        |> change_attribute(:title, "foobuz")
+        |> replace_relationship(:author, nil)
+        |> Api.update!()
+
+      assert post.author == nil
+      assert post.author_id == nil
+    end
   end
 
   describe "creating with required belongs_to relationships" do

test/actions/update_test.exs

@@ -656,8 +656,10 @@ defmodule Ash.Test.Actions.UpdateTest do
       |> replace_relationship(:author, author2)
       |> Api.update!()
 
-      assert Enum.map(Api.get!(Author, author2.id, load: [:posts]).posts, & &1.id) == [
+      author2 = Api.get!(Author, author2.id, load: :posts)
-               Api.get!(Post, post.id).id
+
+      assert Enum.map(author2.posts, & &1.id) == [
+               post.id
              ]
     end