Details
Checked if restoration of the backup fails when a version of the backup is newer than the app.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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 |
| 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. |
| native/backup/use-client-backup.js | ||
|---|---|---|
| 93 ↗ | (On Diff #40227) | I changed the approach - now we're checking the version before restoring. |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1855 ↗ | (On Diff #40346) | shouldn't we throw in this case? |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1852 ↗ | (On Diff #40346) | This isn't very readable. It looks that you expect entries to be exactly one element array. it would be better to just check for that and throw an error if this requirement is not satisfied. |
| native/cpp/CommonCpp/DatabaseManagers/SQLiteQueryExecutor.cpp | ||
|---|---|---|
| 1852 ↗ | (On Diff #40346) | Alternatively if your requirement is not that strict - you let entires array be either empty or one element than you can just update this code to return optional. Returning -1 is very C-style low level |