fix: return upsert identity keys and correlate on the full key set so bulk upserts don't drop records - #812
Merged
zachdaniel merged 2 commits intoAug 11, 2026
Conversation
Bulk upserts correlate each returned row back to its changeset by the upsert identity keys. Since ash-project#786 those keys are no longer read back when the actions select does not include them (e.g. `upsert_fields []` with `select: []`, as Ash.Reactors bulk_create step passes), so nothing correlates and every record is silently dropped from the result while still being written to the database. Add the identity attribute keys to the upsert RETURNING list so correlation always succeeds; `Ash.Actions.Helpers.select/2` masks the extra fields back out. Fix and regression tests originally by @marcnnn (ash-project#808, ash-project#809).
Returning the identity keys (previous commit) is not enough for multi-field identities that leave a key unset: `Map.take(changeset.attributes, keys)` drops the unset key, so the changeset correlation key has fewer fields than the returned rows key and still never matches - the record is silently dropped. Build the changeset correlation key over every identity key, defaulting unset ones to nil, so both sides have the same shape. Covers natural-key identities like [barcode, timestamp, order_id, route_id] with `nils_distinct?: false` where trailing keys are commonly nil.
Contributor
|
🚀 Thank you for your contribution! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Since #786, bulk upserts can silently drop records from the returned result while still
writing them to the database — so
after_actionhooks and notifiers never run for thedropped records. This surfaced on the 2.10.0 → 2.11.0 upgrade (same regression as #808/#809).
bulk_create/3correlates each returned row back to its changeset by the upsert identity'skeys. Two things break that correlation:
The identity keys aren't selected. When the action's
selectexcludes them(e.g.
upsert_fields []withselect: [], asAsh.Reactor'sbulk_createstep passes),the returned rows don't contain the identity columns, so nothing correlates and every
record is dropped. This is the fix from @marcnnn's fix: return upsert identity keys so bulk upserts don't drop records #808, with the regression tests from test: cover bulk upsert dropping records when select excludes identity keys #809.
Unset (nil) identity keys are missing on the changeset side. For a multi-field identity
that leaves a key unset — a nullable field left
nil, common withnils_distinct?: falsenatural keys such as
[barcode, timestamp, order_id, route_id]— the changeset key is builtwith
Map.take(changeset.attributes, keys), which drops the unset key. The changeset'scorrelation key then has fewer fields than the returned row's key (whose
Map.take(row, keys)includes the key as
nil), so it still never matches even after (1).Fix
RETURNINGlist so returned rows alwayscarry them.
Ash.Actions.Helpers.select/2masks the extra fields back out of the records,so this doesn't widen what callers observe. Identity keys that aren't attributes (e.g. an
identity over a calculation) have no column and are left out.
keys to
nil, so both sides have the same shape.Tests
bulk upsert returns inserted records when select excludes the identity keys(from test: cover bulk upsert dropping records when select excludes identity keys #809)bulk upsert returns updated records when select excludes the identity keys(from test: cover bulk upsert dropping records when select excludes identity keys #809)bulk upsert returns records when a multi-field identity leaves a key unset (nil)(new;covers facet 2)
The fix in (1) and the first two tests are originally by @marcnnn (#808, #809); this PR carries
them and adds the multi-field / nil-identity correlation fix and test on top.
Note for reviewers
On PostgreSQL 17 (MERGE path) the pre-existing test
returns a skipped upsert whose identity contains nil(added by #794) fails with a unique-constraint error — that's a separatenil-identity issue in the
return_skipped_upsert?path, unrelated to and not addressed bythis PR.
Disclosure
Drafted with AI assistance, but every change was reviewed and validated: the failure is
reproduced by a failing test first, the fix is verified against those tests, and the whole
change was exercised end-to-end against a real 8k-test downstream suite on PostgreSQL 17.