Adding a method to fetch a user's token from DynamoDB. The client will attempt to contruct and return a Token object from the DynamoDB item.
Details
Details
Successfully fetched tokens from DynamoDB with complete and incomplete data
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- varun/identity_service (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/identity/src/database.rs | ||
|---|---|---|
| 118–180 ↗ | (On Diff #12375) | Man, this gets really deeply nested... I know Rust makes this hard, but is there any way to reduce some of the nesting? |
| services/identity/src/database.rs | ||
|---|---|---|
| 91 ↗ | (On Diff #12375) | Wouldn't it be more readable to explicitly specify the collection's generic args? |
| 91–106 ↗ | (On Diff #12375) | Maybe it's worth mentioning which one of these is a partition key? I know it's intuitive to treat the first one in such a way, but still. I don't feel strongly, it's just an idea, so deferring to you. |
| 92–98 ↗ | (On Diff #12375) | This code's repeated here twice and I've also seen similar snippets around the arising(e.g. D3960) rust codebase. It looks like a boilerplate, maybe there is a way of creating some global function or something to make it shorter and avoid such explicit boilerplate? |
| 119–136 ↗ | (On Diff #12375) | I may be wrong but I have a feeling that I've seen this as well. Unpacking seems to also carry a burden of boilerplate. Isn't there a way of handling this using a shorter code? Or could we maybe create a set of tools for this (some global functions or something)? This code is rather simple but repetitive and long. |
| 162 ↗ | (On Diff #12375) | What exactly does error!(...) do? Will it break the execution here or are we going to return Ok regardless, just with None values? |
| 182 ↗ | (On Diff #12375) |
|
| services/identity/src/database.rs | ||
|---|---|---|
| 91 ↗ | (On Diff #12375) | ehhh i think it's pretty clear that the Key is a String and the Value is an AttributeValue. If you feel strongly about this I can construct the HashMap a different way, but this is way I usually see it done. |
| 91–106 ↗ | (On Diff #12375) | yeah a comment labeling the partition and sort keys makes sense |
| 92–98 ↗ | (On Diff #12375) | yeah let me try to refactor some of this... |
| 118–180 ↗ | (On Diff #12375) | I think we can reduce some of it. Will address in next revision |
| 119–136 ↗ | (On Diff #12375) | I think there's some similar code, I can try to refactor this |
| 162 ↗ | (On Diff #12375) | it's just a log line |
| 182 ↗ | (On Diff #12375) | I think I answered a similar question in an earlier diff. We're matching Ok(GetItemOutput { item: None, .. }), and _ is just shorthand since we don't care about the contents.
No, the situation described in the info log is the only possible one that we could encounter in this arm of the match statement. The compiler also ensures that all possible outcomes are handled (will throw an error if patterns are non-exhaustive). |
| services/identity/src/database.rs | ||
|---|---|---|
| 88–93 ↗ | (On Diff #12582) | How about storing values like
in some kind of "constants" module? |
| 121–139 ↗ | (On Diff #12582) | There's still a lot of boilerplate here. This pattern repeats a lot. I looked closer at this and it indeed seems to be hard to get rid of (please note however that I barely know rust). |
| 144 ↗ | (On Diff #12582) | What does * mean here? |
Comment Actions
Simplify code a little more
| services/identity/src/database.rs | ||
|---|---|---|
| 88–93 ↗ | (On Diff #12582) | I think this is a good idea but I'm going to defer it if that's alright. https://linear.app/comm/issue/ENG-1174/create-constants-module |
| 121–139 ↗ | (On Diff #12582) | the pattern repeats but i don't think the logic really does.. i'm definitely trying to make this code as DRY as possible |
| 144 ↗ | (On Diff #12582) | it's a dereferencing operation |
Comment Actions
Accepting, but pls respond to the doubt about dereferencing.
| services/identity/src/database.rs | ||
|---|---|---|
| 144 ↗ | (On Diff #12582) | Sorry, but I don't understand. So valid is Option<bool>, right (or not)? When I try to dereference something like this (I tried on number type), I get type Option<i32> cannot be dereferenced. Could you please describe what type is dereferenced and how it works because I seem to be missing something? Thanks. |
Comment Actions
The code seems ok, but is really hard to read. One possible solution might be to improve the formatting. Do we have some formatting rules, or are we using the default ones? Could we spend some time searching for something that is more readable?
| services/identity/src/database.rs | ||
|---|---|---|
| 105 ↗ | (On Diff #12965) | It took mi some time to notice that this is an opening bracket of let if. Shouldn't we place it at the end of the previous line? |
| 107 ↗ | (On Diff #12965) | Should we use the same code format rule as in other places, e.g. 80 char line limit? |
Comment Actions
Check out the rustfmt.toml file for our current formatting rules. We default to the Rust community's rules besides max_width and tab_spaces. Happy to add more rules if you think they'll improve readability. https://github.com/rust-lang/rustfmt/blob/master/Configurations.md
| services/identity/src/database.rs | ||
|---|---|---|
| 105 ↗ | (On Diff #12965) | I just use cargo fmt to format the code. I don't love how it looks but it's better than manually formatting. |
| 107 ↗ | (On Diff #12965) | We have a rustfmt.toml file where we set a rule max_width = 80 but rustfmt seems to ignore log lines... tried googling but couldn't find a quick answer. |
| 144 ↗ | (On Diff #12582) | valid is a &bool because we are borrowing it in the if let expression. therefore, to actually assign the value of valid to the new variable with the same name, we have to dereference it. |
| services/identity/src/database.rs | ||
|---|---|---|
| 144 ↗ | (On Diff #12582) | I see, thx. |
Comment Actions
First, break this into subroutines for readability and reuse across DB fetchers. Like you should have functions parseTimestampAttribute, parseAuthType, etc and call parseTimestampAttribute(item.remove("created")).
Second, I don't love how it's logging all these errors and stuff here. There's already duplication in that code and you're going to get more as you add more add more DB fetchers. You should be constructing error types that can display enough info to be useful and outside of this just call error!("user {}'s token for device {} is invalid: {}, user_id, device_id, err). I would add two more types
struct DBItemError {
attribute_errors: Vec<(&'static str, AttributeValue, DBItemAttributeError)>,
}
enum DBItemAttributeError {
Missing,
InvalidTimestamp(chrono::ParseError),
InvalidAuthType,
...
}And you should be able to Display DBItemError well enough.
| services/identity/src/database.rs | ||
|---|---|---|
| 151 ↗ | (On Diff #12965) | If you do item.remove("token") instead of .get you can avoid cloning the String You should do this for all fields |
| 197 ↗ | (On Diff #12965) | I would rename this to InvalidTimestamp(chrono::ParseError) |
Comment Actions
Addressed the feedback about subroutines. Improving error handling was making this diff really big, and it requires changes to other diffs in this stack, so I'm going to defer it for now if that's OK. Created this Linear issue to track: https://linear.app/comm/issue/ENG-1196/improve-error-handling-for-db-module