Details

services/identity/src/client_service.rs
238–247 ↗	(On Diff #25463)	I think this should work, not a fan of a nested if let
services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	All users should have a user id, can we just make this `Result<String, Error>`? `Error::NotFound` relating to whatever would fit best
450 ↗	(On Diff #25463)	Similar to above, collapse `Result<Option<T>, E>` to `Result<T,E>`

This revision now requires changes to proceed.Apr 20 2023, 11:51 AM

address feedback

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	I don't think the db client should decide whether not getting back a user id is an error or not. that's up to the caller
450 ↗	(On Diff #25463)	same reasoning as above

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	I'm saying that a user entry will always have a user_id. If it's not there, then we can assume that it's an error.

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	ok, imagine we're doing some cleanup workflow and we want to make sure a user with username `foo` was deleted by calling this function. in this scenario, `Ok(None)` is the most appropriate return value, hence the `Option` within the `Result`.

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	maybe i'm not fully understanding your point... we can discuss over Comm if that's the case

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	Maybe you already discussed this elsewhere, just wanted to put my point here My reasoning of such cases is: If there's a legit possibility of having table entry without `user_id` (e.g. the user was deleted or sth), returning `None` is OK - the caller will decide what to do If all entries always must have the `user_id`, lack of this field means database inconsistency/corruption, then an error should be thrown.

services/identity/src/database.rs
431–445 ↗	(On Diff #25463)	We could just use the `username_available` method in that case, and we don't have to deal with ternary logic. Seems like are trying to find a problem for a potential solution.

I'm not sure the best path forward for handling the dynamodb logic, but in time I'm sure we will become more clear.

This revision is now accepted and ready to land.Apr 24 2023, 1:08 PM

This revision was automatically updated to reflect the committed changes.

[identity] login start
ClosedPublic
Actions