chore(policy): migration up-down-up roundtrip CI test#3241
Merged
Conversation
📝 WalkthroughWalkthroughAdds a Go integration test harness and two migration tests that run schema-scoped Goose migrations against PostgreSQL, and updates several SQL migration files to make Down/teardown steps idempotent and more failure-tolerant (use IF EXISTS and guarded constraint creation). Changes
Sequence Diagram(s)sequenceDiagram
participant TestHarness as Test Harness
participant Provider as goose.Provider
participant DB as Postgres DB
TestHarness->>DB: CREATE SCHEMA isolated_schema
TestHarness->>Provider: New(providerConfig(schema, pool, migrations))
TestHarness->>Provider: Up / UpTo(version)
Provider->>DB: execute migration SQL (CREATE/ALTER/UPDATE)
TestHarness->>DB: INSERT JSONB row (pre-rename)
TestHarness->>Provider: UpTo(rename_migration)
Provider->>DB: execute rename migration (JSONB transform)
TestHarness->>DB: SELECT JSONB path -> verify renamed field present
TestHarness->>Provider: DownTo(previous_version)
Provider->>DB: execute rollback SQL
TestHarness->>DB: SELECT JSONB path -> verify original field present
TestHarness->>DB: DROP SCHEMA isolated_schema CASCADE
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the reliability of the database migration process by adding an automated integration test. By simulating a full migration lifecycle—applying, rolling back, and re-applying—the system can now proactively detect issues in migration scripts that would otherwise only be discovered during critical production rollback scenarios. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Migrations flow in steady stream, / To keep the schema like a dream. / We roll them back to test the path, / And save ourselves from future wrath. Footnotes
|
jakedoublev
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new integration test, TestMigrationUpDownUp, to verify the database migration process by performing a full roundtrip: applying migrations, rolling them back, and re-applying them. The reviewer suggested several improvements to the test's robustness, including more precise error handling during the rollback phase to identify the end of the migration sequence, ensuring that the number of migrations remains consistent throughout the roundtrip, and explicitly closing the database client to release resources.
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Reportbats-test-results |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
X-Test Failure Report✅ js-pull-3241 |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@service/integration/migration_test.go`:
- Around line 30-43: newMigrationTestHarness currently uses a fixed schema
string which allows concurrent or interrupted runs to reuse existing
schema/goose state; change newMigrationTestHarness to derive a unique schema
name per test run (e.g., combine the provided schema with t.Name() and a short
random or timestamp suffix or use a UUID) before assigning c.DB.Schema and
before building q; update the local schema variable used in q := "CREATE SCHEMA
IF NOT EXISTS " + pgx.Identifier{schema}.Sanitize() so each test executes CREATE
SCHEMA for a unique name and avoid cross-test interference (keep using pgx.Exec
and the existing cleanup logic but ensure the unique name is returned/stored on
migrationTestHarness for teardown).
In `@service/policy/db/migrations/20240618000000_add_delete_cascade.sql`:
- Around line 242-267: The IF NOT EXISTS checks for constraint names
(attribute_value_members_value_id_fkey and
attribute_value_members_member_id_fkey) only filter by table_name, causing
cross-schema collisions; update the queries used in the DO blocks to also
restrict to the current schema (e.g., add table_schema = current_schema() or
equivalent) so the existence checks target attribute_value_members in the
migration’s schema before deciding whether to ADD CONSTRAINT for those foreign
keys referencing attribute_values (id).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68e08f43-4b51-4773-8f1e-8482a6c9b923
📒 Files selected for processing (6)
service/integration/migration_test.goservice/policy/db/migrations/20240213000000_create_attribute_fqn.sqlservice/policy/db/migrations/20240305000000_add_subject_condition_sets.sqlservice/policy/db/migrations/20240402000000_preserve_value_order.sqlservice/policy/db/migrations/20240618000000_add_delete_cascade.sqlservice/policy/db/migrations/20241125220354_keys_table.sql
Contributor
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
jakedoublev
commented
Apr 2, 2026
jakedoublev
commented
Apr 2, 2026
Validates that all goose migrations can be applied forward, fully rolled back, and re-applied without error. Catches broken Down migrations before they're needed in a production rollback. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Introduces migrationTestHarness that constructs a goose.Provider directly, enabling UpTo/DownTo/ApplyVersion for fine-grained migration control and raw SQL execution between steps. Adds TestMigrationData_SelectorFieldRename which seeds data before the JSONB field rename migration (20240405000000), applies it, verifies the field was transformed, rolls back, and verifies the original state. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
- Check goose.ErrNoNextVersion specifically instead of swallowing all errors in the rollback loop (valid Gemini review feedback) - Close db pool in test cleanup (valid review feedback) - Fix syntax error in 20241125220354_keys_table.sql Down section: DROP TRIGGER missing ON clause, wrong table name, missing IF EXISTS, wrong drop order. This pre-existing bug was caught by the new strict error checking. - Use DownTo with a rollback horizon instead of full chain rollback, since several older Down migrations have accumulated incompatibilities from intervening schema changes. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Fix five Down migrations that had pre-existing bugs never caught because Down migrations were not tested in CI: - 20240213000000: wrong table name (attribute_fqn -> attribute_fqns) - 20240305000000: DROP TRIGGER missing ON clause - 20240402000000: DROP TRIGGER missing ON clause, wrong drop order - 20240618000000: constraint name mismatch after table recreate by later migration (20240813000000) - 20241125220354: DROP TRIGGER missing ON clause, wrong table name, wrong drop order Remove rollback horizon from TestMigrationUpDownUp — all 43 migrations now roundtrip cleanly (up, down to 0, re-up). Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Check each MigrationResult.Error in Phase 1 (up) and Phase 3 (re-up), not just the top-level error. Assert count equality across all three phases: 43 up == 43 down == 43 re-up. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
The information_schema.table_constraints queries in the 20240618000000 Down migration could match constraints from other schemas visible to the postgres superuser. Add table_schema = current_schema() to ensure the checks are scoped correctly. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
jakedoublev
force-pushed
the
test/migration-roundtrip-ci
branch
from
April 2, 2026 16:13
2ca593b to
2029593
Compare
- Remove empty StatementBegin/End blocks in 20240213000000 and 20240305000000 (dead code, simple DDL doesn't need them) - Simplify constraint existence checks in 20240618000000 from information_schema queries to exception-based pattern (EXCEPTION WHEN duplicate_object), which is more concise and handles schema isolation automatically Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
X-Test Failure Report |
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
jakedoublev
marked this pull request as ready for review
April 2, 2026 16:25
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@service/policy/db/migrations/20240305000000_add_subject_condition_sets.sql`:
- Around line 117-119: The DROP TABLE statement for subject_condition_set should
be made idempotent like the trigger drop: change the DROP TABLE
subject_condition_set; to include IF EXISTS so that dropping the table is safe
on repeated rollbacks (i.e., use DROP TABLE IF EXISTS for the
subject_condition_set table referenced in this migration).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a7119cce-e98d-4bec-9a35-f9a5e69721b9
📒 Files selected for processing (6)
service/integration/migration_test.goservice/policy/db/migrations/20240213000000_create_attribute_fqn.sqlservice/policy/db/migrations/20240305000000_add_subject_condition_sets.sqlservice/policy/db/migrations/20240402000000_preserve_value_order.sqlservice/policy/db/migrations/20240618000000_add_delete_cascade.sqlservice/policy/db/migrations/20241125220354_keys_table.sql
Make the DROP TABLE subject_condition_set idempotent, consistent with the IF EXISTS pattern used on other drops in this PR. Signed-off-by: jakedoublev <jake.vanvorhis@virtru.com>
Contributor
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Contributor
jakedoublev
enabled auto-merge
April 2, 2026 17:09
jakedoublev
changed the title
jakedoublev
changed the title
strantalis
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
UpTo/DownTocontrolMigration fixes
20240213000000_create_attribute_fqnattribute_fqn→attribute_fqns)20240305000000_add_subject_condition_setsDROP TRIGGERmissingONclause20240402000000_preserve_value_orderDROP TRIGGERmissingONclause, wrong drop order20240618000000_add_delete_cascade20241125220354_keys_tableDROP TRIGGERmissingONclause, wrong table name, wrong drop orderNo Up migrations were modified.
Test plan
TestMigrationUpDownUp— 45 up, 45 down, 45 re-upTestMigrationData_SelectorFieldRename— data integrity through up/down