test(db): regression test for migration 000027 idempotency (closes #246) - #540
Conversation
cristim
added
triaged
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces an integration test for PostgreSQL migration ChangesMigration Idempotency Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/database/postgres/migrations/savings_snapshots_pk_test.go (2)
68-73: ⚖️ Poor tradeoffVersion calculation assumes sequential migration numbering.
The rollback calculation
stepsToRollback := currentVersion - 26assumes migration versions are sequential integers without gaps. If the project's migration numbering scheme introduces gaps or non-sequential versioning, this will roll back to the wrong version.Consider querying the
schema_migrationstable to find the actual position of version 26, or document the assumption that migrations are strictly sequential.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/savings_snapshots_pk_test.go` around lines 68 - 73, The current rollback calculation uses stepsToRollback := currentVersion - 26 which assumes migration versions are sequential; instead compute the number of migrations newer than version 26 by querying schema_migrations (e.g., SELECT COUNT(*) FROM schema_migrations WHERE version > 26) or verify the existence/position of version 26 and set stepsToRollback to that count; update the logic around currentVersion, stepsToRollback and the schema_migrations queries to derive steps from the actual rows rather than subtracting integers.
90-93: 💤 Low valueConsider clarifying the comment about migration 000018.
The comment states "000018 is still in effect (it was not rolled back)", which might be interpreted as the constraint from 000018 still existing. However, the constraint is actually gone because migration 000027's up migration dropped and re-created it. Consider clarifying that "in effect" means the migration entry remains in
schema_migrationsbut the constraint it created was superseded by 000027.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/database/postgres/migrations/savings_snapshots_pk_test.go` around lines 90 - 93, Update the test comment above the require.NoError call (near migrations.RunMigrations and migrationsPath) to clarify that "000018 is still in effect" refers to the migration entry remaining in schema_migrations, not that its constraint still exists; explicitly state that migration 000027's up migration dropped and re-created the constraint so the original constraint from 000018 has been superseded even though 000018 remains recorded.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/database/postgres/migrations/savings_snapshots_pk_test.go`:
- Line 26: Add a local getMigrationsPath function to the test file (used where
migrationsPath := getMigrationsPath()) that mirrors split_savingsplans_test.go:
resolve the current test file path using runtime.Caller and return filepath.Dir
of the filename; also add imports for runtime and filepath to the test file's
import block so the new function compiles. Target the getMigrationsPath symbol
and update the file's imports to include runtime and filepath.
---
Nitpick comments:
In `@internal/database/postgres/migrations/savings_snapshots_pk_test.go`:
- Around line 68-73: The current rollback calculation uses stepsToRollback :=
currentVersion - 26 which assumes migration versions are sequential; instead
compute the number of migrations newer than version 26 by querying
schema_migrations (e.g., SELECT COUNT(*) FROM schema_migrations WHERE version >
26) or verify the existence/position of version 26 and set stepsToRollback to
that count; update the logic around currentVersion, stepsToRollback and the
schema_migrations queries to derive steps from the actual rows rather than
subtracting integers.
- Around line 90-93: Update the test comment above the require.NoError call
(near migrations.RunMigrations and migrationsPath) to clarify that "000018 is
still in effect" refers to the migration entry remaining in schema_migrations,
not that its constraint still exists; explicitly state that migration 000027's
up migration dropped and re-created the constraint so the original constraint
from 000018 has been superseded even though 000018 remains recorded.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d535ec0-785b-4b9f-b383-b4a64b65be73
📒 Files selected for processing (1)
internal/database/postgres/migrations/savings_snapshots_pk_test.go
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai resolve |
✅ Actions performedComments resolved. Approval is disabled; enable |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Migration 000027 (savings_snapshots_pk) already contains the DROP CONSTRAINT IF EXISTS guard that makes it safe on fresh databases where 000018 has already created savings_snapshots_pkey. This commit adds the integration test that would have caught the original failure: - TestMigration_SavingsSnapshotsPK/full_migration_from_scratch: runs all migrations from scratch, verifying 000027 does not fail with "multiple primary keys for table" even though 000018 already added the constraint. - TestMigration_SavingsSnapshotsPK/rollback_to_000026_then_re-apply: rolls back past 000027 (without rolling back 000018) and re-applies all migrations, verifying idempotency in the replay scenario. Closes #246
The ORDER BY CASE k in 000040_split_savingsplans.down.sql referenced column k from a UNION branch, which is out of scope at the outer ORDER BY level in PostgreSQL. Restructure the SP-collapse branch to use a subquery that exposes a numeric priority column, then ORDER BY that column name inside the subquery. The savings_snapshots_pk_test.go rollback sub-test computed stepsToRollback = currentVersion - 26 (= 21 on a 47-migration DB) and passed it in a single RollbackMigrations call, which hit the 10-step safety cap. Call in a loop of at-most-10-step batches.
Move getMigrationsPath() from split_savingsplans_test.go into a new helpers_test.go shared by all integration tests in the package. This removes the implicit cross-file dependency that CodeRabbit flagged in savings_snapshots_pk_test.go: the function was defined in a sibling file and invisible to a reader of the calling file alone. Both files are in the same package and build tag (integration), so they always compiled together. The refactor makes each test file self-evidently complete without relying on symbol spillover from a sibling.
Replace `currentVersion - 26` arithmetic with a direct COUNT query on schema_migrations for rows with version > 26. This is correct even when migration numbering has gaps, since Steps(-n) steps back through n applied migrations (not n version-number units). Also clarify the comment about migration 000018 being "still in effect" to make clear that its schema_migrations entry persists but its original constraint was superseded by 000027. Addresses CodeRabbit finding on PR #540 (nitpick, lines 68-73 and 90-93 of savings_snapshots_pk_test.go).
0e2eefb to
d9a4db6
Compare
|
Addressed all CodeRabbit findings from the initial review: Actionable finding (line 26 - getMigrationsPath missing): Already fixed in commit e44b702 (refactor(migrations): extract getMigrationsPath to shared test helper) - the function lives in helpers_test.go and is shared across the entire migrations_test package. Nitpick finding (lines 68-73 - non-sequential rollback arithmetic): Fixed in commit d9a4db6 (fix(db): derive rollback step count from schema_migrations row count). Replaced SELECT COUNT(*) FROM schema_migrations WHERE version > 26This is correct even when migration numbering has gaps, since Steps(-n) steps back through n applied migrations (actual rows), not n version-number units. Nitpick finding (lines 90-93 - comment clarity about 000018): Also fixed in d9a4db6. Comment now explicitly states that 000018's schema_migrations entry remains but its original constraint was superseded when 000027 ran. Branch rebased onto origin/feat/multicloud-web-frontend and pushed (force-with-lease). @coderabbitai review |
|
Triggering a fresh review of the updated branch now. ✅ Actions performedReview triggered.
|
1 participant
Hand-off PR for partially-completed work from a sub-agent that hit a usage cap. Adds a regression test pinning that migration 000027 (savings_snapshots_pk) is idempotent across replays. If the migration itself still needs hardening, the test will fail and a follow-up commit can land on this branch.\n\nCloses #246.
Summary by CodeRabbit