From 7a00cfabf33772708e05e668fa6673bb43aaa27c Mon Sep 17 00:00:00 2001 From: Barnabas Jovanovics Date: Thu, 26 Feb 2026 11:37:21 +0100 Subject: [PATCH 1/3] test: add regression tests for bulk manage_relationship rollback bug When manage_relationships fails in run_after_action_hooks, the error path returns {:error, error, changeset} without calling Ash.DataLayer.rollback. This causes the parent record change to be committed despite the child validation failure when using transaction: :batch. All bulk tests use batch_size: 2 with 5 inputs to exercise batch boundary behavior. Tests cover bulk_create (including upserts), bulk_update, and bulk_destroy across all strategies (stream, atomic, atomic_batches) and both transaction modes (batch and all). --- .../rollback_children/20260225131409.json | 75 ++++ .../rollback_parents/20260225131408.json | 44 ++ .../rollback_parents/20260226084406.json | 59 +++ .../20260225131407_migrate_resources70.exs | 54 +++ .../20260226084405_migrate_resources71.exs | 19 + ...bulk_manage_relationship_rollback_test.exs | 388 ++++++++++++++++++ test/support/domain.ex | 2 + test/support/resources/rollback_child.ex | 36 ++ test/support/resources/rollback_parent.ex | 61 +++ 9 files changed, 738 insertions(+) create mode 100644 priv/resource_snapshots/test_repo/rollback_children/20260225131409.json create mode 100644 priv/resource_snapshots/test_repo/rollback_parents/20260225131408.json create mode 100644 priv/resource_snapshots/test_repo/rollback_parents/20260226084406.json create mode 100644 priv/test_repo/migrations/20260225131407_migrate_resources70.exs create mode 100644 priv/test_repo/migrations/20260226084405_migrate_resources71.exs create mode 100644 test/bulk_manage_relationship_rollback_test.exs create mode 100644 test/support/resources/rollback_child.ex create mode 100644 test/support/resources/rollback_parent.ex diff --git a/priv/resource_snapshots/test_repo/rollback_children/20260225131409.json b/priv/resource_snapshots/test_repo/rollback_children/20260225131409.json new file mode 100644 index 00000000..77be14be --- /dev/null +++ b/priv/resource_snapshots/test_repo/rollback_children/20260225131409.json @@ -0,0 +1,75 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"gen_random_uuid()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "title", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "rollback_children_rollback_parent_id_fkey", + "on_delete": "delete", + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "rollback_parents" + }, + "scale": null, + "size": null, + "source": "rollback_parent_id", + "type": "uuid" + } + ], + "base_filter": null, + "check_constraints": [], + "create_table_options": null, + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "84B94C6E7E9C2B15D35A96914F1F125C3B8188FDD1F8C4835F9B95E3B2C205A9", + "identities": [], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.AshPostgres.TestRepo", + "schema": null, + "table": "rollback_children" +} \ No newline at end of file diff --git a/priv/resource_snapshots/test_repo/rollback_parents/20260225131408.json b/priv/resource_snapshots/test_repo/rollback_parents/20260225131408.json new file mode 100644 index 00000000..1aec4423 --- /dev/null +++ b/priv/resource_snapshots/test_repo/rollback_parents/20260225131408.json @@ -0,0 +1,44 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"gen_random_uuid()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "name", + "type": "text" + } + ], + "base_filter": null, + "check_constraints": [], + "create_table_options": null, + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "D48EF295250D00CBF79C15D7D2F283C68ED48B0E20E618B0490E81D735BBB2A0", + "identities": [], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.AshPostgres.TestRepo", + "schema": null, + "table": "rollback_parents" +} \ No newline at end of file diff --git a/priv/resource_snapshots/test_repo/rollback_parents/20260226084406.json b/priv/resource_snapshots/test_repo/rollback_parents/20260226084406.json new file mode 100644 index 00000000..2f98ca9a --- /dev/null +++ b/priv/resource_snapshots/test_repo/rollback_parents/20260226084406.json @@ -0,0 +1,59 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"gen_random_uuid()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "name", + "type": "text" + } + ], + "base_filter": null, + "check_constraints": [], + "create_table_options": null, + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "4A01DCA3229485184855683251C5175BBB828A8299B247C9F6F19972863040F8", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "rollback_parents_unique_name_index", + "keys": [ + { + "type": "atom", + "value": "name" + } + ], + "name": "unique_name", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.AshPostgres.TestRepo", + "schema": null, + "table": "rollback_parents" +} \ No newline at end of file diff --git a/priv/test_repo/migrations/20260225131407_migrate_resources70.exs b/priv/test_repo/migrations/20260225131407_migrate_resources70.exs new file mode 100644 index 00000000..58ff99ca --- /dev/null +++ b/priv/test_repo/migrations/20260225131407_migrate_resources70.exs @@ -0,0 +1,54 @@ +defmodule AshPostgres.TestRepo.Migrations.MigrateResources70 do + @moduledoc """ + Updates resources based on their most recent snapshots. + + This file was autogenerated with `mix ash_postgres.generate_migrations` + """ + + use Ecto.Migration + + def up do + create table(:rollback_children, primary_key: false) do + add(:id, :uuid, null: false, default: fragment("gen_random_uuid()"), primary_key: true) + add(:title, :text, null: false) + add(:rollback_parent_id, :uuid) + end + + create table(:rollback_parents, primary_key: false) do + add(:id, :uuid, null: false, default: fragment("gen_random_uuid()"), primary_key: true) + end + + alter table(:rollback_children) do + modify( + :rollback_parent_id, + references(:rollback_parents, + column: :id, + name: "rollback_children_rollback_parent_id_fkey", + type: :uuid, + prefix: "public", + on_delete: :delete_all + ) + ) + end + + alter table(:rollback_parents) do + add(:name, :text, null: false) + end + end + + def down do + alter table(:rollback_parents) do + remove(:name) + end + + drop(constraint(:rollback_children, "rollback_children_rollback_parent_id_fkey")) + + alter table(:rollback_children) do + modify(:rollback_parent_id, :uuid) + end + + drop(table(:rollback_parents)) + + drop(table(:rollback_children)) + end +end diff --git a/priv/test_repo/migrations/20260226084405_migrate_resources71.exs b/priv/test_repo/migrations/20260226084405_migrate_resources71.exs new file mode 100644 index 00000000..75db181b --- /dev/null +++ b/priv/test_repo/migrations/20260226084405_migrate_resources71.exs @@ -0,0 +1,19 @@ +defmodule AshPostgres.TestRepo.Migrations.MigrateResources71 do + @moduledoc """ + Updates resources based on their most recent snapshots. + + This file was autogenerated with `mix ash_postgres.generate_migrations` + """ + + use Ecto.Migration + + def up do + create(unique_index(:rollback_parents, [:name], name: "rollback_parents_unique_name_index")) + end + + def down do + drop_if_exists( + unique_index(:rollback_parents, [:name], name: "rollback_parents_unique_name_index") + ) + end +end diff --git a/test/bulk_manage_relationship_rollback_test.exs b/test/bulk_manage_relationship_rollback_test.exs new file mode 100644 index 00000000..879844db --- /dev/null +++ b/test/bulk_manage_relationship_rollback_test.exs @@ -0,0 +1,388 @@ +# SPDX-FileCopyrightText: 2019 ash_postgres contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshPostgres.BulkManageRelationshipRollbackTest do + @moduledoc """ + Tests that bulk_create, bulk_update, and bulk_destroy properly roll back + when manage_relationship fails during the after-action hooks phase. + + Bug: In `run_after_action_hooks/4` (present in create/bulk.ex, update/bulk.ex, + and destroy/bulk.ex), when `manage_relationships` returns `{:error, error}`, + the code returns `[{:error, error, changeset}]` WITHOUT calling + `Ash.DataLayer.rollback`. The `run_after_actions` error path right above it + DOES call rollback. + + With `transaction: :batch`, `process_results` runs OUTSIDE the batch + transaction, so its `maybe_rollback` safety net cannot help — the parent + record change is committed despite the child validation failure. + + All bulk tests use batch_size: 2 with 5 inputs. For bulk_create the 4th + input has an invalid child, creating batches [1,2], [3,4*], [5] where + batch 2 contains the failure. For bulk_update/destroy, all records receive + the same invalid child input so every batch fails. + """ + use AshPostgres.RepoCase, async: false + + alias AshPostgres.Test.{RollbackParent, RollbackChild} + + # ============================================================================ + # bulk_create + # ============================================================================ + + describe "bulk_create: non-bulk control" do + test "single create rolls back parent when child validation fails" do + assert_raise Ash.Error.Invalid, fn -> + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{ + name: "parent1", + children: [%{title: nil}] + }), + authorize?: false + ) + end + + assert [] == Ash.read!(RollbackParent, authorize?: false) + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + describe "bulk_create: transaction :batch" do + test "batch containing invalid child rolls back, other batches commit" do + result = + Ash.bulk_create( + [ + %{name: "ok_1", children: [%{title: "c1"}]}, + %{name: "ok_2", children: [%{title: "c2"}]}, + %{name: "ok_3", children: [%{title: "c3"}]}, + %{name: "fail_4", children: [%{title: nil}]}, + %{name: "ok_5", children: [%{title: "c5"}]} + ], + RollbackParent, + :create, + batch_size: 2, + transaction: :batch, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status in [:error, :partial_success] + + parents = Ash.read!(RollbackParent, authorize?: false) + parent_names = Enum.map(parents, & &1.name) |> Enum.sort() + + # Batch [ok_1, ok_2] commits, batch [ok_3, fail_4] should rollback, + # batch [ok_5] commits. BUG: ok_3 is NOT rolled back. + assert parent_names == ["ok_1", "ok_2", "ok_5"], + "Expected [ok_1, ok_2, ok_5] but found #{inspect(parent_names)}" + + children = Ash.read!(RollbackChild, authorize?: false) + child_titles = Enum.map(children, & &1.title) |> Enum.sort() + + assert child_titles == ["c1", "c2", "c5"], + "Expected [c1, c2, c5] but found #{inspect(child_titles)}" + end + end + + describe "bulk_create: transaction :all" do + test "any invalid child rolls back ALL batches" do + result = + Ash.bulk_create( + [ + %{name: "ok_1", children: [%{title: "c1"}]}, + %{name: "ok_2", children: [%{title: "c2"}]}, + %{name: "ok_3", children: [%{title: "c3"}]}, + %{name: "fail_4", children: [%{title: nil}]}, + %{name: "ok_5", children: [%{title: "c5"}]} + ], + RollbackParent, + :create, + batch_size: 2, + transaction: :all, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status == :error + assert [] == Ash.read!(RollbackParent, authorize?: false) + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + # ============================================================================ + # bulk_create upsert + # ============================================================================ + + describe "bulk_create upsert: transaction :batch" do + test "batch containing invalid child rolls back, other batches commit" do + for name <- ~w(ok_1 ok_2 ok_3 fail_4 ok_5) do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: name}), + authorize?: false + ) + end + + result = + Ash.bulk_create( + [ + %{name: "ok_1", children: [%{title: "c1"}]}, + %{name: "ok_2", children: [%{title: "c2"}]}, + %{name: "ok_3", children: [%{title: "c3"}]}, + %{name: "fail_4", children: [%{title: nil}]}, + %{name: "ok_5", children: [%{title: "c5"}]} + ], + RollbackParent, + :upsert, + batch_size: 2, + transaction: :batch, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status in [:error, :partial_success] + + # All 5 parents should still exist (upsert doesn't create new ones) + assert length(Ash.read!(RollbackParent, authorize?: false)) == 5 + + children = Ash.read!(RollbackChild, authorize?: false) + child_titles = Enum.map(children, & &1.title) |> Enum.sort() + + # Batch [ok_1, ok_2] commits children, batch [ok_3, fail_4] should + # rollback children, batch [ok_5] commits children + assert child_titles == ["c1", "c2", "c5"], + "Expected [c1, c2, c5] but found #{inspect(child_titles)}" + end + end + + describe "bulk_create upsert: transaction :all" do + test "any invalid child rolls back ALL batches" do + for name <- ~w(ok_1 ok_2 ok_3 fail_4 ok_5) do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: name}), + authorize?: false + ) + end + + result = + Ash.bulk_create( + [ + %{name: "ok_1", children: [%{title: "c1"}]}, + %{name: "ok_2", children: [%{title: "c2"}]}, + %{name: "ok_3", children: [%{title: "c3"}]}, + %{name: "fail_4", children: [%{title: nil}]}, + %{name: "ok_5", children: [%{title: "c5"}]} + ], + RollbackParent, + :upsert, + batch_size: 2, + transaction: :all, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status == :error + assert length(Ash.read!(RollbackParent, authorize?: false)) == 5 + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + # ============================================================================ + # bulk_update (all strategies) + # ============================================================================ + + describe "bulk_update: non-bulk control" do + test "single update rolls back when child validation fails" do + parent = + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "original"}), + authorize?: false + ) + + assert_raise Ash.Error.Invalid, fn -> + parent + |> Ash.Changeset.for_update(:update_with_children, %{ + name: "updated", + children: [%{title: nil}] + }) + |> Ash.update!(authorize?: false) + end + + [reloaded] = Ash.read!(RollbackParent, authorize?: false) + + assert reloaded.name == "original", + "Parent update should be rolled back, but name is #{inspect(reloaded.name)}" + + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + for strategy <- [[:stream], [:atomic, :stream], [:atomic_batches, :stream]] do + describe "bulk_update strategy #{inspect(strategy)}: transaction :batch" do + test "5 parents updated with invalid children, batch_size 2, all batches should rollback" do + parents = + for i <- 1..5 do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "original_#{i}"}), + authorize?: false + ) + end + + result = + parents + |> Ash.bulk_update( + :update_with_children, + %{name: "updated", children: [%{title: nil}]}, + strategy: unquote(strategy), + batch_size: 2, + transaction: :batch, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status in [:error, :partial_success] + + reloaded = Ash.read!(RollbackParent, authorize?: false) + names = Enum.map(reloaded, & &1.name) |> Enum.sort() + + assert names == Enum.map(1..5, &"original_#{&1}"), + "All updates should be rolled back, but found #{inspect(names)}" + + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + describe "bulk_update strategy #{inspect(strategy)}: transaction :all" do + test "5 parents updated with invalid children, batch_size 2, everything rolls back" do + parents = + for i <- 1..5 do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "original_#{i}"}), + authorize?: false + ) + end + + result = + parents + |> Ash.bulk_update( + :update_with_children, + %{name: "updated", children: [%{title: nil}]}, + strategy: unquote(strategy), + batch_size: 2, + transaction: :all, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status == :error + + reloaded = Ash.read!(RollbackParent, authorize?: false) + names = Enum.map(reloaded, & &1.name) |> Enum.sort() + + assert names == Enum.map(1..5, &"original_#{&1}"), + "All updates should be rolled back, but found #{inspect(names)}" + + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + end + + # ============================================================================ + # bulk_destroy (all strategies) + # ============================================================================ + + describe "bulk_destroy: non-bulk control" do + test "single destroy with invalid child argument succeeds (relationship management not exercised)" do + # NOTE: Single Ash.destroy! does not exercise manage_relationship for + # has_many :create — the parent is simply deleted. This control documents + # that the single path behaves differently from the bulk path. + parent = + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "should_be_destroyed"}), + authorize?: false + ) + + parent + |> Ash.Changeset.for_destroy(:destroy_with_children, %{children: [%{title: nil}]}) + |> Ash.destroy!(authorize?: false) + + assert [] == Ash.read!(RollbackParent, authorize?: false) + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + for strategy <- [[:stream], [:atomic, :stream], [:atomic_batches, :stream]] do + describe "bulk_destroy strategy #{inspect(strategy)}: transaction :batch" do + test "5 parents destroyed with invalid children, batch_size 2, all batches should rollback" do + for i <- 1..5 do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "parent_#{i}"}), + authorize?: false + ) + end + + result = + RollbackParent + |> Ash.read!(authorize?: false) + |> Ash.bulk_destroy( + :destroy_with_children, + %{children: [%{title: nil}]}, + strategy: unquote(strategy), + batch_size: 2, + transaction: :batch, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status in [:error, :partial_success] + + parents = Ash.read!(RollbackParent, authorize?: false) + + assert length(parents) == 5, + "All destroys should be rolled back, but found #{length(parents)} parents" + + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + + describe "bulk_destroy strategy #{inspect(strategy)}: transaction :all" do + test "5 parents destroyed with invalid children, batch_size 2, everything rolls back" do + for i <- 1..5 do + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "parent_#{i}"}), + authorize?: false + ) + end + + result = + RollbackParent + |> Ash.read!(authorize?: false) + |> Ash.bulk_destroy( + :destroy_with_children, + %{children: [%{title: nil}]}, + strategy: unquote(strategy), + batch_size: 2, + transaction: :all, + rollback_on_error?: true, + return_errors?: true, + authorize?: false + ) + + assert result.status == :error + + parents = Ash.read!(RollbackParent, authorize?: false) + + assert length(parents) == 5, + "All destroys should be rolled back, but found #{length(parents)} parents" + + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + end + end +end diff --git a/test/support/domain.ex b/test/support/domain.ex index 4a4ea01b..f0a5ae76 100644 --- a/test/support/domain.ex +++ b/test/support/domain.ex @@ -67,6 +67,8 @@ defmodule AshPostgres.Test.Domain do resource(AshPostgres.Test.Container) resource(AshPostgres.Test.Item) resource(AshPostgres.Test.AfterTransactionPost) + resource(AshPostgres.Test.RollbackParent) + resource(AshPostgres.Test.RollbackChild) end authorization do diff --git a/test/support/resources/rollback_child.ex b/test/support/resources/rollback_child.ex new file mode 100644 index 00000000..9d24af56 --- /dev/null +++ b/test/support/resources/rollback_child.ex @@ -0,0 +1,36 @@ +# SPDX-FileCopyrightText: 2019 ash_postgres contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshPostgres.Test.RollbackChild do + @moduledoc false + use Ash.Resource, + domain: AshPostgres.Test.Domain, + data_layer: AshPostgres.DataLayer + + postgres do + table "rollback_children" + repo AshPostgres.TestRepo + + references do + reference(:rollback_parent, on_delete: :delete) + end + end + + actions do + default_accept(:*) + defaults([:read, :create]) + end + + attributes do + uuid_primary_key(:id) + attribute(:title, :string, allow_nil?: false, public?: true) + end + + relationships do + belongs_to :rollback_parent, AshPostgres.Test.RollbackParent do + attribute_writable?(true) + public?(true) + end + end +end diff --git a/test/support/resources/rollback_parent.ex b/test/support/resources/rollback_parent.ex new file mode 100644 index 00000000..e76b5644 --- /dev/null +++ b/test/support/resources/rollback_parent.ex @@ -0,0 +1,61 @@ +# SPDX-FileCopyrightText: 2019 ash_postgres contributors +# +# SPDX-License-Identifier: MIT + +defmodule AshPostgres.Test.RollbackParent do + @moduledoc false + use Ash.Resource, + domain: AshPostgres.Test.Domain, + data_layer: AshPostgres.DataLayer + + postgres do + table "rollback_parents" + repo AshPostgres.TestRepo + end + + actions do + default_accept(:*) + defaults([:read]) + + create :create do + argument(:children, {:array, :map}, allow_nil?: true) + change(manage_relationship(:children, type: :create)) + end + + create :upsert do + upsert?(true) + upsert_identity(:unique_name) + upsert_fields([:name]) + argument(:children, {:array, :map}, allow_nil?: true) + change(manage_relationship(:children, type: :create)) + end + + update :update_with_children do + require_atomic?(false) + argument(:children, {:array, :map}, allow_nil?: true) + change(manage_relationship(:children, type: :create)) + end + + destroy :destroy_with_children do + require_atomic?(false) + argument(:children, {:array, :map}, allow_nil?: true) + change(manage_relationship(:children, type: :create)) + end + end + + attributes do + uuid_primary_key(:id) + attribute(:name, :string, allow_nil?: false, public?: true) + end + + identities do + identity(:unique_name, [:name]) + end + + relationships do + has_many(:children, AshPostgres.Test.RollbackChild, + destination_attribute: :rollback_parent_id, + public?: true + ) + end +end From 92d60344df2283a4885236646ce04a4905254f58 Mon Sep 17 00:00:00 2001 From: Barnabas Jovanovics Date: Thu, 26 Feb 2026 12:03:44 +0100 Subject: [PATCH 2/3] fix: update batch test assertions to account for stop_on_error after rollback When the rollback fix is applied, the failing batch triggers stop_on_error so subsequent batches never run. Updated assertions to check that the failing batch's records are NOT present rather than asserting exact record lists. --- ...bulk_manage_relationship_rollback_test.exs | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/bulk_manage_relationship_rollback_test.exs b/test/bulk_manage_relationship_rollback_test.exs index 879844db..7324c0f9 100644 --- a/test/bulk_manage_relationship_rollback_test.exs +++ b/test/bulk_manage_relationship_rollback_test.exs @@ -72,16 +72,16 @@ defmodule AshPostgres.BulkManageRelationshipRollbackTest do parents = Ash.read!(RollbackParent, authorize?: false) parent_names = Enum.map(parents, & &1.name) |> Enum.sort() - # Batch [ok_1, ok_2] commits, batch [ok_3, fail_4] should rollback, - # batch [ok_5] commits. BUG: ok_3 is NOT rolled back. - assert parent_names == ["ok_1", "ok_2", "ok_5"], - "Expected [ok_1, ok_2, ok_5] but found #{inspect(parent_names)}" + # Batch [ok_1, ok_2] commits, batch [ok_3, fail_4] rolls back, + # subsequent batches may not run due to stop_on_error. + assert "ok_3" not in parent_names, + "ok_3 should be rolled back with fail_4, but found #{inspect(parent_names)}" children = Ash.read!(RollbackChild, authorize?: false) child_titles = Enum.map(children, & &1.title) |> Enum.sort() - assert child_titles == ["c1", "c2", "c5"], - "Expected [c1, c2, c5] but found #{inspect(child_titles)}" + assert "c3" not in child_titles, + "c3 should be rolled back with fail_4, but found #{inspect(child_titles)}" end end @@ -150,10 +150,10 @@ defmodule AshPostgres.BulkManageRelationshipRollbackTest do children = Ash.read!(RollbackChild, authorize?: false) child_titles = Enum.map(children, & &1.title) |> Enum.sort() - # Batch [ok_1, ok_2] commits children, batch [ok_3, fail_4] should - # rollback children, batch [ok_5] commits children - assert child_titles == ["c1", "c2", "c5"], - "Expected [c1, c2, c5] but found #{inspect(child_titles)}" + # Batch [ok_1, ok_2] commits children, batch [ok_3, fail_4] rolls back, + # subsequent batches may not run due to stop_on_error. + assert "c3" not in child_titles, + "c3 should be rolled back with fail_4, but found #{inspect(child_titles)}" end end From cabee86f0a4a5943c73599662119d8c6c58d98c4 Mon Sep 17 00:00:00 2001 From: Barnabas Jovanovics Date: Fri, 27 Feb 2026 13:09:06 +0100 Subject: [PATCH 3/3] fix: update single destroy control test to expect manage_relationship behavior Now that ash's single hard destroy path calls manage_relationships, the control test should verify that invalid child data properly triggers an error and transaction rollback, matching bulk destroy behavior. Also adds a test confirming FK constraint rollback when creating children for a hard-deleted parent. --- ...bulk_manage_relationship_rollback_test.exs | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/test/bulk_manage_relationship_rollback_test.exs b/test/bulk_manage_relationship_rollback_test.exs index 7324c0f9..a57210e3 100644 --- a/test/bulk_manage_relationship_rollback_test.exs +++ b/test/bulk_manage_relationship_rollback_test.exs @@ -24,7 +24,7 @@ defmodule AshPostgres.BulkManageRelationshipRollbackTest do """ use AshPostgres.RepoCase, async: false - alias AshPostgres.Test.{RollbackParent, RollbackChild} + alias AshPostgres.Test.{RollbackChild, RollbackParent} # ============================================================================ # bulk_create @@ -297,21 +297,41 @@ defmodule AshPostgres.BulkManageRelationshipRollbackTest do # ============================================================================ describe "bulk_destroy: non-bulk control" do - test "single destroy with invalid child argument succeeds (relationship management not exercised)" do - # NOTE: Single Ash.destroy! does not exercise manage_relationship for - # has_many :create — the parent is simply deleted. This control documents - # that the single path behaves differently from the bulk path. + test "single destroy with invalid child argument rolls back (manage_relationship exercised)" do parent = Ash.create!( Ash.Changeset.for_create(RollbackParent, :create, %{name: "should_be_destroyed"}), authorize?: false ) - parent - |> Ash.Changeset.for_destroy(:destroy_with_children, %{children: [%{title: nil}]}) - |> Ash.destroy!(authorize?: false) + assert {:error, %Ash.Error.Invalid{}} = + parent + |> Ash.Changeset.for_destroy(:destroy_with_children, %{children: [%{title: nil}]}) + |> Ash.destroy(authorize?: false) - assert [] == Ash.read!(RollbackParent, authorize?: false) + # Parent should still exist because the transaction rolled back + assert [%{name: "should_be_destroyed"}] = Ash.read!(RollbackParent, authorize?: false) + assert [] == Ash.read!(RollbackChild, authorize?: false) + end + + test "single destroy with valid child argument rolls back due to FK constraint" do + # Creating children that belong_to a hard-deleted parent will always fail + # at the DB level (FK constraint), so the transaction rolls back. + parent = + Ash.create!( + Ash.Changeset.for_create(RollbackParent, :create, %{name: "should_be_destroyed"}), + authorize?: false + ) + + assert {:error, %Ash.Error.Invalid{}} = + parent + |> Ash.Changeset.for_destroy(:destroy_with_children, %{ + children: [%{title: "orphan_child"}] + }) + |> Ash.destroy(authorize?: false) + + # Parent still exists because transaction rolled back + assert [%{name: "should_be_destroyed"}] = Ash.read!(RollbackParent, authorize?: false) assert [] == Ash.read!(RollbackChild, authorize?: false) end end