Page MenuHomePhabricator

[lib] Update DB version while running a migration

Authored by tomek on Wed, May 8, 5:13 AM.
Referenced Files
Unknown Object (File)
Fri, May 17, 5:03 PM
Unknown Object (File)
Wed, May 15, 5:55 PM
Unknown Object (File)
Mon, May 13, 4:23 PM
Unknown Object (File)
Sun, May 12, 5:04 PM
Unknown Object (File)
Sun, May 12, 2:22 AM
Unknown Object (File)
Fri, May 10, 4:22 AM
Unknown Object (File)
Fri, May 10, 4:21 AM
Unknown Object (File)
Fri, May 10, 4:20 AM



As a part of new migrations / backup approach, we have to store a version in SQLite. Updating this version should happen in the same transaction as all the other ops. In this new approach, we're introducing a new migrations spec - the functions return not only the state but also the ops that should be applied on the DB. Running the ops becomes a responsibility of createAsyncMigrate, which also adds one op that updates the DB version. From this point we shouldn't add any legacy migration. Also, store versions should match between the platforms.

Depends on D11936

Test Plan

Checked on both web and native:
created one legacy and one new migration, checked if both of them were run and that the DB version was updated in the store (killed and reopened the app, and then checked syncedMetadataStore).

Diff Detail

rCOMM Comm
No Lint Coverage
No Test Coverage

Event Timeline

tomek requested review of this revision.Wed, May 8, 5:30 AM
1305–1308 ↗(On Diff #39926)

Can you write a comment explaining what this is for?

526 ↗(On Diff #39926)

Why is this not 18?

526 ↗(On Diff #39926)

This is explained in ENG-6904

Restoration from an older backup (...) We will need to run Redux Persist migrations after restoration (...) We also need to store redux persist version to know where to start

I talked privately about this diff with @tomek and from what I can gather this diff correctly does what it is supposed to do so there is no reason not to accept it. However now there are two points that developers must be aware of:

  • Since this diff every SQLite migration altering db schema will need to be exposed as JSI that will be awaited inside migration lambda independently of ops. There are numerous ways to make it work. We can:
    • create plain JSI calls in CommCoreModule for every SQLite migration.
    • create separate TurboModule for migration JSI's
    • create one JSI in CommCoreModule that would take a number and mapping from number to C++ lambda directly inside SQLiteQueryExecutor. This one is probably the best as it is easily portable to web.
  • Since this diff every migration must be present on both native and web. Even if migration doesn't do anything on web (it is native specific) we have to create empty mock migration in the same way @tomek does in this diff.
This revision is now accepted and ready to land.Thu, May 9, 2:49 AM
kamil requested changes to this revision.Fri, May 10, 6:29 AM

Looks good, one question about error handling

103 ↗(On Diff #39926)

nit: feels like toString() is more suitable

507–510 ↗(On Diff #39926)

This is just a mock but it still processing store ops to bump verion, shouldn't we somehow use handleReduxMigrationFailure here?

Maybe there is a way to make it part of the spec so developers will not have to worry about failures each time adding migration?

This revision now requires changes to proceed.Fri, May 10, 6:29 AM
tomek marked 3 inline comments as done.

Handle migration errors. This required a lot of changes in types.


This prop is present in both web and native AppState and putting it here simplifies createAsyncMigrate function significantly. Haven't found any downside of putting it here.

507–510 ↗(On Diff #39926)

It's a little more than a mock - we use this migration to bump the version.

Maybe there is a way to make it part of the spec so developers will not have to worry about failures each time adding migration?

Yeah, makes sense. Going to add the error handling.

This revision is now accepted and ready to land.Thu, May 16, 10:08 AM