feat!: support for rolling back migrations#583
Conversation
|
| Project | cot |
| Branch | elijah/migrations-rollback |
| Testbed | github-ubuntu-latest |
🚨 1 Alert
| Benchmark | Measure Units | View | Benchmark Result (Result Δ%) | Upper Boundary (Limit %) |
|---|---|---|---|---|
| empty_router/empty_router | Latency milliseconds (ms) | 📈 plot 🚷 threshold 🚨 alert (🔔) | 13.74 ms(+89.52%)Baseline: 7.25 ms | 12.55 ms (109.52%) |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| empty_router/empty_router | 📈 view plot 🚷 view threshold 🚨 view alert (🔔) | 13.74 ms(+89.52%)Baseline: 7.25 ms | 12.55 ms (109.52%) |
| json_api/json_api | 📈 view plot 🚷 view threshold | 1.12 ms(+3.36%)Baseline: 1.08 ms | 1.37 ms (81.70%) |
| nested_routers/nested_routers | 📈 view plot 🚷 view threshold | 1.05 ms(+4.19%)Baseline: 1.01 ms | 1.24 ms (84.33%) |
| single_root_route/single_root_route | 📈 view plot 🚷 view threshold | 1.01 ms(+4.62%)Baseline: 0.97 ms | 1.21 ms (84.08%) |
| single_root_route_burst/single_root_route_burst | 📈 view plot 🚷 view threshold | 16.87 ms(-2.87%)Baseline: 17.37 ms | 21.69 ms (77.80%) |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
m4tx
left a comment
There was a problem hiding this comment.
Looks like a solid start, but we need more work on this before it can be merged.
| // name. | ||
| // TODO: cli command should take an explicit crate name as arg when workspaces | ||
| // are supported. | ||
| let crate_name = bootstrapper.project().cli_metadata().name; |
There was a problem hiding this comment.
I think we can still potentially want to rollback other libraries' migrations, so I guess we should still support other crates here. For instance, in the admin example we have on the repo, we should still support rolling back cot's m_0001_initial that creates cot__database_user. This is perhaps not terribly useful on its own, but if someone uses libraries that define some more complex migrations, that's more useful. And I believe should be fairly simple to implement, too.
Also, another problem with this in the current form is that we can have multiple migrations called m_0001_initial and it's inclear which one will be chosen.
| } | ||
| } | ||
|
|
||
| struct MigrationRollback; |
There was a problem hiding this comment.
I believe a few features would make it much more useful:
- A subcommand that would list all available migrations
- A dry run mode to see which migrations would actually be rolled back
- Actual log lines being displayed.
cot-clijusteprintlns the info when executingmigration make, which I think we could replicate here.
There was a problem hiding this comment.
For a subcommand to list all available migrations, I believe we already have that in cot-cli via cot migration list and I dont think we should duplicate that in cot crate. Perhaps this is where #587 can come in handy
| /// database or if there is an error while generating the migration | ||
| /// graph or if there is an error while unapplying a migration. | ||
| pub async fn rollback(&self, database: &Database, file: &str, app_name: &str) -> Result<()> { | ||
| info!("Rolling back migrations"); |
There was a problem hiding this comment.
Please add a TODO to add transactions here. And I should finally finish the work on #468...
Sadly, the lack of transactions means that we can potentially leave the database in an invalid state if one of the rollback fails. I'm wondering if the transactions would fix this issue, or are there any database engines that we support that don't support transactions on schema-changing operations? (SQLite could be one, but I'm not sure about the status in 2026)
- add an app flag which defaults to the current running app
|
|
||
| use anstyle::{AnsiColor, Color, Effects, Style}; | ||
|
|
||
| #[doc(hidden)] // Not part of Cot's public API; used by the CLI. |
There was a problem hiding this comment.
I'm wondering if it would make sense to take all the #[doc(hidden)] stuff and just move it into another crate. This shouldn't happen in this PR anyway, but this is just something that maybe we should do at some point.
| const OPERATIONS: &'static [Operation] = &[]; | ||
| } | ||
|
|
||
| struct RollbackApp1Initial; |
There was a problem hiding this comment.
I think all of the changes in the test module here should go into the tests/ directory. They are clearly integration tests (they involve multiple components: the migration engine, operations themselves, multiple database backends, applying the migrations, then rolling them back). This could even go into a completely separate file, considering how big the change here is and how much it tests. This should make the change much cleaner, and as an added bonus, the snapshot test output won't be too far away from the tests themselves.
| chrono = { workspace = true, features = ["alloc", "serde", "clock"] } | ||
| chrono-tz.workspace = true | ||
| clap.workspace = true | ||
| clap = {workspace = true, features = ["string"] } |
There was a problem hiding this comment.
nit:
| clap = {workspace = true, features = ["string"] } | |
| clap = { workspace = true, features = ["string"] } |
Related issue or discussion
fixes #547
Description
This allows rolling back migrations to the specified migration file.
Assuming the project has the following migration files in its migration dir:
The above command will roll back(unapply) the
m_0002_secondfile, leavingm_0001_initialapplied.Alternatively, the migration file number can be used for convenience:
Migrations are currently tied to a crate(the crate name is used), which means rolling back a migration in a project that imports apps from external crates will implicitly (and only) rollback migrations in the current project/crate. This might change when support for workspaces is added
As a bonus, this PR also adds CLI Group tasks as a means to group nested related subcommands under a parent subcommand:
Eg.
Type of change