Fix FKs on sqllite migrations #2083
Conversation
WalkthroughThe changes made to the database migration logic in Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- hscontrol/db/db.go (3 hunks)
Additional comments not posted (2)
hscontrol/db/db.go (2)
126-135: Verify the impact of always callingAutoMigratefor theRoutetable.The removal of invalid routes ensures data integrity. However, always calling
AutoMigratefor theRoutetable might have unintended side effects. Verify if this change affects other parts of the codebase or introduces any performance issues.Run the following script to verify the impact:
Verification successful
The impact of always calling
AutoMigratefor theRoutetable is isolated.The
AutoMigratecall for theRoutetable is only present in thehscontrol/db/db.gofile, indicating that the change is isolated to this location. There are no other occurrences in the codebase, suggesting that there are no broader implications or performance issues introduced by this change.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of always calling `AutoMigrate` for the `Route` table. # Test: Search for the `AutoMigrate` function usage. Expect: Only occurrences of the new usage. rg --type go -A 5 $'AutoMigrate(&types.Route{})'Length of output: 184
Script:
#!/bin/bash # Description: Verify the impact of always calling `AutoMigrate` for the `Route` table. # Test: Search for the `AutoMigrate` function usage with escaped parentheses. rg --type go -A 5 $'AutoMigrate\\(&types\\.Route\\{\\}\\)'Length of output: 297
543-605: Verify the correctness of temporarily disabling foreign key constraints.The
runMigrationsfunction improves the robustness of the migration process by ensuring that foreign key constraints are respected. However, the temporary disabling of foreign key constraints should be carefully reviewed to avoid potential data integrity issues.Run the following script to verify the correctness:
|
Much appreciated, thank you, I'm sorry for not getting to this quicker, a lot of things is taking my time so very happy with the help. Is there any way we can add a test that will fail without this change so we ensure changes to this code does not regress? Like using one of the databases added in the previous PR and validating that the FK is there? I'm happy to have a look when I next have time, but if you have an opportunity that would also be helpful. |
|
I guess the existing tests you've added will cover the regression case: without the proper FK handling, the tests will fail with an empty routes table now that the conditions on the AutoMigrate are removed. I suppose I could write a test that a particular constraint exists in a migrated database, but that seems like an overly specific test for a general fix. I guess the only proper test would be to ensure that the schema of a migrated DB and new DB are the same, but that seems like a lot of work. |
|
Sure, make sense, thank you. I will review this later today/tomorrow properly and get it in as the tests pass. |
My take on the right way to solve #2074 and #2063 that leaves the DB in a consistent state whether it's new or migrated
Summary by CodeRabbit
New Features
Bug Fixes