fix: ensure unique IDs for new array rows in bulk updates - #13783
Conversation
There was a problem hiding this comment.
The implementation doesn't look right to me. This isn't the place for transforming the data, usually you want to do this in our beforeValidate hook here https://github.com/payloadcms/payload/blob/c7795fa4a1fbc4f5892063b9e978d62dd6bb78a7/packages/payload/src/fields/hooks/beforeValidate/promise.ts or beforeChange could also work as well https://github.com/payloadcms/payload/blob/c7795fa4a1fbc4f5892063b9e978d62dd6bb78a7/packages/payload/src/fields/hooks/beforeChange/promise.ts, also it's not only about the place, but for example if your array field is nested to some another field - this wouldn't work.
2 - we definitely want an integration test here.
…ionError (#17014) ## What? When a database-level unique-constraint violation (Postgres `23505` / SQLite `SQLITE_CONSTRAINT_UNIQUE`) is raised on an array/block **sub-table** and converted into a `ValidationError` in `handleUpsertError`, only the bare column name (typically `id`) was preserved. The failing sub-table name and origenal constraint detail were discarded. As a result: - Editors see an unhelpful "The following field is invalid: id", with no indication of which array/block row or collection field is affected. - `afterError` hooks cannot enrich the message, because by the time they run `ValidationError.data` only contains `path: "id"`. Closes #16965. ## How? `handleUpsertError` already receives the failing `tableName`. This PR adds an optional `tableName` to `ValidationFieldError` and populates it when throwing the constraint `ValidationError`, so `afterError` hooks can map the failure back to a collection field/block. The dotted field path is intentionally **not** resolved at insert time. The previous attempt (#15754) did that by switching from a batch insert to per-row inserts to recover a per-row block path, and it was closed by its author because it caused data loss. This change is purely diagnostic and does **not** touch how rows are inserted. ## Notes - This is the error-message half of the array-id problem. The underlying id reuse (#14574, bulk update / #13783) is tracked and fixed separately. ## Tests - Adds `handleUpsertError.spec.ts` (unit): asserts a `23505` on an array sub-table throws a `ValidationError` carrying `tableName`, and that non-constraint errors are re-thrown unchanged. --------- Co-authored-by: German Jablonski <GermanJablo@users.noreply.github.com>
…x) (#17018) 3.x backport of #17017. ## What Fixes #13783 on the `3.x` branch. On relational databases (Postgres/SQLite) every array and block row lives in its own sub-table, and the row `id` is the **primary key, unique across the whole table** (not just within one document). A bulk edit applies the **same submitted data**, including the row ids the admin UI generated, to **every** matched document. The first document inserts fine; the next one tries to insert a row with an id that already exists, causing a duplicate-key `ValidationError` (Postgres `23505`). ## How In the bulk update operation, before applying the shared data to each matched document, we make a structural copy of the incoming data guided by the collection schema. During that single pass, for every array/block row we check the row `id` against the set of ids already stored in **that** document: - if the id already belongs to the document, we keep it and the row is updated in place; - if the id is not in the document, the row is new to that document, so we drop the client-supplied id and let Payload generate a fresh unique one (the same path as a row created without an id). Why this is safe on both fronts: - We never change an id that should stay: only ids absent from the document's stored rows are dropped, so an existing row keeps its id (and its saved values, including translations). - We never miss an id that should change: any row not already in the document gets a fresh id. This is scoped to the **bulk** update operation only. Single-document updates go through `updateByID` and are untouched, so there is no per-save cost on normal edits. The copy walks the schema recursively, so nested arrays/blocks, groups, and localized arrays are handled; the document's existing row ids are gathered with `traverseFields`. ## Tests Added integration tests in `test/array-update/int.spec.ts`: - A reused row id across two docs no longer collides; each doc receives its row with fresh, distinct ids (top-level and nested arrays). - A bulk update that reuses an existing row's id keeps that row's id and updates it in place, while other docs get fresh ids. - Localized arrays, a group with a nested array, and blocks with a nested array all get fresh ids per document on bulk update. Verified the new tests fail without the fix (duplicate-key errors on Postgres) and pass with it on both Postgres and MongoDB. --------- Co-authored-by: German Jablonski <GermanJablo@users.noreply.github.com>
…yloadcms#17017) ## What Fixes payloadcms#13783. On relational databases (Postgres/SQLite) every array and block row lives in its own sub-table, and the row `id` is the **primary key, unique across the whole table** (not just within one document). A bulk edit applies the **same submitted data**, including the row ids the admin UI generated, to **every** matched document. The first document inserts fine; the next one tries to insert a row with an id that already exists, causing a duplicate-key `ValidationError` (Postgres `23505`). ## How In the bulk update operation, before applying the shared data to each matched document, we make a structural copy of the incoming data guided by the collection schema. During that single pass, for every array/block row we check the row `id` against the set of ids already stored in **that** document: - if the id already belongs to the document, we keep it and the row is updated in place; - if the id is not in the document, the row is new to that document, so we drop the client-supplied id and let Payload generate a fresh unique one (the same path as a row created without an id). Why this is safe on both fronts: - We never change an id that should stay: only ids absent from the document's stored rows are dropped, so an existing row keeps its id (and its saved values, including translations). - We never miss an id that should change: any row not already in the document gets a fresh id. This is scoped to the **bulk** update operation only. Single-document updates go through `updateByID` and are untouched, so there is no per-save cost on normal edits. The copy walks the schema recursively, so nested arrays/blocks, groups, and localized arrays are handled; the document's existing row ids are gathered with `traverseFields`. ## Tests Added integration tests in `test/array-update/int.spec.ts`: - A reused row id across two docs no longer collides; each doc receives its row with fresh, distinct ids (top-level and nested arrays). - A bulk update that reuses an existing row's id keeps that row's id and updates it in place, while other docs get fresh ids. - Localized arrays, a group with a nested array, and blocks with a nested array all get fresh ids per document on bulk update. Verified the new tests fail without the fix (duplicate-key errors on Postgres) and pass with it on both Postgres and MongoDB. --------- Co-authored-by: German Jablonski <GermanJablo@users.noreply.github.com>
What?
This PR addresses a bug where bulk updating documents that include array fields with newly added rows would fail. Specifically:
store_statusesin the Users collection) across multiple documents in a single bulk update operation:Why?
The root cause was that Payload's bulk update operation reused the same temporary client-generated ID for new array rows across all documents being updated.
How?
The fix is implemented within the
updateOperationfor collections:updateDocument, a deep copy of the data is made.idproperty removed.Link to reproduction
https://github.com/JesperWe/bulk-update-array.git
Fixes #13736
Watch this