From: Peter Amstutz <peter.amstutz@curii.com>
Date: Mon, 2 Nov 2020 21:20:02 +0000 (-0500)
Subject: 17040: Cache results of User.group_permissions
X-Git-Tag: 2.2.0~238^2~2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/13fa7a29e911e4c0e2a73375e8d312c9b54da8c2

17040: Cache results of User.group_permissions

When requesting a list of groups (either directly, or with the
"shared" endpoint) it calls writable_by for each group.  This
indirectly calls User.group_permissions, which makes a database query.
When it does this for every group (with a database query each time),
which gets very expensive.

To address this, this commit caches the result of group_permissions on
the user object.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>
---

diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 0d7334e44e..eb6ff4c6b8 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -136,6 +136,7 @@ class Link < ArvadosModel
   def call_update_permissions
     if self.link_class == 'permission'
       update_permissions tail_uuid, head_uuid, PERM_LEVEL[name], self.uuid
+      current_user.forget_cached_group_perms
     end
   end
 
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 6f30b27a95..da7e7b310d 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -161,6 +161,10 @@ SELECT 1 FROM #{PERMISSION_VIEW}
     MaterializedPermission.where("user_uuid = ? and target_uuid != ?", uuid, uuid).delete_all
   end
 
+  def forget_cached_group_perms
+    @group_perms = nil
+  end
+
   def remove_self_from_permissions
     MaterializedPermission.where("target_uuid = ?", uuid).delete_all
     check_permissions_against_full_refresh
@@ -191,25 +195,35 @@ SELECT user_uuid, target_uuid, perm_level
   # and perm_hash[:write] are true if this user can read and write
   # objects owned by group_uuid.
   def group_permissions(level=1)
-    group_perms = {}
-
-    user_uuids_subquery = USER_UUIDS_SUBQUERY_TEMPLATE % {user: "$1", perm_level: "$2"}
+    @group_perms ||= {}
+    if @group_perms.empty?
+      user_uuids_subquery = USER_UUIDS_SUBQUERY_TEMPLATE % {user: "$1", perm_level: 1}
 
-    ActiveRecord::Base.connection.
-      exec_query(%{
+      ActiveRecord::Base.connection.
+        exec_query(%{
 SELECT target_uuid, perm_level
   FROM #{PERMISSION_VIEW}
-  WHERE user_uuid in (#{user_uuids_subquery}) and perm_level >= $2
+  WHERE user_uuid in (#{user_uuids_subquery}) and perm_level >= 1
 },
-                  # "name" arg is a query label that appears in logs:
-                  "User.group_permissions",
-                  # "binds" arg is an array of [col_id, value] for '$1' vars:
-                  [[nil, uuid],
-                   [nil, level]]).
-      rows.each do |group_uuid, max_p_val|
-      group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
+                   # "name" arg is a query label that appears in logs:
+                   "User.group_permissions",
+                   # "binds" arg is an array of [col_id, value] for '$1' vars:
+                   [[nil, uuid]]).
+        rows.each do |group_uuid, max_p_val|
+        @group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
+      end
+    end
+
+    case level
+    when 1
+      @group_perms
+    when 2
+      @group_perms.select {|k,v| v[:write] }
+    when 3
+      @group_perms.select {|k,v| v[:manage] }
+    else
+      raise "level must be 1, 2 or 3"
     end
-    group_perms
   end
 
   # create links
@@ -251,6 +265,8 @@ SELECT target_uuid, perm_level
       end
     end
 
+    forget_cached_group_perms
+
     return [repo_perm, vm_login_perm, group_perm, self].compact
   end
 
@@ -290,6 +306,7 @@ SELECT target_uuid, perm_level
     # mark the user as inactive
     self.is_admin = false  # can't be admin and inactive
     self.is_active = false
+    forget_cached_group_perms
     self.save!
   end
 
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index dc40f158ee..37e86976c1 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -149,6 +149,9 @@ module CurrentApiClient
       yield
     ensure
       Thread.current[:user] = user_was
+      if user_was
+        user_was.forget_cached_group_perms
+      end
     end
   end
 
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 5dc77cb98a..ee7dac4cd9 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -123,6 +123,7 @@ class ActiveSupport::TestCase
 
   def set_user_from_auth(auth_name)
     client_auth = api_client_authorizations(auth_name)
+    client_auth.user.forget_cached_group_perms
     Thread.current[:api_client_authorization] = client_auth
     Thread.current[:api_client] = client_auth.api_client
     Thread.current[:user] = client_auth.user