[ARVADOS] created: ec7510c680ee2065d6372fef6a340ef754dbe724

Git user git at public.curoverse.com
Tue Jun 13 12:17:20 EDT 2017


        at  ec7510c680ee2065d6372fef6a340ef754dbe724 (commit)


commit ec7510c680ee2065d6372fef6a340ef754dbe724
Author: Tom Clegg <tom at curoverse.com>
Date:   Tue Jun 13 11:48:49 2017 -0400

    11803: Get group permissions with 1 query instead of N queries.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curoverse.com>

diff --git a/services/api/app/controllers/arvados/v1/repositories_controller.rb b/services/api/app/controllers/arvados/v1/repositories_controller.rb
index 183ed4d..a2c2528 100644
--- a/services/api/app/controllers/arvados/v1/repositories_controller.rb
+++ b/services/api/app/controllers/arvados/v1/repositories_controller.rb
@@ -4,15 +4,13 @@ class Arvados::V1::RepositoriesController < ApplicationController
   before_filter :admin_required, :only => :get_all_permissions
 
   def get_all_permissions
-    # users is a map of {user_uuid => User object}
-    users = {}
     # user_aks is a map of {user_uuid => array of public keys}
     user_aks = {}
     # admins is an array of user_uuids
     admins = []
-    User.eager_load(:authorized_keys).find_each do |u|
-      next unless u.is_active or u.uuid == anonymous_user_uuid
-      users[u.uuid] = u
+    User.
+      where('users.is_active = ? or users.uuid = ?', true, anonymous_user_uuid).
+      eager_load(:authorized_keys).find_each do |u|
       user_aks[u.uuid] = u.authorized_keys.collect do |ak|
         {
           public_key: ak.public_key,
@@ -21,6 +19,7 @@ class Arvados::V1::RepositoriesController < ApplicationController
       end
       admins << u.uuid if u.is_admin
     end
+    all_group_permissions = User.all_group_permissions
     @repo_info = {}
     Repository.eager_load(:permissions).find_each do |repo|
       @repo_info[repo.uuid] = {
@@ -42,8 +41,8 @@ class Arvados::V1::RepositoriesController < ApplicationController
           # A group has permission. Each user who has access to this
           # group also has access to the repository. Access level is
           # min(group-to-repo permission, user-to-group permission).
-          users.each do |user_uuid, user|
-            perm_mask = user.group_permissions[perm.tail_uuid]
+          user_aks.each do |user_uuid, _|
+            perm_mask = all_group_permissions[user_uuid][perm.tail_uuid]
             if not perm_mask
               next
             elsif perm_mask[:manage] and perm.name == 'can_manage'
@@ -54,7 +53,7 @@ class Arvados::V1::RepositoriesController < ApplicationController
               evidence << {name: 'can_read', user_uuid: user_uuid}
             end
           end
-        elsif users[perm.tail_uuid]
+        elsif user_aks.has_key?(perm.tail_uuid)
           # A user has permission; the user exists; and either the
           # user is active, or it's the special case of the anonymous
           # user which is never "active" but is allowed to read
@@ -66,7 +65,7 @@ class Arvados::V1::RepositoriesController < ApplicationController
       ([repo.owner_uuid] | admins).each do |user_uuid|
         # Except: no permissions for inactive users, even if they own
         # repositories.
-        next unless users[user_uuid]
+        next unless user_aks.has_key?(user_uuid)
         evidence << {name: 'can_manage', user_uuid: user_uuid}
       end
       # Distill all the evidence about permissions on this repository
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index d944474..f9308b1 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -140,11 +140,29 @@ class User < ArvadosModel
     end
   end
 
+  # Return a hash of {user_uuid: group_perms}
+  def self.all_group_permissions
+    install_view('permission')
+    all_perms = {}
+    ActiveRecord::Base.connection.
+      exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level)
+                  FROM permission_view
+                  WHERE target_owner_uuid IS NOT NULL
+                  GROUP BY user_uuid, target_owner_uuid',
+                  # "name" arg is a query label that appears in logs:
+                  "all_group_permissions",
+                  ).rows.each do |user_uuid, group_uuid, max_p_val|
+      all_perms[user_uuid] ||= {}
+      all_perms[user_uuid][group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
+    end
+    all_perms
+  end
+
   # Return a hash of {group_uuid: perm_hash} where perm_hash[:read]
   # and perm_hash[:write] are true if this user can read and write
   # objects owned by group_uuid.
   def calculate_group_permissions
-    install_view('permission')
+    self.class.install_view('permission')
 
     group_perms = {}
     ActiveRecord::Base.connection.
diff --git a/services/api/lib/can_be_an_owner.rb b/services/api/lib/can_be_an_owner.rb
index e9f016d..4375d77 100644
--- a/services/api/lib/can_be_an_owner.rb
+++ b/services/api/lib/can_be_an_owner.rb
@@ -4,6 +4,8 @@
 module CanBeAnOwner
 
   def self.included(base)
+    base.extend(ClassMethods)
+
     # Rails' "has_many" can prevent us from destroying the owner
     # record when other objects refer to it.
     ActiveRecord::Base.connection.tables.each do |t|
@@ -22,8 +24,28 @@ module CanBeAnOwner
     base.validate :restrict_uuid_change_breaking_associations
   end
 
+  module ClassMethods
+    def install_view(type)
+      conn = ActiveRecord::Base.connection
+      transaction do
+        # Check whether the temporary view has already been created
+        # during this connection. If not, create it.
+        conn.exec_query "SAVEPOINT check_#{type}_view"
+        begin
+          conn.exec_query("SELECT 1 FROM #{type}_view LIMIT 0")
+        rescue
+          conn.exec_query "ROLLBACK TO SAVEPOINT check_#{type}_view"
+          sql = File.read(Rails.root.join("lib", "create_#{type}_view.sql"))
+          conn.exec_query(sql)
+        ensure
+          conn.exec_query "RELEASE SAVEPOINT check_#{type}_view"
+        end
+      end
+    end
+  end
+
   def descendant_project_uuids
-    install_view('ancestor')
+    self.class.install_view('ancestor')
     ActiveRecord::Base.connection.
       exec_query('SELECT ancestor_view.uuid
                   FROM ancestor_view
@@ -59,22 +81,4 @@ module CanBeAnOwner
       self.owner_uuid = uuid
     end
   end
-
-  def install_view(type)
-    conn = ActiveRecord::Base.connection
-    self.class.transaction do
-      # Check whether the temporary view has already been created
-      # during this connection. If not, create it.
-      conn.exec_query "SAVEPOINT check_#{type}_view"
-      begin
-        conn.exec_query("SELECT 1 FROM #{type}_view LIMIT 0")
-      rescue
-        conn.exec_query "ROLLBACK TO SAVEPOINT check_#{type}_view"
-        sql = File.read(Rails.root.join("lib", "create_#{type}_view.sql"))
-        conn.exec_query(sql)
-      ensure
-        conn.exec_query "RELEASE SAVEPOINT check_#{type}_view"
-      end
-    end
-  end
 end

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list