[ARVADOS] updated: d07004646b375d69ca9d8467cedf9374dc9f6809

Git user git at public.curoverse.com
Thu Aug 31 10:39:49 EDT 2017


Summary of changes:
 .../api/app/controllers/application_controller.rb  |  2 +-
 .../controllers/arvados/v1/groups_controller.rb    | 15 ++++---
 services/api/app/models/arvados_model.rb           | 52 ++++++++++------------
 services/api/test/integration/groups_test.rb       | 16 ++++++-
 4 files changed, 47 insertions(+), 38 deletions(-)

       via  d07004646b375d69ca9d8467cedf9374dc9f6809 (commit)
      from  37fce338b38ca92d09ab2b58fc9b0f422ef7eb9d (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.


commit d07004646b375d69ca9d8467cedf9374dc9f6809
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 31 10:37:13 2017 -0400

    12032: Refactor readable_by to minimize subqueries.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 46b96a9..d09283d 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -180,7 +180,7 @@ class ApplicationController < ActionController::Base
   end
 
   def find_objects_for_index
-    @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name))})
+    @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || 'untrash' == action_name)})
     apply_where_limit_order_params
   end
 
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 9e0cdf8..8e195ff 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -163,13 +163,14 @@ class Arvados::V1::GroupsController < ApplicationController
         end
       end.compact
 
-      if klass == Collection and params[:include_trash]
-        @objects = klass.unscoped.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
-          order(request_order).where(where_conds)
-      else
-        @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
-          order(request_order).where(where_conds)
-      end
+      query_on = if klass == Collection and params[:include_trash]
+                   klass.unscoped
+                 else
+                   klass
+                 end
+      @objects = query_on.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
+                 order(request_order).where(where_conds)
+
       klass_limit = limit_all - all_objects.count
       @limit = klass_limit
       apply_where_limit_order_params klass
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 518d4c8..4a4f7ec 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -265,52 +265,46 @@ class ArvadosModel < ActiveRecord::Base
     # Check if any of the users are admin.
     if users_list.select { |u| u.is_admin }.any?
       if !include_trash
-        # exclude rows that are trashed.
+        # exclude rows that are explicitly trashed.
         if self.column_names.include? "owner_uuid"
-          sql_conds += ["NOT EXISTS(SELECT target_uuid
+          sql_conds += ["NOT EXISTS(SELECT 1
                   FROM permission_view
                   WHERE trashed = 1 AND
                   (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
         else
-          sql_conds += ["NOT EXISTS(SELECT target_uuid
+          sql_conds += ["NOT EXISTS(SELECT 1
                   FROM permission_view
                   WHERE trashed = 1 AND
                   (#{sql_table}.uuid = target_uuid))"]
         end
       end
     else
-      trash_clause = if !include_trash then "trashed = 0 AND" else "" end
-
       # Can read object (evidently a group or user) whose UUID is listed
       # explicitly in user_uuids.
       sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
-
-      direct_permission_check = "EXISTS(SELECT 1 FROM permission_view
-                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.uuid = target_uuid))"
+      trashed_check = if include_trash then "EXISTS" else "0 IN " end
+      perm_check = "perm_level >= 1"
 
       if self.column_names.include? "owner_uuid"
-        # if an explicit permission row exists for the uuid in question, apply
-        # the "direct_permission_check"
-        # if not, check for permission to read the owner instead
-        sql_conds += ["CASE
-                  WHEN EXISTS(select 1 FROM permission_view where target_uuid = #{sql_table}.uuid)
-                  THEN #{direct_permission_check}
-                  ELSE EXISTS(SELECT 1 FROM permission_view
-                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL))
-                  END"]
-        # Can also read if one of the users is the owner of the object.
-        trash_clause = if !include_trash
-                         "1 NOT IN (SELECT trashed
-                             FROM permission_view
-                             WHERE #{sql_table}.uuid = target_uuid) AND"
-                       else
-                         ""
-                       end
-        sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
+        # to be readable:
+        #   row(s) exists granting readable permission to uuid or owner, and none are trashed
+        sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+                      "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
+                      "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL)))"]
+        #   reader is owner, and item is not trashed
+        not_trashed_check = if include_trash then
+                              ""
+                            else
+                              "AND 1 NOT IN (SELECT MAX(trashed) FROM permission_view "+
+                                "WHERE #{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid)"
+                            end
+        sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_check})"]
       else
-        sql_conds += [direct_permission_check]
+        # to be readable:
+        #  * a non-trash row exists with readable permission uuid
+        sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+                                "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
+                                "#{sql_table}.uuid = target_uuid) "]
       end
 
       if sql_table == "links"
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index 6226ffd..c4ab3cf 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -99,7 +99,8 @@ class GroupsTest < ActionDispatch::IntegrationTest
   test "group contents with include trash collections" do
     get "/arvados/v1/groups/contents", {
       include_trash: "true",
-      filters: [["uuid", "is_a", "arvados#collection"]].to_json
+      filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+      limit: 1000
     }, auth(:active)
     assert_response 200
 
@@ -108,4 +109,17 @@ class GroupsTest < ActionDispatch::IntegrationTest
     assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
     assert_includes coll_uuids, collections(:expired_collection).uuid
   end
+
+  test "group contents without trash collections" do
+    get "/arvados/v1/groups/contents", {
+      filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+      limit: 1000
+    }, auth(:active)
+    assert_response 200
+
+    coll_uuids = []
+    json_response['items'].each { |c| coll_uuids << c['uuid'] }
+    assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
+    assert_not_includes coll_uuids, collections(:expired_collection).uuid
+  end
 end

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list