{{ message }}
test(migrate): add reproduction test for varchar partial unique index migration loop#29421
Open
yangbobo2021 wants to merge 1 commit intoprisma:mainfrom
Open
test(migrate): add reproduction test for varchar partial unique index migration loop#29421yangbobo2021 wants to merge 1 commit intoprisma:mainfrom
yangbobo2021 wants to merge 1 commit intoprisma:mainfrom
Conversation
… migration loop (issue prisma#29386) This commit adds a reproduction test for issue prisma#29386: when using `@db.VarChar` with a partial unique index using a negation predicate (`where: { status: { not: "value" } }`), `prisma migrate dev` incorrectly generates the same DROP + CREATE migration on every run. The bug occurs because PostgreSQL stores index predicates with `::text` casts and `<>` operators, while Prisma generates them without. The schema differ doesn't recognize these as semantically equivalent. Root cause: prisma-engines/migration-engine/src/schema_differ.rs The `pg_predicates_semantically_equal()` function needs to handle: - PostgreSQL's automatic `::text` casts for varchar columns - Operator normalization (`!=` vs `<>`) Test execution: TEST_POSTGRES_URI_MIGRATE="postgres://prisma:prisma@localhost:5432/tests-migrate" \ TEST_POSTGRES_SHADOWDB_URI_MIGRATE="postgres://prisma:prisma@localhost:5432/tests-migrate-shadowdb" \ pnpm --filter @prisma/migrate run test -- src/__tests__/MigrateDev.issue29386.test.ts Test results: - First migrate dev: Creates index with WHERE ("status" != 'superseded') - Second migrate dev: BUG - Creates spurious DROP + CREATE migration
|
bobo yang seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
Summary by CodeRabbit
WalkthroughAdds a new Jest integration test with supporting Prisma configuration and schema fixtures to reproduce and verify an issue with Prisma Migrate Dev's handling of PostgreSQL partial unique indexes on Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![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: 4
🤖 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/migrate/src/__tests__/fixtures/issue-29386-varchar-partial-unique/prisma.config.ts`:
- Around line 1-8: Replace the direct process.env access with the env() helper
from `@prisma/config` for consistency: in the default export created by
defineConfig, change the datasource.url value to use
env('TEST_POSTGRES_URI_MIGRATE') instead of
process.env.TEST_POSTGRES_URI_MIGRATE so it matches other fixtures (e.g.,
external-tables) and benefits from the helper's behavior.
In `@packages/migrate/src/__tests__/MigrateDev.issue29386.test.ts`:
- Around line 66-72: Duplicate logic that filters migration directories (using
fs.list, migrationsDir, fs.exists and producing
initialMigrationDirs/allItemsInMigrations) should be extracted into a single
helper (e.g., filterMigrationDirs or getMigrationDirectories) that takes
migrationsDir and returns only directory names (excluding files like
migration_lock.toml); replace both occurrences (the block that creates
initialMigrationDirs and the later identical block) with calls to this helper to
keep the test DRY and clearer.
- Around line 74-76: Remove the unnecessary non-null assertions on
initialMigrationDirs and migrationDirsAfterSecond: after verifying their
lengths, drop the `!` suffixes so code uses the arrays directly (e.g., compute
firstMigrationDir from initialMigrationDirs[initialMigrationDirs.length - 1] and
build firstMigrationPath with path.join(migrationsDir, firstMigrationDir,
'migration.sql')); also remove the same `!` assertions in the other occurrences
around migrationDirsAfterSecond/related variables (the length checks already
guarantee these arrays are defined and non-empty).
- Around line 85-90: The test sets process.env.GITHUB_ACTIONS and process.env.CI
but doesn't restore them, which can leak state into other tests; modify the test
around the MigrateDev.new().parse call to capture the original values of
process.env.GITHUB_ACTIONS and process.env.CI before setting them and restore
those originals in a finally block (or use afterEach) so that the environment is
returned to its prior state after running the code that calls
MigrateDev.new().parse(['--name=should_not_exist'], config, configDir).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c86d58b-7950-4f64-a38c-2b648bc43db4
📒 Files selected for processing (3)
packages/migrate/src/__tests__/MigrateDev.issue29386.test.tspackages/migrate/src/__tests__/fixtures/issue-29386-varchar-partial-unique/prisma.config.tspackages/migrate/src/__tests__/fixtures/issue-29386-varchar-partial-unique/prisma/schema.prisma
Comment on lines
+1
to
+8
| import { defineConfig } from '@prisma/config' | ||
|
|
||
| export default defineConfig({ | ||
| schema: 'prisma/schema.prisma', | ||
| datasource: { | ||
| url: process.env.TEST_POSTGRES_URI_MIGRATE, | ||
| }, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use env() helper for consistency with other fixtures.
Other fixtures (e.g., external-tables/prisma.config.ts) use the env() helper from @prisma/config rather than accessing process.env directly. This maintains consistency and may provide better error handling for missing environment variables.
♻️ Suggested fix
-import { defineConfig } from '@prisma/config'
+import { defineConfig, env } from '@prisma/config'
export default defineConfig({
schema: 'prisma/schema.prisma',
datasource: {
- url: process.env.TEST_POSTGRES_URI_MIGRATE,
+ url: env('TEST_POSTGRES_URI_MIGRATE'),
},
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/migrate/src/__tests__/fixtures/issue-29386-varchar-partial-unique/prisma.config.ts`
around lines 1 - 8, Replace the direct process.env access with the env() helper
from `@prisma/config` for consistency: in the default export created by
defineConfig, change the datasource.url value to use
env('TEST_POSTGRES_URI_MIGRATE') instead of
process.env.TEST_POSTGRES_URI_MIGRATE so it matches other fixtures (e.g.,
external-tables) and benefits from the helper's behavior.
Comment on lines
+66
to
+72
| // Get the migration directories that were created (filter out non-directories like migration_lock.toml) | ||
| const allItemsInMigrations = fs.list(migrationsDir) ?? [] | ||
| const initialMigrationDirs = allItemsInMigrations.filter((item) => { | ||
| const itemPath = path.join(migrationsDir, item) | ||
| return fs.exists(itemPath) === 'dir' | ||
| }) | ||
| expect(initialMigrationDirs.length).toBeGreaterThanOrEqual(1) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated directory filtering logic.
The same filtering logic to identify migration directories (excluding files like migration_lock.toml) appears twice. Consider extracting to a helper function for clarity and DRY compliance.
♻️ Suggested refactor
+ const getMigrationDirs = (dir: string): string[] => {
+ const items = fs.list(dir) ?? []
+ return items.filter((item) => fs.exists(path.join(dir, item)) === 'dir')
+ }
+
// Get the migration directories that were created (filter out non-directories like migration_lock.toml)
- const allItemsInMigrations = fs.list(migrationsDir) ?? []
- const initialMigrationDirs = allItemsInMigrations.filter((item) => {
- const itemPath = path.join(migrationsDir, item)
- return fs.exists(itemPath) === 'dir'
- })
+ const initialMigrationDirs = getMigrationDirs(migrationsDir)And later:
- const allItemsAfterSecond = fs.list(migrationsDir) ?? []
- const migrationDirsAfterSecond = allItemsAfterSecond.filter((item) => {
- const itemPath = path.join(migrationsDir, item)
- return fs.exists(itemPath) === 'dir'
- })
+ const migrationDirsAfterSecond = getMigrationDirs(migrationsDir)Also applies to: 98-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/src/__tests__/MigrateDev.issue29386.test.ts` around lines 66
- 72, Duplicate logic that filters migration directories (using fs.list,
migrationsDir, fs.exists and producing
initialMigrationDirs/allItemsInMigrations) should be extracted into a single
helper (e.g., filterMigrationDirs or getMigrationDirectories) that takes
migrationsDir and returns only directory names (excluding files like
migration_lock.toml); replace both occurrences (the block that creates
initialMigrationDirs and the later identical block) with calls to this helper to
keep the test DRY and clearer.
Comment on lines
+74
to
+76
| // Use the last migration directory (most recent) | ||
| const firstMigrationDir = initialMigrationDirs![initialMigrationDirs!.length - 1] | ||
| const firstMigrationPath = path.join(migrationsDir, firstMigrationDir, 'migration.sql') |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove unnecessary non-null assertions.
The ! assertions on initialMigrationDirs and migrationDirsAfterSecond are redundant. Array.prototype.filter() always returns an array (never null or undefined), and the length check on line 72 already validates the array is non-empty.
♻️ Suggested fix
// Use the last migration directory (most recent)
- const firstMigrationDir = initialMigrationDirs![initialMigrationDirs!.length - 1]
+ const firstMigrationDir = initialMigrationDirs[initialMigrationDirs.length - 1]- if (migrationDirsAfterSecond!.length > initialMigrationDirs!.length) {
+ if (migrationDirsAfterSecond.length > initialMigrationDirs.length) {
// BUG REPRODUCED: A new migration was created when it shouldn't have been
- const newMigrationDir = migrationDirsAfterSecond![migrationDirsAfterSecond!.length - 1]
+ const newMigrationDir = migrationDirsAfterSecond[migrationDirsAfterSecond.length - 1]Also applies to: 104-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/src/__tests__/MigrateDev.issue29386.test.ts` around lines 74
- 76, Remove the unnecessary non-null assertions on initialMigrationDirs and
migrationDirsAfterSecond: after verifying their lengths, drop the `!` suffixes
so code uses the arrays directly (e.g., compute firstMigrationDir from
initialMigrationDirs[initialMigrationDirs.length - 1] and build
firstMigrationPath with path.join(migrationsDir, firstMigrationDir,
'migration.sql')); also remove the same `!` assertions in the other occurrences
around migrationDirsAfterSecond/related variables (the length checks already
guarantee these arrays are defined and non-empty).
Comment on lines
+85
to
+90
| // Ensure CI mode is still enabled | ||
| process.env.GITHUB_ACTIONS = '1' | ||
| process.env.CI = '1' | ||
|
|
||
| // Second migration: should detect NO CHANGES (this is where the BUG manifests) | ||
| await MigrateDev.new().parse(['--name=should_not_exist'], config, configDir) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Environment variables are not restored after the test.
Setting GITHUB_ACTIONS and CI without restoring their original values could affect subsequent tests in the same process. Consider saving and restoring the original values.
♻️ Suggested fix
+ // Save original env values
+ const originalGithubActions = process.env.GITHUB_ACTIONS
+ const originalCi = process.env.CI
+
// Ensure CI mode is still enabled
process.env.GITHUB_ACTIONS = '1'
process.env.CI = '1'
// Second migration: should detect NO CHANGES (this is where the BUG manifests)
await MigrateDev.new().parse(['--name=should_not_exist'], config, configDir)
+
+ // Restore original env values
+ if (originalGithubActions === undefined) delete process.env.GITHUB_ACTIONS
+ else process.env.GITHUB_ACTIONS = originalGithubActions
+ if (originalCi === undefined) delete process.env.CI
+ else process.env.CI = originalCi📝 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
| // Ensure CI mode is still enabled | |
| process.env.GITHUB_ACTIONS = '1' | |
| process.env.CI = '1' | |
| // Second migration: should detect NO CHANGES (this is where the BUG manifests) | |
| await MigrateDev.new().parse(['--name=should_not_exist'], config, configDir) | |
| // Save original env values | |
| const originalGithubActions = process.env.GITHUB_ACTIONS | |
| const originalCi = process.env.CI | |
| // Ensure CI mode is still enabled | |
| process.env.GITHUB_ACTIONS = '1' | |
| process.env.CI = '1' | |
| // Second migration: should detect NO CHANGES (this is where the BUG manifests) | |
| await MigrateDev.new().parse(['--name=should_not_exist'], config, configDir) | |
| // Restore original env values | |
| if (originalGithubActions === undefined) delete process.env.GITHUB_ACTIONS | |
| else process.env.GITHUB_ACTIONS = originalGithubActions | |
| if (originalCi === undefined) delete process.env.CI | |
| else process.env.CI = originalCi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/migrate/src/__tests__/MigrateDev.issue29386.test.ts` around lines 85
- 90, The test sets process.env.GITHUB_ACTIONS and process.env.CI but doesn't
restore them, which can leak state into other tests; modify the test around the
MigrateDev.new().parse call to capture the original values of
process.env.GITHUB_ACTIONS and process.env.CI before setting them and restore
those originals in a finally block (or use afterEach) so that the environment is
returned to its prior state after running the code that calls
MigrateDev.new().parse(['--name=should_not_exist'], config, configDir).
jacek-prisma
reviewed
Apr 9, 2026
Comment on lines
+111
to
+112
| expect(newMigrationContent).toContain('DROP INDEX') | ||
| expect(newMigrationContent).toContain('CREATE UNIQUE INDEX') |
Contributor
There was a problem hiding this comment.
The test should expect the correct behavior here, not the bug (rather than throwing an error later)
jacek-prisma
reviewed
Apr 9, 2026
Comment on lines
+86
to
+87
Contributor
There was a problem hiding this comment.
this should be properly mocked/unmocked to avoid polluting other tests
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
3 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.
This PR adds a reproduction test for #29386.
Problem
When using `@db.VarChar` with a partial unique index using a negation predicate
(`where: { status: { not: "value" } }`), `prisma migrate dev` incorrectly
generates the same DROP + CREATE migration on every run.
Root Cause
PostgreSQL stores index predicates with `::text` casts and `<>` operators,
while Prisma generates them without:
The schema differ in `prisma-engines/migration-engine/src/schema_differ.rs`
doesn't recognize these as semantically equivalent, causing spurious migrations.
This is similar to the fix for #29263 (equality predicates), but affects
negation predicates with VarChar columns.
Test Results
Running `migrate dev` twice:
Execution
```bash
TEST_POSTGRES_URI_MIGRATE="postgres://prisma:prisma@localhost:5432/tests-migrate" \
TEST_POSTGRES_SHADOWDB_URI_MIGRATE="postgres://prisma:prisma@localhost:5432/tests-migrate-shadowdb" \
pnpm --filter @prisma/migrate run test -- src/tests/MigrateDev.issue29386.test.ts
```
Schema
```prisma
model Document {
id String @id @default(uuid())
groupId String
status String @db.VarChar(20)
@@unique([groupId], where: { status: { not: "superseded" } })
}
```
Suggested Fix
In `prisma-engines/migration-engine/src/schema_differ.rs`, update
`pg_predicates_semantically_equal()` to handle:
This is similar to the fix for #29263 (pr-engines #5788, #5790, #5795, #5792).
Would you like me to create a follow-up PR with the fix in prisma-engines?