Uploaded image for project: 'Couchbase Gateway'
  1. Couchbase Gateway
  2. CBG-308

Avoid panic on failed role retrieval

    XMLWordPrintable

Details

    • Bug
    • Resolution: Unresolved
    • Critical
    • 2.5.0
    • 2.1.2
    • SyncGateway
    • 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

       

       

       

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            adamf Adam Fraser
            adamf Adam Fraser
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

              Created:
              Updated:

              Gerrit Reviews

                There are no open Gerrit changes

                PagerDuty