fix(data-connector): scope startup migrations to core history - #1347
Merged
Merged
Conversation
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
slin1237
requested review from
CatherineSue,
XinyueZhang369,
gongwei-130,
key4ng,
zhaowenzi and
zhoug9127
as code owners
April 23, 2026 08:40
📝 WalkthroughWalkthroughThe PR extracts "history-only" migration arrays (versions 1–3) for Oracle and Postgres, switches store startup to run those history migrations, updates Flyway schema pin to version 3, and clarifies README migration docs describing the core history migration scope (conversations, items, responses). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 refactors the database migration logic to distinguish between core history migrations and full subsystem schemas. It introduces specific history migration arrays for Oracle and Postgres, updates the respective stores to use these core migrations, and adjusts documentation and Flyway configurations to reflect this change. The review feedback highlights a maintenance risk due to code duplication between the new history migration arrays and the existing full migration lists, suggesting the use of shared constants to ensure consistency.
github-actions
Bot
added
documentation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-scoped change. Verified migration runner logic handles all deployment scenarios safely (existing v11 deployments, fresh installs, Flyway-managed schemas). No production callers of the full migration arrays remain — dead-code suppression is correctly gated to non-test builds. LGTM.
![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 `@crates/data_connector/src/oracle_migrations.rs`:
- Around line 474-481: The test only checks migration.version; extend it to
assert that each ORACLE_HISTORY_MIGRATIONS entry also has the expected
description string and the expected up function pointer to avoid registry drift:
for each migration in ORACLE_HISTORY_MIGRATIONS (use migration.version to
index), assert migration.description equals the canonical description for that
version and assert the migration.up function pointer equals the canonical up
function (compare function pointers with pointer equality, e.g., casting to
fn(...) or using std::ptr::eq) so the test verifies (version, description, up)
match the authoritative registry.
In `@crates/data_connector/src/postgres_migrations.rs`:
- Around line 419-426: The test
postgres_history_migrations_cover_only_core_history_schema only checks versions
and can miss mismatches in description or the up migration mapping; enhance it
to compare the POSTGRES_HISTORY_MIGRATIONS slice against the prefix of the full
POSTGRES_MIGRATIONS registry by validating that for each index i the version,
description and up handler correspond (e.g., migration.version ==
POSTGRES_MIGRATIONS[i].version, migration.description ==
POSTGRES_MIGRATIONS[i].description, and the up functions map to the same
implementation) so any accidental drift between the history subset and the full
registry is detected.
🪄 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: dabaf6ee-61d4-498b-8336-786c72a58d31
📒 Files selected for processing (6)
crates/data_connector/README.mdcrates/data_connector/src/oracle.rscrates/data_connector/src/oracle_migrations.rscrates/data_connector/src/postgres.rscrates/data_connector/src/postgres_migrations.rsscripts/oracle_flyway/schema-config.yaml
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/data_connector/src/postgres_migrations.rs (1)
415-421: 🧹 Nitpick | 🔵 TrivialHarden the history drift guard test beyond version-only checks
Line 415 currently validates only version numbers. It should also assert
descriptionanduphandler parity against the full registry prefix, so silent drift is caught.♻️ Proposed test hardening
#[test] fn postgres_history_migrations_cover_only_core_history_schema() { let versions: Vec<u32> = POSTGRES_HISTORY_MIGRATIONS .iter() .map(|migration| migration.version) .collect(); assert_eq!(versions, vec![1, 2, 3]); + + for (history, full) in POSTGRES_HISTORY_MIGRATIONS + .iter() + .zip(POSTGRES_MIGRATIONS.iter()) + { + assert_eq!(history.version, full.version); + assert_eq!(history.description, full.description); + assert_eq!(history.up as usize, full.up as usize); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/data_connector/src/postgres_migrations.rs` around lines 415 - 421, The test postgres_history_migrations_cover_only_core_history_schema currently only compares version numbers; update it to also verify description and the up handler match the canonical registry entries: iterate POSTGRES_HISTORY_MIGRATIONS and for each index look up the corresponding registry migration (e.g., CORE_HISTORY_SCHEMA_MIGRATIONS or the intended prefix list), assert migration.description == registry.description, and assert the up handlers are the same (compare function pointers with std::ptr::eq or otherwise compare an identifiable property of the up handler exposed by the migration struct). This ensures silent drift in description or up logic is detected in addition to version mismatches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/data_connector/src/postgres_migrations.rs`:
- Around line 415-421: The test
postgres_history_migrations_cover_only_core_history_schema currently only
compares version numbers; update it to also verify description and the up
handler match the canonical registry entries: iterate
POSTGRES_HISTORY_MIGRATIONS and for each index look up the corresponding
registry migration (e.g., CORE_HISTORY_SCHEMA_MIGRATIONS or the intended prefix
list), assert migration.description == registry.description, and assert the up
handlers are the same (compare function pointers with std::ptr::eq or otherwise
compare an identifiable property of the up handler exposed by the migration
struct). This ensures silent drift in description or up logic is detected in
addition to version mismatches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 93c929e7-1ce4-43c7-87c2-fef50dabcd0d
📒 Files selected for processing (2)
crates/data_connector/src/oracle_migrations.rscrates/data_connector/src/postgres_migrations.rs
2 of 4 tasks
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
1 participant
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
Testing
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Tests
Chores