fix(sql-orm-client): scope update()/delete() to a single row#435
Open
fix(sql-orm-client): scope update()/delete() to a single row#435
Conversation
update() and delete() were compiling UPDATE/DELETE ... WHERE <filters> RETURNING * and then returning rows[0]. The single-row return value masked the fact that every matching row was being mutated. Resolve a single matching primary key first (via the existing first()), then narrow the mutation to that PK so update()/delete() match the single-row contract their return type already implies. The *All variants remain the way to mutate every matching row.
📝 WalkthroughWalkthroughCollection.update() and Collection.delete() now resolve an identity-scoped single-row where (primary key or first unique) via a new helper, run mutations inside withMutationScope() constrained to that where, return null if no identity can be resolved, and return the single affected row. deleteAll() delegates to a shared delete-returning executor. A contract helper ChangesSingle-Row Operations with Identity Constraint
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-telemetry
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Around line 1205-1223: `#findFirstMatchingPkWhere`() currently selects only a
single PK column because it uses resolvePrimaryKeyColumn; implement a new helper
resolvePrimaryKeyColumns(contract, tableName) that returns all PK columns
(array) and replace the single-column projection with selectedFields: pkColumns
(e.g., const pkColumns = resolvePrimaryKeyColumns(this.contract,
this.tableName); const firstRow = await this.#clone({ selectedFields: pkColumns,
includes: [] }).first();), so buildPrimaryKeyFilterFromRow receives the full
composite PK values and produces a unique WHERE filter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8b5c9493-fca6-4024-95fd-0673298fecbb
📒 Files selected for processing (3)
packages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/test/integration/delete.test.tspackages/3-extensions/sql-orm-client/test/integration/update.test.ts
aqrln
reviewed
May 7, 2026
The previous fix scoped update()/delete() by `resolvePrimaryKeyColumn`, which only returns the first PK column. Composite primary keys would collapse to a single column and fail to identify a row uniquely, and PK-less tables (with a unique constraint instead) had no path at all. Introduce `resolveRowIdentityColumns`: PK columns if present, else the first unique constraint, else throw. The lookup helper now projects all identity columns and builds a multi-column WHERE criterion via `shorthandToWhereExpr`, which produces the AND-of-equalities filter that uniquely identifies the row. Also wrap the SELECT-then-mutate pair in `withMutationScope` so both queries run against the same transaction-bound scope, matching what `executeNestedUpdateMutation` already does for the nested-callback path. A new `#withRuntime` helper clones the collection bound to the scoped runtime so the existing Collection-level helpers (`first`, `updateAll`, `#executeDeleteReturning`) continue to work inside the callback.
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/3-extensions/sql-orm-client/test/collection-contract.test.ts (2)
240-240: 💤 Low valueNit: nested under an unrelated outer describe.
The new
resolveRowIdentityColumns()suite is nested insidedescribe('collection-contract capability detection', ...), which is about capability detection. Consider lifting it to a sibling top-leveldescribefor clearer grouping (mirroring theresolvePolymorphismInfo()block below).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/sql-orm-client/test/collection-contract.test.ts` at line 240, The test suite for resolveRowIdentityColumns() is nested inside the describe('collection-contract capability detection', ...) block; lift it out to a sibling top-level describe to match the structure used for resolvePolymorphismInfo(). Move the entire describe('resolveRowIdentityColumns()', ...) block so it is not inside the collection-contract capability detection describe, placing it at the same indentation/level as the resolvePolymorphismInfo() describe to keep grouping consistent and tests isolated.
240-295: 💤 Low valueConsider adding a test for the empty-PK-columns fallback path.
resolveRowIdentityColumnsexplicitly checkstable.primaryKey && table.primaryKey.columns.length > 0before returning PK columns, so a table withprimaryKey: { columns: [] }and a non-empty unique should fall through to the unique. That branch is currently uncovered by these unit tests.♻️ Suggested addition
+ it('falls back to uniques when primary key has empty columns', () => { + expect( + resolveRowIdentityColumns( + buildContract({ + primaryKey: { columns: [] }, + uniques: [{ columns: ['email'] }], + }), + 't', + ), + ).toEqual(['email']); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/sql-orm-client/test/collection-contract.test.ts` around lines 240 - 295, Add a unit test in collection-contract.test.ts that covers the code path where a table has primaryKey: { columns: [] } and a non-empty uniques array; specifically, use the existing buildContract helper and call resolveRowIdentityColumns with table 't' having primaryKey with empty columns and uniques like [{ columns: ['email'] }], and assert the result equals ['email'] to verify resolveRowIdentityColumns falls back to uniques when primaryKey.columns is empty.packages/3-extensions/sql-orm-client/src/collection.ts (2)
1213-1219: 💤 Low valueThrowing on missing PK/unique is a behavior change worth documenting at the API.
Pre-PR,
update()/delete()on a table without a primary key or unique constraint would (incorrectly) mutate every matching row. After this change they throw. That is the right call, but it's a runtime-visible behavior change for a niche case (PK-less tables). A brief note in theupdate()/delete()jsdoc about this precondition would help users diagnose the error fast.The error message itself is good, just consider mentioning that
updateAll()/deleteAll()remain available as an explicit "I really mean every match" escape hatch.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/sql-orm-client/src/collection.ts` around lines 1213 - 1219, Add a jsdoc note to the public update() and delete() methods explaining that they now require the table to have a primary key or unique constraint and will throw (as implemented by `#findFirstMatchingRowIdentityWhere`()) when none exist; mention that updateAll() and deleteAll() remain available as explicit escape hatches to mutate every matching row and include a brief sentence pointing to the thrown error message for diagnosis.
1213-1246: 💤 Low valueIdentity-lookup clone inherits
cursor/distinct/distinctOn/offset.
this.#clone({ selectedFields: [...identityColumns], includes: [] })only overridesselectedFieldsandincludes;cursor,distinct,distinctOn, andoffsetfrom the outer collection still flow into the SELECT used to pick the identity row. PreservingorderByis intentional (determinism), and preservingfilters/offsetis generally what the user wants, butdistinct/distinctOncombined with a forcedselectedFields = identityColumnscan produce a SELECT whose "first row" semantics differ from what the user constructed.Consider being explicit about which state fields are preserved vs. reset for the identity probe (e.g., reset
distinct/distinctOnsince the PK/unique projection makes them moot, and keepfilters/orderBy/offset/cursor). Low-priority since the typical call shape on awhere().update()won't hit this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/sql-orm-client/src/collection.ts` around lines 1213 - 1246, The identity probe in `#findFirstMatchingRowIdentityWhere` currently clones the collection but only overrides selectedFields and includes, letting distinct/distinctOn (and others) leak into the identity SELECT; update the clone call to explicitly reset distinct and distinctOn (e.g., pass distinct: undefined and distinctOn: undefined) so the PK/unique projection isn't affected, keep preserving filters and orderBy as before (and keep offset/cursor if you want their semantics), and ensure the call remains this.#clone({ selectedFields: [...identityColumns], includes: [], distinct: undefined, distinctOn: undefined }) so identity resolution uses a correct projection.packages/3-extensions/sql-orm-client/src/collection-contract.ts (1)
325-342: 💤 Low valueConsider throwing here rather than returning
[].Returning
[]for "no identity available" or "unknown table" pushes the contract-violation detection to every caller. The single caller (Collection.#findFirstMatchingRowIdentityWhere) immediately re-throws onlength === 0, but with a less precise message (it can't tell "no PK/unique" apart from "table doesn't exist"). Throwing distinct errors here would localize the diagnosis and make it harder for future callers to silently misuse the helper. Optional given the current single-caller scope.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-extensions/sql-orm-client/src/collection-contract.ts` around lines 325 - 342, resolveRowIdentityColumns currently returns [] for both "table not found" and "no PK/unique", which defers precise error handling to callers; modify resolveRowIdentityColumns to throw distinct errors instead: throw a TableNotFound (or a clear Error) when contract.storage.tables[tableName] is missing, and throw a NoIdentityColumnsError (or descriptive Error) when the table exists but has no primary key or unique columns. Update any callers (e.g., Collection.#findFirstMatchingRowIdentityWhere) to remove the length===0 check and allow these specific errors to surface or catch them to produce context-specific messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/3-extensions/sql-orm-client/src/collection-contract.ts`:
- Around line 325-342: resolveRowIdentityColumns currently returns [] for both
"table not found" and "no PK/unique", which defers precise error handling to
callers; modify resolveRowIdentityColumns to throw distinct errors instead:
throw a TableNotFound (or a clear Error) when contract.storage.tables[tableName]
is missing, and throw a NoIdentityColumnsError (or descriptive Error) when the
table exists but has no primary key or unique columns. Update any callers (e.g.,
Collection.#findFirstMatchingRowIdentityWhere) to remove the length===0 check
and allow these specific errors to surface or catch them to produce
context-specific messages.
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Around line 1213-1219: Add a jsdoc note to the public update() and delete()
methods explaining that they now require the table to have a primary key or
unique constraint and will throw (as implemented by
`#findFirstMatchingRowIdentityWhere`()) when none exist; mention that updateAll()
and deleteAll() remain available as explicit escape hatches to mutate every
matching row and include a brief sentence pointing to the thrown error message
for diagnosis.
- Around line 1213-1246: The identity probe in
`#findFirstMatchingRowIdentityWhere` currently clones the collection but only
overrides selectedFields and includes, letting distinct/distinctOn (and others)
leak into the identity SELECT; update the clone call to explicitly reset
distinct and distinctOn (e.g., pass distinct: undefined and distinctOn:
undefined) so the PK/unique projection isn't affected, keep preserving filters
and orderBy as before (and keep offset/cursor if you want their semantics), and
ensure the call remains this.#clone({ selectedFields: [...identityColumns],
includes: [], distinct: undefined, distinctOn: undefined }) so identity
resolution uses a correct projection.
In `@packages/3-extensions/sql-orm-client/test/collection-contract.test.ts`:
- Line 240: The test suite for resolveRowIdentityColumns() is nested inside the
describe('collection-contract capability detection', ...) block; lift it out to
a sibling top-level describe to match the structure used for
resolvePolymorphismInfo(). Move the entire
describe('resolveRowIdentityColumns()', ...) block so it is not inside the
collection-contract capability detection describe, placing it at the same
indentation/level as the resolvePolymorphismInfo() describe to keep grouping
consistent and tests isolated.
- Around line 240-295: Add a unit test in collection-contract.test.ts that
covers the code path where a table has primaryKey: { columns: [] } and a
non-empty uniques array; specifically, use the existing buildContract helper and
call resolveRowIdentityColumns with table 't' having primaryKey with empty
columns and uniques like [{ columns: ['email'] }], and assert the result equals
['email'] to verify resolveRowIdentityColumns falls back to uniques when
primaryKey.columns is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a6931cd9-80c8-485d-9670-becc7f6538c4
📒 Files selected for processing (3)
packages/3-extensions/sql-orm-client/src/collection-contract.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/test/collection-contract.test.ts
aqrln
approved these changes
May 8, 2026
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
2 participants
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.
Intent
Collection.update()andCollection.delete()returned a single row, but their SQL was unrestricted:UPDATE/DELETE … WHERE <filters> RETURNING *mutated every row matching the user'swhere()clause and then the JS layer kept onlyrows[0]. The single-row return value silently masked multi-row writes. This branch makes the runtime behavior match the contract the return type already implies:update()/delete()affect exactly one row.updateAll()/deleteAll()remain the way to mutate every match.Change map
packages/3-extensions/sql-orm-client/src/collection.ts—update()anddelete()now resolve a single matching primary key first and re-issue the mutation scoped to that PK. A small private helper (#findFirstMatchingPkWhere) does the lookup using the existingfirst()infrastructure.deleteAll()'s body is extracted into#executeDeleteReturningsodelete()can call it on a cloned (PK-narrowed) instance without tripping thethis:constraint.packages/3-extensions/sql-orm-client/test/integration/update.test.ts,delete.test.ts— new integration tests assert the single-row guarantee: seed three rows where two matchwhere(), confirm exactly one is mutated and the other untouched.The story
The bug is straightforward once you see it:
update()at collection.ts delegated toupdateAll()and returnedrows[0] ?? null.updateAll()compiledUPDATE … WHERE filters RETURNING *with no LIMIT — Postgres updated every row matchingfiltersand returned them all. The caller saw one row come back and assumed one row was changed.delete()had the same shape viadeleteAll().toArray().The fix mirrors what the existing nested-mutation path (
updateFirstGraphinmutation-executor.ts) already does: SELECT the first matching PK, then issue the mutation scoped to that PK. Concretely,#findFirstMatchingPkWherereusesfirst()on a clone withselectedFields: [pkColumn], includes: []to keep the lookup cheap, then callsbuildPrimaryKeyFilterFromRow+shorthandToWhereExprto turn the PK value into aWHEREexpression.update()anddelete()clone themselves with that PK-only filter and delegate to the existing multi-row path —updateAll()/ a private#executeDeleteReturning(). The*Allmethods are untouched and continue to mutate every match.The cleanest fix would be to add a
LIMITclause toUpdateAst/DeleteAstand letupdate()express itself astake(1).updateAll(). That's not a small change:UpdateAstandDeleteAsthave nolimitslot today, Postgres doesn't acceptUPDATE … LIMITat all (only SQLite does, behind a build flag), and a portable LIMIT requires rewritingWHEREtopk IN (SELECT pk … LIMIT n). That deserves its own branch — see follow-ups.Behavior changes & evidence
update()anddelete()now mutate at most one row regardless of how manywhere()matches.updateAll(),deleteAll(),updateCount(),deleteCount()are unchanged in behavior — they remain the multi-row variants.update()/delete()already usefindOneAndUpdate/findOneAndDelete, which are atomic and single-row at the storage layer).New integration tests covering the contract:
update() affects only one row even when where() matches severaldelete() affects only one row even when where() matches severalFull
sql-orm-clientsuite: 480/480 passing.pnpm typecheckandpnpm lint:depsclean.Compatibility / migration / risk
users.where({ name: 'X' }).update({ … })expecting all matching rows to update will now only get one. The fix is to switch those call sites toupdateAll(…)/deleteAll(…). The return-type signature already implied single-row, so any such usage was incorrect against the documented surface.updateFirstGraph) already does, so it's consistent with the rest of the codebase. Mongo'supdate()/delete()remain atomic viafindOneAndUpdate/findOneAndDelete.#findFirstMatchingPkWherepreserves the collection'sorderBy, so callers who care can chainorderBy()to make the choice deterministic.Follow-ups / open questions
withLimittoUpdateAst/DeleteAstand a portableUPDATE/DELETE … WHERE pk IN (SELECT pk … LIMIT n)rewrite for Postgres. That would letupdate()/delete()collapse intotake(1).updateAll()and would also enableupdateAll(n)/deleteAll(n)-style bounded mutations.update()/delete()once the AST supports it, to match Mongo's atomicity.Non-goals / intentionally out of scope
updateAll/deleteAll/updateCount/deleteCount.Summary by CodeRabbit
New Features
Tests
Refactor