From 47530892a8a6b174786316c3881e22dc0864c859 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Wed, 5 Nov 2014 15:57:01 -0500 Subject: [PATCH] 3400: ArvadosResourceList returns all items (paging as necessary) unless limit() is specified. * ArvadosResourceList#each returns results incrementally. ArvadosResourceList#all (or ArvadosResourceList#results) prefetches the entire list. * Added tests. Fixed tests. * Removed hardcoded limit() in cases where it was apparent that it was being used as a workaround for the paging problem. * Removed obsolete name_link code (including CollectionsController#choose override) that was causing tests to fail. --- .../app/controllers/collections_controller.rb | 39 +---- .../app/controllers/users_controller.rb | 6 +- .../app/models/arvados_resource_list.rb | 144 ++++++++++++------ .../views/collections/_choose_rows.html.erb | 18 +-- .../views/collections/_index_tbody.html.erb | 2 +- .../views/collections/_show_recent.html.erb | 4 +- .../app/views/projects/_show_sharing.html.erb | 3 +- .../app/views/users/_show_admin.html.erb | 2 +- .../test/functional/users_controller_test.rb | 1 + .../test/unit/arvados_resource_list_test.rb | 39 +++++ 10 files changed, 154 insertions(+), 104 deletions(-) diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb index e869824be4..23a6896aca 100644 --- a/apps/workbench/app/controllers/collections_controller.rb +++ b/apps/workbench/app/controllers/collections_controller.rb @@ -45,34 +45,6 @@ class CollectionsController < ApplicationController end end - def choose - # Find collections using default find_objects logic, then search for name - # links, and preload any other links connected to the collections that are - # found. - # Name links will be obsolete when issue #3036 is merged, - # at which point this entire custom #choose function can probably be - # eliminated. - - params[:limit] ||= 40 - - find_objects_for_index - @collections = @objects - - @filters += [['link_class','=','name'], - ['head_uuid','is_a','arvados#collection']] - - @objects = Link - find_objects_for_index - - @name_links = @objects - - @objects = Collection. - filter([['uuid','in',@name_links.collect(&:head_uuid)]]) - - preload_links_for_objects (@collections.to_a + @objects.to_a) - super - end - def index # API server index doesn't return manifest_text by default, but our # callers want it unless otherwise specified. @@ -80,7 +52,7 @@ class CollectionsController < ApplicationController base_search = Collection.select(@select) if params[:search].andand.length.andand > 0 tags = Link.where(any: ['contains', params[:search]]) - @collections = (base_search.where(uuid: tags.collect(&:head_uuid)) | + @objects = (base_search.where(uuid: tags.collect(&:head_uuid)) | base_search.where(any: ['contains', params[:search]])). uniq { |c| c.uuid } else @@ -96,12 +68,11 @@ class CollectionsController < ApplicationController offset = 0 end - @collections = base_search.limit(limit).offset(offset) + @objects = base_search.limit(limit).offset(offset) end - @links = Link.limit(1000). - where(head_uuid: @collections.collect(&:uuid)) + @links = Link.where(head_uuid: @objects.collect(&:uuid)) @collection_info = {} - @collections.each do |c| + @objects.each do |c| @collection_info[c.uuid] = { tag_links: [], wanted: false, @@ -207,7 +178,7 @@ class CollectionsController < ApplicationController return super if !@object if current_user if Keep::Locator.parse params["uuid"] - @same_pdh = Collection.filter([["portable_data_hash", "=", @object.portable_data_hash]]).limit(1000) + @same_pdh = Collection.filter([["portable_data_hash", "=", @object.portable_data_hash]]) if @same_pdh.results.size == 1 redirect_to collection_path(@same_pdh[0]["uuid"]) return diff --git a/apps/workbench/app/controllers/users_controller.rb b/apps/workbench/app/controllers/users_controller.rb index 86e982368c..9f8f60907f 100644 --- a/apps/workbench/app/controllers/users_controller.rb +++ b/apps/workbench/app/controllers/users_controller.rb @@ -35,7 +35,7 @@ class UsersController < ApplicationController def activity @breadcrumb_page_name = nil - @users = User.limit(params[:limit] || 1000).all + @users = User.limit(params[:limit]).all @user_activity = {} @activity = { logins: {}, @@ -88,7 +88,7 @@ class UsersController < ApplicationController def storage @breadcrumb_page_name = nil - @users = User.limit(params[:limit] || 1000).all + @users = User.limit(params[:limit]).all @user_storage = {} total_storage = {} @log_date = {} @@ -159,7 +159,7 @@ class UsersController < ApplicationController @persist_state[uuid] = 'cache' end - Link.limit(1000).filter([['head_uuid', 'in', collection_uuids], + Link.filter([['head_uuid', 'in', collection_uuids], ['link_class', 'in', ['tag', 'resources']]]). each do |link| case link.link_class diff --git a/apps/workbench/app/models/arvados_resource_list.rb b/apps/workbench/app/models/arvados_resource_list.rb index 1a3c6b7e3c..6c80c02995 100644 --- a/apps/workbench/app/models/arvados_resource_list.rb +++ b/apps/workbench/app/models/arvados_resource_list.rb @@ -4,6 +4,7 @@ class ArvadosResourceList def initialize resource_class=nil @resource_class = resource_class + @fetch_multiple_pages = true end def eager(bool=true) @@ -44,17 +45,17 @@ class ArvadosResourceList end def where(cond) - cond = cond.dup - cond.keys.each do |uuid_key| - if cond[uuid_key] and (cond[uuid_key].is_a? Array or - cond[uuid_key].is_a? ArvadosBase) + @cond = cond.dup + @cond.keys.each do |uuid_key| + if @cond[uuid_key] and (@cond[uuid_key].is_a? Array or + @cond[uuid_key].is_a? ArvadosBase) # Coerce cond[uuid_key] to an array of uuid strings. This # allows caller the convenience of passing an array of real # objects and uuids in cond[uuid_key]. - if !cond[uuid_key].is_a? Array - cond[uuid_key] = [cond[uuid_key]] + if !@cond[uuid_key].is_a? Array + @cond[uuid_key] = [@cond[uuid_key]] end - cond[uuid_key] = cond[uuid_key].collect do |item| + @cond[uuid_key] = @cond[uuid_key].collect do |item| if item.is_a? ArvadosBase item.uuid else @@ -63,54 +64,113 @@ class ArvadosResourceList end end end - cond.keys.select { |x| x.match /_kind$/ }.each do |kind_key| - if cond[kind_key].is_a? Class - cond = cond.merge({ kind_key => 'arvados#' + arvados_api_client.class_kind(cond[kind_key]) }) + @cond.keys.select { |x| x.match /_kind$/ }.each do |kind_key| + if @cond[kind_key].is_a? Class + @cond = @cond.merge({ kind_key => 'arvados#' + arvados_api_client.class_kind(@cond[kind_key]) }) end end - api_params = { - _method: 'GET', - where: cond - } - api_params[:eager] = '1' if @eager - api_params[:limit] = @limit if @limit - api_params[:offset] = @offset if @offset - api_params[:select] = @select if @select - api_params[:order] = @orderby_spec if @orderby_spec - api_params[:filters] = @filters if @filters - res = arvados_api_client.api @resource_class, '', api_params - @results = arvados_api_client.unpack_api_response res + self + end + + def fetch_multiple_pages(f) + @fetch_multiple_pages = f self end def results - self.where({}) if !@results + if !@results + @results = [] + self.each_page do |r| + @results.concat r + end + end @results end def results=(r) @results = r + @items_available = r.items_available if r.respond_to? :items_available + @result_limit = r.limit if r.respond_to? :limit + @result_offset = r.offset if r.respond_to? :offset + @result_links = r.links if r.respond_to? :links + @results end def all - where({}) + results + self end - def each(&block) - results.each do |m| - block.call m - end - self + def to_ary + results end - def collect - results.collect do |m| - yield m + def each_page + api_params = { + _method: 'GET' + } + api_params[:where] = @cond if @cond + api_params[:eager] = '1' if @eager + api_params[:limit] = @limit if @limit + api_params[:select] = @select if @select + api_params[:order] = @orderby_spec if @orderby_spec + api_params[:filters] = @filters if @filters + + item_count = 0 + + if @offset + offset = @offset + else + offset = 0 end + + if @limit.is_a? Integer + items_to_get = @limit + else + items_to_get = 1000000000 + end + + begin + api_params[:offset] = offset + + res = arvados_api_client.api @resource_class, '', api_params + items = arvados_api_client.unpack_api_response res + + if items.nil? or items.size == 0 + break + end + + @items_available = items.items_available if items.respond_to? :items_available + @result_limit = items.limit + @result_offset = items.offset + @result_links = items.links if items.respond_to? :links + + item_count += items.size + + if items.respond_to? :items_available and + (@limit.nil? or (@limit.is_a? Integer and @limit > items.items_available)) + items_to_get = items.items_available + end + + offset = items.offset + items.size + + yield items + + end while @fetch_multiple_pages and item_count < items_to_get + self end - def first - results.first + def each(&block) + if not @results.nil? + @results.each &block + else + self.each_page do |items| + items.each do |i| + block.call i + end + end + end + self end def last @@ -129,32 +189,28 @@ class ArvadosResourceList end end - def to_ary - results - end - def to_hash - Hash[results.collect { |x| [x.uuid, x] }] + Hash[self.collect { |x| [x.uuid, x] }] end def empty? - results.empty? + self.first.nil? end def items_available - results.items_available if results.respond_to? :items_available + @items_available end def result_limit - results.limit if results.respond_to? :limit + @result_limit end def result_offset - results.offset if results.respond_to? :offset + @result_offset end def result_links - results.links if results.respond_to? :links + @result_links end # Return links provided with API response that point to the diff --git a/apps/workbench/app/views/collections/_choose_rows.html.erb b/apps/workbench/app/views/collections/_choose_rows.html.erb index da0f9759c1..bafc2cc209 100644 --- a/apps/workbench/app/views/collections/_choose_rows.html.erb +++ b/apps/workbench/app/views/collections/_choose_rows.html.erb @@ -1,4 +1,4 @@ -<% @collections.each do |object| %> +<% @objects.each do |object| %>
@@ -22,19 +22,3 @@ <% end %>
<% end %> - -<% @name_links.each do |name_link| %> - <% if (object = get_object(name_link.head_uuid)) %> -
- - <%= name_link.name %> - <% links_for_object(object).each do |tag| %> - <% if tag.link_class == 'tag' %> - <%= tag.name %> - <% end %> - <% end %> -
- <% end %> -<% end %> diff --git a/apps/workbench/app/views/collections/_index_tbody.html.erb b/apps/workbench/app/views/collections/_index_tbody.html.erb index 3d5c1c7a58..25d8796b41 100644 --- a/apps/workbench/app/views/collections/_index_tbody.html.erb +++ b/apps/workbench/app/views/collections/_index_tbody.html.erb @@ -1,4 +1,4 @@ -<% @collections.each do |c| %> +<% @objects.each do |c| %> diff --git a/apps/workbench/app/views/collections/_show_recent.html.erb b/apps/workbench/app/views/collections/_show_recent.html.erb index c958b290e8..6ebb3b2a28 100644 --- a/apps/workbench/app/views/collections/_show_recent.html.erb +++ b/apps/workbench/app/views/collections/_show_recent.html.erb @@ -17,7 +17,7 @@

