feat(target-postgres): flip migration plan to class-flow#364
Merged
Conversation
|
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 pull request introduces class-flow migration authoring for PostgreSQL by adding data-transform operation support, contract validation, and restructuring the migration planning pipeline from descriptor-based to issue-planner-based approaches. It includes new error types, enhanced import rendering, refactored I/O utilities, and comprehensive test coverage including end-to-end CLI journeys. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The review scope is substantial with interdependent architectural changes across multiple packages (error handling, command line tools, migration rendering, Postgres target, SQL family). Key areas demanding attention include: the complete descriptor-planner removal and issue-planner implementation with strategy patterns, the dataTransform operation redesign with runtime contract validation, the new class-flow migration base classes and emit infrastructure, the restructured planner control flow, and the extensive E2E test coverage with real database interactions. Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/middleware-telemetry
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/runtime-executor
@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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts (1)
20-21: Usenode:pathin this integration test.This file lives under
test/integration/test/**, where the suite convention is to use Node's path helpers. Swappingpatheback tonode:pathwill keep it consistent with the other CLI journey tests.Based on learnings, in prisma/prisma-next, for tests under
test/integration/test/"prefer using node:path for path operations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts` around lines 20 - 21, The import currently pulls join from 'pathe'—replace that with Node's built-in path module by importing join from 'node:path' so the test under test/integration/test/ follows the suite convention; update the import statement that includes join (alongside readdirSync/readFileSync/writeFileSync) to use 'node:path' instead of 'pathe'.test/integration/test/cli-journeys/data-transform.e2e.test.ts (1)
19-20: Usenode:pathin this integration test.Under
test/integration/test/**, these journey tests standardize on Node's path helpers rather thanpathe. Please switchjoinhere to keep the file aligned with the rest of the integration suite.Based on learnings, in prisma/prisma-next, for tests under
test/integration/test/"prefer using node:path for path operations."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform.e2e.test.ts` around lines 19 - 20, The test currently imports join from 'pathe' instead of Node's path module; change the import to get join from 'node:path' (i.e., replace "import { join } from 'pathe'" with "import { join } from 'node:path'") and ensure all uses of join in this file remain unchanged so the integration test follows the suite convention; reference symbol: join (alongside the existing readdirSync/readFileSync/writeFileSync imports) to locate the import line to update.packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts (1)
165-168:SAFE_WIDENINGSis duplicated.The same
SAFE_WIDENINGSset is defined twice:
- Line 165: inside
typeChangeStrategy(descriptor flow)- Line 366: module-level for
typeChangeCallStrategy(call flow)Extract to a single shared constant to avoid drift between the two flows.
Suggested refactor
+// Shared between descriptor and call-flow strategies +const SAFE_WIDENINGS = new Set(['int2→int4', 'int2→int8', 'int4→int8', 'float4→float8']); + export const typeChangeStrategy: MigrationStrategy = (issues, ctx) => { const matched: SchemaIssue[] = []; const ops: PostgresMigrationOpDescriptor[] = []; - const SAFE_WIDENINGS = new Set(['int2→int4', 'int2→int8', 'int4→int8', 'float4→float8']); function isSafeWidening(fromType: string, toType: string): boolean { return SAFE_WIDENINGS.has(`${fromType}→${toType}`); } // ... }; -const SAFE_WIDENINGS = new Set(['int2→int4', 'int2→int8', 'int4→int8', 'float4→float8']); - export const typeChangeCallStrategy: CallMigrationStrategy = (issues, ctx) => { // ... };Also applies to: 366-366
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts` around lines 165 - 168, The SAFE_WIDENINGS set is duplicated; extract it into a single module-level constant and have both typeChangeStrategy and typeChangeCallStrategy call a shared isSafeWidening helper that references that constant. Remove the local SAFE_WIDENINGS and local isSafeWidening inside typeChangeStrategy, add a single exported or file-scoped SAFE_WIDENINGS and isSafeWidening (unique symbols: SAFE_WIDENINGS, isSafeWidening) and update both typeChangeStrategy and typeChangeCallStrategy to use the shared helper so the widening rules stay in one place.test/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.ts (1)
20-20: Prefernode:pathoverpathein integration test files.Based on learnings, tests under
test/integration/test/should usenode:pathfor path operations.Suggested fix
-import { join } from 'pathe'; +import { join } from 'node:path';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.ts` at line 20, Replace the import of join from the third-party "pathe" module with the Node builtin by changing the import to use "node:path" (i.e., update the import statement that currently reads import { join } from 'pathe' to import { join } from 'node:path') so all uses of join in this test file (e.g., references to join in data-transform-not-null-backfill.e2e.test.ts) use the Node path API instead of pathe.packages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.ts (1)
52-58: Module-level singleton may complicate testing.The
adapterSingletonpattern creates a module-scoped mutable state. While this avoids repeated adapter instantiation, it could cause issues:
- Test isolation: state persists across test runs in the same process
- Concurrent usage: not thread-safe if called from multiple async contexts
Consider accepting the adapter as an optional parameter with lazy default, or document that this is intentional for single-threaded CLI execution contexts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.ts` around lines 52 - 58, The module-level adapterSingleton creates persistent mutable state (adapterSingleton) accessed via getAdapter() which calls createPostgresAdapter(), causing test isolation and concurrency issues; change getAdapter to accept an optional adapter parameter (e.g., getAdapter(adapter?: ReturnType<typeof createPostgresAdapter>)) and if none is provided lazily create one via createPostgresAdapter(), or alternatively export a resetAdapter() helper to clear adapterSingleton for tests and document that adapterSingleton is intended only for single-threaded CLI usage; update references to getAdapter() to pass an injected adapter in tests or call resetAdapter() in test teardown.test/integration/test/cli-journeys/class-flow-round-trip.e2e.test.ts (1)
29-29: Prefernode:pathoverpathein integration test files.Based on learnings, tests under
test/integration/test/should usenode:pathfor path operations.Suggested fix
-import { join } from 'pathe'; +import { join } from 'node:path';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/class-flow-round-trip.e2e.test.ts` at line 29, Replace the pathe import with the Node built-in path module: change the import that currently reads "import { join } from 'pathe';" to use "node:path" instead and ensure any uses of join in this test file (e.g., within class-flow-round-trip.e2e.test.ts) reference the imported join from node:path; no other logic changes are needed — just swap the module source to node:path so tests under the integration test folder use the Node path API.test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts (1)
21-21: Prefernode:pathoverpathein integration test files.Based on learnings, tests under
test/integration/test/(including cli-journeys) should prefer usingnode:pathfor path operations.Suggested fix
-import { join } from 'pathe'; +import { join } from 'node:path';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts` at line 21, In the integration test file, replace the path utility import so it uses the Node built-in path module: change the import of the join symbol that currently comes from "pathe" to instead import join from the Node core "node:path" module (update the top-level import statement that imports join).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@examples/prisma-next-demo/prisma/migrations/20260421T1000_backfill_user_kind/migration.ts`:
- Around line 7-11: The comment describing the `check` closure is incorrect: it
claims the closure "surfaces the count of rows" but the implementation performs
an existence probe via select('id').limit(1); update the comment to state that
`check` does an existence check (returns whether at least one row would be
updated) and keep the description of `run` as the mutating closure, or
alternatively change the `check` implementation to return an actual count if you
intended a count-based check—refer to the `check` closure and its
select('id').limit(1) call (and the `run` closure) when making the fix.
In `@packages/1-framework/3-tooling/migration/src/io.ts`:
- Around line 88-95: The copyFilesWithRename function currently joins destDir
with file.destName without validation, allowing directory traversal (e.g.,
"../outside.json"); update copyFilesWithRename to validate each file.destName
before copying: ensure destName is not absolute, does not contain path
separators or ".." segments (for example by comparing destName to
path.basename(destName) and checking it equals the original and that
path.isAbsolute(destName) is false), and throw an error for invalid names; only
call copyFile(join(destDir, destName)) when the validation passes to guarantee
writes remain inside destDir.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.ts`:
- Around line 871-893: The enum creation currently passes the contract storage
key (issue.typeName) into CreateEnumTypeCall which becomes the SQL type name;
change this to use the Postgres native name from the resolved type instance:
locate the 'type_missing' case, find the typeInstance variable and where
CreateEnumTypeCall(schemaName, issue.typeName, values) is returned, and replace
the second argument with typeInstance.nativeType so CreateEnumTypeCall receives
the DB-native type name (keeping values extraction as-is).
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 278-294: planIssues is being called without the enforced policy so
operations from the issue-planner branch can bypass policy checks; update the
planIssues(...) invocation to include the caller's policy (pass options.policy)
or otherwise filter issuePlanResult.value.calls by the same policy before adding
them to calls, ensuring the final calls array (built from
buildDatabaseDependencyOperations(...), storageTypePlan.operations and
issuePlanResult.value.calls) respects options.policy; preserve existing
plannerFailure handling for non-ok results.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/postgres-emit.ts`:
- Around line 49-56: The import of the migration module is cached by URL, so
re-importing the same filePath yields stale modules; update the import to add a
cache-busting timestamp query param to the file URL (use the existing
pathToFileURL(filePath).href and append something like ?t=${Date.now()}) and
import that generated URL (keep the same cast to { default?: unknown }); update
the symbol that does the import in postgres-emit.ts (the const fileUrl and
subsequent import(...) usage) so each call uses a fresh URL.
In
`@packages/3-targets/3-targets/postgres/test/migrations/data-transform.test.ts`:
- Line 6: The test suite calls beforeEachTest() directly inside describe which
runs it once at module load and causes lowerSqlPlanMock state to be shared
across tests; change the invocation to register beforeEachTest as a Vitest
beforeEach hook (e.g., call beforeEach(beforeEachTest) or export beforeEachTest
so it can be passed to beforeEach) so it runs before every test, and replace the
manual try/catch assertion block that checks for errors (the block around lines
referencing the error case) with Vitest's expect(...).toThrow() (wrap the
function call that should throw in a callback passed to expect and assert
toThrow with the expected message or error type) to simplify and standardize the
error assertion.
In
`@packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts`:
- Around line 123-143: The test file has grown past the 500-line guideline and
mixes separate concerns; split the single large test file into multiple focused
files (e.g., tests for DataTransformCall behavior including renderTypeScript and
importRequirements, separate tests for renderOps, and separate tests for
TypeScriptRenderablePostgresMigration) so each file stays under 500 lines; move
the DataTransformCall spec block (covering DataTransformCall, renderTypeScript,
importRequirements) into its own test file, move renderOps-related tests into
another, and move TypeScriptRenderablePostgresMigration tests into a third,
updating imports/exports as needed so symbols like DataTransformCall,
renderTypeScript, importRequirements, renderOps, and
TypeScriptRenderablePostgresMigration are imported from the original modules in
each new file.
---
Nitpick comments:
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.ts`:
- Around line 52-58: The module-level adapterSingleton creates persistent
mutable state (adapterSingleton) accessed via getAdapter() which calls
createPostgresAdapter(), causing test isolation and concurrency issues; change
getAdapter to accept an optional adapter parameter (e.g., getAdapter(adapter?:
ReturnType<typeof createPostgresAdapter>)) and if none is provided lazily create
one via createPostgresAdapter(), or alternatively export a resetAdapter() helper
to clear adapterSingleton for tests and document that adapterSingleton is
intended only for single-threaded CLI usage; update references to getAdapter()
to pass an injected adapter in tests or call resetAdapter() in test teardown.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts`:
- Around line 165-168: The SAFE_WIDENINGS set is duplicated; extract it into a
single module-level constant and have both typeChangeStrategy and
typeChangeCallStrategy call a shared isSafeWidening helper that references that
constant. Remove the local SAFE_WIDENINGS and local isSafeWidening inside
typeChangeStrategy, add a single exported or file-scoped SAFE_WIDENINGS and
isSafeWidening (unique symbols: SAFE_WIDENINGS, isSafeWidening) and update both
typeChangeStrategy and typeChangeCallStrategy to use the shared helper so the
widening rules stay in one place.
In `@test/integration/test/cli-journeys/class-flow-round-trip.e2e.test.ts`:
- Line 29: Replace the pathe import with the Node built-in path module: change
the import that currently reads "import { join } from 'pathe';" to use
"node:path" instead and ensure any uses of join in this test file (e.g., within
class-flow-round-trip.e2e.test.ts) reference the imported join from node:path;
no other logic changes are needed — just swap the module source to node:path so
tests under the integration test folder use the Node path API.
In
`@test/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.ts`:
- Line 20: Replace the import of join from the third-party "pathe" module with
the Node builtin by changing the import to use "node:path" (i.e., update the
import statement that currently reads import { join } from 'pathe' to import {
join } from 'node:path') so all uses of join in this test file (e.g., references
to join in data-transform-not-null-backfill.e2e.test.ts) use the Node path API
instead of pathe.
In
`@test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts`:
- Around line 20-21: The import currently pulls join from 'pathe'—replace that
with Node's built-in path module by importing join from 'node:path' so the test
under test/integration/test/ follows the suite convention; update the import
statement that includes join (alongside readdirSync/readFileSync/writeFileSync)
to use 'node:path' instead of 'pathe'.
In `@test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts`:
- Line 21: In the integration test file, replace the path utility import so it
uses the Node built-in path module: change the import of the join symbol that
currently comes from "pathe" to instead import join from the Node core
"node:path" module (update the top-level import statement that imports join).
In `@test/integration/test/cli-journeys/data-transform.e2e.test.ts`:
- Around line 19-20: The test currently imports join from 'pathe' instead of
Node's path module; change the import to get join from 'node:path' (i.e.,
replace "import { join } from 'pathe'" with "import { join } from 'node:path'")
and ensure all uses of join in this file remain unchanged so the integration
test follows the suite convention; reference symbol: join (alongside the
existing readdirSync/readFileSync/writeFileSync imports) to locate the import
line to update.
🪄 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: ffefbeed-7256-4906-af8a-9db9f0ca3c45
⛔ Files ignored due to path filters (3)
projects/postgres-class-flow-migrations/pr-plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/dataTransform-class-flow-design.mdis excluded by!projects/**projects/unify-migration-operations/spec.mdis excluded by!projects/**
📒 Files selected for processing (48)
examples/prisma-next-demo/prisma/migrations/20260421T1000_backfill_user_kind/README.mdexamples/prisma-next-demo/prisma/migrations/20260421T1000_backfill_user_kind/migration.tspackages/1-framework/1-core/errors/src/exports/migration.tspackages/1-framework/1-core/errors/src/migration.tspackages/1-framework/1-core/errors/test/migration.test.tspackages/1-framework/1-core/framework-components/src/control-migration-types.tspackages/1-framework/1-core/ts-render/src/index.tspackages/1-framework/1-core/ts-render/src/render-imports.tspackages/1-framework/1-core/ts-render/src/ts-expression.tspackages/1-framework/1-core/ts-render/test/render-imports.test.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/migration/src/exports/io.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/test/io.test.tspackages/3-mongo-target/1-mongo-target/src/core/render-typescript.tspackages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.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/operation-resolver.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.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/postgres-emit.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/migration.tspackages/3-targets/3-targets/postgres/test/migrations/data-transform.test.tspackages/3-targets/3-targets/postgres/test/migrations/descriptor-planner.scenarios.test.tspackages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/operation-resolver.integration.test.tspackages/3-targets/3-targets/postgres/test/migrations/postgres-emit.test.tstest/integration/test/cli-journeys/class-flow-round-trip.e2e.test.tstest/integration/test/cli-journeys/data-transform-enum-rebuild.e2e.test.tstest/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.tstest/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.tstest/integration/test/cli-journeys/data-transform-type-change.e2e.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-additive-required-name.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-nullable-name-required.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-nullable-name.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-status-enum-shrunk.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-status-enum.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-typechange-int.tstest/integration/test/fixtures/cli/cli-e2e-test-app/fixtures/cli-journeys/contract-typechange-text.tstest/integration/test/utils/journey-test-helpers.ts
💤 Files with no reviewable changes (1)
- packages/3-targets/3-targets/postgres/src/core/migrations/descriptor-planner.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts (1)
69-84:⚠️ Potential issue | 🟠 MajorUse resolved type metadata when rendering defaults for
typeRefcolumns.Line 83 still passes the original
columntorenderDefault, even though Lines 69-76 switch emittednativeTypeto resolved metadata. This can produce defaults rendered from stale type metadata.🔧 Proposed fix
const resolved = resolveColumnTypeMetadata(column, storageTypes); + const resolvedColumn: StorageColumn = { + ...column, + ...resolved, + }; const nativeType = expandNativeType ? expandNativeType({ nativeType: resolved.nativeType, codecId: resolved.codecId, ...ifDefined('typeParams', resolved.typeParams), }) : resolved.nativeType; return { name, nativeType, nullable: column.nullable, ...ifDefined( 'default', - column.default != null && renderDefault ? renderDefault(column.default, column) : undefined, + column.default != null && renderDefault + ? renderDefault(column.default, resolvedColumn) + : undefined, ), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts` around lines 69 - 84, The default renderer is still called with the original column object, which can be stale after resolving type metadata; update the call that builds the returned object in contract-to-schema-ir.ts so that when invoking renderDefault you pass the resolved metadata object (the `resolved` variable) instead of `column` — ensure `renderDefault(column.default, resolved)` is used where defaults are rendered so the rendered default uses the resolved nativeType/codecId/typeParams rather than the original `column` metadata.packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts (1)
179-192:⚠️ Potential issue | 🟠 MajorRemove blind casts and validate
fromContractat the boundary.The
plan()method accepts framework-levelunknowninputs but immediately casts to SQL-specific types without validation (line 189:options as SqlMigrationPlannerPlanOptions; line 191:(options.fromContract ?? null) as Contract<SqlStorage> | null). Downstream code then dereferences.contract.storage.types,.storage.storageHash,.storage.tables, and.schema.dependenciesassuming SQL-specific structure. A malformed caller will fail at property access instead of returning a structured planner error.Either use type predicates to safely narrow the inputs, or fix the type surface to accept properly-typed parameters instead of
unknown.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts` around lines 179 - 192, The plan method currently blind-casts unknown inputs to SqlMigrationPlannerPlanOptions and Contract<SqlStorage> before calling planSql; replace those casts with explicit runtime validation at the plan boundary: add a small type-narrowing check (or predicate) that verifies options conforms to SqlMigrationPlannerPlanOptions (e.g., required properties like contract.storage.types, contract.storage.storageHash, contract.storage.tables and schema.dependencies exist and have expected shapes) and that fromContract is either null or a valid Contract<SqlStorage>, and if validation fails return a structured PostgresPlanResult error instead of calling planSql; modify plan to call planSql only after validation succeeds, referencing the existing planSql, SqlMigrationPlannerPlanOptions and Contract<SqlStorage> symbols so downstream code never dereferences invalid shapes.
♻️ Duplicate comments (1)
packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts (1)
293-309:⚠️ Potential issue | 🟠 MajorIssue-planner calls still bypass the caller’s operation policy.
planIssues()is invoked withoutoptions.policy, andissuePlanResult.value.callsis appended unfiltered. In this branch, allowing'data'can still emit destructive or widening calls even when the caller explicitly disallowed those classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts` around lines 293 - 309, The issue is that planIssues(...) is called without the caller's operation policy so issuePlanResult.value.calls can contain disallowed operations; update the call to planIssues to pass options.policy (or the appropriate policy field) and then enforce the policy before appending calls: either have planIssues return only policy-compliant calls or filter issuePlanResult.value.calls against options.policy before adding them to the calls array (the aggregation that currently combines buildDatabaseDependencyOperations(options), storageTypePlan.operations, and issuePlanResult.value.calls); ensure plannerFailure(...) behavior is unchanged when planIssues fails.
🧹 Nitpick comments (2)
packages/2-sql/9-family/test/contract-to-schema-ir.test.ts (1)
177-226: Add the missing error-path test for unresolvedtypeRef.The implementation now throws when
typeRefis absent fromstorage.types, but this file only covers the success path. Please lock the throw behavior with a dedicated test.🧪 Proposed test addition
+ it('throws when typeRef is missing from storage.types', () => { + const storage: SqlStorage = { + storageHash: 'sha256:test' as StorageHashBase<string>, + tables: { + Post: table({ + columns: { + embedding: col({ + nativeType: 'vector', + codecId: 'pg/vector@1', + typeRef: 'MissingType', + }), + }, + }), + }, + types: {}, + }; + + expect(() => + contractToSchemaIR(wrap(storage), { renderDefault: testRenderer }), + ).toThrow( + 'Column references storage type "MissingType" but it is not defined in storage.types.', + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/9-family/test/contract-to-schema-ir.test.ts` around lines 177 - 226, Add a new unit test that mirrors the existing success case but removes the Embedding1536 entry from storage.types and asserts that contractToSchemaIR throws; specifically create a storage object (using SqlStorage, table, col) where a column has typeRef: 'Embedding1536' but storage.types does not contain that key, call contractToSchemaIR(wrap(storage), { expandNativeType: expand, renderDefault: testRenderer }) and expect the call to throw (use the test framework's toThrow/assert.throws style) to lock the unresolved typeRef error-path.test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts (1)
139-151: Use object matchers for the emitted op shape.These are grouped assertions on one JSON object, so
toMatchObject/expect.objectContaining(...)will read cleaner and match the repo's test style.As per coding guidelines, "Prefer object matchers (toMatchObject) over multiple individual expect().toBe() calls when checking 2 or more related values in tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts` around lines 139 - 151, Replace the multiple individual assertions on the emitted ops with object matchers: for the found dataTransformOp use expect(dataTransformOp).toMatchObject({ operationClass: 'data', check: expect.anything(), run: expect.any(Array) }) (and keep a simple existence assertion like expect(dataTransformOp).toBeDefined()); likewise replace the alterOp assertions with expect(alterOp).toMatchObject({ operationClass: 'destructive' }) after ensuring alterOp is defined. Reference the variables opsAfterEmit, dataTransformOp and alterOp when locating the assertions to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts`:
- Around line 63-66: Seed at least one row whose "score" value will not
auto-cast to int4 (e.g., "10 " or "ten") in the INSERT that uses
sql(db.connectionString, `INSERT INTO "public"."user" ... VALUES (..., '10 '),
(..., '20')`) so the data-transform step must run to normalize it; update the
patched check/run closures (the closures used by the data-transform migration
runner) to target actual seeded ids (not id = -1) and add an assertion after
migration apply that verifies the problematic row's score changed to the
expected normalized int (verifying :run executed, not just ALTER ... USING
score::int4).
---
Outside diff comments:
In `@packages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.ts`:
- Around line 69-84: The default renderer is still called with the original
column object, which can be stale after resolving type metadata; update the call
that builds the returned object in contract-to-schema-ir.ts so that when
invoking renderDefault you pass the resolved metadata object (the `resolved`
variable) instead of `column` — ensure `renderDefault(column.default, resolved)`
is used where defaults are rendered so the rendered default uses the resolved
nativeType/codecId/typeParams rather than the original `column` metadata.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 179-192: The plan method currently blind-casts unknown inputs to
SqlMigrationPlannerPlanOptions and Contract<SqlStorage> before calling planSql;
replace those casts with explicit runtime validation at the plan boundary: add a
small type-narrowing check (or predicate) that verifies options conforms to
SqlMigrationPlannerPlanOptions (e.g., required properties like
contract.storage.types, contract.storage.storageHash, contract.storage.tables
and schema.dependencies exist and have expected shapes) and that fromContract is
either null or a valid Contract<SqlStorage>, and if validation fails return a
structured PostgresPlanResult error instead of calling planSql; modify plan to
call planSql only after validation succeeds, referencing the existing planSql,
SqlMigrationPlannerPlanOptions and Contract<SqlStorage> symbols so downstream
code never dereferences invalid shapes.
---
Duplicate comments:
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 293-309: The issue is that planIssues(...) is called without the
caller's operation policy so issuePlanResult.value.calls can contain disallowed
operations; update the call to planIssues to pass options.policy (or the
appropriate policy field) and then enforce the policy before appending calls:
either have planIssues return only policy-compliant calls or filter
issuePlanResult.value.calls against options.policy before adding them to the
calls array (the aggregation that currently combines
buildDatabaseDependencyOperations(options), storageTypePlan.operations, and
issuePlanResult.value.calls); ensure plannerFailure(...) behavior is unchanged
when planIssues fails.
---
Nitpick comments:
In `@packages/2-sql/9-family/test/contract-to-schema-ir.test.ts`:
- Around line 177-226: Add a new unit test that mirrors the existing success
case but removes the Embedding1536 entry from storage.types and asserts that
contractToSchemaIR throws; specifically create a storage object (using
SqlStorage, table, col) where a column has typeRef: 'Embedding1536' but
storage.types does not contain that key, call contractToSchemaIR(wrap(storage),
{ expandNativeType: expand, renderDefault: testRenderer }) and expect the call
to throw (use the test framework's toThrow/assert.throws style) to lock the
unresolved typeRef error-path.
In `@test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts`:
- Around line 139-151: Replace the multiple individual assertions on the emitted
ops with object matchers: for the found dataTransformOp use
expect(dataTransformOp).toMatchObject({ operationClass: 'data', check:
expect.anything(), run: expect.any(Array) }) (and keep a simple existence
assertion like expect(dataTransformOp).toBeDefined()); likewise replace the
alterOp assertions with expect(alterOp).toMatchObject({ operationClass:
'destructive' }) after ensuring alterOp is defined. Reference the variables
opsAfterEmit, dataTransformOp and alterOp when locating the assertions to
change.
🪄 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: 9e2faaeb-8052-41db-8a05-ca96f32f0145
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlprojects/postgres-class-flow-migrations/plan.mdis excluded by!projects/**
📒 Files selected for processing (48)
.agents/skills/markdown-no-artificial-line-wraps/SKILL.mdexamples/prisma-next-demo/migrations/20260422T0720_initial/contract.d.tsexamples/prisma-next-demo/migrations/20260422T0720_initial/contract.jsonexamples/prisma-next-demo/migrations/20260422T0720_initial/migration.jsonexamples/prisma-next-demo/migrations/20260422T0720_initial/migration.tsexamples/prisma-next-demo/migrations/20260422T0720_initial/ops.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/contract.d.tsexamples/prisma-next-demo/migrations/20260422T0742_migration/contract.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/from-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0742_migration/from-contract.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/migration.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/migration.tsexamples/prisma-next-demo/migrations/20260422T0742_migration/ops.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/contract.d.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/contract.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/from-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/from-contract.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/migration.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/migration.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/ops.jsonexamples/prisma-next-demo/prisma/schema.prismaexamples/prisma-next-demo/src/prisma/contract.d.tsexamples/prisma-next-demo/src/prisma/contract.jsonpackages/1-framework/3-tooling/migration/package.jsonpackages/1-framework/3-tooling/migration/src/migration-ts.tspackages/1-framework/3-tooling/migration/test/migration-ts.test.tspackages/2-sql/9-family/src/core/migrations/contract-to-schema-ir.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/2-sql/9-family/src/core/sql-migration.tspackages/2-sql/9-family/src/core/verify.tspackages/2-sql/9-family/src/exports/control.tspackages/2-sql/9-family/test/contract-to-schema-ir.test.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-typescript.tspackages/3-targets/3-targets/postgres/src/exports/control.tspackages/3-targets/3-targets/postgres/src/exports/migration.tspackages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tspackages/3-targets/3-targets/postgres/test/migrations/planner.authoring-surface.test.tspackages/3-targets/3-targets/postgres/test/migrations/render-typescript.roundtrip.test.tstest/integration/test/cli-journeys/class-flow-round-trip.e2e.test.tstest/integration/test/cli-journeys/data-transform-enum-rebuild.e2e.test.tstest/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.tstest/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.tstest/integration/test/cli-journeys/data-transform-type-change.e2e.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.ts
✅ Files skipped from review due to trivial changes (17)
- packages/1-framework/3-tooling/migration/package.json
- .agents/skills/markdown-no-artificial-line-wraps/SKILL.md
- examples/prisma-next-demo/migrations/20260422T0742_migration/migration.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/ops.json
- examples/prisma-next-demo/migrations/20260422T0748_migration/contract.json
- examples/prisma-next-demo/migrations/20260422T0748_migration/from-contract.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/contract.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/migration.json
- examples/prisma-next-demo/migrations/20260422T0720_initial/contract.json
- examples/prisma-next-demo/migrations/20260422T0748_migration/from-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0742_migration/ops.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/from-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/migration.json
- examples/prisma-next-demo/migrations/20260422T0720_initial/contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0720_initial/migration.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/from-contract.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/3-targets/3-targets/postgres/src/exports/migration.ts
- packages/3-targets/3-targets/postgres/test/migrations/issue-planner.test.ts
- packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts
- test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts
- test/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.ts
- test/integration/test/cli-journeys/data-transform.e2e.test.ts
Contributor
Author
Review iteration (agent)Implemented the bulk of the outstanding inline/CodeRabbit items on this PR:
Not in this commit: splitting Resolving threads: please mark the matching conversations resolved when this lands. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/3-targets/3-targets/postgres/src/core/migrations/issue-planner.ts`:
- Around line 588-590: Replace the ad-hoc name construction for indexes with the
canonical naming used by the schema walker: when iterating
contractTable.indexes, use the index.name if present; otherwise compute the
fallback via the same helper used in planner.ts (defaultIndexName) with the
contract context rather than concatenating table+columns. Apply the same pattern
for uniques and foreign keys (use their explicit name fields or the planner’s
defaultUniqueName/defaultForeignKeyName helpers) so CreateIndexCall,
CreateUniqueCall and CreateForeignKeyCall receive the canonical identifier and
preserve truncation/hash behavior.
In `@packages/3-targets/3-targets/postgres/src/core/migrations/planner.ts`:
- Around line 278-291: The current filter drops all dependency_missing issues,
which incorrectly removes schema:/ext: dependency issues that planIssues() can
handle; change the predicate used for issuesForPlanIssues so it only excludes
dependency_missing issues that are owned by framework components (i.e. the ones
handled by buildDatabaseDependencyOperations) — for example, detect and exclude
dependency_missing only when the issue explicitly identifies a component-owned
install (e.g. issue.owner === 'component' or issue.id.startsWith('component:')
or whatever property your Issue shape uses), but keep dependency_missing issues
whose ids start with "schema:" or "ext:" so planIssues and planIssues() can
still process them; update the filter around schemaIssues -> issuesForPlanIssues
accordingly while leaving the type_missing exclusion in place.
🪄 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: bc12fc80-90b6-4445-b256-3f7edccf1dac
⛔ Files ignored due to path filters (2)
projects/postgres-class-flow-migrations/plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/pr-plan.mdis excluded by!projects/**
📒 Files selected for processing (18)
packages/1-framework/3-tooling/migration/src/errors.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/test/io.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/data-transform.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/postgres-emit.tstest/integration/test/cli-journeys/class-flow-round-trip.e2e.test.tstest/integration/test/cli-journeys/contract-infer-workflow.e2e.test.tstest/integration/test/cli-journeys/data-transform-enum-rebuild.e2e.test.tstest/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.tstest/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.tstest/integration/test/cli-journeys/data-transform-type-change.e2e.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.tstest/integration/test/cli-journeys/db-schema-discovery.e2e.test.tstest/integration/test/cli-journeys/mongo-migration.e2e.test.ts
✅ Files skipped from review due to trivial changes (3)
- test/integration/test/cli-journeys/db-schema-discovery.e2e.test.ts
- test/integration/test/cli-journeys/contract-infer-workflow.e2e.test.ts
- test/integration/test/cli-journeys/mongo-migration.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- test/integration/test/cli-journeys/data-transform.e2e.test.ts
- test/integration/test/cli-journeys/data-transform-type-change.e2e.test.ts
- packages/1-framework/3-tooling/migration/test/io.test.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/postgres-emit.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/3-targets/3-targets/postgres/src/core/migrations/planner-strategies.ts
wmadden
force-pushed
the
tml-2286-phase-2-flip
branch
from
2bb81f4 to
a149f01
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts (1)
97-124: Extract themigration.tspatching helper.This setup block + placeholder replacement flow is now repeated almost verbatim across the other
data-transform-*.e2e.test.tsjourneys. A small helper that injects the runtime prelude and fills named slots would make scaffold-shape changes much cheaper to update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts` around lines 97 - 124, The test repeats a common runtime prelude and placeholder-replacement flow (dbSetupBlock, scaffold.replace(...), filled, writeFileSync(migrationTsPath, filled)) so extract a small helper (e.g., patchMigrationScaffold or buildMigrationContent) that accepts the scaffold string and a map of named slot replacements (like "handle-nulls-user-name:check" and "handle-nulls-user-name:run") and returns the final filled migration content (or writes it given migrationTsPath); replace the inline dbSetupBlock/fill logic in this test with a call to that helper and update the other data-transform-*.e2e.test.ts files to use it to avoid duplicated scaffold shaping and make future prelude changes centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.ts`:
- Around line 348-353: The test currently only asserts the second argument
passed to mocks.copyFilesWithRename; update the assertions to also verify the
first argument (the destination directory) from
mocks.copyFilesWithRename.mock.calls[0] (and the analogous call in the other
artifact-copy test) equals the expected migration destination folder.
Concretely, read the call tuple from mocks.copyFilesWithRename.mock.calls,
assert the first element matches the expected dest directory string, then assert
the second element still equals the expected destinationFiles array; apply this
change for both occurrences that validate copyFilesWithRename.
- Around line 297-320: The test currently verifies writeMigrationTs is called
and emitMigration is not, but misses asserting that contract artifacts are
copied; update the test (the case in migration-plan-command.test around the
errorUnfilledPlaceholder scenario for createMigrationPlanCommand/executeCommand)
to also assert that mocks.copyFilesWithRename was invoked (e.g.,
expect(mocks.copyFilesWithRename).toHaveBeenCalled() or
toHaveBeenCalledTimes(1)) so the placeholder path scaffolds the contract files
before stopping.
In `@packages/1-framework/3-tooling/migration/test/io.test.ts`:
- Around line 423-452: The test should also assert that plain nested destName
values (e.g., 'nested/file.json') are rejected and fold that case into the
existing describe('copyFilesWithRename') block instead of creating a second
suite; update the existing test that currently checks '../outside.json' so it
also calls copyFilesWithRename(destDir, [{ sourcePath: src, destName:
'nested/file.json' }]) and verifies the thrown error via expectMigrationError(e,
'MIGRATION.INVALID_DEST_NAME') and that (e as
MigrationToolsError).details.destName equals 'nested/file.json', then remove the
duplicate describe block to avoid duplicate suite names in Vitest output.
In
`@packages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.ts`:
- Around line 45-50: The DataTransformOptions.run currently allows an empty
array which contradicts the “one or more mutation queries” contract; update the
type to a non-empty tuple (readonly [DataTransformClosure,
...DataTransformClosure[]]) or, if you prefer runtime validation, add a check in
the normalization path that converts/validates DataTransformOptions (the
function that normalizes/accepts DataTransformOptions and the code that handles
run at import time) to throw an error when run is an array of length 0; apply
the same change/validation for the other identical option usages around the
DataTransformOptions definition (the spots mentioned at lines ~67-69) so callers
cannot pass [] and will fail fast with a clear message referencing
DataTransformOptions.run.
- Around line 24-33: The core file is doing plane-crossing work by importing
createPostgresAdapter and lowerSqlPlan and performing emit-time lowering; move
that responsibility out of src/core by removing those imports from this module
and keeping DataTransformOperation, SerializedQueryPlan and any validation
(e.g., errorDataTransformContractMismatch) purely authoring-time (accepting
either a raw SqlQueryPlan or a serialized plan shape). Create the lowering step
in the control/emit layer (a function that consumes SerializedQueryPlan or
SqlQueryPlan and uses lowerSqlPlan + createPostgresAdapter to produce runtime
SQL) and update callers to invoke that emitter before runtime execution; ensure
the core API surface (types and validators) remains unchanged and no adapter or
lowering logic remains in the DataTransformOperation implementation or this core
file.
---
Nitpick comments:
In
`@test/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.ts`:
- Around line 97-124: The test repeats a common runtime prelude and
placeholder-replacement flow (dbSetupBlock, scaffold.replace(...), filled,
writeFileSync(migrationTsPath, filled)) so extract a small helper (e.g.,
patchMigrationScaffold or buildMigrationContent) that accepts the scaffold
string and a map of named slot replacements (like "handle-nulls-user-name:check"
and "handle-nulls-user-name:run") and returns the final filled migration content
(or writes it given migrationTsPath); replace the inline dbSetupBlock/fill logic
in this test with a call to that helper and update the other
data-transform-*.e2e.test.ts files to use it to avoid duplicated scaffold
shaping and make future prelude changes centralized.
🪄 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: 35a40f37-78ef-41cc-bed3-18863d968c12
⛔ Files ignored due to path filters (2)
projects/postgres-class-flow-migrations/pr-plan.mdis excluded by!projects/**projects/postgres-class-flow-migrations/specs/dataTransform-class-flow-design.mdis excluded by!projects/**
📒 Files selected for processing (39)
.gitattributesexamples/mongo-demo/migrations/20260409T1030_migration/end-contract.d.tsexamples/mongo-demo/migrations/20260409T1030_migration/end-contract.jsonexamples/mongo-demo/migrations/20260415_add-posts-author-index/end-contract.d.tsexamples/mongo-demo/migrations/20260415_add-posts-author-index/end-contract.jsonexamples/prisma-next-demo/migrations/20260422T0720_initial/end-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0720_initial/end-contract.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/end-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0742_migration/end-contract.jsonexamples/prisma-next-demo/migrations/20260422T0742_migration/start-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0742_migration/start-contract.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/end-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/end-contract.jsonexamples/prisma-next-demo/migrations/20260422T0748_migration/migration.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/start-contract.d.tsexamples/prisma-next-demo/migrations/20260422T0748_migration/start-contract.jsonexamples/retail-store/migrations/20260413T0314_migration/end-contract.d.tsexamples/retail-store/migrations/20260413T0314_migration/end-contract.jsonexamples/retail-store/migrations/20260415_add-product-validation/end-contract.d.tsexamples/retail-store/migrations/20260415_add-product-validation/end-contract.jsonexamples/retail-store/migrations/20260416_backfill-product-status/end-contract.d.tsexamples/retail-store/migrations/20260416_backfill-product-status/end-contract.jsonexamples/retail-store/migrations/20260416_backfill-product-status/migration.tspackages/1-framework/1-core/errors/src/migration.tspackages/1-framework/3-tooling/cli/src/commands/migration-new.tspackages/1-framework/3-tooling/cli/src/commands/migration-plan.tspackages/1-framework/3-tooling/cli/test/commands/migration-plan-command.test.tspackages/1-framework/3-tooling/migration/src/io.tspackages/1-framework/3-tooling/migration/test/io.test.tspackages/2-sql/9-family/src/core/migrations/types.tspackages/3-targets/3-targets/postgres/src/core/migrations/op-factory-call.tspackages/3-targets/3-targets/postgres/src/core/migrations/operations/data-transform.tspackages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.tstest/integration/test/cli-journeys/class-flow-round-trip.e2e.test.tstest/integration/test/cli-journeys/data-transform-enum-rebuild.e2e.test.tstest/integration/test/cli-journeys/data-transform-not-null-backfill.e2e.test.tstest/integration/test/cli-journeys/data-transform-nullable-tightening.e2e.test.tstest/integration/test/cli-journeys/data-transform-type-change.e2e.test.tstest/integration/test/cli-journeys/data-transform.e2e.test.ts
✅ Files skipped from review due to trivial changes (17)
- .gitattributes
- examples/mongo-demo/migrations/20260415_add-posts-author-index/end-contract.json
- examples/retail-store/migrations/20260413T0314_migration/end-contract.json
- examples/retail-store/migrations/20260415_add-product-validation/end-contract.json
- examples/retail-store/migrations/20260416_backfill-product-status/end-contract.json
- examples/mongo-demo/migrations/20260409T1030_migration/end-contract.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/end-contract.json
- examples/prisma-next-demo/migrations/20260422T0748_migration/end-contract.json
- examples/prisma-next-demo/migrations/20260422T0742_migration/start-contract.json
- packages/1-framework/1-core/errors/src/migration.ts
- examples/prisma-next-demo/migrations/20260422T0720_initial/end-contract.json
- examples/prisma-next-demo/migrations/20260422T0748_migration/start-contract.json
- examples/prisma-next-demo/migrations/20260422T0720_initial/end-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/start-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/end-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0742_migration/start-contract.d.ts
- examples/prisma-next-demo/migrations/20260422T0742_migration/end-contract.d.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/1-framework/3-tooling/cli/src/commands/migration-new.ts
- packages/1-framework/3-tooling/migration/src/io.ts
- packages/3-targets/3-targets/postgres/test/migrations/op-factory-call.test.ts
- packages/2-sql/9-family/src/core/migrations/types.ts
- examples/prisma-next-demo/migrations/20260422T0748_migration/migration.ts
- packages/1-framework/3-tooling/cli/src/commands/migration-plan.ts
- test/integration/test/cli-journeys/class-flow-round-trip.e2e.test.ts
wmadden
commented
Apr 22, 2026
Postgres-specific emit implementation that dynamic-imports migration.ts, dispatches on class/factory shapes, persists ops.json, and returns ops to the framework for attestation. Not yet wired into the target descriptor.
…s, storageTypes The retargeted strategies will need these to construct PostgresOpFactoryCall instances. Existing descriptor strategies ignore the new fields. The descriptor planner defaults them from options when not provided.
Parallel CallMigrationStrategy implementations for all four data-safe strategies (notNullBackfill, typeChange, nullableTightening, enumChange) that emit PostgresOpFactoryCall[] with DataTransformCall placeholder stubs. Dormant — not yet wired to any planner.
…lanIssues Renames the file to reflect its role as the issue-based planner. Adds planIssues() that maps SchemaIssue[] to PostgresOpFactoryCall[] via the new call strategies plus a mapIssueToCall default handler. The existing planDescriptors() is preserved alongside for the descriptor flow.
Per-strategy tests for the class-flow issue planner covering all four data-safe strategies plus default issue mapping and TypeScript rendering. Fixes missing CreateTableCall import in issue-planner.ts.
Removes planWithDescriptors, resolveDescriptors, renderDescriptorTypeScript from the Postgres target descriptor and registers emit: postgresEmit. migrationStrategy() now returns class-based for Postgres. Tests that exercised the descriptor hooks through the target descriptor now call planDescriptors/resolveOperations directly.
When the class-flow planner emits PostgresOpFactoryCall[] containing DataTransformCall with unfilled placeholder() stubs, plan.operations now throws PN-MIG-2001. Catch it in executeMigrationPlanCommand so that we still write migration.ts, skip ops.json/migration.json, and return a MigrationPlanResult with pendingPlaceholders: true. The human-readable output instructs the user to replace the placeholders and re-emit.
…-flow authoring - postgres-emit.test.ts: MigrationManifest type ships from @prisma-next/migration-tools/types, not /io. - data-transform.e2e.test.ts: skip; exercises descriptor-flow authoring (createBuilders / resolveDescriptors), which Postgres no longer ships. Re-enable once the class-flow dataTransform authoring surface lands.
- F04: return foreignKeyConflict notOk when the destination contract lacks
a matching FK entry, instead of constructing a degenerate AddForeignKeyCall
with empty references that would emit invalid DDL at apply time.
- F05: rewrite issue-planner.ts module preamble to describe both pipelines
(planDescriptors and planIssues) during the class-flow transition.
- F06: add transient-purpose preamble to descriptor-planner.scenarios.test.ts
and operation-resolver.integration.test.ts explaining that they hand-assemble
descriptor pipelines no longer reachable from the capability, pending
deletion in a later phase.
- F07: drop duplicated module header doc-block in descriptor-planner.scenarios.test.ts.
- F08: add a strategies:[] override test asserting the default mapIssueToCall
emits plain AddColumnCall without the notNullBackfill data-safety stubs.
- F09: replace classifyCall default with an exhaustiveness check so adding
a new PostgresOpFactoryCall subclass forces an update at every dispatch site.
- F10: tighten the DataTransformCall.toOp() assertion to match the structured
PN-MIG-2001 error shape {code, domain} rather than a loose /placeholder/i regex.
PR 2 of postgres-class-flow-migrations. Captures the consolidated
`dataTransform(contract, name, { check, run })` factory design, the
contract-artifact copy into the migration dir (Mongo parity), the
`ImportRequirement` extension for default imports + attributes, and the
inlining of the descriptor-flow resolver. Also records the new
`PN-MIG-2005` error, the explicit non-inclusion of contract.json in
attestation (per ADR 199), and a hand-off section with commit sequence,
verified type shapes, known hazards, and verification commands for the
implementing agent.
- ImportRequirement extension stays as two optional fields (kind + attributes). - Replace the over-specialized copyContractToMigrationDir with a generic copyFilesWithRename helper; callers invoke it once per contract, driven by the emitter files list landing in #356. - Combined named+default import lines deferred to whatever falls out of the renderer implementation. - F02 (e2e un-skip) stays in PR 2, consistent with the PR-2 merge gate. - Follow-up Linear ticket TML-2291 filed for type-level contract-hash enforcement (SqlQueryPlan<C, _Row>).
Distinguishes authoring-time contract/query-plan hash mismatches from the runtime plan-hash check (PN-RUN-*). Surfaced by the class-flow `dataTransform` factory when the user 's query builder was configured with a different contract than the one passed to `dataTransform(contract, ...)`.
…ibutes
Add optional `kind: "named" | "default"` and `attributes` fields to
`ImportRequirement`, plus a shared `renderImports` helper that aggregates
requirements per module, combines default + named on one line when
attributes agree, and throws on structural conflicts (duplicate default
symbols; divergent attribute maps).
Postgres and Mongo `render-typescript.ts` now delegate to the shared
helper instead of duplicating the aggregator. Enables the class-flow
`dataTransform` scaffold to declare its `import contract from
"./contract.json" with { type: "json" }` via the standard mechanism.
…pyFilesWithRename Generalize the contract-copy helper to any (source, destName) pair so callers own the list of files and the naming convention. Unblocks the class-flow dataTransform scaffold, which needs to copy both the destination contract as contract.* and the source contract as from-contract.*.
`migration new` and `migration plan` now copy the destination contract
as `contract.{json,d.ts}` and the source contract (when present) as
`from-contract.{json,d.ts}`, using the prior migration's already-copied
contract files as the source. TODOs flag #356 where the emitter will
own the artifact list.
…resolver Drops the dependency on the legacy `createDataTransform` factory in `operations/data-transform.ts` so the next commit can replace that module with the consolidated user-facing `dataTransform` factory. The descriptor-flow resolver now constructs `DataTransformOperation` directly from the resolved `check` and `run` plans.
…rations
Replaces the legacy descriptor-only `createDataTransform` helper with a
user-facing `dataTransform(contract, name, { check, run })` factory
exported from `@prisma-next/target-postgres/migration`. Closures may
return either a `SqlQueryPlan` or a `Buildable`; each returned plan is
asserted against the supplied `contract` (`PN-MIG-2005` on hash
mismatch) and lowered through the Postgres adapter to a serialized
`{sql, params}` payload for `ops.json`. Addresses code-review F01/F02
from the class-flow design spec.
…lots
Updates `DataTransformCall.renderTypeScript` to emit the new
4-argument shape `dataTransform(contract, label, { check, run })`
with `placeholder()` slots for both closures, and extends
`importRequirements` with the `contract` default import using the
`with { type: \"json\" }` attribute. Keeps the planner-emitted
`migration.ts` aligned with the consolidated factory landed in the
previous commit.
Re-enables the previously skipped data-transform e2e and rewrites the filled `migration.ts` to use the class-flow shape: a `Migration` subclass returning `dataTransform(contract, ...)` operations against a `db` configured with the same `contract`. Confirms the consolidated factory wires through `migration apply` end-to-end against a live Postgres database.
Addresses PR 2 code-review F02 (strict reading): the planner-assisted NOT-NULL backfill e2e now calls migration apply a second time after the dataTransform + setNotNull recipe completes and asserts the output contains "Already up to date". This pins both the runner marker-CAS ledger advance and the dataTransform check-driven idempotency skip (its check query returns 0 rows once every NULL is backfilled), closing the idempotency half of AC4.2 inline alongside the strategy it belongs to rather than relying on the separate class-flow round-trip e2e.
Addresses PR 2 code-review F03. The existing pr-plan.md committed PR 2 to re-scaffolding every examples/**/migrations/*/ and running per-example apply/verify as a merge gate, but during PR 2 migration plan and db update still run through different pipelines (class-flow vs walk-schema), so a scaffold-everything pass here would couple the flip to walk-schema edge cases that PR 3 is specifically designed to eliminate. PR 2 now commits only a single hand-authored reference class-flow migration (pins the authoring surface), pins schema equivalence via the per-strategy live-DB e2es plus the class-flow round-trip e2e, and moves the full per-example re-scaffold + apply merge gate to PR 3 where the unified planning pipeline makes one migration plan run the source of truth for every examples class-flow emission. Updates the phase-to-PR mapping, PR 2 contents/merge-gate/ review strategy/size/rollback, PR 3 contents/merge-gate/size, and the guardrails to match.
Instructs agents not to hard-wrap Markdown prose at fixed column widths; viewers wrap at display time.
Canonical skill lives under .agents/skills; .claude/skills re-exports via symlink like other project skills.
…-wraps Skill remains at .agents/skills/markdown-no-artificial-line-wraps/SKILL.md.
By the end of the project there is no descriptor alternative, so the "class-flow" qualifier no longer carries meaning. Add a final rename phase covering code, tests, and current docs; leave ADR bodies and the archived project directory alone as historical records.
…bined export default shape
Generated migration.ts files previously extended `SqlMigration<PostgresPlanTargetDetails>` from `@prisma-next/family-sql/migration`, forcing users to know the target-details generic and redeclare `targetId`. They also split the class declaration from its default export.
Introduce a target-owned `PostgresMigration` abstract class that pins the target details and `targetId`, and re-export it as `Migration` from `@prisma-next/target-postgres/migration`. The class-flow renderer now emits `export default class M extends Migration { … }` with a single import, matching the shape users would hand-write.
`writeMigrationTs` now pipes the rendered TypeScript through prettier (typescript parser, single quotes, 100-char width) before writing to disk, so generated and regenerated migration files land in the same shape users would get from `pnpm format`. Avoids noisy diffs and matches the authoring surface of hand-written migrations.
…s in class-flow planIssues When planSql dispatches to the class-flow `planIssues` path, it now filters out `dependency_missing` and `type_missing` schema issues before handing them to the planner. Those issue kinds are already handled by the framework-level `buildDatabaseDependencyOperations` and target-level `buildStorageTypeOperations`, so letting them through produced spurious `Unknown dependency type …` failures (e.g. for `postgres.extension.vector`) and duplicate `CREATE TYPE` operations for enums.
…rated initial Delete the hand-written `20260421T1000_backfill_user_kind` "reference" migration that predated class-flow planning. Its `migration.ts` no longer typechecked (wrong base class generic, missing contract.json) and, more importantly, a demo app should ship a real migration rather than a broken convenience file. Regenerate the initial migration via `prisma-next migration plan`; the resulting `20260422T0720_initial` bundle (contract.json, contract.d.ts, migration.json, ops.json, migration.ts) is the canonical attested output that `migration apply` consumes.
…ding `readMarker` previously relied on catching a `relation "prisma_contract.marker" does not exist` error to signal "no marker yet". That works against plain Postgres but leaves the pg-protocol stream in a bad state against some implementations (notably PGlite's TCP proxy), where a subsequent `client.query` receives the previous query's CommandComplete and the connection eventually crashes with `Received unexpected commandComplete message from backend`. Probe for the marker table via `information_schema.tables` first; return null when it is not yet present, and only run the parameterised marker select when the table exists. The happy path is unchanged and the error-based branch is no longer needed.
Exercise the class-flow plan/apply round trip end-to-end: add an optional `displayName` field to the User model, re-emit the contract, and plan + apply the resulting `20260422T0734_migration`.
`contractToSchemaIR` expanded a column`s `nativeType` from the fields on `StorageColumn` directly, ignoring `typeRef`. Columns that delegate their codec/nativeType/typeParams to a named `storage.types` entry (e.g. `post.embedding → Embedding1536` carrying `length: 1536`) therefore emitted a bare `"vector"` while `verify-sql-schema.ts` `renderExpectedNativeType` resolved the same typeRef and emitted `"vector(1536)"`. Diffing a contract revision against itself then surfaced a spurious `type_mismatch` issue, which `migration plan` translated into an unnecessary `alterColumnType` operation. Resolve the typeRef inside `convertColumn` before expanding, and relax `buildNativeTypeExpander` in the postgres control descriptor to fall back to the bare native type when the codec has no `expandNativeType` hook — matching the long-standing fallback in `renderExpectedNativeType`. Enum-style codecs whose `typeParams` describe a value set (not a DDL suffix), such as `pg/enum@1`, no longer crash schema-IR construction now that their `typeRef` resolves. Adds a regression test covering the `vector(1536)` case.
…nType
With the contract-to-schema typeRef fix in place, replanning the
`displayName` change yields just the intended `addColumn` op, without
the no-op `alterColumnType("post"."embedding" → vector(1536))` the
buggy planner previously emitted. Drops the stale migration directory
in favour of the clean one.
…rations `SqlMigration<TDetails>` was parameterized only on `SqlMigrationPlanOperation<TDetails>`, so user-authored `migration.ts` files could not intermix `dataTransform(contract, …)` calls (returning `DataTransformOperation`) with DDL factory calls (returning `SqlMigrationPlanOperation`) in a single `operations` array — TypeScript rejected the union even though the runner already dispatches both branches. Mirror the Mongo-family shape: introduce `AnySqlMigrationOperation<TDetails> = SqlMigrationPlanOperation<TDetails> | DataTransformOperation` and re-parameterize `SqlMigration` on it. Planner-produced subclasses (e.g. `TypeScriptRenderablePostgresMigration`) keep their narrower `readonly Op[]` return via covariant override.
…ration Turn the previously-nullable `User.displayName` column into a required one and ship the backfill migration `20260422T0748_migration` alongside the schema change. The planner emits a `dataTransform` placeholder for the null backfill followed by `setNotNull`; the authored `migration.ts` fills the check/run closures with real SQL builder queries against the migration-local contract, backfilling nulls to `"Anonymous"`. Doubles as an end-to-end demonstration of the class-flow data-migration flow: `migration plan` emits `placeholder()` stubs, `migration emit` fails with `PN-MIG-2001` until the user replaces the stubs, then `migration apply` succeeds with idempotent check → run → check semantics.
The file only ever hosted renderDescriptorTypeScript, which has no production callers as of PR 2 (Phase 3) — only scaffolding.test.ts imports it. Nothing needs to remain in the file, so Phase 5 deletes it along with scaffolding.test.ts in its entirety rather than trimming down to "non-descriptor helpers".
…o-end The previous pr-plan.md explicitly deferred wholesale example re-scaffolding to PR 3 (aff9570, rebased as 8b81ce3), but PR 2 subsequently shipped it anyway for the one Postgres example in the repo (prisma-next-demo) — `e3a23777f`, `db537007b`, `00dce22b5`, `6f0f6b67a` — and that effort uncovered four real bugs in the newly-live planViaIssues dispatch path (`08952faac`, `24774023c`, `6cbb28ae2`, `4b0aad40d`). In retrospect doing the re-scaffolding in PR 2 was correct: it was the first production-shaped driver of the class-flow pipeline and surfaced defects that unit tests and isolated e2es had not. Update pr-plan.md to document what actually shipped: - PR 2 Contents gets the demo re-scaffolding bullet, the four surfaced fixes, and the `PostgresMigration` base class + prettier improvements. - PR 2 Behavior change describes the three-migration demo history. - PR 2 Merge gate names the demo apply/verify step explicitly (replaces the "deferred to PR 3" criterion). - PR 2 Review strategy, Size estimate, and Rollback all updated to match. - PR 3 drops the re-scaffolding bullet and narrows its scope / size estimate accordingly; keeps an optional post-collapse replan check as validation. - Guardrails updated to reflect PR 2 being the large PR (not PR 3). - Phase-to-PR table net-effect columns updated. Closes code-review F05 in reviews/pr-2-flip/code-review.md.
- Harden copyFilesWithRename destName; add MIGRATION.INVALID_DEST_NAME - Issue planner: use nativeType for CreateEnumTypeCall - planViaIssues: filter calls by MigrationOperationPolicy - postgresEmit: ESM import cache-bust with truncated mtime - DataTransformCall: jsonToTsSource for renderTypeScript - Dedupe TYPE_SAFE_WIDENINGS in planner-strategies - cli-journeys: node:path for join - Strengthen data-transform type-change e2e; note adapter singleton
…-contract
Within a migration directory, rename the on-disk contract files and the
planner-emitted scaffold import so authoring-surface vocabulary matches
the semantics: `end-contract.{json,d.ts}` is the schema that must be
true at the end of the migration, and `start-contract.{json,d.ts}` is
the schema true at the start. Framework-level `from`/`to` tracking
vocabulary (manifest fields, `Migration.describe()`, internal hashes)
is unchanged — it names edges in the DAG, a distinct concern.
Per ADR 199, `computeMigrationId` strips contract payloads before
hashing, so this rename does not invalidate attestation on existing
demo migrations.
- DataTransformCall now emits `dataTransform(endContract, ...)` and
declares a default import from `./end-contract.json`.
- `migration plan` / `migration new` copy artifacts as
`end-contract.{json,d.ts}` (and, when a prior migration exists,
`start-contract.{json,d.ts}`).
- Rename existing example migration artifacts and update the two
hand-authored `migration.ts` files that import the contract.
- Mark the new patterns `linguist-generated` in `.gitattributes`.
- Update unit + e2e tests and the spec / pr-plan / review artifacts.
Live source-package `src/prisma/contract.{json,d.ts}` is unchanged — it
is the unqualified live contract, not a point-in-time snapshot.
…ract copies Replace the sibling-.d.ts string-slice expedient in `migration plan` and `migration new` with the emitter-owned `getEmittedArtifactPaths()` API (landed in #356). The destination and prior-migration source contracts are now derived symmetrically through the same helper, centralising the `.json` / `.d.ts` pairing in `@prisma-next/emitter` and removing the TODO(#356) markers.
- Add required `displayName` field to `prisma-next-demo` ORM client inputs (`create-user`, `create-user-with-address`, `upsert-user`) and derive a display name from the email in `main.ts`, unblocking the workspace typecheck. - Seed `displayName` values in the `repositories.integration.test.ts` seed data and direct `ormClientCreate*` / `ormClientUpsertUser` call sites so the `user.displayName NOT NULL` constraint is satisfied. - In the Postgres planner `planViaIssues` path, skip codec-hook-emitted enum rebuild / add-value operations for types whose `enum_values_changed` issues are already owned by `enumChangeCallStrategy`. This prevents duplicate rebuild ops (raw SQL + class-flow `DataTransformCall` + `createEnumType`/`alterColumnType` etc.), restoring the expected `placeholder()` scaffold in `data-transform-enum-rebuild.e2e`. - Update `data-transform-enum-rebuild.e2e` and `mongo-migration.e2e` assertions to match Prettier single-quoted output produced by `writeMigrationTs`.
Coverage was failing the 90% branches/functions thresholds on `render-imports.ts` because all existing tests used single-key `attributes` maps, leaving the sort comparators unreachable and several conflict branches in `attributesEqual` untested. - Add tests for multi-key attribute rendering, empty attribute maps, conflict detection across differing keys and differing map sizes, and sorted stringification in conflict messages. - Replace the hand-rolled `a < b ? -1 : a > b ? 1 : 0` comparators with `localeCompare`, eliminating the unreachable equal-keys branch (object keys in a `Record` are unique). - Refactor `renderImports` to iterate over `byModule.entries()`, removing the dead `if (!group) continue` guard left over from the prior `byModule.keys() + byModule.get()` pattern. Result: `render-imports.ts` is now at 100% statements/branches/ functions/lines.
…ngle-quote output The planner renders scaffold `migration.ts` files through Prettier, which rewrites string literals to single quotes. Update the three `data-transform-*.e2e.test.ts` scenarios to expect `placeholder(...)` stubs with single-quoted slot names to match. Also revert the strengthening of the `text → int4` type-change scenario to the pre-existing no-op (`id = -1`) pattern: the "real normalization" variant introduced in the PR 364 fix-up commit relied on a `check` query that could not correctly express "rows whose text value is not castable to int4" through the int4-typed ORM surface, leaving a violation that was never resolved. That failure was masked until now by the quote-style assertion failing first. The test still covers the `dataTransform → alterColumnType` pipeline end-to-end; a proper data-normalization scenario needs an ORM escape hatch (or pre-alter text-typed access) we do not have yet.
Three specs capturing the plane-hygiene + adapter-injection workstream surfaced by PR review on postgres-class-flow-migrations: - projects/relocate-sql-migration/ — move sql-migration.ts into core/migrations/ and register that directory as migration plane, resolving a latent shared→migration violation in the SQL family package. - projects/target-migration-plane-registration/ — register target-postgres (and target-sqlite) src/core/migrations/** as migration plane so dep-cruiser enforces the boundary, surfacing the currently-latent violations in data-transform.ts and operation-resolver.ts. - projects/migration-control-adapter-di/ — inject a lowering-capable control adapter into the Migration instance so src/core/migrations/** no longer imports @prisma-next/adapter-postgres or @prisma-next/sql-runtime; split the Migration abstract class from its run() orchestrator so the abstract authoring surface can live in shared plane. Linear: TML-2300, TML-2301, TML-2302.
wmadden
force-pushed
the
tml-2286-phase-2-flip
branch
from
a1a557e to
d6459a8
Compare
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.
closes TML-2286
Decision
Postgres
migration planis now the same kind of thing as Mongo: it writes aclass M extends Migrationinmigration.ts, and that file is the source of truth—node migration.ts(or the CLI) runs in-process emit to produceops.json, instead of a separate “descriptor” pipeline and resolver.Concretely, the Postgres target stops advertising
planWithDescriptors/resolveDescriptors/renderDescriptorTypeScriptand starts advertisingemit: postgresEmit(mirroring the Mongo pattern in ADR 196). Data migrations that need user-written SQL usedataTransform(…, { check, run })withplaceholder()slots; unfilled slots surface as a structured error so the CLI can scaffold without emitting half-bakedops.json.Not decided here: deleting the old walk-schema planner and merging every
db updateandmigration planpath into one—tracked for the next PR. While this split exists, we routemigration plan(which allows thedataoperation class) through the issue-based planner;db updatestill uses the walk-schema path. Wholesale re-scaffolding of allexamples/**/migrations/*/is explicitly deferred to PR 3 so we don’t couple this change to walk-schema edge cases the follow-up is meant to remove.Grounding example
You add a non-nullable column to a table that already has rows. The contract diff says: new column, no default, required.
What you do not get anymore is a single opaque “plan” you resolve elsewhere. What you get is a generated
migration.tswhose operations read like the real work: add the column nullable, run adataTransformwhosecheck/runareplaceholder("…:check")andplaceholder("…:run")until you fill them, then tighten toNOT NULL. You backfill in real query-builder code; thenmigration emitandmigration applyrun against a real database.The CLI notices unfilled placeholders (
PN-MIG-2001), writesmigration.tsplus the contract files it needs, and skips emittingops.json/migration.jsonuntil you’ve filled the slots—so you never “apply” a migration that silently pretends the data step happened.That one scenario is the template for the other data-safety paths (unsafe type change, nullable tightening, enum value removal): same placeholder contract, different surrounding DDL.
Why
Mongo already shipped class-flow as the canonical migration style (ADR 193). Postgres was still on “descriptors + resolver +
migration emitas a second phase.” This PR aligns Postgres with that product direction: one authoring model, in-process emit, and an honest place for backfill code (dataTransform) instead of hiding data work in a side pipeline.What ships (in order of “you’ll notice it”)
createPlanner,createRunner,contractToSchema, andemitonly. Class-flow is selected by migration-strategy.ts the same way as Mongo.migration.ts, runMigration’soperations, writeops.json; placeholder errors bubble to the CLI.planIssues/mapIssueToCallalongside the legacy descriptor mapping (descriptor code remains for tests and a later delete). planner-strategies.ts adds call-strategy twins that emitPostgresOpFactoryCall+DataTransformCallfor the four data-safety cases.dataclass (asmigration plandoes), the target planner delegates toplanIssuesso those strategies are actually used in production—not only in unit tests.migration.ts, don’t call emit until the user fills slots. migration.ts (exports) re-exportsplaceholdernext todataTransformand DDL helpers.dataTransform(contract, name, { check, run })validates contract hash, lowers query plans to executed SQL. Demo reference migration under 20260421T1000_backfill_user_kind.How to read the diff
planDescriptors, resolvers) are still in the tree for the phased removal; new work is the call path and the flip on the capability.Proof
foreignKeyConflictwhen a modeled FK can’t be matched to the contract).data-transform-not-null-backfill,data-transform-type-change,data-transform-nullable-tightening,data-transform-enum-rebuild), manualdataTransformauthoring (data-transform.e2e), and full round-trip + idempotency (class-flow-round-trip.e2e.test.ts) under cli-journeys.Non-goals (this PR)
migration emitas a separate product surface, or Mongo changes—covered by later phases in the plan.Summary by CodeRabbit
Release Notes
New Features
dataTransform()factory for authoring data migrations with contract validation and placeholder support.start-contractandend-contractartifacts.Tests