[lib] Update DB version while running a migration
AcceptedPublic
Actions

Authored by tomek on Wed, May 8, 5:13 AM.

Details

Reviewers

kamil
marcin
inka

Summary

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.

https://linear.app/comm/issue/ENG-7006/back-up-redux-version

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

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Wed, May 8, 5:13 AM

Herald added a subscriber: ashoat. · View Herald TranscriptWed, May 8, 5:13 AM

Harbormaster completed remote builds in B28756: Diff 39926.Wed, May 8, 5:30 AM

tomek requested review of this revision.Wed, May 8, 5:30 AM

inka added inline comments.Wed, May 8, 6:10 AM

native/redux/persist.js
1305–1308 ↗	(On Diff #39926)	Can you write a comment explaining what this is for?
web/redux/persist.js
526 ↗	(On Diff #39926)	Why is this not 18?

inka added inline comments.Wed, May 8, 6:27 AM

web/redux/persist.js
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

tomek added a child revision: D11942: [native] Set the DB version after registration.Wed, May 8, 6:59 AM

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

Looks good, one question about error handling

lib/shared/create-async-migrate.js
103 ↗	(On Diff #39926)	nit: feels like `toString()` is more suitable
web/redux/persist.js
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

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

tomek added inline comments.Wed, May 15, 7:42 AM

lib/types/redux-types.js
179	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.
web/redux/persist.js
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.