Skip to content

feat(sql-orm-client): complete id-less ORM support#440

Open
jkomyno wants to merge 51 commits into
mainfrom
feat/orm-requires-primary-key-gate
Open

feat(sql-orm-client): complete id-less ORM support#440
jkomyno wants to merge 51 commits into
mainfrom
feat/orm-requires-primary-key-gate

Conversation

@jkomyno
Copy link
Copy Markdown
Contributor

@jkomyno jkomyno commented May 8, 2026

Linear: TML-2432 — sql-orm-client: complete id-less ORM mutation paths

Stacked on #424. Targets feat/idless-models so the doc section it references lands in the same chain; rebase on main once #424 merges.

Intent

Make id-less SQL models (tables emitted from PSL without @id/@@id) work end-to-end through the ORM lane. This PR landed in three increments along the plan in plans/feat-complete-idless-orm-support.md. After this PR, the doc section docs/architecture docs/subsystems/3. Query Lanes.md § "Id-less tables" describes a stable end state with no "future work" bullets.

What changed

M0 — Typed-error gate (eabd81a3b)

Convert the silent 'id' default in resolvePrimaryKeyColumn into an operation-tagged error. Stops the silent-failure mode where ORM helpers built SQL referencing a literal id column that may not exist on id-less tables.

M1 — Counts via UPDATE/DELETE…RETURNING (b6b7eba8e)

Replace the SELECT-PK-first-then-UPDATE pattern in updateCount() / deleteCount() with a single UPDATE … RETURNING / DELETE … RETURNING that counts the streamed rows. Side effects:

  • No PK fallback at these sites — id-less compatible.
  • One round-trip instead of two.
  • Closes the prior SELECT-then-UPDATE race window.
  • updateCount/deleteCount now formally require the returning capability.

M2 — Row-identity criterion for mutation reload (f977896b7)

Rename buildPrimaryKeyFilterFromRow to buildRowIdentityCriterion with two paths:

  • PK fast path: when the table declares a primary key, build a PK-keyed criterion. Composite PKs include every PK column (the previous helper used only columns[0], a latent bug).
  • Id-less path: build a criterion from the row's mapped non-null column values. The row was just produced by RETURNING in the open mutation transaction, so the criterion matches by construction.

Drop the #reloadMutationRowByPrimaryKey wrapper; both nested-mutation reload sites now call #reloadMutationRowByCriterion directly.

M3 — Doc rewrite + id-less integration tests (90043c101)

Rewrite docs/architecture docs/subsystems/3. Query Lanes.md § "Id-less tables" as the live end state. Drop the "Future work" bullets. Narrow the PK-required call list to MTI polymorphism (which is PK-required by design — the variant join cannot be expressed in one statement without a stable identity column).

Add test/integration/idless.test.ts exercising updateCount() and deleteCount() on a Tag collection bound to a contract with primaryKey stripped from the storage table.

Behavior change summary

Surface Before After
resolvePrimaryKeyColumn on id-less tables returns 'id' (silent) throws operation-tagged error
updateCount / deleteCount SELECT id WHERE … then UPDATE / DELETE UPDATE / DELETE … RETURNING + count rows
Mutation reload after nested create/update PK-only criterion (single column) composite-PK aware on PK tables; row-tuple criterion on id-less tables
MTI polymorphism on id-less base table silently used 'id' (broken) throws operation-tagged error (intentional design constraint)

No previously valid PK-tabled contract regresses: every M1/M2 change preserves PK-table behavior on the fast path. The composite-PK widening at M2 fixes a latent bug where the previous helper ignored every PK column except the first.

Validation

  • pnpm --filter @prisma-next/sql-orm-client test — 498 pass, 3 skipped (no regressions; +5 new tests across M1/M2/M3).
  • pnpm --filter @prisma-next/sql-orm-client typecheck — clean.
  • pnpm test:packages — 110 / 110 tasks green.
  • pnpm lint:deps — 0 dependency violations.

Test plan

  • Unit: updateCount() issues a single UPDATE…RETURNING, applies generated updatedAt defaults to the SET clause.
  • Unit: buildRowIdentityCriterion PK fast path (single + composite PK), id-less path (non-null criterion + empty-row error).
  • Integration: id-less updateCount() + deleteCount() against PGlite with a real PG table whose primaryKey is stripped from the contract.
  • Existing PK integration tests for updateCount/deleteCount/update/delete continue to pass — regression guard.

Out of scope (deferred, documented in plan)

  • Per-model capability primitive in the contract schema (models.<X>.capabilities.pkRequired). Not needed once all PK-fallback sites are resolved or design-required.
  • Explicit-predicate variants of count helpers — COUNT(*)-equivalent via RETURNING already removes the dependency.
  • findUnique analogues keyed on non-PK uniques — where(uniqueShape).first() already covers it.
  • MySQL adapter changes — no MySQL adapter in tree.
  • Compile-time TS gating of MTI methods on id-less base tables — runtime gate is sufficient.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for ORM operations on tables without primary keys, using unique constraints or row-identity criteria for mutations.
  • Documentation

    • Documented id-less table constraints: nested updates/deletes require a primary key or unique constraint; MTI polymorphism unsupported on id-less base tables.
    • Added SQLite caveat: AFTER triggers may affect id-less mutation reload accuracy.
  • Improvements

    • Refactored updateCount() and deleteCount() to use single-statement RETURNING queries instead of separate SELECTs.
  • Tests

    • Added integration tests for id-less table mutations and count operations.

jkomyno added 30 commits May 4, 2026 22:39
Adds phase-specific execution defaults to SQL TS authoring so generated IDs stay on-create-only while updatedAt can use both create and update phases.

Wires Prisma-compatible @updatedat lowering for Postgres and SQLite PSL, plus field.updatedAt() helpers backed by target-owned timestampNow generators. Empty update payloads skip onUpdate defaults so no-op updates do not advance updatedAt.
Reject @updatedat before relation fields are discarded so invalid PSL cannot be accepted silently. Keep the adapter mutation default descriptors contextually typed and narrow optional runtime generator hooks in tests so package typechecks remain green.
Resolves todos/056 (P1) and todos/057 (P1) from the PR #422 review.

The phased mutation-default shape `executionDefaults: { onCreate?, onUpdate? }`
was added in 682714e alongside the legacy single-slot `executionDefault?`. The
legacy slot is `{ onCreate: X }` in disguise, so carrying both forces a runtime
guard, a dynamic property-name label, and duplicated default+nullable validation
across every layer (FieldNode, ValueObjectFieldNode, AuthoringFieldPresetOutput,
ScalarFieldState, ResolvedField). It also sets a precedent that adding onDelete
later requires another conditional in the reconciler.

Changes:

- Drop `executionDefault` from FieldNode, ValueObjectFieldNode,
  AuthoringFieldPresetOutput, ScalarFieldState, AnyScalarFieldState, and
  ResolvedField.
- Every site that produced the singular form now wraps as
  `executionDefaults: { onCreate: X }`. Affects field.generated, the family-sql
  id helper presets (uuidv4, uuidv7, ulid, nanoid, cuid2, ksuid), and PSL
  `lowerDefaultForField`.
- Delete `resolveExecutionDefaultPhases` and the dynamic `executionDefaultProperty`
  branch from build-contract.ts. The loop body checks `field.executionDefaults`
  directly.
- Loosen the nullable check (todo 057): only fire when `onCreate` is set.
  An `onUpdate`-only mutation default is fine on a nullable field — it can
  legitimately start as NULL until first update. New regression test:
  `allows nullable fields with onUpdate-only executionDefaults`.
- PSL `psl-field-resolution.ts` now combines `loweredDefault.executionDefaults`
  (on-create from generated `@default(...)`) and `updatedAtExecutionDefaults`
  (both phases from `@updatedAt`) via `??`. The two are mutually exclusive —
  `lowerUpdatedAtAttribute` rejects `@updatedAt @default(...)`.

Net change: -33 lines across 13 files. One validator function gone, one
runtime guard gone, one dynamic property-name label branch gone.

Validation:
- pnpm typecheck (123/123 tasks)
- pnpm lint:deps (clean)
- @prisma-next/framework-components test: 131/131
- @prisma-next/sql-contract-ts test: 218/218
- @prisma-next/sql-contract-psl test: 115/115
- @prisma-next/family-sql test: 285/285
- @prisma-next/adapter-postgres test: 615/615
- @prisma-next/adapter-sqlite test: 110/110
…agnostic (todos 061, 066)

- Extract `rejectUpdatedAtOnNonScalar` helper in `psl-field-resolution.ts` and
  call it from the two upstream relation/list-field early-return sites instead
  of duplicating the inline diagnostic. Scalar-field rules continue to live in
  `lowerUpdatedAtAttribute` as the authoritative codec gate, while the new
  helper is the single owner of the relation/list rejection wording.
- Pass `targetId` into `lowerUpdatedAtAttribute` and rewrite the
  `PSL_INVALID_DEFAULT_APPLICABILITY` message raised when the
  `timestampNow` generator is absent. The message now points users at the
  responsible target adapter rather than surfacing the SPI-level concept of a
  mutation default generator. The diagnostic code is unchanged so existing
  matchers continue to work.
…ext creation (todo 063)

Surface RUNTIME.MISSING_MUTATION_DEFAULT_GENERATOR at createExecutionContext
time when the contract references generator ids the assembled stack does
not provide. Walks contract.execution.mutations.defaults across both
onCreate and onUpdate phases, deduplicates missing ids, and throws a
single structured error. Aligns with ADR 031/065's startup-time
capability negotiation: contracts naming a generator id whose runtime
implementation isn't shipped now fail fast at context construction
rather than at first applyMutationDefaults call.