-<%= render partial: "paging", locals: {results: @collections, object: @object} %> +<%= render partial: "paging", locals: {results: @objects, object: @object} %>

@@ -49,7 +49,7 @@
-<%= render partial: "paging", locals: {results: @collections, object: @object} %> +<%= render partial: "paging", locals: {results: @objects, object: @object} %> <% content_for :footer_js do %> $(document).on('click', 'form[data-remote] input[type=submit]', function() { diff --git a/apps/workbench/app/views/projects/_show_sharing.html.erb b/apps/workbench/app/views/projects/_show_sharing.html.erb index 95a7ee100d..cc862c425c 100644 --- a/apps/workbench/app/views/projects/_show_sharing.html.erb +++ b/apps/workbench/app/views/projects/_show_sharing.html.erb @@ -2,7 +2,7 @@ uuid_map = {} if @share_links [User, Group].each do |type| - type.limit(10000) + type .filter([['uuid','in',@share_links.collect(&:tail_uuid)]]) .each do |o| uuid_map[o.uuid] = o @@ -40,7 +40,6 @@ by_project: false, preview_pane: false, multiple: true, - limit: 10000, filters: choose_filters[share_class].to_json, action_method: 'post', action_href: share_with_project_path, diff --git a/apps/workbench/app/views/users/_show_admin.html.erb b/apps/workbench/app/views/users/_show_admin.html.erb index 4c76ede515..262dfa0687 100644 --- a/apps/workbench/app/views/users/_show_admin.html.erb +++ b/apps/workbench/app/views/users/_show_admin.html.erb @@ -47,7 +47,7 @@
<% permitted_group_perms = {} - Link.limit(10000).filter([ + Link.filter([ ['tail_uuid', '=', @object.uuid], ['head_uuid', 'is_a', 'arvados#group'], ['link_class', '=', 'permission'], diff --git a/apps/workbench/test/functional/users_controller_test.rb b/apps/workbench/test/functional/users_controller_test.rb index a734391e98..9f0c00bae9 100644 --- a/apps/workbench/test/functional/users_controller_test.rb +++ b/apps/workbench/test/functional/users_controller_test.rb @@ -32,6 +32,7 @@ class UsersControllerTest < ActionController::TestCase test "show repositories with read, write, or manage permission" do get :manage_account, {}, session_for(:active) + use_token :active assert_response :success repos = assigns(:my_repositories) assert repos diff --git a/apps/workbench/test/unit/arvados_resource_list_test.rb b/apps/workbench/test/unit/arvados_resource_list_test.rb index 619c346ccc..7a9a7d5ebc 100644 --- a/apps/workbench/test/unit/arvados_resource_list_test.rb +++ b/apps/workbench/test/unit/arvados_resource_list_test.rb @@ -37,4 +37,43 @@ class ResourceListTest < ActiveSupport::TestCase "Expected links_for to return all link_classes") end + test 'get all items by default' do + use_token :admin + a = 0 + Collection.where(owner_uuid: 'zzzzz-j7d0g-0201collections').each do + a += 1 + end + assert_equal 201, a + end + + test 'prefetch all items' do + use_token :admin + a = 0 + Collection.where(owner_uuid: 'zzzzz-j7d0g-0201collections').all.each do + a += 1 + end + assert_equal 201, a + end + + test 'get limited items' do + use_token :admin + a = 0 + Collection.where(owner_uuid: 'zzzzz-j7d0g-0201collections').limit(51).each do + a += 1 + end + assert_equal 51, a + end + + test 'get single page of items' do + use_token :admin + a = 0 + c = Collection.where(owner_uuid: 'zzzzz-j7d0g-0201collections').fetch_multiple_pages(false) + c.each do + a += 1 + end + + assert a < 201 + assert_equal c.result_limit, a + end + end -- 2.39.5