Details
-
Bug
-
Resolution: Unresolved
-
Critical
-
2.1.2
-
Security Level: Public
-
None
-
CBG Sprint 19
Description
user.GetRoles is currently panicking if a role can't be retrieved, but may be invoked in scenarios where that panic isn't recovered.
The benefit of using panic inside GetRoles is that we defer loading of roles on a user object, and subsequently have many different entry points to the user that can that trigger lazy loading. Using panic (and relying on the the REST handler to recover) makes the user/role code much cleaner - requiring an error check on every user API method that might trigger lazy loading adds a lot of undesirable complexity to the API (and its use).
Removing the deferred loading altogether isn't a good alternative - there are several REST API calls that only require authentication, and don't need to evaluate the users' channel set. We don't want to pay for the additional kv operations for role retrieval here.
For that reason, we may want to consider two other potential approaches:
- Include recover handling in goroutines spawned by REST API handlers
- _changes is the only current REST call that spawns a goroutine that doesn't include recovery and also invokes GetRoles
- This approach may be feasible in the short term, but leaves open the possibility of future misuse
- Remove the lazy loading, but add a second version of GetUser that doesn't instantiate roles, to be used in scenarios where the user channel set is known to be never needed. Users created with the new API would panic on invocation of GetRoles