Details

Reviewers

kamil
michal
bartek

Commits

rCOMM9b9d3a542cb8: Implement tools to manage backup files

Summary

This differential introduces API that allows to get the path to a backup file associated with particular backup ID and to remove backup files directory at once during
log-out.

Test Plan

Call PlatformSpecificTools::getBackupFilePath() in C++ code which execution can be easily triggered manually (e.x. draft update).
Trigger execution of the code above.
Download application container from XCode or examine Device File Explorer in Android studio to ensure that empty backup directory was created.
Log out.
Examine application file system again. Ensure directory was deleted.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

marcin created this revision.Jan 2 2024, 4:18 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJan 2 2024, 4:18 AM

marcin added a child revision: D10503: Implement SQLiteQueryExecutor method to create main compaction.Jan 2 2024, 4:34 AM

marcin added a parent revision: D10450: Use SQLCipher as SQLite engine on web.Jan 2 2024, 4:57 AM

Rebase to update commit message

Harbormaster completed remote builds in B25424: Diff 35125.Jan 2 2024, 5:22 AM

Harbormaster completed remote builds in B25427: Diff 35128.Jan 2 2024, 6:04 AM

marcin requested review of this revision.Jan 2 2024, 6:04 AM

As we discussed in the office, the convention should be:

backup_1234 for compaction
backup_1234_attachments for attachments

where 1234 is the backup ID.

Also, initially, I thought it was better to implement getting only a directory in PlatformSpecificTools and create a file path in shared C++ implementation, but on the other hand, now are 100% confident about the separator correctness so it's probably okay as it is.

native/android/app/src/main/java/app/comm/android/fbjni/PlatformSpecificTools.java
41–42 ↗	(On Diff #35128)
52 ↗	(On Diff #35128)	shouldn't we throw if `mkdirs()` return `false`?
82 ↗	(On Diff #35128)	same as above, I think we should throw if delete returns `false`
native/ios/Comm/PlatformSpecificTools.mm
54 ↗	(On Diff #35128)	I think we can attach `err` message here to make it more descriptive
87–98 ↗	(On Diff #35128)	we can move it to a separate method and avoid code duplication with `getBackupFilePath()`

This revision now requires changes to proceed.Jan 5 2024, 4:20 AM

michal added inline comments.Jan 5 2024, 6:22 AM

native/android/app/src/main/java/app/comm/android/fbjni/PlatformSpecificTools.java
32 ↗	(On Diff #35128)	This probably also needs to be exposes to C++/Rust as I will need to iterate over the files in the directory.

Update backup files naming schema.
Make a public method to get just backup directory path
Improve error handling in Java files.

marcin added inline comments.Jan 8 2024, 4:00 AM

native/android/app/src/main/java/app/comm/android/fbjni/PlatformSpecificTools.java
54 ↗	(On Diff #35356)	Did a little research and it looks like methods we are using here are throwing one of two exceptions: `SecurityException` or `NullPointerException`. So by narrowing down the `catch` scope we can throw inside `try` without risking that we will catch our own exception in the same code that is throwing (which wouldn't make much sense).