test: regression tests for bulk manage_relationship rollback bug#702
test: regression tests for bulk manage_relationship rollback bug#702
Conversation
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 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.
It is strange to manage_relationship on destroy but regardless bulk + non-bulk semantics should be identical. Would you mind opening a separate issue about that? Great catch 🙇 |
Sure, is the expectation that manage_relationship is just not supported by I can't really see a good usecase for it, I guess with the right options you might be able to delete something using it, but for that we have the cascade_destroy change, or datalayer level things. Just wanted to make sure I know the way you wanna go with it before opening the issue |
It's meant to work roughly the same, especially considering that destroy actions could actually be |
… 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.
The single hard destroy path in `destroy.ex` never called `manage_relationships`, causing any `manage_relationship` changes declared on hard destroy actions to be silently ignored. The bulk destroy, soft destroy, create, and update paths all properly called `manage_relationships`. Adds `manage_relationships/4` to `Ash.Actions.Destroy` following the same pattern used in `Ash.Actions.Update`, and pipes the data layer result through it before the notification/select pipeline. Also adds comprehensive tests for manage_relationship on destroy actions: single hard destroy, single soft destroy, bulk hard destroy, and bulk soft destroy. Closes ash-project/ash_postgres#702
2 participants
Summary
run_after_action_hooksin bulk actions does not callAsh.DataLayer.rollbackwhenmanage_relationshipsfailsbulk_create(including upserts),bulk_update, andbulk_destroybatch_size: 2with 5 inputs to exercise batch boundary behavior[:stream],[:atomic, :stream],[:atomic_batches, :stream]) and both transaction modes (:batchand:all)Related
Destroy path inconsistency
Worth noting: the non-bulk
Ash.destroy!/2control test behaves differently fromAsh.create!/2andAsh.update!/2. Single create and update both exercisemanage_relationshipand correctly roll back when the child validation fails. Single destroy does not exercisemanage_relationshipforhas_manywithtype: :createat all — the parent is simply deleted without attempting child creation. The bulk destroy path does exercise it (viarun_after_action_hooks), so the bug manifests there.This means there's an inconsistency between the single and bulk destroy paths regarding when
manage_relationshipis evaluated. It may be intentional (managing relationships on destroy is unusual), but it's worth investigating whether the single destroy path should also honormanage_relationshipchanges, or whether the bulk destroy path should skip them to match.Test plan
mix test test/bulk_manage_relationship_rollback_test.exs→ 19 tests, 7 failures (all:batch)ASH_VERSION=local): 19 tests, 0 failures