Page MenuHomePhabricator

[services][identity] refactor streaming RPCs to remove loops, use new client method to add users to DDB
ClosedPublic

Authored by varun on Feb 13 2023, 2:12 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 11:02 AM
Unknown Object (File)
Fri, Oct 18, 10:57 AM
Unknown Object (File)
Sep 29 2024, 12:46 AM
Subscribers

Details

Summary
  • due to how interconnected this code is, it was hard to split it out into more diffs
  • using the update_item API to add new users to the users table in DynamoDB was a mistake. we should be using put_item (hence why we now call the add_user_to_users_table fn, which uses the put_item API)
  • since we know exactly how many messages we'll receive in the stream, we can avoid looping through messages on the server side
Test Plan

Called register_user and login_user from main.rs and confirmed that both workflows still work

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Feb 13 2023, 2:29 PM

rearrange and rename structs

services/identity/src/database.rs
398 ↗(On Diff #22506)

made this pub so we can use it in service.rs

401 ↗(On Diff #22506)

Use our Error type here for better error handling in service.rs

services/identity/src/service.rs
797–805 ↗(On Diff #22506)

Using .0 and .1 to access fields in a tuple is bad for readability, so I've replaced the tuples with structs

jon requested changes to this revision.Feb 13 2023, 5:08 PM
jon added inline comments.
services/identity/src/service.rs
697 ↗(On Diff #22506)

This file is already getting to be ~800 lines long, do you mind moving these modules to separate files?

This revision now requires changes to proceed.Feb 13 2023, 5:08 PM

move submodules to separate files

Do we need to clone client and config so often? I'm assuming that config would be relatively static after creation. And some of the client clones seem to be unnecessary as they seem to fall out of scope from both scopes after their use.

services/identity/src/service.rs
797–805 ↗(On Diff #22506)

Yes, thank you.

services/identity/src/service/registration.rs
42 ↗(On Diff #22517)

Don't think the clone is necessary anymore, we already take ownership of it, and don't use it again in the function.

I think the best thing for pake_registration_start would be for it to take a reference to config, and just copy the key when needed.

In D6723#200503, @jon wrote:

Do we need to clone client and config so often? I'm assuming that config would be relatively static after creation. And some of the client clones seem to be unnecessary as they seem to fall out of scope from both scopes after their use.

Good catch! I removed all the unnecessary clones and changed the helper functions to take a reference to Config

Looks fine, probably a few ways to remove a lot of the boiler plate, but we can do that later.

This revision is now accepted and ready to land.Feb 20 2023, 12:18 PM