[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