[ARVADOS] created: 47530892a8a6b174786316c3881e22dc0864c859
git at public.curoverse.com
git at public.curoverse.com
Wed Nov 5 15:57:06 EST 2014
at 47530892a8a6b174786316c3881e22dc0864c859 (commit)
commit 47530892a8a6b174786316c3881e22dc0864c859
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Wed Nov 5 15:57:01 2014 -0500
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.
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index e869824..23a6896 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', at 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 86e9823..9f8f609 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 1a3c6b7..6c80c02 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 da0f975..bafc2cc 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| %>
<div class="row filterable selectable" data-object-uuid="<%= object.uuid %>"
data-preview-href="<%= chooser_preview_url_for object %>"
style="margin-left: 1em; border-bottom-style: solid; border-bottom-width: 1px; border-bottom-color: #DDDDDD">
@@ -22,19 +22,3 @@
<% end %>
</div>
<% end %>
-
-<% @name_links.each do |name_link| %>
- <% if (object = get_object(name_link.head_uuid)) %>
- <div class="row filterable selectable" data-object-uuid="<%= name_link.uuid %>"
- data-preview-href="<%= chooser_preview_url_for object %>"
- style="margin-left: 1em; border-bottom-style: solid; border-bottom-width: 1px; border-bottom-color: #DDDDDD">
- <i class="fa fa-fw fa-archive"></i>
- <%= name_link.name %>
- <% links_for_object(object).each do |tag| %>
- <% if tag.link_class == 'tag' %>
- <span class="label label-info"><%= tag.name %></span>
- <% end %>
- <% end %>
- </div>
- <% 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 3d5c1c7..25d8796 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| %>
<tr class="collection" data-object-uuid="<%= c.uuid %>">
<td>
diff --git a/apps/workbench/app/views/collections/_show_recent.html.erb b/apps/workbench/app/views/collections/_show_recent.html.erb
index c958b29..6ebb3b2 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 @@
</div>
<p/>
-<%= render partial: "paging", locals: {results: @collections, object: @object} %>
+<%= render partial: "paging", locals: {results: @objects, object: @object} %>
<div style="padding-right: 1em">
@@ -49,7 +49,7 @@
</div>
-<%= 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 95a7ee1..cc862c4 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', at 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 4c76ede..262dfa0 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 @@
</div>
<form>
<% 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 a734391..9f0c00b 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 619c346..7a9a7d5 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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list