Fix progress reporting for key import #4131
Conversation
BillCarsonFr
force-pushed
the
valere/backup/fix_import_keys_progress_report
branch
from
d75b11c
to
9b33030
Compare
BillCarsonFr
force-pushed
the
valere/backup/fix_import_keys_progress_report
branch
from
9b33030
to
7dcce0f
Compare
| const backupVersion = backupInfo.version!; | ||
| let success = 0; | ||
| let failures = 0; | ||
| const partialProgress = (stage: ImportRoomKeyProgressData): void => { |
There was a problem hiding this comment.
seems like stage is a confusing name, when ImportRoomKeyProgressData also has a property called stage ?
| let success = 0; | ||
| let failures = 0; |
There was a problem hiding this comment.
could we give these clearer names, like thisChunkSuccesses / thisChunkFailures ?
| let success = 0; | ||
| let failures = 0; | ||
| const partialProgress = (stage: ImportRoomKeyProgressData): void => { | ||
| success = stage.successes ?? 0; |
There was a problem hiding this comment.
I am led to wonder if we couldn't just change the type definition for ImportRoomKeyProgressData to make successes and failures mandatory.
| }); | ||
| totalImported += chunk.length; | ||
| totalImported += success; | ||
| totalFailures += failures; | ||
| } catch (e) { | ||
| totalFailures += chunk.length; |
There was a problem hiding this comment.
this looks kinda wrong now: we could import some keys and then throw an exception, so we would end up with success + failure > total.
Maybe:
| totalFailures += chunk.length; | |
| totalFailures += (chunk.length - success); |
| const backupVersion = backupInfo.version!; | ||
| let success = 0; | ||
| let failures = 0; | ||
| const partialProgress = (stage: ImportRoomKeyProgressData): void => { |
There was a problem hiding this comment.
I think it would be clearer to inline this function? YMMV
| @@ -265,6 +265,17 @@ export class RustBackupManager extends TypedEventEmitter<RustBackupCryptoEvents, | |||
| }, | |||
| backupVersion, | |||
| ); | |||
| // call the progress callback one last time with the final state | |||
There was a problem hiding this comment.
| // call the progress callback one last time with the final state | |
| // Call the progress callback one last time with the final state. |
There was a problem hiding this comment.
Could you also extend this comment to explain why this is required? It seems like OlmMachine.importBackedUpRoomKeys runs the callback after each key so it's not obvious.
| } | ||
| }); | ||
| // @ts-ignore - mock a private method for testing purpose | ||
| aliceCrypto.importBackedUpRoomKeys = importMockImpl; |
There was a problem hiding this comment.
we should definitely not do this. Doing so turns all of these tests from integration tests (where we test the whole stack, including the rust bits) into unit tests (where we are just testing the behaviour of MatrixClient).
To be honest, it is a problem that we have existing tests that mock out CryptoApi.importBackedUpRoomKeys but which claim to be integration tests, but that's a problem for another day.
| @@ -540,6 +551,85 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("megolm-keys backup (%s)", (backe | |||
| expect(result.imported).toStrictEqual(expectedTotal - decryptionFailureCount); | |||
| }); | |||
|
|
|||
| it("Should report failures when decryption works but import fails", async function () { | |||
| // @ts-ignore - mock a private method for testing purpose | |||
| aliceCrypto.importBackedUpRoomKeys = jest | |||
There was a problem hiding this comment.
Again: by mocking out importBackedUpRoomKeys, these are unit tests rather than integration tests. We have no real evidence that the actual RustCrypto implementation behaves the same way that these mocks do.
Could we not make up some data which causes RustCrypto.importBackedUpRoomKeys to report a failure, without having to mock this out?
richvdh
changed the title
|
@BillCarsonFr I'm going to close this for now as it seems to be bitrotting. |
|
wait, sorry, it was updated last week |
Fixes element-hq/element-web#27235
Checklist
public/exportedsymbols have accurate TSDoc documentation.