The existing lazy fallback in computeExecutionDefaultValue
(RUNTIME.MUTATION_DEFAULT_GENERATOR_MISSING) is retained as
defense-in-depth — the new contract-time check supplements it, it does
not replace it.
…(todos 058, 059, 060, 064)

- 064: export `TIMESTAMP_NOW_GENERATOR_ID` constant and
  `ExecutionMutationDefaultPhases` type from canonical sources; replace
  duplicated literals/aliases across PSL/TS authoring and adapters.
- 059: add `MutationDefaultGeneratorDescriptor.buildPhases?`; PSL
  `lowerUpdatedAtAttribute` now delegates phase construction to the
  generator descriptor (no inline literal). PSL/TS authoring stay
  byte-equivalent for any future params-bearing generator.
- 058: centralize `timestampNow` wiring in `@prisma-next/family-sql` via
  `timestampNowControlDescriptor(applicableCodecIds)` and
  `timestampNowRuntimeGenerator()`. Postgres + SQLite adapters opt in
  rather than re-declare the body.
- 060: add `RuntimeMutationDefaultGenerator.applyOnEmptyUpdate?` and
  switch `applyMutationDefaults` from a global empty-update short-circuit
  to a per-generator opt-out. Default `false` preserves @updatedat RD2;
  future onUpdate generators can opt in to sentinel-write semantics.

Test counts (post-change):
- @prisma-next/framework-components: 133 tests
- @prisma-next/sql-contract-ts: 218 tests
- @prisma-next/sql-contract-psl: 115 tests (incl. ts-psl-parity unchanged)
- @prisma-next/family-sql: 285 tests
- @prisma-next/sql-runtime: 186 tests + 1 skipped (incl. new
  applyOnEmptyUpdate=true regression)
- @prisma-next/adapter-postgres: 615 tests
- @prisma-next/adapter-sqlite: 110 tests
… rows

The runtime contract already encoded @updatedat as a both-phase execution
mutation default (timestampNow generator on onCreate and onUpdate), but
only the create path threaded it through applyMutationDefaults. ORM
update, updateAll, updateCount, the upsert update branch, and the nested
update branch in updateFirstGraph all skipped the call, so a non-empty
update never advanced updatedAt. Wire applyMutationDefaults into every
update path. Empty payloads still short-circuit before the runtime call,
preserving the no-write-no-advance rule.

Bulk inserts (createAll/createMany) called applyMutationDefaults per
row, which made timestampNow generate a fresh Date per row and drift
within a single ORM operation. Add a stableAcrossRows generator flag
plus an acrossRowsCache parameter to applyMutationDefaults so bulk
operations reuse one generated value across every row that needs the
default. Mark timestampNow as stableAcrossRows: true. Per-row
generators (cuid, uuidv4, etc.) stay independent because the cache
only honors stableAcrossRows generators.

Adds runtime-side tests for the cache contract and orm-client tests
covering create, createAll, updateAll, updateCount, upsert (both
branches), and explicit-value preservation.
…ability

Add Milestone 4 to the created-updated-at-authoring plan covering the
two compatibility gaps surfaced by comparing against Prisma 6's
@updatedat semantics: ORM update paths skipped applyMutationDefaults,
and bulk inserts drifted timestampNow per row. Also record the
naming choice (stableAcrossRows / acrossRowsCache) under Resolved
Decisions so future readers see why it didn't end up as bulkStable.
Reframe the project around a generic PSL field-preset dispatch path
(mirroring the existing TS field-preset registry), exposed as
temporal.createdAt() / temporal.updatedAt() in PSL and
field.temporal.createdAt() / field.temporal.updatedAt() in TS.
The Prisma-flavored @updatedat attribute is removed; codec
applicability stops being a user-facing diagnostic and becomes a
registry-coherence concern. Contract IR, runtime, and ORM-client
work from prior milestones survive untouched (consolidated into
"Inherited Foundation"). All open design questions resolved
inline; reviewer feedback applied where it didn't conflict with
prior decisions.
Adds a generic field-preset dispatch path to PSL, symmetric with the
existing type-constructor dispatch. The path resolves namespaced calls
in field-type position against authoringContributions.field, instantiates
the preset via the same instantiateAuthoringFieldPreset machinery TS
uses, and produces default / executionDefaults / id / unique / nullable
contributions on the resolved field.

Field presets resolve before type constructors (presets carry richer
semantics; the more complete answer wins). The temporal namespace is
added to the curated-namespaces exemption list, alongside db/family/
target — chosen for forward-compatibility with the JS/TS Temporal API.

Preset usage is constrained per spec FR7: optional preset (?), list
preset ([]), preset + @default, preset + @id, and preset + @updatedat
are all hard errors with stable diagnostic codes. PSL → typed-args
coercion happens in instantiatePslFieldPreset via mapPslHelperArgs;
instantiateAuthoringFieldPreset itself stays typed-input-only.

Test fixture proves genericness via a synthetic temporal.exampleField()
preset (not a temporal-specific code path).
…sion check

Adds defense-in-depth and user-facing diagnostics that close out Phase A
of the field-preset pivot:

PSL preset-misuse diagnostics:
- PSL_PRESET_NOT_OPTIONAL — `temporal.X()?` rejected (preset is a complete
  declaration; system owns the value, optional contradicts that).
- PSL_PRESET_NOT_LIST — `temporal.X()[]` rejected (presets aren't list
  elements).
- PSL_PRESET_AND_DEFAULT_CONFLICT — `temporal.X() @default(...)` rejected
  (preset already specifies its own defaults).
- PSL_PRESET_AND_ID_CONFLICT — `temporal.X() @id` rejected when the preset
  doesn't itself contribute id semantics.
- PSL_PRESET_AND_UPDATED_AT_CONFLICT — `temporal.X() @updatedAt` rejected
  (preset already specifies execution defaults).
- PSL_UNKNOWN_FIELD_PRESET — typo in a curated namespace surfaces a
  spelling-targeted hint instead of falling through to the generic
  PSL_UNSUPPORTED_FIELD_TYPE.

Compose-time registry collision check:
- createComposedAuthoringHelpers now rejects any path registered as both
  a field preset and a type constructor with a clear "Ambiguous authoring
  registry path" error. Belt-and-suspenders to RD9's runtime field-preset-
  first ordering — the error surfaces at composition, not at PSL
  resolution.

Tests:
- Five preset-misuse cases in interpreter.defaults.test.ts.
- Cross-registry collision case in authoring-helper-runtime.test.ts.

Validation gate (all green):
- @prisma-next/sql-contract-psl: 121 tests
- @prisma-next/sql-contract-ts: 219 tests
- @prisma-next/framework-components: 133 tests
- pnpm lint:deps: 716 modules, 0 violations
The original RD11 said to rename PSL_INVALID_TYPE_CONSTRUCTOR_ARITY →
PSL_INVALID_NAMESPACED_CALL_ARITY. Phase A revealed the assumption was
wrong: that code name doesn't exist. Type-constructor arity/arg errors
today emit PSL_INVALID_ATTRIBUTE_ARGUMENT (a code shared with genuine
attribute-arg errors like @id(badarg)). The honest rename to a
PSL_INVALID_NAMESPACED_CALL_ARGUMENT code would need to disambiguate
those cases too, which is a wider refactor than this project warrants.

Decision: field-preset arity/arg errors emit PSL_INVALID_ATTRIBUTE_ARGUMENT
for now, matching the existing type-constructor pattern. Honest rename
deferred to a follow-up.

This commit:
- Updates FR7 to enumerate diagnostic codes that match what's actually
  implemented, with notes on which cases are tested in Phase A vs deferred.
- Marks AC5 sub-items by implementation status (a, b, d, e, h, j shipped
  with tests; c, f, g, i deferred or covered indirectly).
- Adds AC5j for the transient PSL_PRESET_AND_UPDATED_AT_CONFLICT code that
  disappears in Phase C alongside the @updatedat attribute path.
- Rewrites RD11 to reflect the discovery and the deferral decision.
- Updates plan.md task list and Resolved Decisions consistently.
Moves the createdAt/updatedAt field-preset registrations from flat
target.authoring.field.{createdAt,updatedAt} to nested
target.authoring.field.temporal.{createdAt,updatedAt} for both Postgres
and SQLite targets. The TS surface follows: field.createdAt() /
field.updatedAt() → field.temporal.createdAt() / field.temporal.updatedAt().

PSL was already wired in Phase A — temporal.createdAt() /
temporal.updatedAt() now resolve through the field-preset dispatch path
once the registry move lands here.

Updates to keep parity with the new namespace:
- Test fixtures: contract-builder.dsl.helpers.test.ts,
  ts-psl-parity.test.ts (sqliteTimestampTargetPack,
  postgresTimestampTargetPack, sqlFamilyPack).
- Examples: react-router-demo and prisma-next-demo TS contracts.
- CLI init templates (TS template) + snapshot.
- Integration type tests.
- Doc comments in mutation-default-types.ts and timestamp-now-generator.ts.

PSL examples (.prisma) and the @updatedat attribute path are unchanged
in Phase B — they ship intact through Phase C, which deletes the
attribute and updates PSL examples to use temporal.updatedAt().

Validation gate (all green):
- pnpm test:packages: 110/110 turbo tasks
- pnpm lint:deps: 716 modules, 0 violations
Deletes the Prisma-flavored @updatedat attribute path that landed on
this branch in commit 682714e. The codec-applicability validation
that motivated the pivot is gone; users now write temporal.updatedAt()
in PSL and field.temporal.updatedAt() in TS, both consuming the same
field-preset registry shipped in Phases A+B.

What's deleted from psl-field-resolution.ts (~110 lines):
- 'updatedAt' entry in BUILTIN_FIELD_ATTRIBUTE_NAMES
- reportInvalidUpdatedAt, rejectUpdatedAtOnNonScalar (helpers)
- lowerUpdatedAtAttribute (the codec-applicability path)
- The relation-field @updatedat rejection branches in
  collectResolvedFields
- The @updatedat lowering merge with @default(generator())
- The PSL_PRESET_AND_UPDATED_AT_CONFLICT branch (no longer reachable
  once the attribute is gone)

Migration hint:
- New helper getRemovedAttributeHint() returns a per-attribute
  suggestion. Currently has one entry: 'updatedAt' → 'Use
  `temporal.updatedAt()` as a field-preset call instead.'
- The hint is appended to the standard PSL_UNSUPPORTED_FIELD_ATTRIBUTE
  message when the attribute name matches and the field doesn't
  already declare a temporal.* preset (suppression check).
- Implementation is the requested hardcoded if-branch — no map.
  Adding more migrations later means adding another branch.

Tests:
- Replaced four @updatedAt-attribute tests in interpreter.defaults
  with temporal.updatedAt()-preset tests (Postgres + SQLite, plus
  inline preset registrations matching the production target shape).
- Added two new tests: migration-hint emission, and hint suppression
  for already-migrated fields (the `temporal.updatedAt() @updatedAt`
  half-migrated case).
- Removed the obsolete PSL_PRESET_AND_UPDATED_AT_CONFLICT test.
- Updated the PSL parity test (sqlite + postgres) to use
  temporal.updatedAt() PSL syntax.

Diagnostic-code inventory (per RD11): grep confirmed
PSL_INVALID_DEFAULT_APPLICABILITY remains in use elsewhere (storage
default lowering); PSL_PRESET_AND_UPDATED_AT_CONFLICT is now
orphaned and removed.

Doc-comment updates in:
- packages/1-framework/1-core/framework-components/src/shared/mutation-default-types.ts
- packages/3-extensions/sql-orm-client/src/collection.ts

