Page MenuHomePhabricator

[native] Verify if the backup version is correct
Needs ReviewPublic

Authored by tomek on Mon, May 13, 3:30 AM.
Tags
None
Referenced Files
F1784140: D12013.id40108.diff
Sat, May 18, 12:00 PM
F1778568: version.png
Fri, May 17, 2:12 PM
Unknown Object (File)
Thu, May 16, 3:23 PM
Unknown Object (File)
Mon, May 13, 6:39 AM
Unknown Object (File)
Mon, May 13, 6:39 AM
Unknown Object (File)
Mon, May 13, 6:39 AM
Subscribers

Details

Reviewers
kamil
marcin
varun
Summary
Test Plan

Checked if restoration of the backup fails when a version of the backup is newer than the app.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Mon, May 13, 3:45 AM
kamil added inline comments.
native/backup/use-client-backup.js
93–96 ↗(On Diff #40108)

would love to see this using sqliteAPI and being wrapped in a utils function from /lib like verifyBackupVersion() to have a symmetric condition on both platforms but guess it's not that easy - mostly because in D11947 on web part this is done by the worker

97 ↗(On Diff #40108)

There is a difference between web and native about calling setPersistStorageItem - to unify that we first need ENG-3504

This revision is now accepted and ready to land.Tue, May 14, 11:35 PM
native/backup/use-client-backup.js
93–96 ↗(On Diff #40108)

This code will probably change as a result of a discussion from https://linear.app/comm/issue/ENG-7008/detect-that-the-compaction-is-from-a-newer-version-than-the-app-and#comment-2712fb37. But overall, I agree that making the code more symmetric is beneficial. On the other hand, this could get expensive.

native/backup/use-client-backup.js
93 ↗(On Diff #40227)

This approach is confusing to me. It looks like we need to let restoration happen to be able to learn whether we should have performed restore at all.

If we want to proceed this way we need to delete database in this case.

In the comment you mention that this will change due to discussion on Linear and you are going to modify rust to take expected database version. Is this modification going to include database deletion if there is a version mismatch? We can't let the app remain in a state where database is corrupted.

Check the version before restoring

tomek requested review of this revision.Fri, May 17, 2:09 PM
tomek added inline comments.
native/backup/use-client-backup.js
93 ↗(On Diff #40227)

I changed the approach - now we're checking the version before restoring.

tomek edited the test plan for this revision. (Show Details)