Page MenuHomePhabricator

[client-backup] implement initial restore backup protocol
ClosedPublic

Authored by kamil on Aug 29 2023, 2:47 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 1, 5:20 PM
Unknown Object (File)
Sun, Oct 27, 4:09 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:56 PM
Unknown Object (File)
Sep 27 2024, 10:50 PM
Subscribers

Details

Summary

Function performing restoring backup.
It's initial version which will return very detailed result of what went wrong - relevant message will be shown to user in rest of the stack.

This is complicated, but some things can work independently (eg. even is UserKeys download fails we still want to check if we can download UserData but we will never be able to decrypt it).

Depends on D9001

Test Plan

Call function and confirm result

Diff Detail

Repository
rCOMM Comm
Branch
client-backup
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Aug 29 2023, 3:36 AM
kamil added inline comments.
native/backup/use-client-backup.js
119

we use UserID, not username because it will require backup to ask identity for userID which is not ready for now

native/backup/use-client-backup.js
119

Assuming that it is correct that getBackupID does not take BackupAuth then we should let getBackupID and CommCoreModule promises run simultaneously.

138

getUserData and getUserKeysAuth look like independent network requests so for the sake of performance we should let those promises run simultaneously.

native/backup/use-client-backup.js
113

Is this only temporary? Should we also add accessToken to the if at the beginning?

119

That's correct although we should probably add a code comment with explanation that this is only for the initial backup version.

129

(not sure in which diff this function was defined, but my suggestions about endpoint naming also apply here)

native/backup/use-client-backup.js
119

Link to related discussion to give more context: https://phab.comm.dev/D8965#inline-57358

native/backup/use-client-backup.js
113
119

That's correct although we should probably add a code comment with explanation that this is only for the initial backup version.

good call

Assuming that it is correct that getBackupID does not take BackupAuth then we should let getBackupID and CommCoreModule promises run simultaneously.

I did it on purpose to then return very detailed result of which promises failed and which did not.

138

I did it on purpose to then return very detailed result of which promises failed and which did not.

native/backup/use-client-backup.js
138

I think you could still run them simultaneously if you wrap the whole try{}catch{} in async functions and run them with Promise.all

native/backup/use-client-backup.js
129

+1

138

This way should be possible - but it slightly decreases readability:

const userKeysResponsePromise = (async () => {
      try {
        const userKeysResponse = await getUserKeysAuth(backupID, backupAuth);
        result.getUserKeys = true;
        return userKeysResponse;
      } catch (e) {
        result.getUserKeys = false;
        result.error = e;
      }
})();

const userDataResponsePromise = // ...

const [userKeysResponse, userDataResponse] = await Promise.all([
  userKeysResponsePromise, userDataResponsePromise
]);
native/backup/use-client-backup.js
138

I think we should follow @bartek suggestion. This is both better for performance and it does not violate our code conventions that state we should never let two independent promises run sequentially.

Refactor code to run promises simultaneously.
It will be even cleaner when we will use Promise.allSettled, but current RN version doesn't support this :/

This revision is now accepted and ready to land.Sep 4 2023, 5:02 AM