tapd+sqlc: separate SQL migrations from code migrations #1881
Conversation
ViktorT-11
changed the title
Summary of ChangesHello @ViktorT-11, 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 implements a significant refactoring of the migration system within tapd to align with a new design philosophy that mandates a clear separation between SQL schema changes and application-level code migrations. By extracting existing code migrations from their original combined SQL/code versions (33 and 37) into new, standalone migration files (48 and 49), the system gains greater modularity and clarity. A key focus of this change is ensuring that this transition is seamless for existing users, with robust testing confirming that the re-execution of these now-separated code migrations will not introduce any unintended side effects or data modifications. Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Will address any failing CI checks on Monday. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively separates SQL migrations from code migrations for versions 33 and 37 by moving the code migration logic to new, dedicated migration versions 48 and 49. This is a solid architectural improvement that enhances the clarity and maintainability of the migration process.
The new tests, TestMigration48ScriptKeyTypeReplay and TestMigration49BurnReplay, are well-designed and crucial for ensuring backward compatibility. They correctly simulate the upgrade path for existing users and verify that re-running the code migrations is idempotent, which is essential for a safe transition.
However, there is one critical issue: the LatestMigrationVersion constant in tapdb/migrations.go has not been updated to 49. The comment for this constant explicitly states that it must be updated when new migrations are added. Failing to do so can compromise the database downgrade protection mechanism. Please ensure this constant is updated to reflect the new latest migration version.
Pull Request Test Coverage Report for Build 19631813200Details
💛 - Coveralls |
Previously, SQL migrations and code migrations have been designed to be a single migration version, where the SQL migration was run first, followed by a post step migration callback that executed the code migration. Due to a re-definition of the migration design, which separates SQL and code migrations, we need to update tapd to become compatible with the new migration design before updating to the new design. This commit therefore separates the code migrations from migration `33` & `37`, into separate migration files, and adds those two new migrations to the `tapdb/sqlc/migrations` directory. Additionally, since users that have already run the code migrations during migration `33` & `37` will have those migrations re-executed once more after updating to this PR, we test that re-execution of those code migrations will be a no-op for those users, and won't add or change any data in the database.
ViktorT-11
force-pushed
the
2025-11-separate-code-migrations
branch
from
0346e31 to
18e9478
Compare
With the separation of code migrations and SQL migrations in added with the previous commit, it now makes sense to not re-execute a code migration during a database downgrade. Therefore, we need to loosen the strictness of the check-migration-numbering.sh script, to allow for missing .down.sql files for code migrations specifically. This commit implements this change.
|
The failing tapgarden unit test passes for me locally on this branch. I'll kick the CI job again and see if it proves flaky here. (EDIT: yeah, it was a flake.) |
ffranr
left a comment
ffranr
left a comment
There was a problem hiding this comment.
Nice! I thought this would be a bigger change for some reason, nice surprise.
I think we should hold off on merging this until lightninglabs/migrate#3 is final.
| // Executing the code migration again (now at migration 48) should not | ||
| // change or add any new values than the values assigned when the code | ||
| // migration was run for migration version 33. | ||
| err = db.ExecuteMigrations(TargetLatest, WithPostStepCallbacks( | ||
| makePostStepCallbacks(db, postMigrationChecks), | ||
| )) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Target 48 explicitly here? TargetVersion(48) Or maybe that's not necessary and I'm missing something.
There was a problem hiding this comment.
Same below for TargetVersion(49).
There was a problem hiding this comment.
might be nice to have const values for migration numbers (33, 49 and 48, etc) in these unit tests. Start/replacement. Maybe?
| # Error if the .up.sql is an SQL migration, but we're missing the | ||
| # corresponding .down.sql. This doesn't apply if the .up.sql file is a code | ||
| # migration. | ||
| if is_sql_migration "$base_filename.up.sql" && \ | ||
| [ ! -f "$base_filename.down.sql" ]; then |
There was a problem hiding this comment.
Updated:
Do we need to include this commit? Is it required for your end goal?
We don’t often have no-op down migrations, but IMO it’s fine to include them. It keeps the migration sequence contiguous, and IMO it’s cleaner to always provide a down file for every up, even if the down is empty.
There was a problem hiding this comment.
Do we need to include this commit? Is it required for your end goal?
It’s not required, but I want to explain why I think this change is beneficial.
This change specifically targets code migrations, allowing them to omit a down.sql file when appropriate.
To illustrate why SQL and code migrations should behave differently, consider the following example of migrations:
-
0001_example.up.sql
CREATE TABLE example (id INTEGER PRIMARY KEY); -
0001_example.down.sql
DROP TABLE example; -
Followed by a code migration:
0002_example_code_migration.up.sql
(This migration inserts data into theexampletable.)
If we also add a down.sql for the code migration:
0002_example_code_migration.down.sql
(This would simply re-run the code migration and insert the data again.)
This highlights the mismatch:
- For SQL migrations, the
down.sqlundoes theup.sql. - For code migrations, a
down.sqlwould unintentionally re-execute the migration instead of undoing the changes.
In this example, the data inserted by the code migration would actually be removed only when the table itself is dropped by0001_example.down.sql.
Currently, the code migrations in tapd are structured as no-ops when re-executed, but that assumption can't be true for all code migrations. For example, it'd be much more complex for migrations like the kvdb → SQL migration in litd.
For that reason, I think the code migration itself should indicate whether a down.sql should be present or not, rather than having CI enforce for all types of migrations. The migrate library explicitly supports migrations without a down.sql file, and I think this is a case where that functionality makes sense to use.
There was a problem hiding this comment.
We don’t often have no-op down migrations, but IMO it’s fine to include them. It keeps the migration sequence contiguous, and IMO it’s cleaner to always provide a down file for every up, even if the down is empty.
And just to clarify, this if clause here will only trigger when the up.sql case contains any executable content, i.e. when the up.sql is an SQL migration. It's not checking if the down.sql file is empty.
jtobin
left a comment
jtobin
left a comment
There was a problem hiding this comment.
LGTM. 👍 👍 The only change I would suggest is to more explicitly reference the migration design change in your commit message (e.g. by issue/PR, etc.) so that the source is better-captured in the history.
|
Thanks for the reviews @ffranr & @jtobin 🎉! It'll address your feedback and rebase on |
4 participants
Previously, SQL migrations and code migrations have been designed to be a single migration version, where the SQL migration was run first, followed by a post step migration callback that executed the code migration.
Due to a re-definition of the migration design, which separates SQL and code migrations, we need to update tapd to become compatible with the new migration design before updating to the new design.
This PR therefore separates the code migrations from migration
33&37, into separate migration files, and adds those two new migrations to thetapdb/sqlc/migrationsdirectory.Additionally, since users that have already run the code migrations during migration
33&37will have those migrations re-executed once more after updating to this PR, we test that re-execution of those code migrations will be a no-op for those users, and won't add or change any data in the database.