Page MenuHomePhabricator

[services][identity] change DDB schema for public keys to map
ClosedPublic

Authored by varun on Sep 20 2022, 9:49 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 12:26 AM
Unknown Object (File)
Tue, Nov 19, 3:28 PM
Unknown Object (File)
Tue, Nov 12, 12:46 AM
Unknown Object (File)
Sun, Nov 10, 3:10 PM
Unknown Object (File)
Sat, Nov 9, 7:38 PM
Unknown Object (File)
Sat, Nov 9, 7:38 PM
Unknown Object (File)
Sat, Nov 9, 7:38 PM
Unknown Object (File)
Sat, Nov 9, 7:35 PM
Subscribers

Details

Summary

We need to store a public key for each device in the users table in ddb. Currently, we only store one public key. This diff changes the table schema to include a mapping of device IDs to public keys.

Test Plan

modified main.rs to call the update_users_table method, and verified that the map was updated appropriately when repeat and new keys were set in the update expression.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Sep 20 2022, 9:59 PM
tomek requested changes to this revision.Sep 21 2022, 4:03 AM

Do we have a schema file (e.g. terraform) that needs to be updated?

services/identity/src/database.rs
84–91 ↗(On Diff #16937)

How does this function work if there was a device id? Are we going to override it?

107–123 ↗(On Diff #16937)

Could you explain a little bit what's happening here?

109 ↗(On Diff #16937)

Why can't we just use USERS_TABLE_DEVICES_MAP_ATTRIBUTE_NAME?

131–143 ↗(On Diff #16937)

What happens when we use an empty hash map?

This revision now requires changes to proceed.Sep 21 2022, 4:03 AM
varun added inline comments.
services/identity/src/database.rs
84–91 ↗(On Diff #16937)

I don't fully understand your question. Are you asking if we would override the public key value associated with the device id if one already existed in ddb? The answer is yes. But we shouldn't have multiple public keys associated with a single device

107–123 ↗(On Diff #16937)

yes, sorry, could have annotated this better. basically, we have a map attribute in the users table called devices. nested within that, we have a bunch of maps attributes for each device. each of these inner maps contains a mapping from "userPublicKey" to the actual key value.

we have to use the "expression attribute name" feature of DDB to get around the fact that you can't use attribute names beginning with numbers in the actual expression.

109 ↗(On Diff #16937)

the way we're doing it here, this is what an item might look like:

{
userID: "123",
.
.
devices {
  999: {
    userPublicKey: "a;sldjf"
    }
  }
}

if we just used USERS_TABLE_DEVICES_MAP_ATTRIBUTE_NAME in the update expression, the item would look like:

{
userID: "123",
.
.
999: {
  userPublicKey: "a;sldjf"
  }
}

this looks a little strange, since the device ID is in the outermost scope of the item without any description of what it actually is

131–143 ↗(On Diff #16937)

DDB complains that the expression attribute names/values can't be empty. i.e., it can be None but it can't be Some(nothing)

jon added inline comments.
services/identity/src/constants.rs
14–15 ↗(On Diff #16937)

not related to changes, but these should probably be &'static str, which allows side stepping certain ownership scenarios

services/identity/src/database.rs
93–94 ↗(On Diff #16937)

Feel like we could just use a Vec<(String, String)> here for collecting the keys + values. Then to pass the keys/values separately, just need to do a let (expression_keys, expression_values) = expression_attributes.unzip()

https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.unzip

117 ↗(On Diff #16937)

feel like to_string() is more descriptive

119 ↗(On Diff #16937)

I think with the static life time, you might be able to get away with this

This revision now requires changes to proceed.Sep 22 2022, 11:50 AM
varun added inline comments.
services/identity/src/constants.rs
14–15 ↗(On Diff #16937)

sure. will add a diff before in the stack

services/identity/src/database.rs
93–94 ↗(On Diff #16937)

no, we need a HashMap to satisfy the parameter requirements for the aws_sdk_dynamodb::Client methods

117 ↗(On Diff #16937)

ack

119 ↗(On Diff #16937)

hmm no i don't think so. the set_expression_attribute_values method expects a HashMap<std::string::String, crate::model::AttributeValue>

services/identity/src/constants.rs
14–15 ↗(On Diff #16937)

wait constants are 'static by default. we don't need to change anything here

Feel like there's probably a more elegant way to do the Option<Hashmap<,>> logic, but nothing comes to my mind.

services/identity/src/constants.rs
14–15 ↗(On Diff #16937)

static has an inferred timeline of 'static. const just means can't be mut.

https://doc.rust-lang.org/rust-by-example/custom_types/constants.html

services/identity/src/database.rs
93–94 ↗(On Diff #16937)
services/identity/src/constants.rs
14–15 ↗(On Diff #16937)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=bc0042977b2d5ed5ce6b812e348651ab

as you can see in the above example, const also has a 'static lifetime

Thanks for the explanation - it makes a lot more sense to me now!

This revision is now accepted and ready to land.Sep 23 2022, 5:57 AM