Fix DB downgrades to match creating from scratch #1428
Conversation
The `fromVersion` reference inside this test body, for each of the test cases, was referring to the same one, mutable `fromVersion` variable. So by the time any of them ran, its value was 5, and they tested migrating from 5 to 2, 5 to 3, 5 to 4, 5 to 5 respectively. Meanwhile the test names reflected the intended migrations, because the names were computed eagerly, reading the value of `fromVersion` before moving on to the next iteration. To fix the issue, make a fresh `fromVersion` binding inside each iteration of the loop.
Conceptually this is really a static fact about the class, not a fact about a given instance of it. (The implementation's body is always just a constant integer.) Make things a bit clearer -- and make it easier to get hold of the value from outside the class -- by making that explicit, with a static const field as the home of the value.
And adjust a test to make that true in tests too.
As explained in the parent commit, and asserted just above the _dropAndCreateAll call site, this fact always holds. That lets us simplify how downgrades work, and as a bonus cut one step from the checklist for making schema updates. This reverts some of the changes made in 601936d.
No transactions here will do any good if the changes aren't in a transaction with the `PRAGMA user_version =` statements that update the database's record of what version the schema is at, and therefore of which migrations might be needed. And if Drift is setting up for us such a transaction, it'll necessarily enclose the whole migration anyway.
| for (final toVersion in versions.skip(1)) { | ||
| final fromVersion = prev; |
There was a problem hiding this comment.
Interesting! Thanks for fixing that. This reminds me of the use of IIFEs that pass arguments to the enclosed scope.
There was a problem hiding this comment.
Yeah, exactly — that's another way to handle the same sort of situation.
| await m.createAll(); | ||
| // Corresponds to `from4to5` above. | ||
| await into(globalSettings).insert(GlobalSettingsCompanion()); | ||
| } |
There was a problem hiding this comment.
I briefly looked into beforeOpen since it is introduced as a way to "populate data after the database has been created", but I think this PR's approach is still better. Because it fully utilizes when the row needs to be inserted (beforeOpen is "called whenever the database is opened, regardless of whether a migration actually ran or not").
| /// This includes tables that aren't known to the schema, for example because | ||
| /// they were defined by a future (perhaps experimental) version of the app | ||
| /// before switching back to the version currently running. | ||
| static Future<void> _dropAll(Migrator m) async { |
There was a problem hiding this comment.
db: Cut transaction wrappers within migrations
Makes sense to me. I think drift doesn't set up a transaction for the entire migration.
If we really need one, we can probably wrap the entirety of onUpgrade (the line responsible for PRAGMA user_version = can be found in VersionedSchema.runMigrationSteps, which is called through Migrator.runMigrationSteps). While the transaction might prevent corrupting the database if a migration goes wrong, it's not so useful unless the app can tolerate/recover from said migration failure, which is not a goal of this PR.
|
Thanks for the review! Merging. |
Fixes #1427.
Prerequisite for #1421.
Selected commit messages
f644501 db test: Fix migration tests to use intended source versions
The
fromVersionreference inside this test body, for each of thetest cases, was referring to the same one, mutable
fromVersionvariable. So by the time any of them ran, its value was 5, and they
tested migrating from 5 to 2, 5 to 3, 5 to 4, 5 to 5 respectively.
Meanwhile the test names reflected the intended migrations, because
the names were computed eagerly, reading the value of
fromVersionbefore moving on to the next iteration.
To fix the issue, make a fresh
fromVersionbinding inside eachiteration of the loop.
8ed6c5b db [nfc]: Assert downgrades are always to current version
And adjust a test to make that true in tests too.
e90ea10 db [nfc]: Simplify downgrades by using that target is latest
As explained in the parent commit, and asserted just above the
_dropAndCreateAll call site, this fact always holds. That lets
us simplify how downgrades work, and as a bonus cut one step
from the checklist for making schema updates.
This reverts some of the changes made in 601936d.
4d8a667 db: Cut transaction wrappers within migrations
No transactions here will do any good if the changes aren't in a
transaction with the
PRAGMA user_version =statements that updatethe database's record of what version the schema is at, and therefore
of which migrations might be needed.
And if Drift is setting up for us such a transaction, it'll
necessarily enclose the whole migration anyway.
f036308 db: Fix downgrades so they match creating from scratch
Fixes #1427.