TML-2927: typed entity reference threaded through Postgres migration ops#877
Open
wmadden-electric wants to merge 11 commits into
Open
TML-2927: typed entity reference threaded through Postgres migration ops#877wmadden-electric wants to merge 11 commits into
wmadden-electric wants to merge 11 commits into
Conversation
Introduce PostgresEntityRef (frozen-class/visitor): PostgresTableRef and PostgresColumnRef carry the live namespace node and render their own qualified identifier via quoteIdentifier. Add tableRef/columnRef factories + a polymorphic quoteTable on PostgresSchema (the unbound subclass elides the schema prefix). Thread the ref through the CREATE TABLE path end-to-end: the PostgresCreateTable AST node, the contract-free createTable builder, the adapter renderer (node.ref.qualified(), no schema/table string composition), CreateTableCall, marker bootstrap, the planner missing_table construction, and the authoring surface. Fix hydrateSqlNamespaceEntry: any unbound-id slot hydrates as PostgresUnboundSchema carrying its entries, not only the all-empty case — a non-empty unbound namespace no longer mistypes as base PostgresSchema and leaks "__unbound__"."Doc". Byte-parity preserved (fixtures:check zero-diff). The alterTable vertical, bound-schema.ts, and the ddlSchemaName fallbacks follow in the next slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Convert PostgresAlterTable, the contract-free alterTable builder, and the adapter ALTER TABLE renderer to carry/consume PostgresTableRef. Convert the Calls on this path — AddColumn, DropDefault, AddNotNullColumnDirect, AddNotNullColumnWithTempDefault — plus the operations/columns.ts and planner-recipes.ts helpers they route through, to carry the ref. Drop the boundSchema calls from these sites. Byte-parity preserved (fixtures:check zero-diff). The remaining raw-op Calls and the bound-schema.ts + ddlSchemaName-fallback deletions follow. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…schema + fallbacks Convert the last 13 table/column migration Calls (DropTable, DropColumn, AlterColumnType, Set/DropNotNull, SetDefault, primary-key/unique/foreign-key, DropConstraint, add/drop check, create/drop index) to carry a PostgresTableRef; their raw operations/*.ts helpers keep their string signatures, fed by ref.namespace.id / ref.name (the raw SQL bodies convert in their own tickets). Planner + authoring build refs from the resolved namespace node. Delete bound-schema.ts (now zero callers) and collapse the isPostgresSchema(ns) ? ns.ddlSchemaName(...) : namespaceId fallbacks in control-policy + verify-postgres-namespaces to assertions — the namespace is always a PostgresSchema after TML-2891. CreateSchemaCall (names a schema, not a table) and the RLS-policy Calls stay on strings; the policy ref is the postgres-rls project's entity kind and slots into the family later. Byte-parity preserved (fixtures:check zero-diff). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io> Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Contributor
📝 WalkthroughWalkthroughTyped PostgreSQL table references now replace schema/table string pairs across DDL construction, migration planning, SQL rendering, and namespace hydration. ChangesPostgres table reference rollout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.23.0)packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.18][ERROR]: unable to find a config; path packages/3-targets/3-targets/postgres/src/contract-free/control-bootstrap.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.51][ERROR]: unable to find a config; path packages/3-targets/3-targets/postgres/src/core/ddl/nodes.ts┌──────────────┐ �[32m✔�[39m �[1mOpengrep OSS�[0m [00.47][ERROR]: unable to find a config; path
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/extension-supabase
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/config-loader
@prisma-next/emitter
@prisma-next/language-server
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/3-targets/3-targets/postgres/src/core/migrations/postgres-migration.ts (1)
120-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated namespace/tableRef construction into a helper.
The block
postgresCreateNamespace({ id: options.schema ?? UNBOUND_NAMESPACE_ID, entries: { table: {} } }).tableRef(options.table)is duplicated verbatim across ~15 operation helpers in this file. A small private helper would cut the repetition and keep the unbound-fallback policy in one place.♻️ Example helper
private tableRefFor(options: { readonly schema?: string; readonly table: string }): PostgresTableRef { return postgresCreateNamespace({ id: options.schema ?? UNBOUND_NAMESPACE_ID, entries: { table: {} }, }).tableRef(options.table); }Then each call site becomes e.g.
new CreateTableCall(this.tableRefFor(options), options.columns, options.constraints).🤖 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/postgres-migration.ts` around lines 120 - 129, Extract the repeated postgresCreateNamespace(...).tableRef(options.table) construction into a private helper on PostgresMigration, such as a tableRefFor method, so the unbound schema fallback lives in one place. Update the existing operation helpers that build CreateTableCall and similar calls to use this helper instead of inlining the namespace/tableRef logic, keeping the helper keyed off options.schema, options.table, and UNBOUND_NAMESPACE_ID.packages/3-targets/3-targets/postgres/test/postgres-contract-serializer.test.ts (1)
577-580: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: assert the concrete
PostgresUnboundSchematype to match the test title.
toBeInstanceOf(PostgresSchema)passes for both the base class and the (pre-fix) buggy path, so it doesn't directly assert what the title claims. The real regression guard here is thequoteTable('Doc')behavioral check, which is sufficient. If you want the type assertion to actually pin the fix, import and assertPostgresUnboundSchema.♻️ Strengthen the type assertion
- expect(ns).toBeInstanceOf(PostgresSchema); + expect(ns).toBeInstanceOf(PostgresUnboundSchema);Requires adding
PostgresUnboundSchemato the../src/core/postgres-schemaimport.🤖 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/test/postgres-contract-serializer.test.ts` around lines 577 - 580, The test in postgres-contract-serializer.test.ts currently asserts the base PostgresSchema type, which does not match the title or distinguish the fixed path from the buggy one. Update the assertion around ns to use PostgresUnboundSchema from ../src/core/postgres-schema so the test explicitly verifies the unbound schema case, while keeping the existing quoteTable('Doc') behavior check as the regression guard.packages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.ts (1)
64-66: 📐 Maintainability & Code Quality | 🔵 TrivialRemove redundant guard at Line 66
The check
if (ddlName === UNBOUND_NAMESPACE_ID) continue;at Line 66 is unreachable.
- Line 64 explicitly skips
namespaceId === UNBOUND_NAMESPACE_ID.resolveDdlSchemaNamecallsnamespace.ddlSchemaName(storage), which returnsthis.idforPostgresSchema.- Only the singleton
PostgresUnboundSchemahasid === UNBOUND_NAMESPACE_ID; all other instances (guarded by Line 64) have a differentid.- No other override or fallback exists that could return
UNBOUND_NAMESPACE_IDfor a bound namespace.Remove Line 66 to eliminate dead code.
Relevant diff context
if (namespaceId === UNBOUND_NAMESPACE_ID) continue; const ddlName = resolveDdlSchemaName(contract.storage, namespaceId); - if (ddlName === UNBOUND_NAMESPACE_ID) continue; if (existing.has(ddlName)) continue;🤖 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/verify-postgres-namespaces.ts` around lines 64 - 66, The guard in verifyPostgresNamespaces is redundant because resolveDdlSchemaName cannot return UNBOUND_NAMESPACE_ID once namespaceId has already been filtered out. Remove the unreachable check on ddlName in the verifyPostgresNamespaces flow and keep the existing namespaceId === UNBOUND_NAMESPACE_ID skip as the only bound/unbound filter, using the resolveDdlSchemaName and ddlSchemaName symbols to locate the dead branch.
🤖 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/issue-planner.ts`:
- Around line 220-238: `mapIssueToCall` is still throwing inside
`tableRef(issue)` instead of surfacing planner conflicts through its `Result`
return path. Update `tableRef` to return `Result<PostgresTableRef,
SqlPlannerConflict>` (or equivalent) for the missing-table-payload and
non-Postgres-namespace cases, then handle those failures at the `mapIssueToCall`
call sites by returning `notOk(issueConflict(...))` just like the existing
`missing_table` and `missing_column` branches; use the `tableRef`,
`resolveNamespaceIdForIssue`, and `mapIssueToCall` symbols to place the fix.
---
Nitpick comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/postgres-migration.ts`:
- Around line 120-129: Extract the repeated
postgresCreateNamespace(...).tableRef(options.table) construction into a private
helper on PostgresMigration, such as a tableRefFor method, so the unbound schema
fallback lives in one place. Update the existing operation helpers that build
CreateTableCall and similar calls to use this helper instead of inlining the
namespace/tableRef logic, keeping the helper keyed off options.schema,
options.table, and UNBOUND_NAMESPACE_ID.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.ts`:
- Around line 64-66: The guard in verifyPostgresNamespaces is redundant because
resolveDdlSchemaName cannot return UNBOUND_NAMESPACE_ID once namespaceId has
already been filtered out. Remove the unreachable check on ddlName in the
verifyPostgresNamespaces flow and keep the existing namespaceId ===
UNBOUND_NAMESPACE_ID skip as the only bound/unbound filter, using the
resolveDdlSchemaName and ddlSchemaName symbols to locate the dead branch.
In
`@packages/3-targets/3-targets/postgres/test/postgres-contract-serializer.test.ts`:
- Around line 577-580: The test in postgres-contract-serializer.test.ts
currently asserts the base PostgresSchema type, which does not match the title
or distinguish the fixed path from the buggy one. Update the assertion around ns
to use PostgresUnboundSchema from ../src/core/postgres-schema so the test
explicitly verifies the unbound schema case, while keeping the existing
quoteTable('Doc') behavior check as the regression guard.
🪄 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: 9c83984e-5881-43c1-9dc2-7d9bc1a7c00d
⛔ Files ignored due to path filters (1)
projects/typed-ddl-migration-ops/slices/pg-typed-entity-ref/spec.mdis excluded by!projects/**
📒 Files selected for processing (34)
packages/3-targets/3-targets/postgres/src/contract-free/control-bootstrap.tspackages/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/entity-ref.tspackages/3-targets/3-targets/postgres/src/core/migrations/bound-schema.tspackages/3-targets/3-targets/postgres/src/core/migrations/control-policy.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/columns.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/migrations/postgres-migration.tspackages/3-targets/3-targets/postgres/src/core/migrations/verify-postgres-namespaces.tspackages/3-targets/3-targets/postgres/src/core/postgres-contract-serializer.tspackages/3-targets/3-targets/postgres/src/core/postgres-schema.tspackages/3-targets/3-targets/postgres/src/exports/types.tspackages/3-targets/3-targets/postgres/test/contract-free/ddl.test.tspackages/3-targets/3-targets/postgres/test/entity-ref.test.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.check-constraints.test.tspackages/3-targets/3-targets/postgres/test/migrations/postgres-migration-schema-required.test-d.tspackages/3-targets/3-targets/postgres/test/postgres-contract-serializer.test.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/test/ddl-add-column-lowering.test.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/lower-to-execute-request.test.tspackages/3-targets/6-adapters/postgres/test/migrations/native-array-columns.integration.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/render-typescript.roundtrip.test.tspackages/3-targets/6-adapters/postgres/test/migrations/rls-verify-extension-issues.integration.test.tstest/integration/test/cross-package/postgres-issue-planner.test.ts
💤 Files with no reviewable changes (1)
- packages/3-targets/3-targets/postgres/src/core/migrations/bound-schema.ts
Address drive-code-review findings: replace the inconsistent throw/return/silent-continue responses to a non-PostgresSchema namespace with a single loud assertPostgresSchema helper across the planner strategies (the namespace is always a PostgresSchema after TML-2891; a silent skip could under-plan a migration if that invariant broke). Tighten the non-empty-unbound hydration test to assert PostgresUnboundSchema. Document the control-policy schemaName duck-typing dependency on the still-string RLS/field-event Calls. Restore the resolveDdlSchemaForNamespace doc comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The frozen-class/visitor hierarchy (abstract PostgresEntityRef +
PostgresEntityRefVisitor + PostgresTableRef + PostgresColumnRef) was
over-built — nothing dispatched over the visitor and only the table
subclass was consumed. Collapse to one concrete PostgresEntityRef
carrying { namespace, name, parent? }: a bare ref is a table, a ref with
a parent is a nested entity (column/policy), and qualified() handles both.
Drop the unused columnRef factory and the visitor.
Byte-parity preserved (fixtures:check zero-diff).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
wmadden
requested changes
Jun 26, 2026
wmadden
left a comment
wmadden
left a comment
Contributor
There was a problem hiding this comment.
I think you're underutilizing the new entitiy ref
Comment on lines
+224
to
+238
| const tableRef = (issue: SchemaIssue): PostgresEntityRef => { | ||
| if (!('table' in issue) || typeof issue.table !== 'string') { | ||
| throw new Error(`mapIssueToCall: issue kind "${issue.kind}" carries no table`); | ||
| } | ||
| if (issue.kind === 'extra_table') { | ||
| return postgresCreateNamespace({ id: schemaName, entries: { table: {} } }).tableRef( | ||
| issue.table, | ||
| ); | ||
| } | ||
| const namespaceId = resolveNamespaceIdForIssue(issue); | ||
| const ns = ctx.toContract.storage.namespaces[namespaceId]; | ||
| if (!isPostgresSchema(ns)) { | ||
| throw new Error(`mapIssueToCall: namespace "${namespaceId}" is not a PostgresSchema`); | ||
| } | ||
| return ns.tableRef(issue.table); |
Contributor
There was a problem hiding this comment.
This logic seems way overcomplicated
Contributor
Author
There was a problem hiding this comment.
Reworked: the helper now returns Result<PostgresEntityRef, SqlPlannerConflict> (no throw inside the Result-returning planner) and the call sites propagate notOk like the existing arms. Commit d55f0cc.
Comment on lines
+265
to
+262
| if (this.schemaName !== UNBOUND_NAMESPACE_ID) { | ||
| opts.push(`schema: ${jsonToTsSource(this.schemaName)}`); | ||
| if (this.ref.namespace.id !== UNBOUND_NAMESPACE_ID) { | ||
| opts.push(`schema: ${jsonToTsSource(this.ref.namespace.id)}`); |
Contributor
There was a problem hiding this comment.
This is the kind of logic we were trying to remove by adding the entity ref. Can we remove it?
Contributor
Author
There was a problem hiding this comment.
Removed — the sentinel comparison is gone; PostgresSchema.authoredSchema() (data, overridden to undefined on the unbound subclass) now drives schema-omission, and the renderTypeScript prefix is one shared helper. Commit a5a9963.
Comment on lines
+600
to
+581
| this.label = `Set default on "${tableName}"."${columnName}"`; | ||
| this.label = `Set default on "${ref.name}"."${columnName}"`; |
Contributor
There was a problem hiding this comment.
Isn't this precisely what ref.qualified() is for?
Contributor
Author
…spec The entity ref identifies a node by its dictionary key, not a SQL table name; `name` carried a SQL bias that does not belong in a cross-target coordinate. Rename PostgresEntityRef.name -> .id, uniform with namespace.id. Pure rename, no behavior change. Also rewrite the slice spec to the corrected design from the round-2 review: a reference is a coordinate, not a renderer — rendering to SQL is the adapter`s job. (Implementation of that correction follows.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…e adapter A reference is a coordinate, not a renderer. Delete PostgresEntityRef .qualified() (the ref no longer has any methods) and delete qualifyTableName (which rebuilt a throwaway namespace from a string to re-derive the qualifier the ref already held). The adapter DDL renderers compose the qualified name from the ref`s own namespace + id; the raw operations/*.ts helpers take the ref instead of a (schema, table) string pair. Byte-parity preserved (fixtures:check zero-diff). Follow-ups (flagged, out of scope): the quoteTable (escaping) vs qualifyTable (raw) duplication on PostgresSchema, and the two remaining namespace rebuilds for string-sourced references (the FK target table and the RLS Call, neither of which carries a ref yet). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…lified name renderTypeScript no longer hand-detects the unbound namespace by comparing the framework sentinel: PostgresSchema gains an authoredSchema() data accessor (the schema name, or undefined for the unbound subclass), and the shared tableRefAuthoringOpts helper reads it. The 17 hand-rolled "table"."column" labels/messages route through one quotedPair helper. Byte-identical (fixtures:check zero-diff; labels were not fixture-pinned). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
mapIssueToCall returns a Result, but its inner table-ref resolution threw for the no-table and non-PostgresSchema cases, crashing the planning run. Replace it with resolveTableRef returning Result<PostgresEntityRef, SqlPlannerConflict>; each call site propagates notOk like the existing missing_table / missing_column arms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
packages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.ts (1)
22-30: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCarry a typed ref for the referenced FK table.
fk.referencesstill usesschema/tablestrings, so this path rehydrates a namespace only to qualifyREFERENCES. That leaves FK targets outside the new authoritative ref model; prefer storing the referenced table as a table ref and qualifying from that ref. This is the same manual re-qualification concern already raised on an earlier review.🤖 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/operations/constraints.ts` around lines 22 - 30, The foreign key rendering in renderForeignKeySql still rehydrates a namespace from fk.references.schema/table strings to build the REFERENCES target, which bypasses the new typed ref model. Update the ForeignKeySpec usage so the referenced table is stored as a table ref and render REFERENCES directly from that ref instead of calling postgresCreateNamespace(...).qualifyTable(...). Keep the rest of the SQL assembly in renderForeignKeySql unchanged while switching the referenced-target qualification to the authoritative ref field.packages/3-targets/3-targets/postgres/src/core/migrations/operations/rls.ts (1)
80-100: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThread the table ref through RLS too.
This still takes
schemaName/tableNameand recreates a namespace just to qualify the table. Prefer accepting the table ref and usingref.namespace.qualifyTable(ref.id)so RLS does not keep a separate qualification path. This appears to be the same unresolved manual-qualification concern noted in the previous review comments.🤖 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/operations/rls.ts` around lines 80 - 100, The row-level security migration still reconstructs table qualification from separate schema/table strings, which leaves a manual qualification path. Update enableRowLevelSecurity to accept the table ref and use ref.namespace.qualifyTable(ref.id) instead of postgresCreateNamespace and schemaName/tableName lookups; then thread that ref through rlsEnabledAst, targetDetails, and the precheck/execute steps so all RLS SQL uses the same source of truth.
🤖 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/issue-planner.ts`:
- Around line 225-228: The extra-table branch in issue-planner’s issue handling
is using ctx.schemaName instead of the issue’s own namespace, so dropped tables
can be planned against the wrong schema. Update the extra_table case in the
planner logic to build the table ref from issue.namespaceId (or the namespace
derived from it) before calling
postgresCreateNamespace(...).tableRef(issue.table), and keep the namespace
association consistent through the DropTableCall generation path.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/indexes.ts`:
- Line 49: The DDL identifier rendering in the indexes migration logic is using
raw `qualifyTable()` output, which can break when table or index names contain
quotes. Update the affected code in the indexes operations flow to use
`quoteTable()` instead of `ref.namespace.qualifyTable(ref.id)`, and apply the
same escaping path consistently at the other referenced spot in this module so
all DDL identifiers are safely quoted.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/shared.ts`:
- Around line 25-27: The quotedPair helper currently builds identifiers by
string interpolation, which can produce invalid or inconsistent SQL when names
contain quotes; update quotedPair to reuse the existing quoteIdentifier helper
for both parts so it matches the rest of the SQL rendering logic. Keep the
change localized to quotedPair in shared.ts and ensure the output format remains
schema-qualified using the same identifier escaping rules as quoteIdentifier.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.ts`:
- Around line 3-12: The table-scoped migration helpers are typed too broadly:
`dropTable` currently accepts `PostgresEntityRef`, which can let non-table refs
flow into table DDL generation. Narrow this operation to the table-specific ref
type used by table/column/constraint/index paths, and update `dropTable` to use
that type consistently so `schemaName`, `tableName`, and `qualifyTable(...)` are
only derived from a real table ref. Also propagate the narrower ref type through
any shared table-operation callers in this migrations module.
---
Duplicate comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.ts`:
- Around line 22-30: The foreign key rendering in renderForeignKeySql still
rehydrates a namespace from fk.references.schema/table strings to build the
REFERENCES target, which bypasses the new typed ref model. Update the
ForeignKeySpec usage so the referenced table is stored as a table ref and render
REFERENCES directly from that ref instead of calling
postgresCreateNamespace(...).qualifyTable(...). Keep the rest of the SQL
assembly in renderForeignKeySql unchanged while switching the referenced-target
qualification to the authoritative ref field.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/operations/rls.ts`:
- Around line 80-100: The row-level security migration still reconstructs table
qualification from separate schema/table strings, which leaves a manual
qualification path. Update enableRowLevelSecurity to accept the table ref and
use ref.namespace.qualifyTable(ref.id) instead of postgresCreateNamespace and
schemaName/tableName lookups; then thread that ref through rlsEnabledAst,
targetDetails, and the precheck/execute steps so all RLS SQL uses the same
source of truth.
🪄 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: 7e49a9ed-1796-4f91-ba8b-13771a3ef195
⛔ Files ignored due to path filters (1)
projects/typed-ddl-migration-ops/slices/pg-typed-entity-ref/spec.mdis excluded by!projects/**
📒 Files selected for processing (24)
packages/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/entity-ref.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/columns.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/indexes.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/rls.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-recipes.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/src/core/postgres-schema.tspackages/3-targets/3-targets/postgres/src/exports/planner-sql-checks.tspackages/3-targets/3-targets/postgres/src/exports/types.tspackages/3-targets/3-targets/postgres/test/contract-free/ddl.test.tspackages/3-targets/3-targets/postgres/test/entity-ref.test.tspackages/3-targets/3-targets/postgres/test/migrations/index-ddl.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.check-constraints.test.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/test/migrations/planner-sql-checks.test.tstest/integration/test/cross-package/postgres-issue-planner.test.ts
💤 Files with no reviewable changes (1)
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-sql-checks.ts
✅ Files skipped from review due to trivial changes (2)
- packages/3-targets/3-targets/postgres/test/migrations/planner.check-constraints.test.ts
- test/integration/test/cross-package/postgres-issue-planner.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/3-targets/3-targets/postgres/test/contract-free/ddl.test.ts
- packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts
- packages/3-targets/3-targets/postgres/src/contract-free/ddl.ts
- packages/3-targets/3-targets/postgres/src/core/ddl/nodes.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.ts
Comment on lines
+225
to
+228
| if (issue.kind === 'extra_table') { | ||
| return ok( | ||
| postgresCreateNamespace({ id: ctx.schemaName, entries: { table: {} } }).tableRef(issue.table), | ||
| ); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Preserve the issue namespace when dropping extra tables.
extra_table ignores issue.namespaceId and creates the ref from ctx.schemaName, so a table removed from another namespace can generate DropTableCall for the wrong schema.
Proposed fix
if (issue.kind === 'extra_table') {
+ const namespaceId = resolveNamespaceIdForIssue(issue);
return ok(
- postgresCreateNamespace({ id: ctx.schemaName, entries: { table: {} } }).tableRef(issue.table),
+ postgresCreateNamespace({ id: namespaceId, entries: { table: {} } }).tableRef(issue.table),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (issue.kind === 'extra_table') { | |
| return ok( | |
| postgresCreateNamespace({ id: ctx.schemaName, entries: { table: {} } }).tableRef(issue.table), | |
| ); | |
| if (issue.kind === 'extra_table') { | |
| const namespaceId = resolveNamespaceIdForIssue(issue); | |
| return ok( | |
| postgresCreateNamespace({ id: namespaceId, entries: { table: {} } }).tableRef(issue.table), | |
| ); |
🤖 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 225 - 228, The extra-table branch in issue-planner’s issue handling
is using ctx.schemaName instead of the issue’s own namespace, so dropped tables
can be planned against the wrong schema. Update the extra_table case in the
planner logic to build the table ref from issue.namespaceId (or the namespace
derived from it) before calling
postgresCreateNamespace(...).tableRef(issue.table), and keep the namespace
association consistent through the DropTableCall generation path.
Comment on lines
+25
to
+27
| export function quotedPair(a: string, b: string): string { | ||
| return `"${a}"."${b}"`; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the existing identifier quoter here.
quotedPair('a"b', 'c') currently renders an invalid/misleading identifier. Reuse quoteIdentifier so labels stay consistent with SQL rendering.
Proposed fix
+import { quoteIdentifier } from '../../sql-utils';
+
export function quotedPair(a: string, b: string): string {
- return `"${a}"."${b}"`;
+ return `${quoteIdentifier(a)}.${quoteIdentifier(b)}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function quotedPair(a: string, b: string): string { | |
| return `"${a}"."${b}"`; | |
| } | |
| import { quoteIdentifier } from '../../sql-utils'; | |
| export function quotedPair(a: string, b: string): string { | |
| return `${quoteIdentifier(a)}.${quoteIdentifier(b)}`; | |
| } |
🤖 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/operations/shared.ts`
around lines 25 - 27, The quotedPair helper currently builds identifiers by
string interpolation, which can produce invalid or inconsistent SQL when names
contain quotes; update quotedPair to reuse the existing quoteIdentifier helper
for both parts so it matches the rest of the SQL rendering logic. Keep the
change localized to quotedPair in shared.ts and ensure the output format remains
schema-qualified using the same identifier escaping rules as quoteIdentifier.
Comment on lines
+3
to
+12
| import type { PostgresEntityRef } from '../../entity-ref'; | ||
| import { type Op, step, targetDetails } from './shared'; | ||
|
|
||
| export async function dropTable( | ||
| schemaName: string, | ||
| tableName: string, | ||
| ref: PostgresEntityRef, | ||
| lowerer: ExecuteRequestLowerer, | ||
| ): Promise<Op> { | ||
| const qualified = qualifyTableName(schemaName, tableName); | ||
| const schemaName = ref.namespace.id; | ||
| const tableName = ref.id; | ||
| const qualified = ref.namespace.qualifyTable(ref.id); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Narrow table-scoped operation APIs to table refs.
dropTable only makes sense for a table, but PostgresEntityRef also allows non-table refs. If a column/entity ref reaches this path, ref.id is treated as the table name and qualifyTable(ref.id) can generate DDL for the wrong relation. Please use the table-specific ref type end-to-end for table/column/constraint/index operations.
Example direction for this operation
-import type { PostgresEntityRef } from '../../entity-ref';
+import type { PostgresTableRef } from '../../entity-ref';
export async function dropTable(
- ref: PostgresEntityRef,
+ ref: PostgresTableRef,
lowerer: ExecuteRequestLowerer,
): Promise<Op> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import type { PostgresEntityRef } from '../../entity-ref'; | |
| import { type Op, step, targetDetails } from './shared'; | |
| export async function dropTable( | |
| schemaName: string, | |
| tableName: string, | |
| ref: PostgresEntityRef, | |
| lowerer: ExecuteRequestLowerer, | |
| ): Promise<Op> { | |
| const qualified = qualifyTableName(schemaName, tableName); | |
| const schemaName = ref.namespace.id; | |
| const tableName = ref.id; | |
| const qualified = ref.namespace.qualifyTable(ref.id); | |
| import type { PostgresTableRef } from '../../entity-ref'; | |
| import { type Op, step, targetDetails } from './shared'; | |
| export async function dropTable( | |
| ref: PostgresTableRef, | |
| lowerer: ExecuteRequestLowerer, | |
| ): Promise<Op> { | |
| const schemaName = ref.namespace.id; | |
| const tableName = ref.id; | |
| const qualified = ref.namespace.qualifyTable(ref.id); |
🤖 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/operations/tables.ts`
around lines 3 - 12, The table-scoped migration helpers are typed too broadly:
`dropTable` currently accepts `PostgresEntityRef`, which can let non-table refs
flow into table DDL generation. Narrow this operation to the table-specific ref
type used by table/column/constraint/index paths, and update `dropTable` to use
that type consistently so `schemaName`, `tableName`, and `qualifyTable(...)` are
only derived from a real table ref. Also propagate the narrower ref type through
any shared table-operation callers in this migrations module.
Four expect.objectContaining({ name }) matchers on ref were missed by the
rename (runtime matchers, invisible to typecheck; the integration-tests
package was not run in that pass).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
wmadden
requested changes
Jun 27, 2026
| export async function alterColumnType( | ||
| schemaName: string, | ||
| tableName: string, | ||
| ref: PostgresEntityRef, |
Contributor
There was a problem hiding this comment.
You've used ref over and over in this diff and nobody who reads it will know wtf it refers to! table: PostgresEntityRef
Contributor
Author
There was a problem hiding this comment.
Done — renamed every PostgresEntityRef binding (field/param/local) from ref to table, since each identifies a table. Commit eeaba89.
… table) Every PostgresEntityRef in the migration pipeline identifies a table; naming the field/param/local `ref` told the reader nothing. Rename the bindings to `table` (the type stays PostgresEntityRef). Pure rename. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-targets/3-targets/postgres/src/core/ddl/nodes.ts`:
- Around line 92-105: Narrow the table-only DDL node types from
PostgresEntityRef to PostgresTableRef so create-table and alter-table cannot
accept non-table refs at compile time. Update the relevant node
classes/constructors in nodes.ts and the related DDL contract usage so the table
property and constructor option use PostgresTableRef consistently, preserving
the typed ref split introduced by this PR.
🪄 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: 1a0ab23e-d8e6-4759-8fc0-d3b196239ec0
📒 Files selected for processing (19)
packages/3-targets/3-targets/postgres/src/contract-free/control-bootstrap.tspackages/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/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/indexes.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.tspackages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.tspackages/3-targets/3-targets/postgres/test/contract-free/ddl.test.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.check-constraints.test.tspackages/3-targets/6-adapters/postgres/src/core/control-adapter.tspackages/3-targets/6-adapters/postgres/test/ddl-add-column-lowering.test.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/lower-to-execute-request.test.tstest/integration/test/cross-package/postgres-issue-planner.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- packages/3-targets/3-targets/postgres/test/contract-free/ddl.test.ts
- packages/3-targets/6-adapters/postgres/src/core/control-adapter.ts
- packages/3-targets/3-targets/postgres/src/contract-free/control-bootstrap.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/indexes.ts
- packages/3-targets/3-targets/postgres/test/migrations/planner.check-constraints.test.ts
- packages/3-targets/6-adapters/postgres/test/ddl-add-column-lowering.test.ts
- test/integration/test/cross-package/postgres-issue-planner.test.ts
- packages/3-targets/6-adapters/postgres/test/ddl-create-table-lowering.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/tables.ts
- packages/3-targets/6-adapters/postgres/test/lower-to-execute-request.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/constraints.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-recipes.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/operations/columns.ts
- packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.ts
Comment on lines
+92
to
105
| readonly table: PostgresEntityRef; | ||
| readonly ifNotExists: boolean | undefined; | ||
| readonly columns: ReadonlyArray<DdlColumn>; | ||
| readonly constraints: ReadonlyArray<DdlTableConstraint> | undefined; | ||
|
|
||
| constructor(options: { | ||
| readonly table: string; | ||
| readonly schema?: string; | ||
| readonly table: PostgresEntityRef; | ||
| readonly ifNotExists?: boolean; | ||
| readonly columns: readonly DdlColumn[]; | ||
| readonly constraints?: readonly DdlTableConstraint[]; | ||
| }) { | ||
| super(); | ||
| this.table = options.table; | ||
| this.schema = options.schema; | ||
| this.ifNotExists = options.ifNotExists; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Narrow these table-only nodes to PostgresTableRef.
PostgresEntityRef is broader than the contract here, so create-table/alter-table can now accept non-table refs at compile time. That weakens the typed ref split this PR is introducing and propagates the looser type into contract-free/ddl.ts as well.
Suggested change
- readonly table: PostgresEntityRef;
+ readonly table: PostgresTableRef;
...
- readonly table: PostgresEntityRef;
+ readonly table: PostgresTableRef;
...
- readonly table: PostgresEntityRef;
+ readonly table: PostgresTableRef;
...
- readonly table: PostgresEntityRef;
+ readonly table: PostgresTableRef;Also applies to: 135-143
🤖 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/ddl/nodes.ts` around lines 92
- 105, Narrow the table-only DDL node types from PostgresEntityRef to
PostgresTableRef so create-table and alter-table cannot accept non-table refs at
compile time. Update the relevant node classes/constructors in nodes.ts and the
related DDL contract usage so the table property and constructor option use
PostgresTableRef consistently, preserving the typed ref split introduced by this
PR.
wmadden
requested changes
Jun 28, 2026
| lowerer: ExecuteRequestLowerer, | ||
| ): Promise<Op> { | ||
| const qualified = qualifyTableName(schemaName, tableName); | ||
| const schemaName = table.namespace.id; |
Contributor
There was a problem hiding this comment.
This looks suspicious. namespace.id will return __unbound__ for the unbound ns. The whole point of the ref was to avoid reading the namespace directly
Comment on lines
+23
to
+26
| const referencesQualified = postgresCreateNamespace({ | ||
| id: fk.references.schema, | ||
| entries: { table: {} }, | ||
| }).qualifyTable(fk.references.table); |
Contributor
There was a problem hiding this comment.
This is stupid. There's no point constructing a ref from a schema name just to qualify a table name
| }); | ||
| } | ||
|
|
||
| function resolveTableRef( |
Contributor
There was a problem hiding this comment.
This is really stupid. The SchemaIssues are derived from SchemaIR. Their table/schema name is flattened off the schema IR, and now here we're rebuilding it.
Comment on lines
+120
to
+123
| const ns = postgresCreateNamespace({ | ||
| id: options.schema ?? UNBOUND_NAMESPACE_ID, | ||
| entries: { table: {} }, | ||
| }); |
Contributor
There was a problem hiding this comment.
This is stupid for the same reasons
Comment on lines
+112
to
+120
| /** | ||
| * The schema name as it appears in an authoring `{ schema: … }` option | ||
| * — the string a user would write to address this namespace. Named schemas | ||
| * return their id; the unbound-schema singleton overrides this to return | ||
| * `undefined` so callers omit the `schema` option entirely. | ||
| */ | ||
| authoredSchema(): string | undefined { | ||
| return this.id; | ||
| } |
Contributor
There was a problem hiding this comment.
I don't like this. A namespace doesn't have an "authored schema", this is a gross conflation of concerns. Justify why we need this because it looks to me like we already do this with just id. It's exclusively an authoring-side concern, how to map un-namespaced entities to hte unbound namespace, not a structural concern of the schema IR.
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.
Linked issue
Refs TML-2927. Builds on TML-2891 (#864), which made
SqlNamespaceabstract andcreateNamespacerequired — that landing is what lets the planner treat the namespace node as authoritative here.At a glance
The adapter renderer used to re-derive the qualified table name from two strings. Now it asks the reference to render itself:
node.refis aPostgresTableRefcarrying the live namespace node;qualified()is polymorphic over bound vs. unbound — a named schema renders"public"."user", the search-path-resolved unbound namespace renders"user". The renderer no longer knows or cares which.Decision
Replace the
(schemaName: string, tableName: string)pair that every Postgres migration*Callcarried with a typed entity reference threaded end to end. The reference holds the live namespace node and renders its own qualified identifier, so the qualified-vs-unqualified decision lives in exactly one place (the node's polymorphism) and is reached through the reference rather than re-derived from strings at each site.Three things land together:
PostgresEntityRef(frozen-class/visitor), withPostgresTableRefandPostgresColumnRef— designed so the non-table kinds (CREATE EXTENSION in TML-2920, RLS policies in the postgres-rls project) slot in without reopening the base.*CallIR, the contract-freecreateTable/alterTablebuilders, thePostgresCreateTable/PostgresAlterTableAST nodes, the adapter renderer, the planner construction sites, the authoring surface, and the marker/ledger bootstrap.bound-schema.ts(the sentinel→undefinedmapping) and theisPostgresSchema(ns) ? ns.ddlSchemaName(storage) : namespaceIdfallbacks incontrol-policy.tsandverify-postgres-namespaces.ts.A latent hydration bug surfaced and is fixed as part of making the node authoritative (see below).
How it fits together
The reference carries the node, not a resolved string.
PostgresTableRefholds{ namespace: PostgresSchema, name }and delegatesqualified()to the namespace node's existing bound/unbound polymorphism, reusingquoteIdentifierso the rendered bytes are identical to the old composition.PostgresSchema.tableRef(name)/columnRef(table, column)build them. See entity-ref.ts and thequoteTable/factory additions in postgres-schema.ts.The planner resolves the node and builds the reference. Because TML-2891 made
createNamespacerequired,storage.namespaces[id]is always a realPostgresSchemaon the Postgres path, so the planner asserts that and callstableRef(...)instead of resolving a schema-name string through the old fallback. See issue-planner.ts and planner-strategies.ts.The builders and AST nodes carry the reference; the renderer renders it.
createTable/alterTable(ddl.ts) andPostgresCreateTable/PostgresAlterTable(nodes.ts) hold aPostgresTableRef; the adapter asksref.qualified(). Op ids, targets, and catalog-existence checks still use the raw namespace id / table name, derived fromref.namespace.id/ref.name, so those bytes are unchanged.The dead string machinery is removed. With every table
*Callcarrying a reference,bound-schema.tshas no callers and is deleted, and the twoddlSchemaNamefallbacks collapse to assertions — the: namespaceIdbranch was only ever reachable by a non-PostgresSchemanamespace, which can no longer occur.The non-empty-unbound hydration bug is fixed. A non-empty unbound namespace (the common single-namespace app) previously hydrated as the base
PostgresSchemacarrying the sentinel id, whosequalifyTablewould emit"__unbound__"."Doc". The old planner fallback masked it; with the node authoritative it would leak.hydrateSqlNamespaceEntrynow hydrates any unbound-id slot asPostgresUnboundSchemacarrying its entries. See postgres-contract-serializer.ts.Behavior changes & evidence
*Callcarries aPostgresTableRefinstead of(schemaName, tableName). op-factory-call.ts. Evidence: op-factory-call.lowering.test.ts, op-factory-call.rendering.test.ts.PostgresEntityRef/PostgresTableRef/PostgresColumnRefrender qualified identifiers polymorphically over bound/unbound. entity-ref.ts. Evidence: entity-ref.test.ts.PostgresUnboundSchemaand renders unqualified DDL (no"__unbound__"leak). postgres-contract-serializer.ts. Evidence: postgres-contract-serializer.test.ts.bound-schema.tsand theddlSchemaNamestring fallbacks are gone; resolution is node-authoritative. control-policy.ts, verify-postgres-namespaces.ts. Evidence: byte-parity viafixtures:check(below).Reviewer notes
*Callswaps two string fields for onerefand feeds the unchanged rawoperations/*.tshelpers viaref.namespace.id/ref.name. Spot-check that the raw SQL bodies inoperations/*.tsare untouched (they convert to typed DDL in their own tickets, not here).qualified()reusesquoteIdentifier, reproducing the old"schema"."table"composition exactly;fixtures:checkis zero-diff. The one subtlety:qualified()deliberately does not delegate to the node's lighterqualifyTable(raw"${id}"interpolation) — it appliesquoteIdentifieritself so embedded-quote escaping is preserved.PostgresColumnRefis built and tested but not yet threaded into an op. No current migration op needs a fully-qualified column literal (columns are named bare inside ALTER TABLE), so it ships as the family member the ticket asked us to design for. Easy to drop if you'd rather it land with its first consumer.CreateSchemaCalland the three RLS-policy Calls intentionally keep aschemaNamestring. A CREATE SCHEMA names the namespace itself (not a table-in-a-namespace), and the policy reference is the postgres-rls project's entity kind that slots into this family later.createNamespacerequired it resolved the namespace-materialization gap centrally, so this branch was re-based onto it and a redundant test-corpus migration was dropped.Verification
Run on the final HEAD:
pnpm fixtures:check— zero-diff (byte-parity; the hard gate for migration-op changes).pnpm typecheck(full repo) — clean.pnpm lint:deps,pnpm lint:casts(delta 0),pnpm lint:rules:symlinks,pnpm check:upgrade-coverage --mode pr— clean.pnpm --filter @prisma-next/target-postgres test(478) andpnpm --filter @prisma-next/adapter-postgres test(659 + 4 expected-fail) — pass.pnpm test:e2e(109) — pass.pnpm test:integration— all cases pass; observed pre-existing PGlite connection-teardown flakes at the file level (the affected tests pass on isolated re-run), unrelated to this change.@prisma-next/config-loader's path-sensitivec12tests fail under the.claude/worktrees/path. The package is byte-identical tomain(this diff is postgres-only) and passed TML-2891: delete the SQL family placeholder namespace; SqlNamespace is now abstract #864's merge-queue CI, so it is green on a clean checkout.Alternatives considered
(schema, table)strings and add a polymorphicPostgresSchema.qualify(name)accessor. Tried and reverted during TML-2919 (TML-2919: convert not-null temp-default recipe to typed DDL; delete buildAddColumnSql #840): it left string-typed references end to end and reconstituted the schema node inside*Call.toOp()just to callqualify()once — a half-step. The reference owning its own render supersedes it.(namespace, entityKind, identifier); a table-only reference would re-narrow an already-general model and force a second type when CREATE EXTENSION and RLS policies arrive. The family is general (frozen-class/visitor over entity kind) with table + column implemented now.Checklist
TML-NNNN: …conventionfixtures:check) verifiedSummary by CodeRabbit