TML-2754: Postgres migration planner builds CREATE TABLE / CREATE SCHEMA as typed DDL through the adapter#751
Open
wmadden-electric wants to merge 29 commits into
Conversation
Decompose the planner-adoption work (project slice 4): grounding showed slice 1 (TML-2761) shipped DDL-AST nodes only for CreateTable/CreateSchema while the planner emits ~21 Postgres + ~7 SQLite DDL ops, so full adoption is many slices, not one. This first slice migrates planner CreateTable (PG+SQLite) + CreateSchema (PG) onto the DDL AST and extends the CreateTable node with a target-contributed table-level constraint surface (composite PK / FK / unique). Plan: 3 dispatches (constraint-shape spike → constraint surface → planner migration). Record that planner-adoption spans all three targets (Mongo migration planner adoption is a tracked follow-up) in the project spec DoD + plan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…ering Adds `PrimaryKeyConstraint`, `ForeignKeyConstraint`, and `UniqueConstraint` frozen-class nodes to `relational-core/ast/ddl-types.ts`, extends `PostgresCreateTable` and `SqliteCreateTable` with a `constraints?` array, and implements constraint rendering in both adapter DDL visitors. A worked-example test (`post_tags` join table with composite PK, two FKs, and a named UNIQUE) passes on both targets. `pnpm fixtures:check` is green — no existing DDL output changed. Design decision recorded in design-notes.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
D1 spike review (SATISFIED): the table-level constraint node shape is settled, recorded in design-notes.md, and proven to lower on PG + SQLite. Orchestrator close-out: correct the design-notes REFERENTIAL_ACTION_SQL reuse claim (F1; the map is re-declared per renderer, the type is shared), and fold two D1-review amendments into the plan — D2 consolidates the 5 REFERENTIAL_ACTION_SQL copies; D3 byte-parity covers table-body indentation as well as identifier quoting. Trace: D1 dispatch-start..dispatch-end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…straints constructor
- Extract one shared REFERENTIAL_ACTION_SQL map to @prisma-next/sql-contract
(packages/2-sql/1-core/contract/src/referential-action-sql.ts) and export it
via the new ./referential-action-sql entrypoint.
- Replace 5 identical local copies with imports of the shared map:
postgres target: planner-ddl-builders.ts + operations/constraints.ts
sqlite target: operations/shared.ts
postgres adapter: ddl-renderer.ts
sqlite adapter: ddl-renderer.ts
- Add constraints?: readonly DdlTableConstraint[] parameter to the contract-free
createTable() constructors (postgres + sqlite src/contract-free/ddl.ts).
- Switch D1 worked-example tests to build CreateTable via the constructor
rather than hand-instantiating node classes.
pnpm lint:deps clean; all target + adapter unit tests pass; fixtures:check green;
migration snapshot output byte-identical after consolidation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Will Madden <madden@prisma.io>
…n SATISFIED D2 SATISFIED: contract-free createTable() now accepts table-level constraints; the 5 REFERENTIAL_ACTION_SQL copies are consolidated to one shared @prisma-next/sql-contract/referential-action-sql map (snapshots byte-identical, lint:deps clean). Substrate ready for D3 planner adoption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…wering Thread SqlControlFamilyInstance from createPlanner(family) through to CreateTableCall.toOp(lower) and CreateSchemaCall.toOp(lower) so the planner-produced SQL is generated by the DDL AST adapter rather than direct string concatenation. Byte-parity: PG and SQLite DDL renderers updated to match the planner's canonical format — quoted identifiers, 2-space indent, uppercase SQL keywords, DEFAULT (expr) for function defaults. Delete buildCreateTableSql (PG); rename renderCreateTableSql to buildTableBodySql (SQLite, kept private for recreateTable). Update all affected test assertions and remove the buildCreateTableSql test section. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
D3 review returned ANOTHER ROUND: SQLite was unmigrated (the diff was PG-only), the grep gate passed via a rename not a deletion, PG kept a string-build fallback that masked the real path in tests, parseDefaultSql round-tripped rendered SQL (lossy for sequence defaults), and no test drove the planner lower path (byte-parity unproven). Operator-approved: split SQLite CreateTable adoption to its own follow-up slice (Postgres-then-SQLite fan-out the plan already allowed); narrow this slice + AC-2/AC-3 to Postgres. Round 3 finishes PG cleanly (remove fallback, fix parseDefaultSql, fix cast, add the toOp(lower) byte-parity test, revert SQLite). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
… lowerer Completes the full Postgres planner DDL adoption for D3 (round 3): - CreateTableCall.toOp(lower) and CreateSchemaCall.toOp(lower) now require a LowerFn; the string-build fallback is removed entirely. `git grep buildCreateTableSql` returns zero. - UNBOUND_NAMESPACE_ID (__unbound__) is mapped to schema: undefined so the DDL renderer emits an unqualified table name rather than CREATE TABLE "__unbound__"."item" which Postgres rejects. - DdlColumnDefault is built from structured PostgresColumnDefault data (literal, function, sequence) via postgresDefaultToDdlColumnDefault; parseDefaultSql is gone. - render-typescript.ts threads the optional LowerFn through renderCallsToTypeScript so CreateTableCall and CreateSchemaCall scaffold entries embed the pre-lowered SQL string instead of the raw column spec array. - All call sites of createPostgresMigrationPlanner() updated to pass a lower function (issue-planner, control-policy-planner, planner unit tests, adapter integration tests, pgvector tests). - AC-3 keystone tests added: composite-PK CREATE TABLE byte-parity, CREATE SCHEMA byte-parity, __unbound__ unqualified name, and sequence default nextval SQL. - SQLite adapter reverted to bf6e969 state (DDL adoption is a follow-up slice). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…e to column-list form Reverts the wrong signature change introduced in R3: - Restores `createTable(schemaName, tableName, columns, primaryKey?)` as the public factory in operations/tables.ts; the internal `createTableOp` helper (sql-string form) is kept so `CreateTableCall.toOp(lower)` can still route through the DDL adapter. - Restores `createSchema(schemaName)` in operations/dependencies.ts with a new internal `createSchemaOp(schemaName, sql)` helper for the same reason. - Removes the `renderCall` override in render-typescript.ts that was emitting SQL-string `createTable`/`createSchema` calls in the scaffold; the scaffold now uses each call's own `renderTypeScript()` which emits the column-list form. - Drops the `lower` parameter from `renderCallsToTypeScript` (no longer needed for TS rendering — the runtime lowerer is still used by `renderOps`). - Reverts the SQLite `buildTableBodySql` rename back to `renderCreateTableSql` so `git diff bf6e969 -- '*sqlite*'` is empty. - Adds a `renderCallsToTypeScript` pinning test that asserts the scaffold emits the column-list form and never the SQL-string form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Replace import aliases (createTable as createTableDdl, createSchema as
createSchemaDdl) with a namespace import (* as contractFreeDdl) to
eliminate false positives from the no-bare-cast plugin. Replace the
genuine (exhaustive as { kind: string }).kind cast with
blindCast<{ kind: string }, '...'>(exhaustive).kind in the exhaustiveness
default branch. pnpm lint:casts now reports delta=0.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Will Madden <madden@prisma.io>
…on SATISFIED Slice complete (Postgres-only). AC-1/2/3 pass: the constraint node shape is settled + recorded; PG CreateTableCall/CreateSchemaCall toOp() build a DDL node and lower it via the injected SqlControlAdapter at plan time (no string-build fallback; buildCreateTableSql gone); byte-parity vs pre-D3 is pinned by a toOp(lower) test; the generated migration.ts scaffold is unchanged. Trace backstop passes (3 dispatches). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a shared REFERENTIAL_ACTION_SQL export, table-level constraint AST nodes and contract-free builders, threads an adapter Lowerer through planner → render paths, updates op factories to lower DDL nodes to SQL, extends Postgres/SQLite renderers to emit constraints and quoted identifiers, and aligns tests and example migrations to the new DSL. ChangesShared Referential-Action SQL Mapping
Table-Level Constraint AST and Node Support
Planner-to-Lowerer Wiring
Op Factory and Operation Builders
DDL Builder Cleanup
DDL Constraint Rendering
Tests, Examples, and Adapter Integration
Estimated code review effort: Possibly related PRs:
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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)
packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts (1)
952-953:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
identityKeyFor()now throws for create-table and create-schema calls.
CreateTableCall.toOp()andCreateSchemaCall.toOp()now require a lowerer, but this helper still invokescall.toOp()with none. Any dedup/reconciliation path that sees either variant will crash instead of returning a stable id. Compute those ids directly from the call fields, or thread the lowerer throughidentityKeyFor()too.🤖 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-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts` around lines 952 - 953, identityKeyFor currently calls call.toOp() without a Lowerer which crashes for CreateTableCall and CreateSchemaCall because their toOp requires a lowerer; either compute stable ids directly from the call's identifying fields (e.g., table/schema name and relevant options on CreateTableCall/CreateSchemaCall) inside identityKeyFor, or change identityKeyFor signature to accept and forward a Lowerer to call.toOp(). Locate identityKeyFor and the PostgresOpFactoryCall implementations (CreateTableCall, CreateSchemaCall) and implement one of the two fixes: derive the id from call properties for deterministic identity keys, or add a lowerer parameter to identityKeyFor and pass it into call.toOp() so toOp has the required context.
🧹 Nitpick comments (1)
packages/3-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.ts (1)
13-101: ⚡ Quick winAdd one regression case for quoted/mixed-case identifiers in table constraints.
Current cases only use lowercase-safe names, so quoting regressions in
constraint.name/ FK target table rendering won’t be caught. A single case with mixed-case or keyword-like identifiers would harden this suite.🤖 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-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.ts` around lines 13 - 101, Tests only cover lowercase-safe identifiers so regressions in quoting for constraint names and FK target tables can slip; add a new test in ddl-table-constraints-lowering.test.ts that creates a table using mixed-case/keyword-like identifiers (use createTable, col, and constraints like ForeignKeyConstraint/UniqueConstraint with a name containing capitals or spaces) and assert that createPostgresAdapter().lower(ast, { contract: {} as PostgresContract }) produces SQL with properly quoted identifiers (e.g., CONSTRAINT "My_Const" or REFERENCES "UserTable" ("Id")), mirroring the style of the existing cases (check lowered.sql and lowered.params) to catch quoting regressions.
🤖 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-targets/3-targets/postgres/src/core/migrations/operations/shared.ts`:
- Around line 24-29: The ColumnSpec interface currently exposes the planner-only
columnDefault (type PostgresColumnDefault) which breaks public factory
signatures and causes scaffold/output to include planner internals; update the
implementation by either (a) moving columnDefault into a separate internal
interface (e.g., InternalColumnSpec) used only by the planner/lowering code
paths and keep public ColumnSpec unchanged, or (b) make columnDefault
optional/hidden and ensure CreateTableCall.renderTypeScript() (and any code that
serializes this.columns) strips or omits columnDefault when emitting scaffolded
migration.ts, and update callers like createTable() and addColumn() to accept
the public ColumnSpec while the planner uses the internal spec. Ensure all
references to ColumnSpec, columnDefault, PostgresColumnDefault, createTable(),
addColumn(), and CreateTableCall.renderTypeScript() are adjusted so public APIs
and scaffold output never include the planner-only field.
In `@packages/3-targets/6-adapters/postgres/src/core/ddl-renderer.ts`:
- Around line 21-43: The constraint SQL currently outputs constraint.name and
constraint.refTable as raw text causing issues with mixed-case/reserved
identifiers; update renderPrimaryKeyConstraint and renderForeignKeyConstraint to
wrap constraint.name and constraint.refTable with quoteIdentifier (the same
helper used for columns) so CONSTRAINT names and referenced table names are
properly quoted; ensure any place building `CONSTRAINT ${constraint.name}` or
`REFERENCES ${constraint.refTable}` uses quoteIdentifier(constraint.name) and
quoteIdentifier(constraint.refTable) respectively.
- Line 95: The return line currently emits DEFAULT '${JSON.stringify(value)}'
without escaping, which can break SQL when the JSON contains single quotes;
replace the interpolation with a call to the existing escapeLiteral helper so it
becomes DEFAULT '<escaped JSON>' (i.e. call escapeLiteral(JSON.stringify(value))
in place of JSON.stringify(value)); update the same return in ddl-renderer.ts
where JSON.stringify(value) is used and ensure you import/use the existing
escapeLiteral function the file already uses for string defaults.
---
Outside diff comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts`:
- Around line 952-953: identityKeyFor currently calls call.toOp() without a
Lowerer which crashes for CreateTableCall and CreateSchemaCall because their
toOp requires a lowerer; either compute stable ids directly from the call's
identifying fields (e.g., table/schema name and relevant options on
CreateTableCall/CreateSchemaCall) inside identityKeyFor, or change
identityKeyFor signature to accept and forward a Lowerer to call.toOp(). Locate
identityKeyFor and the PostgresOpFactoryCall implementations (CreateTableCall,
CreateSchemaCall) and implement one of the two fixes: derive the id from call
properties for deterministic identity keys, or add a lowerer parameter to
identityKeyFor and pass it into call.toOp() so toOp has the required context.
---
Nitpick comments:
In
`@packages/3-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.ts`:
- Around line 13-101: Tests only cover lowercase-safe identifiers so regressions
in quoting for constraint names and FK target tables can slip; add a new test in
ddl-table-constraints-lowering.test.ts that creates a table using
mixed-case/keyword-like identifiers (use createTable, col, and constraints like
ForeignKeyConstraint/UniqueConstraint with a name containing capitals or spaces)
and assert that createPostgresAdapter().lower(ast, { contract: {} as
PostgresContract }) produces SQL with properly quoted identifiers (e.g.,
CONSTRAINT "My_Const" or REFERENCES "UserTable" ("Id")), mirroring the style of
the existing cases (check lowered.sql and lowered.params) to catch quoting
regressions.
🪄 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: 8ae1a832-2fc1-4803-a5c4-a56bdc30ee57
⛔ Files ignored due to path filters (7)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/migrate-marker-ledger-to-typed-query-ast-commands/design-notes.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/planner-create-table-adopts-ddl-ast/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/planner-create-table-adopts-ddl-ast/spec.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/spec.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/trace.jsonlis excluded by!projects/**
📒 Files selected for processing (46)
packages/2-sql/1-core/contract/package.jsonpackages/2-sql/1-core/contract/src/exports/referential-action-sql.tspackages/2-sql/1-core/contract/src/referential-action-sql.tspackages/2-sql/1-core/contract/tsdown.config.tspackages/2-sql/4-lanes/relational-core/src/ast/ddl-types.tspackages/3-extensions/pgvector/test/migrations/planner.behavior.test.tspackages/3-extensions/pgvector/test/migrations/planner.contract-to-schema-ir.test.tspackages/3-extensions/pgvector/test/migrations/planner.storage-types.test.tspackages/3-targets/3-targets/postgres/package.jsonpackages/3-targets/3-targets/postgres/src/contract-free/ddl.tspackages/3-targets/3-targets/postgres/src/core/ddl/nodes.tspackages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/dependencies.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/shared.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-ddl-builders.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-ops.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-typescript.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/planner-ddl-builders.tspackages/3-targets/3-targets/postgres/test/migrations/control-policy-planner.test.tspackages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.tspackages/3-targets/3-targets/sqlite/src/contract-free/ddl.tspackages/3-targets/3-targets/sqlite/src/core/ddl/nodes.tspackages/3-targets/3-targets/sqlite/src/core/migrations/operations/shared.tspackages/3-targets/6-adapters/postgres/src/core/ddl-renderer.tspackages/3-targets/6-adapters/postgres/test/ddl-create-table-lowering.test.tspackages/3-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/marker-table-ddl.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.construction.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.lowering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.rendering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner-ddl-builders.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.codec-field-event.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.fk-config.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.reconciliation.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.referential-actions.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.tspackages/3-targets/6-adapters/postgres/test/migrations/render-typescript.roundtrip.test.tspackages/3-targets/6-adapters/sqlite/src/core/ddl-renderer.tspackages/3-targets/6-adapters/sqlite/test/ddl-table-constraints-lowering.test.ts
💤 Files with no reviewable changes (1)
- packages/3-targets/3-targets/postgres/src/exports/planner-ddl-builders.ts
wmadden
requested changes
Jun 7, 2026
Comment on lines
+34
to
+42
| const qualified = qualifyTableName(schemaName, tableName); | ||
| const columnDefs = columns.map(renderColumnDefinition); | ||
| const constraintDefs: string[] = []; | ||
| if (primaryKey) { | ||
| constraintDefs.push(`PRIMARY KEY (${primaryKey.columns.map(quoteIdentifier).join(', ')})`); | ||
| } | ||
| const allDefs = [...columnDefs, ...constraintDefs]; | ||
| const createSql = `CREATE TABLE ${qualified} (\n ${allDefs.join(',\n ')}\n)`; | ||
| return createTableOp(schemaName, tableName, createSql); |
Contributor
There was a problem hiding this comment.
Why is this still gluing SQL strings together?
Contributor
Author
There was a problem hiding this comment.
Gone. createTable/createSchema are now Migration methods that lower a typed DDL node through the adapter — no string-gluing, no createTableOp(sql). The authoring form is now this.createTable({ schema, table, columns: [col(...)], constraints }), zero SQL in the signature.
Comment on lines
+59
to
+61
| const lower: LowerFn = | ||
| typeof family === 'function' ? family : (ast, ctx) => family.lowerAst(ast, ctx); | ||
| return new PostgresMigrationPlanner(lower); |
Contributor
There was a problem hiding this comment.
I don't love this. Why isn't this just receiving an adapter interface?
Contributor
Author
There was a problem hiding this comment.
Done — the planner now depends on the Lowerer interface (@prisma-next/family-sql), a structural subset of SqlControlAdapter, instead of capturing family.lowerAst as a callback.
Comment on lines
+14
to
+17
| export type LowerFn = ( | ||
| ast: AnyQueryAst | DdlNode, | ||
| context: LowererContext<unknown>, | ||
| ) => LoweredStatement; |
Contributor
There was a problem hiding this comment.
No. Define an interface for the dependency, e.g. interface Lowerer { lower(): ... } which should be a structural subset of the adapter
Contributor
Author
There was a problem hiding this comment.
Done — interface Lowerer { lower(ast, ctx): LoweredStatement } now lives in @prisma-next/family-sql; SqlControlAdapter structurally satisfies it; the LowerFn type and the blindCast are gone.
| } | ||
|
|
||
| export function renderOps(calls: readonly OpFactoryCall[]): Op[] { | ||
| export function renderOps(calls: readonly OpFactoryCall[], lower?: LowerFn): Op[] { |
Contributor
There was a problem hiding this comment.
Why is this a free-standing function? Isn't it only ever used by the planner?
Contributor
Author
There was a problem hiding this comment.
Looked into this: renderOps is a public export (@prisma-next/target-postgres/render-ops) imported directly by tests and by TypeScriptRenderablePostgresMigration — none via the planner class — so moving it onto the planner would break those consumers. Left it free-standing; happy to cut it differently if you have a shape in mind.
PR #751 review (operator + CodeRabbit): the migration op factories are the user-facing authoring API and must contain no SQL. D4 makes createTable/createSchema Migration methods that take the contract-free builder options and lower a typed DDL node through the adapter (this.createTable({ schema, table, columns: [col(...)], constraints })). Kills the string-gluing, createTableOp(sql), the LowerFn callback, and the columnDefault leak on the shared ColumnSpec; adds the Lowerer interface and the constraint-quote + literal-escape renderer fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
createTable/createSchema become instance methods on PostgresMigration
that take contract-free builder options, build a typed DDL node, and
lower through the stored SqlControlAdapter — no SQL strings in the
authoring surface. Generated migrations use:
this.createTable({ schema, table, columns: [col(...)], constraints: [primaryKey([...])] })
this.createSchema({ schema: 'public' })
Changes:
- Lowerer interface added to @prisma-next/family-sql/control-adapter
- PostgresMigration.createTable()/createSchema() instance methods added,
mirroring dataTransform(); buildCreateTableOp/buildCreateSchemaOp wired
- CreateTableCall.toOp(lowerer) builds node from structured fields and
lowers through adapter; no parseDefaultSql/SQL string round-trips
- CreateTableCall.renderTypeScript() emits this.createTable({...}) with
col() columns + structured constraints; zero SQL
- CreateSchemaCall.renderTypeScript() emits this.createSchema({schema})
- Migration exports: col/primaryKey/foreignKey/unique/lit/fn re-exported
from @prisma-next/sql-relational-core/contract-free
- DdlColumn.default typed as AnyDdlColumnDefault (concrete union, not
abstract base) to enable discriminated narrowing without casts
- buildColumnTypeSql widened to ReadonlyMap — removes 2 pre-existing
bare casts in toColumnSpec, delta=-2 for lint:casts
- ddl-renderer.ts: constraint.name quoted; refTable segments quoted;
JSON literal defaults escaped via escapeLiteral
- All committed example/app migrations regenerated to new form
- Pinning test: renderCallsToTypeScript emits this.createTable({...})
with col() columns, asserts .not.toMatch(/CREATE TABLE/)
- Round-trip test: TS scaffold → tsx execution → ops.json byte-matches
renderOps(calls)
Gates:
- grep: createTableOp/buildCreateTableSql/parseDefaultSql/LowerFn = 0
(only in CLI test helpers and project docs)
- ColumnSpec has no columnDefault field
- pnpm typecheck: all touched packages clean
- target-postgres: 299/299 passed; adapter-postgres: 566+4xfail/575
(5 failures all *.integration.test.ts requiring live Postgres)
- pnpm fixtures:check: exit 0, no regeneration
- pnpm lint:deps: clean
- pnpm lint:casts: delta=-2 (reduced, not increased)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Will Madden <madden@prisma.io>
…ityKeyFor, renderOps) F11a: add quoteQualifiedIdentifier to ddl-renderer and use it for FK refTable so schema-qualified refs like "app.users" render as "app"."users" instead of "app.users". F11b: add test asserting mixed-case constraint name (MyPK) quotes correctly and schema-qualified FK refTable (app.users) splits to "app"."users". F12: add test asserting JSON-object lit() default with embedded single quote (x'y) renders with the quote escaped via escapeLiteral, not breaking the SQL literal. F10: delete identityKeyFor() — zero callers, and calling toOp() with no lowerer now throws for CreateTableCall/CreateSchemaCall; dead code with a latent crash path. F13: renderOps stays a free function (not a planner method) because it is publicly re-exported from the target package (@prisma-next/target-postgres/render-ops) and consumed directly by tests and the TypeScriptRenderablePostgresMigration class outside the planner — inlining it into the planner class would break those call sites and make the export surface incoherent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Migration op factories are now the user-facing authoring API with no SQL: createTable/createSchema are Migration methods taking contract-free builder options and lowering a typed DDL node through the adapter. Resolves all operator + CodeRabbit comments (Lowerer interface, no string-gluing, columnDefault leak removed, constraint/refTable quoting, JSON-default escaping). Trace backstop passes (4 dispatches). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
packages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.ts (2)
211-214: ⚡ Quick winPrefer
ifDefinedfor conditional object spreads.The codebase convention is to use
ifDefined(key, value)from@prisma-next/utils/definedfor conditional properties instead of ternary-based spreads.♻️ Suggested refactor
+import { ifDefined } from '`@prisma-next/utils/defined`'; + function toDdlColumn( name: string, column: StorageColumn, codecHooks: ReadonlyMap<string, CodecControlHooks>, storageTypes: Readonly<Record<string, StorageTypeInstance | PostgresEnumStorageEntry>>, ): DdlColumn { const typeSql = buildColumnTypeSql(column, codecHooks, storageTypes); const ddlDefault = postgresDefaultToDdlColumnDefault(column.default); return contractFree.col(name, typeSql, { - ...(!column.nullable ? { notNull: true } : {}), - ...(ddlDefault ? { default: ddlDefault } : {}), + ...ifDefined('notNull', column.nullable ? undefined : true), + ...ifDefined('default', ddlDefault), }); }Based on learnings: "prefer
ifDefinedfromprisma-next/utils/definedfor conditional object spreads instead of inline ternary-based spreads."🤖 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-targets/3-targets/postgres/src/core/migrations/issue-planner.ts` around lines 211 - 214, Replace the inline ternary-based conditional spreads in the contractFree.col call with the ifDefined helper from `@prisma-next/utils/defined`: instead of spreading ...(!column.nullable ? { notNull: true } : {}) and ...(ddlDefault ? { default: ddlDefault } : {}), call ifDefined for each conditional property (e.g., ifDefined("notNull", true) and ifDefined("default", ddlDefault)) and include those results in the options object passed to contractFree.col(name, typeSql, ...), keeping name, typeSql, column.nullable and ddlDefault as the source values.Source: Learnings
264-270: 💤 Low valueConsider
ifDefinedfor the constraint name spread.Line 267 uses a ternary-based conditional spread pattern. For consistency with the codebase convention, consider using
ifDefined.♻️ Suggested refactor
+import { ifDefined } from '`@prisma-next/utils/defined`'; + const ddlConstraints: DdlTableConstraint[] | undefined = contractTable.primaryKey ? [ contractFree.primaryKey(contractTable.primaryKey.columns, { - ...(contractTable.primaryKey.name ? { name: contractTable.primaryKey.name } : {}), + ...ifDefined('name', contractTable.primaryKey.name), }), ] : undefined;Based on learnings: "prefer
ifDefinedfromprisma-next/utils/definedfor conditional object spreads."🤖 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-targets/3-targets/postgres/src/core/migrations/issue-planner.ts` around lines 264 - 270, The conditional spread for the primary key name should use the shared ifDefined helper: replace the ternary spread inside the contractFree.primaryKey call (currently using contractTable.primaryKey ? { ...(contractTable.primaryKey.name ? { name: contractTable.primaryKey.name } : {}) } ) with ifDefined(contractTable.primaryKey?.name, name => ({ name })); update the ddlConstraints assignment that constructs the DdlTableConstraint so the primary key options use ifDefined from prisma-next/utils/defined while keeping the same contractTable.primaryKey and contractFree.primaryKey usage.Source: Learnings
packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts (2)
181-194: ⚡ Quick winPrefer
ifDefinedfor conditional object spreads in constructor.Lines 191-192 use ternary-based conditional spreads. The codebase convention is to use
ifDefined(key, value)from@prisma-next/utils/defined.♻️ Suggested refactor
+import { ifDefined } from '`@prisma-next/utils/defined`'; + constructor( schemaName: string, tableName: string, columns: readonly DdlColumn[], constraints?: readonly DdlTableConstraint[], ) { super(); this.schemaName = schemaName; this.tableName = tableName; this.columns = Object.freeze([...columns]); - this.constraints = constraints ? Object.freeze([...constraints]) : undefined; + this.constraints = constraints !== undefined ? Object.freeze([...constraints]) : undefined; this.label = `Create table "${tableName}"`; this.freeze(); }Note: In this case, the ternary is checking for parameter presence rather than building a spread. However, for consistency with the repo's
ifDefinedpattern when building objects passed to DDL functions (see lines 203-207 intoOp), consider whether the pattern applies here. Actually, reviewing more carefully, line 192 is just conditionally assigning the field, which is fine. TheifDefinedpattern is specifically for object spread construction, not assignment.Based on learnings: "prefer
ifDefinedfromprisma-next/utils/definedfor conditional object spreads."Actually, wait - let me reconsider. Line 192 is a conditional assignment, not a spread. The
ifDefinedlearning applies specifically to object spreads like{ ...ifDefined('key', value) }. Line 192 is fine as-is.🤖 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-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts` around lines 181 - 194, The ternary assignment in the constructor setting this.constraints is a simple conditional assignment (constructor -> this.constraints) and not an object spread, so leave it as-is; if you need the repo convention for conditional object properties use the ifDefined helper from `@prisma-next/utils/defined` when building objects (see toOp where ...ifDefined('key', value) would apply), but do not replace the current ternary in the constructor.Source: Learnings
196-209: ⚡ Quick winPrefer
ifDefinedfor conditional object spread.Line 206 uses a ternary-based conditional spread
...(this.constraints ? { constraints: this.constraints } : {}). The codebase convention is to useifDefined(key, value).♻️ Suggested refactor
+import { ifDefined } from '`@prisma-next/utils/defined`'; + toOp(lower?: Lowerer): Op { if (lower === undefined) { throw new Error( `CreateTableCall.toOp: a DDL lowerer is required on the Postgres planner path (table "${this.tableName}"). Pass a SqlControlFamilyInstance to createPostgresMigrationPlanner.`, ); } const ddlNode = contractFreeDdl.createTable({ ...(this.schemaName !== UNBOUND_NAMESPACE_ID ? { schema: this.schemaName } : {}), table: this.tableName, columns: this.columns, - ...(this.constraints ? { constraints: this.constraints } : {}), + ...ifDefined('constraints', this.constraints), }); return buildCreateTableOp(ddlNode, lower); }Based on learnings: "prefer
ifDefinedfromprisma-next/utils/definedfor conditional object spreads."🤖 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-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts` around lines 196 - 209, Replace the ternary-based conditional spreads in CreateTableCall.toOp with the project utility ifDefined: import ifDefined from 'prisma-next/utils/defined' (or the correct module) and use it to set optional fields instead of constructs like ...(this.constraints ? { constraints: this.constraints } : {}); update the spread for schema as well (currently checking UNBOUND_NAMESPACE_ID against this.schemaName) so the ddlNode creation uses ifDefined('schema', this.schemaName) and ifDefined('constraints', this.constraints) before calling buildCreateTableOp(lower).Source: Learnings
packages/3-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.ts (1)
27-35: ⚡ Quick winPrefer the cast-free lowering pattern used in sibling test files.
Files
render-typescript.roundtrip.test.tsandop-factory-call.lowering.test.tsachieve the same lowering setup without casts by typing thelowerparameters directly asParameters<typeof testAdapter.lower>[0]and[1]. This avoids the casts on lines 31-32 and the explicitAnyQueryAst | DdlNode, LowererContext<unknown>imports on line 19.♻️ Align with the pattern in render-typescript.roundtrip.test.ts
-import type { AnyQueryAst, DdlNode, LowererContext } from '`@prisma-next/sql-relational-core/ast`'; import type { SqlSchemaIR } from '`@prisma-next/sql-schema-ir/types`'; import { createPostgresMigrationPlanner } from '`@prisma-next/target-postgres/planner`'; import { applicationDomainOf } from '`@prisma-next/test-utils`'; @@ -26,11 +25,11 @@ describe('PostgresMigrationPlanner - semantic satisfaction', () => { const testAdapter = createPostgresAdapter(); const planner = createPostgresMigrationPlanner({ - lower(ast: AnyQueryAst | DdlNode, ctx: LowererContext<unknown>) { - return testAdapter.lower( - ast as Parameters<typeof testAdapter.lower>[0], - ctx as Parameters<typeof testAdapter.lower>[1], - ); + lower( + ast: Parameters<typeof testAdapter.lower>[0], + ctx: Parameters<typeof testAdapter.lower>[1], + ) { + return testAdapter.lower(ast, ctx); }, });🤖 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-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.ts` around lines 27 - 35, The lowering callback currently uses broad types and runtime casts in the createPostgresMigrationPlanner call; replace the parameter typings to mirror the sibling tests by declaring the lower function parameters as (ast: Parameters<typeof testAdapter.lower>[0], ctx: Parameters<typeof testAdapter.lower>[1]) so you can call testAdapter.lower(ast, ctx) without casts, and remove the now-unnecessary AnyQueryAst | DdlNode and LowererContext imports; update the createPostgresMigrationPlanner invocation to use this cast-free lower signature (symbols: testAdapter, createPostgresAdapter, createPostgresMigrationPlanner, lower).
🤖 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-targets/3-targets/postgres/src/core/migrations/issue-planner.ts`:
- Around line 211-214: Replace the inline ternary-based conditional spreads in
the contractFree.col call with the ifDefined helper from
`@prisma-next/utils/defined`: instead of spreading ...(!column.nullable ? {
notNull: true } : {}) and ...(ddlDefault ? { default: ddlDefault } : {}), call
ifDefined for each conditional property (e.g., ifDefined("notNull", true) and
ifDefined("default", ddlDefault)) and include those results in the options
object passed to contractFree.col(name, typeSql, ...), keeping name, typeSql,
column.nullable and ddlDefault as the source values.
- Around line 264-270: The conditional spread for the primary key name should
use the shared ifDefined helper: replace the ternary spread inside the
contractFree.primaryKey call (currently using contractTable.primaryKey ? {
...(contractTable.primaryKey.name ? { name: contractTable.primaryKey.name } :
{}) } ) with ifDefined(contractTable.primaryKey?.name, name => ({ name }));
update the ddlConstraints assignment that constructs the DdlTableConstraint so
the primary key options use ifDefined from prisma-next/utils/defined while
keeping the same contractTable.primaryKey and contractFree.primaryKey usage.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts`:
- Around line 181-194: The ternary assignment in the constructor setting
this.constraints is a simple conditional assignment (constructor ->
this.constraints) and not an object spread, so leave it as-is; if you need the
repo convention for conditional object properties use the ifDefined helper from
`@prisma-next/utils/defined` when building objects (see toOp where
...ifDefined('key', value) would apply), but do not replace the current ternary
in the constructor.
- Around line 196-209: Replace the ternary-based conditional spreads in
CreateTableCall.toOp with the project utility ifDefined: import ifDefined from
'prisma-next/utils/defined' (or the correct module) and use it to set optional
fields instead of constructs like ...(this.constraints ? { constraints:
this.constraints } : {}); update the spread for schema as well (currently
checking UNBOUND_NAMESPACE_ID against this.schemaName) so the ddlNode creation
uses ifDefined('schema', this.schemaName) and ifDefined('constraints',
this.constraints) before calling buildCreateTableOp(lower).
In
`@packages/3-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.ts`:
- Around line 27-35: The lowering callback currently uses broad types and
runtime casts in the createPostgresMigrationPlanner call; replace the parameter
typings to mirror the sibling tests by declaring the lower function parameters
as (ast: Parameters<typeof testAdapter.lower>[0], ctx: Parameters<typeof
testAdapter.lower>[1]) so you can call testAdapter.lower(ast, ctx) without
casts, and remove the now-unnecessary AnyQueryAst | DdlNode and LowererContext
imports; update the createPostgresMigrationPlanner invocation to use this
cast-free lower signature (symbols: testAdapter, createPostgresAdapter,
createPostgresMigrationPlanner, lower).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a92748cf-782e-44dd-aef8-3306cc215b3b
⛔ Files ignored due to path filters (3)
projects/migrate-marker-ledger-to-typed-query-ast-commands/design-notes.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/slices/planner-create-table-adopts-ddl-ast/plan.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/trace.jsonlis excluded by!projects/**
📒 Files selected for processing (41)
examples/prisma-next-demo/fixtures/converging-branches/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-demo/fixtures/diamond/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-demo/fixtures/long-spine/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-demo/fixtures/multi-branch/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-demo/fixtures/showcase/migrations/app/20260601T0719_init/migration.tsexamples/prisma-next-demo/fixtures/skip-rollback/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-demo/fixtures/wide-fan/migrations/app/20260301T1000_init/migration.tsexamples/prisma-next-postgis-demo/migrations/app/20260512T1309_migration/migration.tspackages/2-sql/4-lanes/relational-core/src/ast/ddl-types.tspackages/2-sql/4-lanes/relational-core/src/contract-free/column.tspackages/2-sql/4-lanes/relational-core/src/exports/contract-free.tspackages/2-sql/9-family/src/core/control-adapter.tspackages/2-sql/9-family/src/exports/control-adapter.tspackages/3-extensions/pgvector/test/migrations/planner.behavior.test.tspackages/3-extensions/pgvector/test/migrations/planner.contract-to-schema-ir.test.tspackages/3-extensions/pgvector/test/migrations/planner.storage-types.test.tspackages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/dependencies.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/shared.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-ddl-builders.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner.tspackages/3-targets/3-targets/postgres/src/core/migrations/postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/render-ops.tspackages/3-targets/3-targets/postgres/src/exports/migration.tspackages/3-targets/3-targets/postgres/test/migrations/control-policy-planner.test.tspackages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.tspackages/3-targets/6-adapters/postgres/src/core/ddl-renderer.tspackages/3-targets/6-adapters/postgres/test/ddl-create-table-lowering.test.tspackages/3-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.construction.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.lowering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.rendering.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.codec-field-event.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.fk-config.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.reconciliation.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.referential-actions.test.tspackages/3-targets/6-adapters/postgres/test/migrations/planner.semantic-satisfaction.test.tspackages/3-targets/6-adapters/postgres/test/migrations/render-typescript.roundtrip.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/2-sql/4-lanes/relational-core/src/exports/contract-free.ts
- packages/2-sql/9-family/src/exports/control-adapter.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/shared.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/3-targets/6-adapters/postgres/test/migrations/planner.fk-config.test.ts
- packages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-produced-postgres-migration.ts
- packages/3-targets/6-adapters/postgres/test/migrations/planner.referential-actions.test.ts
- packages/3-targets/6-adapters/postgres/test/migrations/planner.reconciliation.test.ts
- packages/3-targets/6-adapters/postgres/test/ddl-create-table-lowering.test.ts
- packages/3-targets/6-adapters/postgres/test/migrations/planner.codec-field-event.test.ts
- packages/3-extensions/pgvector/test/migrations/planner.storage-types.test.ts
- packages/3-extensions/pgvector/test/migrations/planner.behavior.test.ts
- packages/3-targets/3-targets/postgres/test/migrations/control-policy-planner.test.ts
- packages/2-sql/4-lanes/relational-core/src/ast/ddl-types.ts
- packages/3-targets/6-adapters/postgres/test/migrations/op-factory-call.construction.test.ts
- packages/3-targets/6-adapters/postgres/src/core/ddl-renderer.ts
- packages/3-extensions/pgvector/test/migrations/planner.contract-to-schema-ir.test.ts
- packages/3-targets/6-adapters/postgres/test/ddl-table-constraints-lowering.test.ts
….toOp() Move Op-assembly from the free `buildCreateTableOp`/`buildCreateSchemaOp` functions into `CreateTableCall.toOp(lower)` and `CreateSchemaCall.toOp(lower)` directly. `PostgresMigration.createTable(options)` and `createSchema(options)` now construct the appropriate `*Call` and call `.toOp(this.controlAdapter)`. Delete the free functions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Per operator: the OpFactoryCall IR nodes are the common interface. buildCreateTableOp/buildCreateSchemaOp folded into CreateTableCall.toOp() / CreateSchemaCall.toOp(); PostgresMigration.createTable/createSchema build a *Call and call .toOp(this.controlAdapter). Free assembler functions deleted. No planner change; snapshots byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…ect end Once every Postgres op adopts its *Call.toOp(), the free Op-builder modules under operations/ have no callers and must be deleted (starting with operations/tables.ts, now just dropTable). No free string-gluing builder should survive the planner-adoption phase. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts (1)
204-209:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the
ifNotExistsoption through the new DDL-call IR.These new call objects can no longer represent the existing migration-surface contract:
CreateTableCalldropsifNotExistsentirely, andCreateSchemaCallnow hardcodes it totrue.PostgresMigration.createTable()/createSchema()still accept that option, so callers can ask for behavior this IR cannot emit, and round-tripping will silently rewrite it.Either thread
ifNotExiststhrough both*Callclasses and their renderers, or remove the option from the migration API so the surface matches what can actually be lowered.Also applies to: 939-940
🤖 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-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts` around lines 204 - 209, The CreateTableCall and CreateSchemaCall IRs currently lose the ifNotExists flag (see contractFreeDdl.createTable usage and CreateSchemaCall behavior), causing PostgresMigration.createTable/createSchema callers to be able to request an option that cannot be emitted; update the IR to carry ifNotExists: add an ifNotExists boolean field to CreateTableCall and CreateSchemaCall, propagate that flag where contractFreeDdl.createTable / createSchema are constructed (e.g., in op-factory-call.ts and any factory functions that build these Call objects), and update the renderer/emitters that turn these Call objects into SQL/DDL to honor the ifNotExists value (or, if you prefer the other approach, remove the ifNotExists parameter from PostgresMigration.createTable/createSchema so the public API matches the IR) so the migration surface and IR are consistent.
🤖 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.
Outside diff comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts`:
- Around line 204-209: The CreateTableCall and CreateSchemaCall IRs currently
lose the ifNotExists flag (see contractFreeDdl.createTable usage and
CreateSchemaCall behavior), causing PostgresMigration.createTable/createSchema
callers to be able to request an option that cannot be emitted; update the IR to
carry ifNotExists: add an ifNotExists boolean field to CreateTableCall and
CreateSchemaCall, propagate that flag where contractFreeDdl.createTable /
createSchema are constructed (e.g., in op-factory-call.ts and any factory
functions that build these Call objects), and update the renderer/emitters that
turn these Call objects into SQL/DDL to honor the ifNotExists value (or, if you
prefer the other approach, remove the ifNotExists parameter from
PostgresMigration.createTable/createSchema so the public API matches the IR) so
the migration surface and IR are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 46c45fd3-ec86-46c7-af15-0e05fa57c9a8
⛔ Files ignored due to path filters (2)
projects/migrate-marker-ledger-to-typed-query-ast-commands/design-notes.mdis excluded by!projects/**projects/migrate-marker-ledger-to-typed-query-ast-commands/trace.jsonlis excluded by!projects/**
📒 Files selected for processing (4)
packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/dependencies.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.tspackages/3-targets/3-targets/postgres/src/core/migrations/postgres-migration.ts
💤 Files with no reviewable changes (2)
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/dependencies.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.ts
…anup) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
requested changes
Jun 7, 2026
Comment on lines
+60
to
+66
| const lower: Lowerer = | ||
| 'lowerAst' in family | ||
| ? { | ||
| lower: (ast: AnyQueryAst | DdlNode, ctx: LowererContext<unknown>) => | ||
| family.lowerAst(ast, ctx), | ||
| } | ||
| : family; |
Contributor
Author
There was a problem hiding this comment.
Valid — this is the leftover shim from the Lowerer rework. The factory takes SqlControlFamilyInstance | Lowerer and branches on 'lowerAst' in family because the family exposes lowerAst(ast, ctx) while Lowerer is lower(ast, ctx) — different method names, so the family does not structurally satisfy Lowerer, and the shim adapts it. Cleaning it up so the factory takes a plain Lowerer and the family→Lowerer adaptation moves to the single call site that has a family.
Contributor
Author
There was a problem hiding this comment.
Followed up on this. PR #751 fixes the planner path: the family now exposes a readonly adapter member (the SqlControlAdapter, which structurally satisfies Lowerer), and createPlanner passes family.adapter directly — no more wrapping family.lowerAst. The deeper root cause (deleting lowerAst from the family interface entirely and migrating the runner marker-bootstrap callers to family.adapter.lower()) is scheduled as TML-2856, since it touches the shared SQL family interface + the merged marker/ledger runners.
…r demolition Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…mily duck-type shim) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…ot family.lowerAst) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…ct ids in code) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
reviewed
Jun 7, 2026
| * authoring surface. | ||
| */ | ||
| export class PostgresMigrationPlanner implements MigrationPlanner<'sql', 'postgres'> { | ||
| readonly #lower: Lowerer | undefined; |
Contributor
There was a problem hiding this comment.
This is incorrect, it's a lowerer. This is a holdover from the old, shitty implementation
Suggested change
| readonly #lower: Lowerer | undefined; | |
| readonly #lowerer: Lowerer | undefined; |
Contributor
Author
There was a problem hiding this comment.
Done in 83ecf67 — renamed #lower → #lowerer (and the factory/constructor param to match). It is a Lowerer, not the old family callback.
wmadden
reviewed
Jun 7, 2026
Comment on lines
+110
to
+111
| createPlanner(family: SqlControlFamilyInstance) { | ||
| return createPostgresMigrationPlanner(family.adapter); |
Contributor
There was a problem hiding this comment.
What? Just pass it the fucking adapter!
Contributor
Author
There was a problem hiding this comment.
Done in 83ecf67. The planner now takes family.adapter directly (it satisfies Lowerer) — no lowerAst wrapper.
While verifying that, the real defect surfaced: family.adapter was returning getControlAdapter(), and getControlAdapter = () => adapter.create(stack) built a fresh adapter on every call — so every family.adapter read (and every lowerAst/readMarker/verify/bootstrap call) re-instantiated the adapter. Fixed by constructing it once when the family instance is created and holding it, the way createExecutionStack already does in the runtime plane. family.adapter is now that single held instance.
The deeper move — the orchestrator (CLI) constructing the adapter once and threading the same instance to the planner, the runner bootstrap, and PostgresMigration (whose constructor still calls stack.adapter.create(stack) a second time), then dropping lowerAst/family.adapter off the family interface entirely — stays TML-2856.
Contributor
Author
There was a problem hiding this comment.
Corrected — done properly now in e15da44 (my earlier "stays TML-2856" reply was wrong). createPlanner now takes the control adapter directly across the framework SPI, the SQL descriptor SPI, and the Postgres/SQLite/Mongo impls:
createPlanner(adapter: SqlControlAdapter<'postgres'>) {
return createPostgresMigrationPlanner(adapter);
}family.adapter is removed from SqlControlFamilyInstance entirely. The adapter is built by the orchestrators that already hold the control stack and threaded down to createPlanner: the CLI control client builds it once per dbInit/dbUpdate (buildControlAdapter(), mirroring createExecutionStack) and threads it through db-init/db-update → db-run → the aggregate PlannerInput → synthStrategy → createPlanner; migration plan/migration new build it from their local stack and pass it directly. The aggregate PlannerInput/SynthStrategyInputs now carry adapter, not familyInstance.
What is left for TML-2856 is the rest of the same cleanup: the family still builds its own adapter for its non-planner methods (readMarker/verify/lowerAst/bootstrap*), and PostgresMigration builds another — those get the single threaded instance too, then getControlAdapter/lowerAst come off the family.
…apter Will flagged `family.adapter` returning `getControlAdapter()` as a red flag — and rightly: `getControlAdapter = () => adapter.create(stack)` built a fresh adapter on every call, so each `family.adapter` read (and every `lowerAst`/`readMarker`/`verify`/bootstrap call) re-instantiated the adapter. The fix is to construct it once, the way the runtime plane already does in `createExecutionStack`. - `control-instance.ts`: build `controlAdapter = adapter.create(stack)` a single time when the family instance is created and hold it; `getControlAdapter()` returns that held instance, and `family.adapter` is now a plain held value rather than a getter that re-creates. - `control.ts`: the Postgres planner takes `family.adapter` directly (it satisfies `Lowerer`) — no `lowerAst` wrapper. - `planner.ts`: the held dependency is a lowerer, not the old family callback — rename `#lower` → `#lowerer` (Will's review note). The deeper refactor — the orchestrator (CLI) constructing the adapter once and threading the *same* instance to the planner, the runner's marker bootstrap, and `PostgresMigration` (whose constructor still calls `stack.adapter.create(stack)` a second time), then dropping `lowerAst` / `family.adapter` off the family interface entirely — stays TML-2856. The plan bullet is updated to reflect what landed here vs. what remains. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
…o control Capture the architectural follow-up Will called out in PR #751 review so it isn't lost: "construct the adapter once and hold it in the family" is less bad, but the family is still the wrong owner. The execution plane already has the right shape (`createExecutionStack` builds adapter+driver once and the orchestrator threads them); the control plane should match it. The CLI control client is already the orchestrator and already builds the stack, family, and driver once — it just never constructs the adapter, so that leaked into the family (built-once-and-held after #751) and into `PostgresMigration` (a second instance). The family's adapter-backed methods already take `driver` as explicit input; they should take the adapter the same way, or move onto the adapter — the family should not retain a live adapter at all. - spec.md: new Non-goal making the control-plane stack-lifecycle boundary explicit, pointing at TML-2856. - plan.md: rewrite the parallel-group-B follow-up bullet with the full context (control-client reuse, thread the one instance, family stops owning the adapter). Linear TML-2856 rewritten to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
Will's review: the planner factory should receive the adapter directly, not the family. Done — `createPlanner` now takes the control adapter instance (which satisfies `Lowerer`) across the framework SPI, the SQL descriptor SPI, and the Postgres/SQLite/Mongo impls. `family.adapter` is removed from `SqlControlFamilyInstance` entirely. The adapter is constructed by the orchestrators that already hold the control stack, then threaded down to `createPlanner`: - The CLI control client builds the adapter once per `dbInit`/`dbUpdate` call (`buildControlAdapter()`), mirroring how the runtime plane builds the execution adapter once in `createExecutionStack`. It is not built for read-only operations (emit/verify/show/status), which never need it. It threads through the db-init/db-update operation options → `db-run` → the aggregate `PlannerInput` → `synthStrategy` → `createPlanner`. - The `migration plan` / `migration new` commands construct the adapter from their local stack (`config.adapter.create(stack)`) and pass it to `createPlanner` directly. The aggregate `PlannerInput` / `SynthStrategyInputs` now carry `adapter` instead of `familyInstance` (the family was only ever forwarded to `createPlanner`). Not in scope (still TML-2856): the SQL family still builds its OWN adapter internally for its non-planner methods (readMarker/verify/initMarker/ lowerAst/bootstrap*) via `getControlAdapter`, and `PostgresMigration` still constructs a second one. The end state threads the single orchestrator-built adapter to those too and drops lowerAst/getControlAdapter. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
The planner path of the control-plane ownership cleanup landed in this PR (createPlanner takes the orchestrator-built adapter; family.adapter gone), so the spec non-goal and the plan's TML-2856 bullet are updated to say what landed vs. what remains (runner bootstrap + PostgresMigration + dropping getControlAdapter/lowerAst from the family). Linear TML-2856 retitled and rescoped to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
reviewed
Jun 7, 2026
Comment on lines
+405
to
+412
| // Construct the control adapter once, when the family instance is built, | ||
| // and hold it — mirroring how `createExecutionStack` builds the runtime | ||
| // adapter a single time. Family-instance methods accept | ||
| // `SqlControlDriverInstance<string>` (the family API isn't generic on the | ||
| // target id); the adapter descriptor's `create` returns the concrete | ||
| // `SqlControlAdapter<TTargetId>`, and widening the target id to `string` | ||
| // here matches the family-level driver type without a per-method probe. | ||
| const controlAdapter: SqlControlAdapter<string> = adapter.create(stack); |
Contributor
There was a problem hiding this comment.
This looks like a bad idea. The adapter should be lazily created, otherwise you will affect the build/load/instantiate semantics of the whole stack wherever it's used
Contributor
Author
There was a problem hiding this comment.
Agreed, fixed in 9bc0c73. Now lazy + memoized:
let controlAdapter: SqlControlAdapter<string> | undefined;
const getControlAdapter = (): SqlControlAdapter<string> =>
(controlAdapter ??= adapter.create(stack));So building a family instance no longer instantiates the adapter (it stays side-effect-free for emit/verify/read paths that never need it), and the adapter is built at most once on first use rather than fresh per call. The family owning the adapter at all goes away in TML-2856; this is the right shape while it still does.
Will's review: building the adapter eagerly when the family instance is constructed changes the load/instantiate semantics of the whole stack — every family creation (CLI command, emit, verify, …) would instantiate the adapter, not just the migration paths that need it. Make it lazy and memoized: construct on first use, cache thereafter. This keeps family-instance creation side-effect-free while still avoiding the per-operation re-instantiation the original `() => adapter.create(stack)` caused (a fresh adapter on every call). (The family owning the adapter at all is removed in TML-2856; this is the correct interim shape while it still does.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Will Madden <madden@prisma.io>
wmadden
approved these changes
Jun 7, 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.
At a glance
The Postgres migration planner's
CREATE TABLE/CREATE SCHEMAno longer concatenate raw SQL — they build a typed DDL-AST node and lower it through the adapter at plan time, the same path the marker/ledger bootstrap already uses.What changed
CreateTablenode only modelled column-level PK/not-null/default (all the marker table needed). Real user tables carry composite primary keys, foreign keys, and table-level unique, soCreateTablegained a target-contributedconstraintssurface (PrimaryKeyConstraint/ForeignKeyConstraint/UniqueConstraint), rendered by the adapter DDL visitor.ReferentialActionis reused from@prisma-next/sql-contract(no new enum), and the 5 duplicateREFERENTIAL_ACTION_SQLmaps are consolidated to one shared@prisma-next/sql-contract/referential-action-sql.CreateTableCall.toOp()/CreateSchemaCall.toOp()build the node via the contract-freecreateTable(...)/createSchema(...)constructors and lower it to theexecutestep's SQL viaadapter.lower().buildCreateTableSqlis deleted; there is no string-build fallback.The load-bearing decision — how the planner reaches the adapter
The planner lives in the target package, but lowering lives in the adapter — and the target can't import the adapter (
adapter-postgresdepends ontarget-postgres, so the reverse is circular). Resolution: the adapter interfaceSqlControlAdapterlives in@prisma-next/family-sql(a layer below both), the migration class holds the concrete adapter, andtoOp()lowers through that interface at plan / JSON-write time — mirroring the existingdataTransform()pattern. No relocating the renderer into the target; no deferring to runtime. (Seedesign-notes.md § Planner DDL lowering mechanism.)Byte-parity — no behaviour change
The adapter DDL renderer was reconciled to the planner's canonical format (quoted identifiers, 2-space indent), so migration golden/snapshot output is byte-identical, pinned by a
toOp(lower)test (composite-PK table,CREATE SCHEMA, asequence/nextvaldefault, and the__unbound__schema → unqualified name). The generatedmigration.tsscaffold is unchanged — thecreateTable(schema, table, [columns])helper that users' migration files call keeps its column-list form; only the plan operation's executed SQL routes through the adapter. ArenderTypeScript()pinning test guards that.Scope — deliberately Postgres-only
Planner adoption spans all three targets and the full DDL vocabulary; those are tracked follow-up slices in the project (
projects/migrate-marker-ledger-to-typed-query-ast-commands/):CreateTableadoption — split out during review (the two-dialect dispatch proved too large to land cleanly as one unit; this PR's constraint node + constructor + SQLite adapter constraint-rendering substrate is reused).DropTable, SQLiteRecreateTable.Verification
git grep buildCreateTableSql→ zero; thetoOp(lower)byte-parity test + therenderTypeScript()pinning test pass; PG migration snapshot/golden tests green with no regeneration;pnpm lint:deps,pnpm lint:casts(delta 0),pnpm fixtures:checkclean;target-postgres299,target-sqlite111,adapter-sqlite178 pass.Refs: TML-2754
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests