fix: return upsert identity keys so bulk upserts don't drop records - #808
Conversation
A bulk upsert pairs each returned row back up with its changeset by the upsert
identity's keys. Those keys come out of `RETURNING`, which is built from the
action's `action_select` - so when the select doesn't include them, every row
reads back with `nil` for each key, nothing correlates, and every record is
dropped from the result. The rows are still written, so the caller sees
`%Ash.BulkResult{status: :success, errors: [], records: []}` for a batch that
actually inserted or updated fine.
An action whose `action_select` is narrow enough to trigger this isn't unusual:
`action_select` falls back to just the primary key for any create action with no
changes and no notifiers, and `Ash.Reactor`'s `bulk_create` step always passes
`select: []`, so every reactor bulk upsert against such an action hits this.
Add the identity keys to `returning` when upserting, skipping keys that aren't
attributes (an identity over a calculation, e.g. `upper(thing)`, has no column
to return). `Ash.Actions.Helpers.select/2` masks the extra fields back out of
the records afterwards, so this doesn't widen what callers observe.
Affects both upsert paths - `INSERT ... ON CONFLICT` and the PostgreSQL 17+
`MERGE` path - so the regression tests are untagged and run across the matrix.
|
Please review AI generated code prior to opening a PR, as this creates a notification for me to come look at it. |
|
Sorry for creating a not-helpful notification. It was in the evening for me, and I thought raising awareness for a regression in a minior version would help, even before I understand fully and can explain the problem in my own words. Now, with a fresh mind, I can explain the regression in my own words. The problem is: On upsert the columns that need to be selected to match the updated rows with the rows from the database are not selected anymore. This is a regression that is related to #786 and #638 In my case, an upsert_identity with unique_index defines the upsert semantic already, so the select was left out and is filled by the upsert reactor with the empty list. The tests reproduce the problem. Should I create a new PR or an issue, or do we continue here? Thanks, |
|
Yes a PR with a regression test would be wonderful, thank you 🙇 |
… bulk upserts don't drop
records (#812)
Bulk upserts correlate each returned row back to its changeset by the upsert
identity keys. Since #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 (#808, #809).
* fix: include unset (nil) identity keys in bulk upsert correlation
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.
2 participants
My dependabot branch for the ash_postgres update from 2.10.0 to 2.11.0 failed CI.
I send my AI to create a fix.
I will read the surrunding code until I can verify the AI code but open this as draft until then.
Contributor checklist
Leave anything that you believe does not apply unchecked.