[services][backup] Add helper utilities
ClosedPublic
Actions

Authored by bartek on Jan 9 2023, 4:00 AM.

Details

Reviewers

Commits

rCOMM9e8bcfef27fd: [services][backup] Add helper utilities

Summary

Added the following helpers:

generate_backup_id - generates IDs for new backups. C++ counterpart: link
generate_blob_holder - generates holder string for blob service. C++ counterpart: link
generate_random_string - random alphanumeric str of given length. C++ counterpart: link
BackupItem::new - database entity convenience constructor
handle_db_error - utility for mapping database errors into grpc status. It's almost copy-pasted from Identity service.

These are all needed for the CreateBackup handler.

Depends on D6196

Test Plan

These will be tested altogether with the CreateBackup endpoint. I'm wondering if adding unit tests is worth doing here.

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

bartek held this revision as a draft.

bartek published this revision for review.Jan 9 2023, 5:35 AM

Rebase

tomek added inline comments.

services/backup/src/service/handlers/create_backup.rs
84 ↗	(On Diff #20705)	This is different than what we had in the previous implementation - there was no separator. But I think that introducing it isn't a bad idea.

services/backup/src/service/handlers/create_backup.rs
84 ↗	(On Diff #20705)	Yeah, I introduced it for clarity, because for our integration tests, the backup id was: before: `device00001673363193561` - indistinguishable when deviceID ends and timestamp starts after: `device0000_1673363193561`

services/backup/src/database.rs
44 ↗	(On Diff #20705)	probably outside of the original scope. But what does a random string have to do with recovery_data? If this is being used as an identifer (which we already have a backup_id), why don't we use something like a UUID. or it's hash if we are describing data?

services/backup/src/database.rs
44 ↗	(On Diff #20705)	I don't have opinion on this, I'm only following behavior of the C++ service

varun requested changes to this revision.Jan 11 2023, 9:28 PM

varun added inline comments.

services/backup/src/service/handlers/create_backup.rs
84 ↗	(On Diff #20705)	i know we're trying to keep things consistent with the C++ implementation, but should we have a nondeterministic component to the backup_id?
services/backup/src/utils.rs
23 ↗	(On Diff #20705)	I think this function should take a second parameter `rng: &mut (impl Rng + CryptoRng)`. makes it easier if we need to change the rng later
24–28 ↗	(On Diff #20705)	this could just be `Alphanumeric.sample_string(rng, length)` I think

This revision now requires changes to proceed.Jan 11 2023, 9:28 PM

services/backup/src/database.rs
44 ↗	(On Diff #20705)	It looks like this is an artifact of Karol's initial implementation and is just mock data. In the initial diff, he had a "TODO add recovery data", but that seems to have been removed at some point
services/backup/src/service/handlers/create_backup.rs
84 ↗	(On Diff #20705)	The reason we have a timestamp is to differentiate from other backups. I don't think this counts as "nondeterministic" exactly... the ID is basically the device that generated the backup, along with the timestamp at which it started generating the backup. We want to support multiple backups for a given device (so that eg. we can start a new backup before we delete the old one, or in the future maybe we want to allow the user to preserve multiple backups from the past)

Refactored generate_random_string to take a rng argument and simplified its code
Added a note about recovery data being mocked and removed the unnecessary constant
Left the backup_id as is