Spec/plan updates:
- AC4 marked complete; references PSL_UNSUPPORTED_FIELD_ATTRIBUTE
  (the actual code) instead of the spec's earlier PSL_UNKNOWN_ATTRIBUTE
  (which doesn't exist).
- AC5j superseded note: half-migrated fields now use the suppression
  path instead of a dedicated diagnostic.

Validation gate: 120 PSL tests pass, full lint:deps clean (716 modules,
0 violations). adapter-postgres (615) and target-mongo (447) pass
standalone; turbo-aggregate flake unrelated to this commit.

BREAKING CHANGE: PSL @updatedat is no longer a recognized attribute.
Migrate to `temporal.updatedAt()` field-preset syntax. The unknown-
attribute diagnostic includes the migration hint inline.
Update existing references to the renamed/removed surfaces:

- docs/products/psl/README.md — timestamp section now lists
  temporal.createdAt() / temporal.updatedAt() as the preset surface
  alongside @default(now()), and notes that @updatedat is no longer
  recognized (with the migration-hint diagnostic).
- packages/2-sql/2-authoring/contract-psl/README.md — Responsibilities
  bullet and "Supported timestamp authoring surface" section.
- packages/2-sql/2-authoring/contract-ts/{README.md,API.md} — bulk
  rename field.createdAt() / field.updatedAt() →
  field.temporal.createdAt() / field.temporal.updatedAt() across
  helper-vocabulary lists, code examples, and timestamp-helper notes.

Scope-bounded: only updates pre-existing references — no
restructuring, no new sections, no docs that weren't already
in the inventory.

Validation: 110/110 turbo tasks (adapter-postgres flake confirmed
unrelated by standalone re-run, 615 tests pass), lint:deps 716
modules / 0 violations.
…gets

Hoist the `temporal.{createdAt,updatedAt}` field-preset descriptors into a
single `temporalAuthoringPresets({ codecId, nativeType })` helper exported
from `@prisma-next/family-sql/control`. Postgres and SQLite each pass only
their target-specific codec/native-type pair; the rest of the descriptor
(default expression, generator id, both phases) is owned by the helper, so
PSL `temporal.updatedAt()` and TS `field.temporal.updatedAt()` lower to
byte-identical contracts across targets by construction.

Also drops the unused flat `dateTime` preset from SQLite authoring — it had
no callers in `packages/`, `examples/`, or tests, and was a parity wart
relative to Postgres' pre-existing flat `dateTime`. The Postgres flat
`dateTime` is left untouched since it predates this branch.
The `applyOnEmptyUpdate?: boolean` flag on `RuntimeMutationDefaultGenerator`
was added as a forward-compat hook for hypothetical future generators that
want sentinel-write semantics on empty update payloads. No production
generator opts in; the only caller was a self-test of the flag itself.

Per CLAUDE.md ("Don't design for hypothetical future requirements"), drop
the flag, the runtime branch in `applyMutationDefaults`, and the self-test.
Empty-update skip is now unconditional. Re-add when a real generator needs
sentinel semantics, with a test exercising real production code.

RD2 ("empty update payloads skip onUpdate defaults") is still enforced —
just simpler now.
`build-contract.ts` previously allowed nullable fields when only
`executionDefaults.onUpdate` was present (no `onCreate`), reasoning that
the column could start NULL until first update. That branch was a
forward-compat hook for hypothetical onUpdate-only presets — dead code with
respect to `temporal.updatedAt()`, which always sets both phases and is
also rejected at the PSL layer via `PSL_PRESET_NOT_OPTIONAL`.

Per CLAUDE.md ("Don't design for hypothetical future requirements"),
collapse the check to "nullable + any executionDefaults = error" and
update the error message accordingly. Drop the dedicated test that
asserted the permissive branch. Re-add the allowance when a real preset
(e.g. `temporal.lastModifiedAt()` with onUpdate-only and a nullable
column) needs it, alongside that preset's own tests.
- Replace Phase B parity-note for SQLite's flat `dateTime` preset with a
  consolidation note pointing at the new `temporalAuthoringPresets()`
  helper (the flat preset was dropped; the consolidation makes per-target
  drift structurally impossible).
- Update the "temporal.updatedAt() semantics" RD to reflect that the
  `nullable + onUpdate-only` allowance is no longer speculatively built
  in — a future onUpdate-only preset will introduce it alongside its
  own tests.
- Add a "YAGNI cuts at PR-close" RD recording the two forward-compat
  hooks removed before merge: `applyOnEmptyUpdate` opt-in and
  `nullable + onUpdate-only` allowance.
Resolves a single modify/delete conflict on `docs/products/psl/README.md`.
The file was deleted on main in commit b5c3381 ("elevate PSL authoring
to a top-level May workstream") for being "significantly stale,
unreferenced, and actively misleading." This branch had previously made
small edits to the same file as part of Phase D (spec AC9 / FR10 named it
as the canonical PSL doc).

Resolution: accept main's deletion. The temporal-preset writeup survives
in the package READMEs (`packages/2-sql/2-authoring/contract-psl/README.md`,
`packages/2-sql/2-authoring/contract-ts/{README.md,API.md}`), none of
which link back to the deleted file. A fresh canonical PSL doc is owned
by the May WS5 workstream and is out of scope for this PR.

`plan.md` records the AC9/FR10 adjustment.

Validation: full package test suite green post-merge (sql-contract-psl,
sql-contract-ts, sql-runtime, sql-orm-client) plus `pnpm lint:deps`.
…sting

Per CodeRabbit review on PR #422: `resolveAuthoringColumnDefaultTemplate`
in `framework-authoring.ts` was casting `value as ColumnDefaultLiteralInputValue`
without runtime validation. Since `resolveAuthoringTemplateValue` returns
`unknown` (it dereferences caller-provided `AuthoringArgRef` indices into
arbitrary user args), the cast would silently emit class instances or
non-JSON-serializable types into the contract.

Add an `isColumnDefaultLiteralInputValue` predicate to `contract/types.ts`
that recursively validates JSON primitives, plain arrays/objects of JSON
values, and `Date` instances. Use it in `framework-authoring.ts` to throw
a clear error when a resolved literal default doesn't match the input
shape — mirrors the pattern already used by
`resolveExecutionMutationDefaultPhase` with `isExecutionMutationDefaultValue`.

Drops the cast entirely; the predicate narrows `value` to the exact type.
…dule

Per CodeRabbit review on PR #422: `src/core/timestamp-now-generator.ts`
mixed control-plane (`timestampNowControlDescriptor`, `temporalAuthoringPresets`)
and runtime-plane (`timestampNowRuntimeGenerator`) implementations in a
single file. The runtime function pulled in `RuntimeMutationDefaultGenerator`
from `@prisma-next/sql-runtime`, dragging a runtime-plane import into a
file otherwise consumed only by control-plane callers.

Extract `timestampNowRuntimeGenerator` into a dedicated
`src/core/timestamp-now-runtime-generator.ts`. The control-plane file no
longer imports from `@prisma-next/sql-runtime`. `src/exports/runtime.ts`
re-exports from the new location.

No behavior change; `temporal.updatedAt()` and the per-target adapter
wiring continue to resolve the same descriptor + generator pair.
Per CodeRabbit review on PR #422: the `stableAcrossRows` test compared
`.getTime()` across the three Dates observed in a 3-row bulk insert. That
assertion would still pass if the generator produced three distinct `Date`
instances within the same millisecond, defeating the point of the test.

Tighten to identity (`toBe`), so the test fails unless the cached `Date`
instance is reused across rows — which is exactly what `stableAcrossRows`
+ `acrossRowsCache` claim to do.
…69, 070)

Reject the four silent-acceptance gaps surfaced by the PR #424 review:
duplicate @@id blocks, duplicate fields inside an @@id list, and optional
fields used as inline @id or in an @@id list. Adds the missing diagnostic
tests for each new branch and for the previously-untested inline-vs-block
conflict.
…K invariant (todos 071, 072, 073, 074, 075)

Replace the three-variable (inlinePrimaryKey/modelPrimaryKey/span) plus
post-loop merge with a single primaryKey local; co-locate the inline-vs-block
and duplicate-@@id conflict checks inside the @@id branch. Reject inline
@id on multiple fields (PSL composite identity must use @@id), drop the
two avoidable as-SqlStorage casts in the new tests, add round-trip
companion tests that exercise the printer's snapshot output through the
interpreter, and document the implicit ORM single-PK invariant in
docs/architecture docs/subsystems/3. Query Lanes.md.
…el (todo 078)

Rename messagePrefix and contextLabel parameters on parseAttributeFieldList
and mapFieldNamesToColumns to entityLabel, matching the dominant naming
convention across psl-attribute-parsing, psl-column-resolution,
psl-authoring-arguments, and framework-components/control. Hoist the
"Model X @@<attr>" label to a single per-iteration local in the model
attribute loop instead of recomputing it 6 times across the @@id /
@@unique / @@index branches. No behavior change; diagnostic message text
unchanged.
Lift the duplicate-field-name check that PR #424 added to the @@id branch
to @@unique and @@index, where the same gap existed: a list like
@@unique([email, email]) previously emitted columns: ['email', 'email']
and only failed at DDL time (CREATE UNIQUE INDEX rejects duplicates),
while @@index([email, email]) silently produced a wasteful index.

Extracted findDuplicateFieldName into psl-attribute-parsing alongside the
other field-list helpers; called from all three model-attribute branches
with the unified entityLabel diagnostic shape.
Preserve literal temporal preset codec and native types so the integration type assertions keep exact target strings.

Stabilize the package tests by matching the nullable emitter round-trip timeout to the other TypeScript round-trip cases, isolating the CLI emitter mock baseline, and disabling CLI file parallelism under non-isolated Vitest.

Invalidate Vite generated contract artifacts after each successful emit so framework SSR modules reload fresh contract JSON during dev re-emits.
jkomyno added 5 commits May 8, 2026 10:26
# Conflicts:
#	packages/1-framework/3-tooling/cli/src/commands/init/templates/code-templates.ts
#	packages/1-framework/3-tooling/cli/test/commands/init/__snapshots__/templates.test.ts.snap
#	packages/2-sql/2-authoring/contract-psl/src/psl-column-resolution.ts
#	packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts
#	packages/2-sql/2-authoring/contract-psl/test/interpreter.defaults.test.ts
#	packages/2-sql/2-authoring/contract-psl/test/ts-psl-parity.test.ts
#	packages/2-sql/2-authoring/contract-ts/src/composed-authoring-helpers.ts
#	packages/2-sql/4-lanes/relational-core/src/query-lane-context.ts
#	packages/2-sql/5-runtime/src/sql-context.ts
#	packages/2-sql/5-runtime/test/sql-context.test.ts
#	packages/2-sql/9-family/src/core/timestamp-now-generator.ts
#	packages/2-sql/9-family/src/core/timestamp-now-runtime-generator.ts
#	packages/3-extensions/sql-orm-client/src/collection.ts
#	packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts
#	packages/3-targets/6-adapters/postgres/src/core/control-mutation-defaults.ts
#	packages/3-targets/6-adapters/postgres/src/exports/runtime.ts
#	packages/3-targets/6-adapters/postgres/test/control-mutation-defaults.test.ts
#	packages/3-targets/6-adapters/sqlite/src/core/control-mutation-defaults.ts
#	packages/3-targets/6-adapters/sqlite/src/core/runtime-adapter.ts
#	packages/3-targets/6-adapters/sqlite/test/control-mutation-defaults.test.ts
Replace the silent `'id' default in `resolvePrimaryKeyColumn` with an
operation-tagged error. Previously, ORM helpers that fall back to the
primary key (mutation reload, updateCount/deleteCount, MTI polymorphism)
would silently use the literal string `'id' for tables that omit
`primaryKey`, then fail opaquely at the database with an unknown-column
error. They now fail at the orm-client boundary with a message that
names the operation and table.

This is the prototype gate for the per-model primary-key invariant
recorded in `docs/architecture docs/subsystems/3. Query Lanes.md`
§ "Id-less tables and primary-key fallback". Future work (capability
flag, explicit-predicate APIs, `findUnique` analogues) is intentionally
deferred and called out in the same doc section.

- collection-contract: `resolvePrimaryKeyColumn(contract, table, operation)` throws on missing PK
- collection.ts: MTI create context, updateCount, deleteCount pass operation labels
- mutation-executor.ts: reload-by-PK passes operation label
- query-plan-select.ts: MTI polymorphism base PK passes operation label
- collection-contract.test: drop the silent-fallback assertion, assert id-less throws
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This PR introduces id-less table support to the SQL ORM client by replacing primary-key-based mutation reload with a row-identity criterion derived from RETURNING rows, refactoring count helpers to use single-statement RETURNING iteration instead of separate SELECT, enforcing PK/unique constraints for nested updates, pruning query-plan exports, and providing comprehensive unit/integration tests and decision documentation.

Changes

Id-less ORM Support

Layer / File(s) Summary
Architecture Documentation
docs/architecture docs/subsystems/3. Query Lanes.md, packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts
Query Lanes doc adds "Id-less tables" specification describing supported ORM operations, reload identity rules, nested-mutation constraints, MTI restrictions, and SQLite AFTER-trigger caveat. PSL interpreter test validates id-less and composite-PK table round-trips.
Contract API Helpers
packages/3-extensions/sql-orm-client/src/collection-contract.ts
getPrimaryKeyColumns helper centralizes primary-key access; resolvePrimaryKeyColumn now requires explicit operation context and throws when table lacks PK (removes fallback to 'id').
Row-Identity Criterion
packages/3-extensions/sql-orm-client/src/mutation-executor.ts
New buildRowIdentityCriterion builds deterministic identity from RETURNING rows: uses PK-mapped fields when present, otherwise non-null mapped fields; throws when no identity fields exist.
Nested-Update Guards
packages/3-extensions/sql-orm-client/src/mutation-executor.ts
updateFirstGraph enforces PK or unique constraint requirement for id-less nested updates; validates unique-constraint columns are non-null when no PK exists; throws descriptive errors.
Collection Reload Refactor
packages/3-extensions/sql-orm-client/src/collection.ts
Nested create() and update() now reload via row-identity criterion instead of PK-based reload. MTI create() threads operation message to resolvePrimaryKeyColumn. Removes private #reloadMutationRowByPrimaryKey helper.
Count Helpers via RETURNING
packages/3-extensions/sql-orm-client/src/collection.ts
updateCount() and deleteCount() now compile UPDATE/DELETE RETURNING, iterate rows, and return count. New #returningColumnForCount() selects minimal column (preferring PK).
Query-Plan Exports Pruned
packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts, packages/3-extensions/sql-orm-client/src/query-plan.ts
compileDeleteCount removed from implementation and re-exports; compileUpdateCount re-exported separately from query-plan-mutations.
MTI and Imports
packages/3-extensions/sql-orm-client/src/query-plan-select.ts, packages/3-extensions/sql-orm-client/src/mutation-executor.ts
buildMtiJoins passes explicit operation message to resolvePrimaryKeyColumn. Mutation-executor imports refactored to use buildRowIdentityCriterion and getPrimaryKeyColumns; adds FK-rewrite cleanup comment.
Test Helpers
packages/3-extensions/sql-orm-client/test/helpers.ts, packages/3-extensions/sql-orm-client/test/integration/helpers.ts
New withoutPrimaryKey helper strips primaryKey from contract tables; createIdlessTagsCollection and createIdlessUsersCollection return id-less Collection instances with RETURNING capability.
Unit Tests
packages/3-extensions/sql-orm-client/test/collection-contract.test.ts, packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts, packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts, packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
Tests for PK-column resolution with operation context, id-less identity criterion building, single-column and composite PKs, error cases, nested-update guards, count helper capabilities requiring RETURNING, and single-execution behavior.
Integration Tests
packages/3-extensions/sql-orm-client/test/integration/idless.test.ts
Three integration tests: updateCount() on id-less tags matching one row; deleteCount() on id-less tags deleting one row; nested update() on id-less users with unique-constraint-based reload and created posts verification.
Decision & Issue Tracking
todos/001-* through todos/014-*
P1 correctness guard for silent multi-row updates on id-less duplicate-tuple tables (implemented in updateFirstGraph); P2 performance/capability issues (wide RETURNING payload, materialization via .toArray(), integration test gaps, dead code, capability coverage); P3 refinements (SQLite caveat docs, asymmetric gates, comment clarity, helper extraction, error message trimming, test consolidation).

🎯 4 (Complex) | ⏱️ ~60 minutes


🐰 A tale of identity without keys,
Row-returns leap where PK'd please,
Guards prevent the duplicate sprawl,
Counts now iterate through all—
Id-less tables, finally whole!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 21.88% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat(sql-orm-client): complete id-less ORM support' directly and accurately summarizes the main change: completing ORM support for id-less SQL tables across multiple files, tests, and documentation.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/orm-requires-primary-key-gate

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 8, 2026

Open in StackBlitz

@prisma-next/mongo-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-runtime@440

@prisma-next/family-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/family-mongo@440

@prisma-next/extension-arktype-json

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-arktype-json@440

@prisma-next/extension-cipherstash

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-cipherstash@440

@prisma-next/middleware-telemetry

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/middleware-telemetry@440

@prisma-next/mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo@440

@prisma-next/extension-paradedb

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-paradedb@440

@prisma-next/extension-pgvector

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/extension-pgvector@440

@prisma-next/postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/postgres@440

@prisma-next/sql-orm-client

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-orm-client@440

@prisma-next/sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sqlite@440

@prisma-next/sql-runtime

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-runtime@440

@prisma-next/family-sql

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/family-sql@440

@prisma-next/target-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-mongo@440

@prisma-next/adapter-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-mongo@440

@prisma-next/driver-mongo

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-mongo@440

@prisma-next/contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract@440

@prisma-next/utils

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/utils@440

@prisma-next/config

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/config@440

@prisma-next/errors

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/errors@440

@prisma-next/framework-components

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/framework-components@440

@prisma-next/operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/operations@440

@prisma-next/ts-render

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/ts-render@440

@prisma-next/contract-authoring

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/contract-authoring@440

@prisma-next/ids

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/ids@440

@prisma-next/psl-parser

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-parser@440

@prisma-next/psl-printer

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/psl-printer@440

@prisma-next/cli

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/cli@440

@prisma-next/emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/emitter@440

@prisma-next/migration-tools

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/migration-tools@440

prisma-next

npm i https://pkg.pr.new/prisma/prisma-next@440

@prisma-next/vite-plugin-contract-emit

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/vite-plugin-contract-emit@440

@prisma-next/mongo-codec

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-codec@440

@prisma-next/mongo-contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract@440

@prisma-next/mongo-value

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-value@440

@prisma-next/mongo-contract-psl

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract-psl@440

@prisma-next/mongo-contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-contract-ts@440

@prisma-next/mongo-emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-emitter@440

@prisma-next/mongo-schema-ir

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-schema-ir@440

@prisma-next/mongo-query-ast

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-query-ast@440

@prisma-next/mongo-orm

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-orm@440

@prisma-next/mongo-query-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-query-builder@440

@prisma-next/mongo-lowering

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-lowering@440

@prisma-next/mongo-wire

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/mongo-wire@440

@prisma-next/sql-contract

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract@440

@prisma-next/sql-errors

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-errors@440

@prisma-next/sql-operations

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-operations@440

@prisma-next/sql-schema-ir

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-schema-ir@440

@prisma-next/sql-contract-psl

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-psl@440

@prisma-next/sql-contract-ts

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-ts@440

@prisma-next/sql-contract-emitter

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-contract-emitter@440

@prisma-next/sql-lane-query-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-lane-query-builder@440

@prisma-next/sql-relational-core

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-relational-core@440

@prisma-next/sql-builder

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/sql-builder@440

@prisma-next/target-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-postgres@440

@prisma-next/target-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/target-sqlite@440

@prisma-next/adapter-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-postgres@440

@prisma-next/adapter-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/adapter-sqlite@440

@prisma-next/driver-postgres

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-postgres@440

@prisma-next/driver-sqlite

npm i https://pkg.pr.new/prisma/prisma-next/@prisma-next/driver-sqlite@440

commit: 76aca22

jkomyno added 3 commits May 8, 2026 11:59
…NING

Replace the SELECT-PK-first-then-UPDATE pattern in updateCount() and
deleteCount() with a single UPDATE…RETURNING / DELETE…RETURNING that
counts the streamed rows. Side effects:

- No PK fallback at these sites (id-less compatible).
- One round-trip instead of two.
- Closes the prior SELECT-then-UPDATE race window.
- updateCount/deleteCount now formally require the returning capability,
  same as update/delete/updateAll/deleteAll.

Drop the now-unused compileUpdateCount/compileDeleteCount/compileSelect
imports from collection.ts. The compilers themselves remain in
query-plan-mutations.ts since mutation-executor.ts:executeUpdateCount
still uses compileUpdateCount for relation FK rewrites.

Test: collection-mutation-defaults.test.ts updateCount block now
asserts a single execution (the UPDATE…RETURNING) rather than the
two-execution SELECT+UPDATE pattern.
Rename buildPrimaryKeyFilterFromRow to buildRowIdentityCriterion and
restructure as two paths:

  * PK fast path: when the table declares a primary key, build a
    PK-keyed criterion. Composite PKs now include every PK column
    (the previous helper used only columns[0]).
  * Id-less path: when the table has no primary key, build a
    criterion from the row's mapped non-null column values.

The reload runs in the open mutation transaction against a row just
produced by RETURNING, so the criterion matches by construction. For
id-less tables without unique constraints, the criterion may match
additional rows if duplicate tuples exist; users who need single-row
identity should declare a unique constraint. SQLite caveat documented
in JSDoc: SQLite RETURNING does not reflect AFTER-trigger column
rewrites, so AFTER-trigger-rewritten columns present in the criterion
may fail to match.

Drop the #reloadMutationRowByPrimaryKey wrapper in collection.ts; both
nested-mutation reload sites now call #reloadMutationRowByCriterion
directly.

Tests:
  * Rename and split the PK helper test into PK fast-path tests
    (single-column + new composite-PK) and id-less path tests
    (non-null criterion + empty-row error).
…tion tests

Plan: plans/feat-complete-idless-orm-support.md captures the M1/M2/M3
breakdown and the deferred non-goals (per-model capability primitive,
findUnique-style APIs, MySQL adapter changes, compile-time TS gating).

Doc: rewrite Query Lanes.md § "Id-less tables and primary-key fallback"
as "Id-less tables". The section now describes the live end state -
predicate ops, count helpers, and nested mutations with select/include
all work on id-less tables; only MTI polymorphism still requires a
primary key on the base table by design. Drop the "Future work"
bullets that are now resolved or deferred elsewhere.

Integration tests: test/integration/idless.test.ts exercises
updateCount() and deleteCount() on a Tag collection bound to a contract
with primaryKey stripped from the storage table. Adds
createIdlessTagsCollection helper alongside the existing
createReturningTagsCollection.

Validation: pnpm test:packages 110/110 green; pnpm lint:deps 0
violations.
@jkomyno jkomyno changed the title feat(sql-orm-client): gate primary-key fallback paths on id-less tables feat(sql-orm-client): complete id-less ORM support May 8, 2026
Base automatically changed from feat/idless-models to main May 8, 2026 12:27
jkomyno added 3 commits May 8, 2026 14:46
14 todo files covering one P1 (multi-row UPDATE footgun on id-less
duplicate-tuple tables, now resolved in this PR), five P2s (test for
the new returning-required gate; wide RETURNING payload in count
helpers; .toArray() materialization; integration coverage gap on the
id-less reload code path; dead compileDeleteCount), and eight P3s.

Findings synthesized from kieran-typescript-reviewer,
architecture-strategist, code-simplicity-reviewer,
pattern-recognition-specialist, performance-oracle, and
data-integrity-guardian. Each todo follows the file-todos template:
status, priority, problem statement, findings with citations,
2-3 proposed solutions with pros/cons/effort/risk, technical
details, acceptance criteria, work log, resources.

Todo #1 lands as complete in the P1 fix commit that follows.
…on id-less tables

updateFirstGraph builds a row-identity criterion from the
RETURNING-row tuple and uses it as the WHERE clause for
compileUpdateReturning. compileUpdateReturning emits a plain
UPDATE...WHERE...RETURNING with no row cap. On id-less tables that
also lack any unique constraint, the criterion can match multiple
duplicate-tuple rows; a single-row API call (db.where(...).update(...))
would silently mutate every match.

Add a runtime guard in updateFirstGraph: when the target table has
neither a primary key nor any unique constraint, refuse with an
operation-tagged error. Preserves the prior behavior on PK and
PK-or-unique tables (the criterion is forced single-row by SQL
semantics). Documented in Query Lanes.md.

Resolves todo #1-p1.
…-less reload

Resolves todos #2-#6 from the PR #440 multi-agent review:

* #2 — Add unit tests for the new returning-capability requirement
  on updateCount() / deleteCount(). Both throw with an explanatory
  message when the contract lacks the returning capability, locking
  the breaking-change behavior introduced in M1.

* #3 — Pass a single trivial column to compileUpdateReturning /
  compileDeleteReturning in updateCount() / deleteCount() instead of
  the previous undefined (which expanded to "every column"). New
  helper pickCountReturningColumn() in collection-contract.ts prefers
  the first PK column when present (always non-null) and falls back
  to any storage column for id-less tables.

* #4 — Stream-count the RETURNING result instead of materializing
  the full row buffer via .toArray(). Both count helpers now use
  for-await + counter, dropping memory cost from O(N×W) to O(1).

* #5 — Add integration test covering the id-less mutation reload
  code path: nested update() on an id-less Users contract (PK
  stripped, UNIQUE (email) retained) routes through
  buildRowIdentityCriterion id-less branch and reloads the row via
  the row-identity criterion. New createIdlessUsersCollection helper.

* #6 — Remove dead code: compileDeleteCount has no callers post-M1.
  Drops the function from query-plan-mutations.ts, the export from
  query-plan.ts, and the corresponding test fixture.

Mark todos 002-006 as complete in the todos/ directory.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
todos/006-pending-p2-dead-code-compiledeletecount.md (1)

1-61: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

compileDeleteCount dead code should be removed in this PR, not deferred.

This PR (M1, b6b7eba8e) is precisely what rendered compileDeleteCount dead. Deferring the cleanup creates a gap between the milestone that caused the orphan and the commit that cleans it up, making the history harder to follow. Both deletion sites are localized:

  • query-plan-mutations.ts:287–299 — remove the function body
  • query-plan.ts:6 — remove the re-export

The project's own CLAUDE.md golden rule is quoted in this very document: "Don't add exports for backwards compatibility unless requested". The same principle applies to keeping an unreferenced export around "just in case."

Would you like me to generate the exact diff for the two deletion sites?

🤖 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 `@todos/006-pending-p2-dead-code-compiledeletecount.md` around lines 1 - 61,
Remove the dead helper compileDeleteCount from query-plan-mutations.ts (delete
the entire function definition named compileDeleteCount) and remove its
re-export from query-plan.ts so the symbol is no longer exported; ensure no
other references remain (grep for compileDeleteCount) and run the test suite to
verify nothing broke.
🧹 Nitpick comments (4)
todos/003-pending-p2-wide-returning-payload.md (1)

1-62: ⚡ Quick win

Consider resolving before production usage — the fix is one line per call site.

Solution A (pass a single-column returning list) is a minimal change and the todo correctly identifies the exact call sites. Given the regression only materialises at scale (wide tables × high affected-row counts), P2/deferred is acceptable for this PR, but it's worth closing this before the id-less feature lands in any performance-sensitive environment.

🤖 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 `@todos/003-pending-p2-wide-returning-payload.md` around lines 1 - 62,
updateCount and deleteCount call compileUpdateReturning/compileDeleteReturning
with returningColumns undefined which buildReturningColumns expands to every
table column, shipping wide payloads; fix by passing an explicit single-column
returning list (e.g. primaryKeyColumns[0] or first storage column) from the
updateCount and deleteCount call sites instead of undefined so
compileUpdateReturning/compileDeleteReturning emit RETURNING [trivialColumn]
rather than all columns (refer to updateCount, deleteCount,
compileUpdateReturning, compileDeleteReturning, and
primaryKeyColumns/table.columns to locate the spots).
packages/3-extensions/sql-orm-client/src/mutation-executor.ts (1)

141-148: 💤 Low value

Use Object.hasOwn() in the id-less mapped-field loop.

Line 144 uses the in operator, which traverses the prototype chain. Mapped-row objects from mapStorageRowToModelFields are plain enough today that this works in practice, but the surrounding codebase enforces Object.hasOwn() for property-existence checks on row-like objects.

🧹 Switch to Object.hasOwn()
   for (const fieldName of Object.keys(fieldToColumn)) {
-    if (!(fieldName in row)) continue;
+    if (!Object.hasOwn(row, fieldName)) continue;
     const value = row[fieldName];
     if (value === undefined || value === null) continue;
     criterion[fieldName] = value;
   }

As per coding guidelines: "Always use Object.hasOwn() instead of hasOwnProperty() to check for own properties."

🤖 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/mutation-executor.ts` around lines
141 - 148, The loop building criterion uses the `in` operator which checks the
prototype chain; change the existence check to use Object.hasOwn(row, fieldName)
instead (keep the surrounding logic: obtain fieldToColumn via
getFieldToColumnMap(contract, modelName), iterate Object.keys(fieldToColumn),
skip when Object.hasOwn(row, fieldName) is false, then keep the existing value
null/undefined checks and assignment to criterion[fieldName]); this aligns with
how mapStorageRowToModelFields produces plain row objects and follows the
codebase guideline to always use Object.hasOwn().
packages/3-extensions/sql-orm-client/src/collection.ts (2)

1175-1188: ⚡ Quick win

deleteCount() has the same materialize-to-count pattern.

Mirrors the updateCount() concern: .toArray() buffers every deleted row and undefined selection likely returns the full row payload, when all the caller wants is a count. Apply the same fix — iterate the async result and increment a counter, optionally with a minimal projection.

♻️ Stream-count delete result
-    const compiled = compileDeleteReturning(
-      this.contract,
-      this.tableName,
-      this.state.filters,
-      undefined,
-    );
-    const deletedRows = await executeQueryPlan<Record<string, unknown>>(
-      this.ctx.runtime,
-      compiled,
-    ).toArray();
-
-    return deletedRows.length;
+    const compiled = compileDeleteReturning(
+      this.contract,
+      this.tableName,
+      this.state.filters,
+      undefined,
+    );
+    let count = 0;
+    for await (const _ of executeQueryPlan<Record<string, unknown>>(this.ctx.runtime, compiled)) {
+      count++;
+    }
+    return count;
🤖 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 1175 -
1188, deleteCount() currently materializes all deleted rows via .toArray() and
an undefined selection; change it to stream the results and increment a counter
instead. Update the call to compileDeleteReturning(this.contract,
this.tableName, this.state.filters, ...) to request a minimal projection (e.g.,
primary key or a tiny count marker) rather than undefined, then run
executeQueryPlan<Record<string, unknown>>(this.ctx.runtime, compiled) and
consume the async iterator with a for-await loop (or iterator.next loop)
incrementing a local counter for each yielded item; return that counter and
remove the .toArray() call.

1113-1135: ⚡ Quick win

updateCount() materializes the entire RETURNING result just to count it.

.toArray() collects every updated row into memory and then takes .length. Streaming iteration with a for await loop would avoid materialization. Passing undefined for the projection likely produces a wide RETURNING row payload, so consider also emitting a minimal projection (e.g., a single literal or PK column) for the count path to reduce data transfer.

🤖 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 1113 -
1135, The current updateCount() materializes the full RETURNING set via
.toArray(); change it to stream and count instead: after mapping
(mapModelDataToStorageRow) and applyUpdateDefaults, call compileUpdateReturning
with a minimal projection (not undefined) to reduce payload—e.g., a single
literal or the table PK column (use this.tableName/this.contract to determine
PK)—then call executeQueryPlan(..., compiled) and iterate the returned async
iterable with a for await loop to increment a counter and return the count,
removing the .toArray() allocation.
🤖 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/mutation-executor.ts`:
- Around line 141-158: The id-less UPDATE guard is unsafe because
buildRowIdentityCriterion drops nulls, allowing a "unique" constraint on a
nullable column to be ignored and UPDATE to match multiple rows; fix by
tightening the nested-update guard so before allowing the UPDATE you verify that
at least one unique constraint’s every mapped column is present and non-null in
the existingRow returned by findFirstByFilters (i.e., for each unique constraint
retrieved from contract, check all its columns exist and are !== null in
existingRow); only allow the id-less UPDATE when such a fully non-null unique
constraint exists (alternatively, if you prefer option 2, change
buildRowIdentityCriterion to include null-valued mapped fields as explicit IS
NULL predicates so nulls are part of the criterion).

In `@todos/001-complete-p1-multirow-update-on-idless-duplicate-tuples.md`:
- Line 2: The todo mistakenly marks "status: complete" while acceptance
criterion 2 (an integration test exercising duplicate-tuple behavior using a
schema without UNIQUE (name)) is not implemented: either revert the status to
"pending" and check only criteria 1 and 3, or actually implement criterion 2 by
adding a new integration test that uses a fixtures schema (not the current tags
PG schema) with the UNIQUE (name) constraint removed, exercise the id-less
duplicate-tuple scenario, and then update the checklist to mark all three
criteria; reference the checklist items (criteria 1, 2, 3), the id-less
integration tests, and the tags PG schema/UNIQUE (name) constraint when making
the change.

In `@todos/002-pending-p2-test-new-returning-required-gate.md`:
- Around line 9-52: Add unit tests that assert updateCount() and deleteCount()
throw when contract lacks the returning capability: create a contract with empty
capabilities via getTestContract() (or withCapabilities(getTestContract(), {})),
build an execution context with createExecutionContext(...), instantiate
Collection and call collection.where({}).updateCount(...) and
collection.where({}).deleteCount(), and expect rejects.toThrow matching
/updateCount\(\) requires contract capability "returning"/ and /deleteCount\(\)
requires contract capability "returning"/ respectively; this ensures the
assertReturningCapability(...) checks wired into updateCount and deleteCount are
covered.

---

Outside diff comments:
In `@todos/006-pending-p2-dead-code-compiledeletecount.md`:
- Around line 1-61: Remove the dead helper compileDeleteCount from
query-plan-mutations.ts (delete the entire function definition named
compileDeleteCount) and remove its re-export from query-plan.ts so the symbol is
no longer exported; ensure no other references remain (grep for
compileDeleteCount) and run the test suite to verify nothing broke.

---

Nitpick comments:
In `@packages/3-extensions/sql-orm-client/src/collection.ts`:
- Around line 1175-1188: deleteCount() currently materializes all deleted rows
via .toArray() and an undefined selection; change it to stream the results and
increment a counter instead. Update the call to
compileDeleteReturning(this.contract, this.tableName, this.state.filters, ...)
to request a minimal projection (e.g., primary key or a tiny count marker)
rather than undefined, then run executeQueryPlan<Record<string,
unknown>>(this.ctx.runtime, compiled) and consume the async iterator with a
for-await loop (or iterator.next loop) incrementing a local counter for each
yielded item; return that counter and remove the .toArray() call.
- Around line 1113-1135: The current updateCount() materializes the full
RETURNING set via .toArray(); change it to stream and count instead: after
mapping (mapModelDataToStorageRow) and applyUpdateDefaults, call
compileUpdateReturning with a minimal projection (not undefined) to reduce
payload—e.g., a single literal or the table PK column (use
this.tableName/this.contract to determine PK)—then call executeQueryPlan(...,
compiled) and iterate the returned async iterable with a for await loop to
increment a counter and return the count, removing the .toArray() allocation.

In `@packages/3-extensions/sql-orm-client/src/mutation-executor.ts`:
- Around line 141-148: The loop building criterion uses the `in` operator which
checks the prototype chain; change the existence check to use Object.hasOwn(row,
fieldName) instead (keep the surrounding logic: obtain fieldToColumn via
getFieldToColumnMap(contract, modelName), iterate Object.keys(fieldToColumn),
skip when Object.hasOwn(row, fieldName) is false, then keep the existing value
null/undefined checks and assignment to criterion[fieldName]); this aligns with
how mapStorageRowToModelFields produces plain row objects and follows the
codebase guideline to always use Object.hasOwn().

In `@todos/003-pending-p2-wide-returning-payload.md`:
- Around line 1-62: updateCount and deleteCount call
compileUpdateReturning/compileDeleteReturning with returningColumns undefined
which buildReturningColumns expands to every table column, shipping wide
payloads; fix by passing an explicit single-column returning list (e.g.
primaryKeyColumns[0] or first storage column) from the updateCount and
deleteCount call sites instead of undefined so
compileUpdateReturning/compileDeleteReturning emit RETURNING [trivialColumn]
rather than all columns (refer to updateCount, deleteCount,
compileUpdateReturning, compileDeleteReturning, and
primaryKeyColumns/table.columns to locate the spots).
🪄 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: f1705808-eeb8-4603-98e0-1b2d1c5c756d

📥 Commits

Reviewing files that changed from the base of the PR and between 57cdd5e and fbabbdc.

⛔ Files ignored due to path filters (2)
  • projects/created-updated-at-authoring/plan.md is excluded by !projects/**
  • projects/created-updated-at-authoring/spec.md is excluded by !projects/**
📒 Files selected for processing (42)
  • docs/architecture docs/subsystems/3. Query Lanes.md
  • packages/1-framework/3-tooling/cli/test/control-api/client.test.ts
  • packages/1-framework/3-tooling/cli/vitest.config.ts
  • packages/1-framework/3-tooling/emitter/test/emitter.roundtrip.test.ts
  • packages/1-framework/3-tooling/vite-plugin-contract-emit/src/plugin.ts
  • packages/1-framework/3-tooling/vite-plugin-contract-emit/test/plugin.test.ts
  • packages/2-sql/1-core/contract/src/validators.ts
  • packages/2-sql/1-core/contract/test/validators.test.ts
  • packages/2-sql/2-authoring/contract-psl/src/interpreter.ts
  • packages/2-sql/2-authoring/contract-psl/src/psl-attribute-parsing.ts
  • packages/2-sql/2-authoring/contract-psl/src/psl-field-resolution.ts
  • packages/2-sql/2-authoring/contract-psl/test/interpreter.diagnostics.test.ts
  • packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts
  • packages/2-sql/2-authoring/contract-ts/src/build-contract.ts
  • packages/2-sql/2-authoring/contract-ts/test/contract-builder.contract-definition.test.ts
  • packages/2-sql/2-authoring/contract-ts/test/contract-builder.dsl.test.ts
  • packages/2-sql/3-tooling/emitter/src/index.ts
  • packages/2-sql/3-tooling/emitter/test/emitter-hook.structure.test.ts
  • packages/3-extensions/sql-orm-client/src/collection-contract.ts
  • packages/3-extensions/sql-orm-client/src/collection.ts
  • packages/3-extensions/sql-orm-client/src/mutation-executor.ts
  • packages/3-extensions/sql-orm-client/src/query-plan-select.ts
  • packages/3-extensions/sql-orm-client/test/collection-contract.test.ts
  • packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts
  • packages/3-extensions/sql-orm-client/test/integration/helpers.ts
  • packages/3-extensions/sql-orm-client/test/integration/idless.test.ts
  • packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts
  • plans/feat-complete-idless-orm-support.md
  • todos/001-complete-p1-multirow-update-on-idless-duplicate-tuples.md
  • todos/002-pending-p2-test-new-returning-required-gate.md
  • todos/003-pending-p2-wide-returning-payload.md
  • todos/004-pending-p2-toarray-materialization-for-counts.md
  • todos/005-pending-p2-integration-coverage-idless-reload.md
  • todos/006-pending-p2-dead-code-compiledeletecount.md
  • todos/007-pending-p3-sqlite-after-trigger-mitigation.md
  • todos/008-pending-p3-asymmetric-returning-gate.md
  • todos/009-pending-p3-trim-what-comment-in-rowidentitycriterion.md
  • todos/010-pending-p3-typed-builder-without-primary-key.md
  • todos/011-pending-p3-cut-redundant-idless-zero-test.md
  • todos/012-pending-p3-extract-getprimarykeycolumns-helper.md
  • todos/013-pending-p3-stale-plan-name-references.md
  • todos/014-pending-p3-trim-resolveprimarykey-error-message.md
💤 Files with no reviewable changes (2)
  • packages/2-sql/2-authoring/contract-ts/src/build-contract.ts
  • packages/2-sql/3-tooling/emitter/src/index.ts

Comment thread packages/3-extensions/sql-orm-client/src/mutation-executor.ts
Comment thread todos/001-complete-p1-multirow-update-on-idless-duplicate-tuples.md
Comment thread todos/002-complete-p2-test-new-returning-required-gate.md Outdated
jkomyno added 8 commits May 8, 2026 14:59
* #7 — SQLite AFTER-trigger mitigation: decision recorded as defer.
  JSDoc on buildRowIdentityCriterion + Query Lanes.md paragraph remain
  the documented mitigation; no runtime guard added until the SQLite
  adapter actually exercises id-less + AFTER-trigger contracts.

* #8 — Asymmetric `returning` gate: comment at executeUpdateCount
  records the explicit design choice. Surface-level updateCount/
  deleteCount require the capability; FK-rewrite cleanup
  (disconnect/setNull) does not, since no row data is read.

* #9 — Trim WHAT-restating sentence from buildRowIdentityCriterion
  comment block. Remaining lines explain the genuinely non-obvious
  WHY: RETURNING-by-construction invariant + duplicate-tuple caveat
  + SQLite AFTER-trigger caveat.

* #10 — Add typed `withoutPrimaryKey<TTable>(contract, table)` helper
  to test/helpers.ts mirroring `withCapabilities`. Migrate four call
  sites (createIdlessTagsCollection, createIdlessUsersCollection, two
  unit tests in mutation-executor.test.ts) from inline
  `as unknown as TestContract` casts to the helper. Cast lives in one
  place now.

* #11 — Drop redundant `updateCount returns zero` integration test
  on id-less Tags. The zero-match path is already covered by unit
  tests on PK contracts and does not exercise id-less specifics.

* #12 — Extract `getPrimaryKeyColumns(contract, tableName): readonly
  string[]` helper in collection-contract.ts. Replaces three
  duplicated raw-storage accesses (resolvePrimaryKeyColumn,
  pickCountReturningColumn, buildRowIdentityCriterion, the new P1
  guard).

* #13 — Plan doc references the post-rename name throughout.

* #14 — Trim `resolvePrimaryKeyColumn` error to one sentence; the
  API guidance prose duplicated content already in Query Lanes.md.

All 8 P3 todos marked complete. pnpm test:packages 110/110 green;
pnpm lint:deps 0 violations. sql-orm-client suite is 501 (one fewer
than before due to dropped redundant test).
…rface

After M1, compileUpdateCount has only one caller — the FK-rewrite cleanup
helper executeUpdateCount in mutation-executor.ts. Stop re-exporting it
from query-plan.ts; the cross-file consumer now imports it directly from
query-plan-mutations.ts as a file-internal helper. Test imports updated
to match.

Pure surface reduction; no behavior change.
…e collection method

Move the helper from collection-contract.ts (where it was a public-but-
single-purpose export) into collection.ts as a private
#returningColumnForCount() method. updateCount() and deleteCount() now
call the method directly instead of importing a free function. Logic
is identical: prefer the first PK column, fall back to any storage
column.

Drops the helper from the cross-file surface — its single responsibility
(pick a trivial column for count RETURNING) lives next to its only two
call sites.
The plan described M1/M2/M3 work that has now landed end-to-end on this
branch. Following the same close-out convention used for
projects/id-less-sql-models in commit c1d6b15, delete the plan file
post-implementation. Historical references in the todos/ directory are
preserved as immutable review artifacts.
Source content for the Linear ticket that this PR should land under.
Follow-up to TML-2380 (PR #424); covers the ORM-lane completion work
that builds on the authoring/emission support TML-2380 introduced.

Includes scope, out-of-scope, acceptance criteria, and references.
Maintainer will create the Linear ticket from this content and either
replace this file with the ticket URL or delete it post-creation.
…tion

Linear ticket created: https://linear.app/prisma-company/issue/TML-2432
The Linear: line is now at the top of the PR description, matching the
convention used by the parent PR #424 (TML-2380).
# Conflicts:
#	docs/architecture docs/subsystems/3. Query Lanes.md
…-unique

CodeRabbit review (#440) flagged that the nested-mutation update guard
accepted any unique constraint as proof of single-row identity, but
buildRowIdentityCriterion strips null mapped fields. A unique constraint
sitting on a nullable column whose value is null in the RETURNING'd row
would be silently excluded from the criterion — and Postgres unique
indexes default to NULLS DISTINCT, so multiple rows can share NULL on
that column with identical other-column tuples and broaden the UPDATE.

Add a second tier to the guard: when the table is id-less, require at
least one unique constraint whose every column is non-null in the
existing row before allowing the UPDATE. The pre-existing structural
"declare a primary key or unique constraint" check stays first so its
error message and unit test are unchanged. Cover the new path with a
dedicated unit test against users.uniques = [{ columns: ['invited_by_id'] }]
where invitedById is null.

Also reconcile the supporting todos with reality: todo 001 marks the
duplicate-tuple integration fixture as deferred (the existing tags PG
schema carries UNIQUE (name)), todo 002 records the work log for the
already-merged returning-capability gate tests in
collection-mutation-defaults.test.ts.
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts (1)

264-303: ⚡ Quick win

Consider extracting the duplicated capability-stripped setup into a shared helper.

Lines 266–277 and 287–298 are identical. A small helper that mirrors setupTagCollection would eliminate the repetition and make future capability-gate tests one line each.

♻️ Suggested extraction
+function setupTagCollectionWithoutReturning(): {
+  collection: Collection<TestContract, 'Tag'>;
+  runtime: MockRuntime;
+} {
+  const contract = buildTagWithUpdatedAtContract();
+  const stripped = { ...contract, capabilities: {} } as TestContract;
+  const context = createExecutionContext({
+    contract: stripped,
+    stack: createSqlExecutionStack({
+      target: postgresTarget,
+      adapter: postgresAdapter,
+      extensionPacks: [pgvectorRuntime],
+    }),
+  });
+  const runtime: MockRuntime = createMockRuntime();
+  return { collection: new Collection({ context, runtime }, 'Tag'), runtime };
+}

   it('throws when the contract does not declare the returning capability', async () => {
-    // setupTagCollection wires `withReturningCapability`; rebuild without it.
-    const contract = buildTagWithUpdatedAtContract();
-    const stripped = { ...contract, capabilities: {} } as TestContract;
-    const context = createExecutionContext({
-      contract: stripped,
-      stack: createSqlExecutionStack({
-        target: postgresTarget,
-        adapter: postgresAdapter,
-        extensionPacks: [pgvectorRuntime],
-      }),
-    });
-    const runtime: MockRuntime = createMockRuntime();
-    const collection = new Collection({ context, runtime }, 'Tag');
+    const { collection } = setupTagCollectionWithoutReturning();

     await expect(
       collection.where({ id: tagId(TAG_ID_1) }).updateCount({ name: 'x' }),
     ).rejects.toThrow(/updateCount\(\) requires contract capability "returning"/);
   });

Apply the same substitution to the deleteCount block.

🤖 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-mutation-defaults.test.ts`
around lines 264 - 303, Extract the duplicated "capability-stripped" test setup
into a reusable helper to remove repetition: create a function (e.g.,
makeStrippedTagCollection or setupStrippedTagCollection) that calls
buildTagWithUpdatedAtContract(), creates stripped = { ...contract, capabilities:
{} } as TestContract, then builds the context via createExecutionContext({
contract: stripped, stack: createSqlExecutionStack({ target: postgresTarget,
adapter: postgresAdapter, extensionPacks: [pgvectorRuntime] }) }), creates
runtime with createMockRuntime(), and returns new Collection({ context, runtime
}, 'Tag'); replace the two duplicated blocks that currently inline those steps
in the updateCount and deleteCount tests with a single call to this helper.
🤖 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/2-sql/2-authoring/contract-psl/test/interpreter.test.ts`:
- Around line 559-621: Remove the duplicated test block that re-declares
describe('round-trips printer output', ...) with its two it() cases ("accepts
the printer output for an id-less table" and "accepts the printer output for a
composite-PK table"); keep the canonical copy and delete the second identical
describe block so each test (the two it() cases) appears only once in
interpreter.test.ts.

In `@todos/007-complete-p3-sqlite-after-trigger-mitigation.md`:
- Line 2: The TODO file currently sets "status: complete" while the acceptance
checklist still has unchecked items; update the document so the status and
checklist match by either ticking the completed acceptance criteria checkboxes
or reverting "status: complete" to an appropriate in-progress state, and repeat
the same change for the related entries referenced (the entries around the
acceptance checklist items noted as 47-49); ensure the README/rules/links note
is updated to reflect the corrected status so docs remain consistent.

In `@todos/014-complete-p3-trim-resolveprimarykey-error-message.md`:
- Line 2: The frontmatter value "status: complete" conflicts with an unchecked
acceptance-criteria checkbox (the unchecked item at line 47 and similarly lines
45-48); update the frontmatter or the checklist so they match by either changing
"status: complete" to a non-complete state (e.g., "status: in-progress" or
"status: needs-work") or by checking the relevant checkbox(es) to mark those
criteria done, ensuring consistency between the "status: complete" symbol and
the acceptance checklist.

---

Nitpick comments:
In
`@packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts`:
- Around line 264-303: Extract the duplicated "capability-stripped" test setup
into a reusable helper to remove repetition: create a function (e.g.,
makeStrippedTagCollection or setupStrippedTagCollection) that calls
buildTagWithUpdatedAtContract(), creates stripped = { ...contract, capabilities:
{} } as TestContract, then builds the context via createExecutionContext({
contract: stripped, stack: createSqlExecutionStack({ target: postgresTarget,
adapter: postgresAdapter, extensionPacks: [pgvectorRuntime] }) }), creates
runtime with createMockRuntime(), and returns new Collection({ context, runtime
}, 'Tag'); replace the two duplicated blocks that currently inline those steps
in the updateCount and deleteCount tests with a single call to this helper.
🪄 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: 7516805f-fd4b-441d-b0e5-96f0e3920229

📥 Commits

Reviewing files that changed from the base of the PR and between fbabbdc and ea0a9cd.

📒 Files selected for processing (26)
  • packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts
  • packages/3-extensions/sql-orm-client/src/collection-contract.ts
  • packages/3-extensions/sql-orm-client/src/collection.ts
  • packages/3-extensions/sql-orm-client/src/mutation-executor.ts
  • packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts
  • packages/3-extensions/sql-orm-client/src/query-plan.ts
  • packages/3-extensions/sql-orm-client/test/collection-mutation-defaults.test.ts
  • packages/3-extensions/sql-orm-client/test/helpers.ts
  • packages/3-extensions/sql-orm-client/test/integration/helpers.ts
  • packages/3-extensions/sql-orm-client/test/integration/idless.test.ts
  • packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts
  • packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
  • todos/001-complete-p1-multirow-update-on-idless-duplicate-tuples.md
  • todos/002-complete-p2-test-new-returning-required-gate.md
  • todos/003-complete-p2-wide-returning-payload.md
  • todos/004-complete-p2-toarray-materialization-for-counts.md
  • todos/005-complete-p2-integration-coverage-idless-reload.md
  • todos/006-complete-p2-dead-code-compiledeletecount.md
  • todos/007-complete-p3-sqlite-after-trigger-mitigation.md
  • todos/008-complete-p3-asymmetric-returning-gate.md
  • todos/009-complete-p3-trim-what-comment-in-rowidentitycriterion.md
  • todos/010-complete-p3-typed-builder-without-primary-key.md
  • todos/011-complete-p3-cut-redundant-idless-zero-test.md
  • todos/012-complete-p3-extract-getprimarykeycolumns-helper.md
  • todos/013-complete-p3-stale-plan-name-references.md
  • todos/014-complete-p3-trim-resolveprimarykey-error-message.md
💤 Files with no reviewable changes (2)
  • packages/3-extensions/sql-orm-client/src/query-plan.ts
  • packages/3-extensions/sql-orm-client/src/query-plan-mutations.ts
✅ Files skipped from review due to trivial changes (11)
  • todos/003-complete-p2-wide-returning-payload.md
  • todos/013-complete-p3-stale-plan-name-references.md
  • todos/012-complete-p3-extract-getprimarykeycolumns-helper.md
  • todos/010-complete-p3-typed-builder-without-primary-key.md
  • todos/009-complete-p3-trim-what-comment-in-rowidentitycriterion.md
  • todos/004-complete-p2-toarray-materialization-for-counts.md
  • todos/008-complete-p3-asymmetric-returning-gate.md
  • todos/011-complete-p3-cut-redundant-idless-zero-test.md
  • todos/005-complete-p2-integration-coverage-idless-reload.md
  • todos/006-complete-p2-dead-code-compiledeletecount.md
  • todos/001-complete-p1-multirow-update-on-idless-duplicate-tuples.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/3-extensions/sql-orm-client/src/collection-contract.ts
  • packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts
  • packages/3-extensions/sql-orm-client/src/collection.ts
  • packages/3-extensions/sql-orm-client/src/mutation-executor.ts

Comment thread packages/2-sql/2-authoring/contract-psl/test/interpreter.test.ts Outdated
Comment thread todos/007-complete-p3-sqlite-after-trigger-mitigation.md
Comment thread todos/014-complete-p3-trim-resolveprimarykey-error-message.md
@jkomyno jkomyno requested a review from a team as a code owner May 11, 2026 10:32
@jkomyno jkomyno force-pushed the feat/orm-requires-primary-key-gate branch from b533111 to 7c2059e Compare May 11, 2026 10:33
…mary-key-gate

Signed-off-by: jkomyno <12381818+jkomyno@users.noreply.github.com>

# Conflicts:
#	packages/3-extensions/sql-orm-client/test/query-plan-mutations.test.ts
@jkomyno jkomyno force-pushed the feat/orm-requires-primary-key-gate branch from 7c2059e to 76aca22 Compare May 11, 2026 10:36
CodeRabbit (PR #440 review at fbabbdc) flagged that the file declared
`status: complete` while acceptance criterion 2 (duplicate-tuple
integration test) was still unchecked. The criterion was written when
solutions B and C were on the table; chosen action A (runtime guard)
makes the duplicate-tuple integration fixture not load-bearing, since
the unit-test path through `executeNestedUpdateMutation` already
exercises the error contract end-to-end.

Re-frame criterion 2 as "triaged + withdrawn under chosen action A"
with the rationale preserved inline, so the checklist matches the
frontmatter without losing the deferral context. No code change.

Signed-off-by: jkomyno <12381818+jkomyno@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant