[ARVADOS] updated: 1d4a39ab3e97c031683ada9f6c98e4c7365fa414
git at public.curoverse.com
git at public.curoverse.com
Mon Feb 9 17:10:23 EST 2015
Summary of changes:
.../app/controllers/projects_controller.rb | 4 +-
.../workbench/app/controllers/search_controller.rb | 3 +-
doc/api/methods/collections.html.textile.liquid | 2 +-
doc/api/methods/groups.html.textile.liquid | 9 +--
.../api/app/controllers/application_controller.rb | 19 ++++--
.../arvados/v1/collections_controller.rb | 7 +-
.../controllers/arvados/v1/groups_controller.rb | 76 ++++++++--------------
.../app/controllers/arvados/v1/users_controller.rb | 2 +-
services/api/app/models/arvados_model.rb | 6 ++
.../arvados/v1/groups_controller_test.rb | 17 ++++-
services/api/test/unit/arvados_model_test.rb | 22 +++++++
11 files changed, 90 insertions(+), 77 deletions(-)
via 1d4a39ab3e97c031683ada9f6c98e4c7365fa414 (commit)
via f6ab9be0046a6f8d760259c1a0eba8ab7c636903 (commit)
via 231242b6378abda494f2c684995519a259cfe174 (commit)
via 1963df31ffb7e95b72e53a0ec5c891f539b6dadb (commit)
via b59b310e23b588c4007af84741d4b94bc9f595f1 (commit)
from d6ec5672045b29aeaf983a78c5487ae354ccb20d (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 1d4a39ab3e97c031683ada9f6c98e4c7365fa414
Merge: d6ec567 f6ab9be
Author: Brett Smith <brett at curoverse.com>
Date: Mon Feb 9 17:10:03 2015 -0500
Merge branch '4464-api-project-contents-wip'
Refs #4464. Closes #5140.
commit f6ab9be0046a6f8d760259c1a0eba8ab7c636903
Author: Brett Smith <brett at curoverse.com>
Date: Mon Feb 9 16:40:08 2015 -0500
4464: Simplify API collection list method docs.
diff --git a/doc/api/methods/collections.html.textile.liquid b/doc/api/methods/collections.html.textile.liquid
index 8760fe8..f5e685e 100644
--- a/doc/api/methods/collections.html.textile.liquid
+++ b/doc/api/methods/collections.html.textile.liquid
@@ -55,7 +55,7 @@ table(table table-bordered table-condensed).
|filters|array|Conditions for filtering collections.|query||
|select|array|Data fields to return in the result list.|query|@["uuid", "manifest_text"]@|
-N.B.: Because adding access tokens to manifests can be computationally expensive, the @manifest_text@ field is not included in results by default. If you need it, pass a @select@ parameter that includes @manifest_text at .
+Note: Because adding access tokens to manifests can be computationally expensive, the @manifest_text@ field is not included in results by default. If you need it, pass a @select@ parameter that includes @manifest_text at .
h2. update
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index b9639aa..9f20a88 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -16,7 +16,7 @@ Required arguments are displayed in %{background:#ccffcc}green%.
h2. contents
-Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item).
+Retrieve a list of items owned by the group.
Arguments:
@@ -27,7 +27,7 @@ table(table table-bordered table-condensed).
|order|string|Order in which to return matching items. Sort within a resource type by prefixing the attribute with the resource name and a dot.|query|@"collections.modified_at desc"@|
|filters|array|Conditions for filtering items.|query|@[["uuid", "is_a", "arvados#job"]]@|
-N.B.: Because adding access tokens to manifests can be computationally expensive, the @manifest_text@ field is not included in listed collections. If you need it, request a "list of collections":{{site.baseurl}}/api/methods/collections.html with the filter @["owner_uuid", "=", GROUP_UUID]@, and @"manifest_text"@ listed in the select parameter.
+Note: Because adding access tokens to manifests can be computationally expensive, the @manifest_text@ field is not included in listed collections. If you need it, request a "list of collections":{{site.baseurl}}/api/methods/collections.html with the filter @["owner_uuid", "=", GROUP_UUID]@, and @"manifest_text"@ listed in the select parameter.
h2. create
commit 231242b6378abda494f2c684995519a259cfe174
Author: Brett Smith <brett at curoverse.com>
Date: Thu Feb 5 11:58:43 2015 -0500
4464: API group contents no longer include manifests.
This change achieves symmetry with the collections list method, which
doesn't return manifests unless you expressly request them. We don't
have a good way to support the select parameter in group contents at
all right now, so this commit also makes that clear through the
discovery document, and documents the limitation. This change should
help avoid performance problems like witnessed in #4464.
Some refactoring/clean-up to help support this change:
* Extract out a method that generates the full hash response to list
@objects. Reuse that code in the groups contents method.
* The collections controller had code to generate a list of a model's
selectable attributes. Move that to ArvadosModel and reuse it in
the groups contents method.
* Make signatures consistent across all definitions of apply_filters
and apply_where_limit_order_params. The latter simply passes
arguments to the former, so it can have the same signature, and it
should respect the provided model_class argument the same way. Same
for the override in the users controller.
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index e6e1e28..b9639aa 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -27,6 +27,8 @@ table(table table-bordered table-condensed).
|order|string|Order in which to return matching items. Sort within a resource type by prefixing the attribute with the resource name and a dot.|query|@"collections.modified_at desc"@|
|filters|array|Conditions for filtering items.|query|@[["uuid", "is_a", "arvados#job"]]@|
+N.B.: Because adding access tokens to manifests can be computationally expensive, the @manifest_text@ field is not included in listed collections. If you need it, request a "list of collections":{{site.baseurl}}/api/methods/collections.html with the filter @["owner_uuid", "=", GROUP_UUID]@, and @"manifest_text"@ listed in the select parameter.
+
h2. create
Create a new Group.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index a65bf3e..69c03bd 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -205,8 +205,9 @@ class ApplicationController < ActionController::Base
end
end
- def apply_where_limit_order_params *args
- apply_filters *args
+ def apply_where_limit_order_params model_class=nil
+ model_class ||= self.model_class
+ apply_filters model_class
ar_table_name = @objects.table_name
if @where.is_a? Hash and @where.any?
@@ -271,7 +272,7 @@ class ApplicationController < ActionController::Base
columns_list = @select.
flat_map { |attr| api_column_map[attr] }.
uniq.
- map { |s| "#{table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }
+ map { |s| "#{ar_table_name}.#{ActiveRecord::Base.connection.quote_column_name s}" }
@objects = @objects.select(columns_list.join(", "))
end
@@ -436,8 +437,8 @@ class ApplicationController < ActionController::Base
end
accept_param_as_json :reader_tokens, Array
- def render_list
- @object_list = {
+ def object_list
+ list = {
:kind => "arvados##{(@response_resource_name || resource_name).camelize(:lower)}List",
:etag => "",
:self_link => "",
@@ -446,11 +447,15 @@ class ApplicationController < ActionController::Base
:items => @objects.as_api_response(nil, {select: @select})
}
if @objects.respond_to? :except
- @object_list[:items_available] = @objects.
+ list[:items_available] = @objects.
except(:limit).except(:offset).
count(:id, distinct: true)
end
- send_json @object_list
+ list
+ end
+
+ def render_list
+ send_json object_list
end
def remote_ip
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 922fa6a..956de8e 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -33,10 +33,6 @@ class Arvados::V1::CollectionsController < ApplicationController
end
end
- def index
- super
- end
-
def find_collections(visited, sp, &b)
case sp
when ArvadosModel
@@ -184,8 +180,7 @@ class Arvados::V1::CollectionsController < ApplicationController
def load_limit_offset_order_params *args
if action_name == 'index'
# Omit manifest_text from index results unless expressly selected.
- @select ||= model_class.api_accessible_attributes(:user).
- map { |attr_spec| attr_spec.first.to_s } - ["manifest_text"]
+ @select ||= model_class.selectable_attributes - ["manifest_text"]
end
super
end
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 9d28be1..2356f2e 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -1,12 +1,14 @@
class Arvados::V1::GroupsController < ApplicationController
def self._contents_requires_parameters
- _index_requires_parameters.
+ params = _index_requires_parameters.
merge({
uuid: {
type: 'string', required: false, default: nil
},
})
+ params.delete(:select)
+ params
end
def render_404_if_no_object
@@ -30,22 +32,21 @@ class Arvados::V1::GroupsController < ApplicationController
end
def contents
- load_searchable_objects(owner_uuid: @object.andand.uuid)
- @object_list = {
- :kind => "arvados#objectList",
+ load_searchable_objects
+ send_json({
+ :kind => "arvados#objectList",
:etag => "",
:self_link => "",
:offset => @offset,
:limit => @limit,
:items_available => @items_available,
:items => @objects.as_api_response(nil)
- }
- send_json @object_list
+ })
end
protected
- def load_searchable_objects opts
+ def load_searchable_objects
all_objects = []
@items_available = 0
@@ -64,39 +65,31 @@ class Arvados::V1::GroupsController < ApplicationController
Job, PipelineInstance, PipelineTemplate,
Collection,
Human, Specimen, Trait].each do |klass|
- @objects = klass.readable_by(*@read_users)
- if klass == Group
- @objects = @objects.where(group_class: 'project')
- end
- if opts[:owner_uuid]
- conds = []
- cond_params = []
- conds << "#{klass.table_name}.owner_uuid = ?"
- cond_params << opts[:owner_uuid]
- if conds.any?
- cond_sql = '(' + conds.join(') OR (') + ')'
- @objects = @objects.where(cond_sql, *cond_params)
- end
- end
+ # If the currently requested orders specifically match the
+ # table_name for the current klass, apply that order.
+ # Otherwise, order by recency.
+ request_order =
+ request_orders.andand.find { |r| r =~ /^#{klass.table_name}\./i } ||
+ "created_at desc"
- # If the currently requested orders specifically match the table_name for the current klass, apply the order
- request_order = request_orders && request_orders.find{ |r| r =~ /^#{klass.table_name}\./i }
- if request_order
- @objects = @objects.order(request_order)
- else
- # default to created_at desc, ignoring any currently requested ordering because it doesn't apply to this klass
- @objects = @objects.order("#{klass.table_name}.created_at desc")
+ @select = nil
+ where_conds = {}
+ where_conds[:owner_uuid] = @object.uuid if @object
+ if klass == Collection
+ @select = klass.selectable_attributes - ["manifest_text"]
+ elsif klass == Group
+ where_conds[:group_class] = "project"
end
+ @objects = klass.readable_by(*@read_users).
+ order(request_order).where(where_conds)
@limit = limit_all - all_objects.count
apply_where_limit_order_params klass
- klass_items_available = @objects.
- except(:limit).except(:offset).
- count(:id, distinct: true)
+ klass_object_list = object_list
+ klass_items_available = klass_object_list[:items_available] || 0
@items_available += klass_items_available
@offset = [@offset - klass_items_available, 0].max
-
- all_objects += @objects.to_a
+ all_objects += klass_object_list[:items]
end
@objects = all_objects
diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 887a5e4..131ee52 100644
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -141,7 +141,7 @@ class Arvados::V1::UsersController < ApplicationController
}
end
- def apply_filters
+ def apply_filters(model_class=nil)
return super if @read_users.any? &:is_admin
if params[:uuid] != current_user.andand.uuid
# Non-admin index/show returns very basic information about readable users.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 9442a6f..5f9c014 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -56,6 +56,12 @@ class ArvadosModel < ActiveRecord::Base
"#{current_api_base}/#{self.class.to_s.pluralize.underscore}/#{self.uuid}"
end
+ def self.selectable_attributes(template=:user)
+ # Return an array of attribute name strings that can be selected
+ # in the given template.
+ api_accessible_attributes(template).map { |attr_spec| attr_spec.first.to_s }
+ end
+
def self.searchable_columns operator
textonly_operator = !operator.match(/[<=>]/)
self.columns.select do |col|
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 502c580..922612f 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -291,6 +291,20 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
end
end
+ test "Collection contents don't include manifest_text" do
+ authorize_with :active
+ get :contents, {
+ id: groups(:aproject).uuid,
+ filters: [["uuid", "is_a", "arvados#collection"]],
+ format: :json,
+ }
+ assert_response :success
+ refute(json_response["items"].any? { |c| not c["portable_data_hash"] },
+ "response included an item without a portable data hash")
+ refute(json_response["items"].any? { |c| c.include?("manifest_text") },
+ "response included an item with a manifest text")
+ end
+
test 'get writable_by list for owned group' do
authorize_with :active
get :show, {
diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb
index cb69127..750b933 100644
--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@@ -145,4 +145,26 @@ class ArvadosModelTest < ActiveSupport::TestCase
end
end
end
+
+ test "selectable_attributes includes database attributes" do
+ assert_includes(Job.selectable_attributes, "success")
+ end
+
+ test "selectable_attributes includes non-database attributes" do
+ assert_includes(Job.selectable_attributes, "node_uuids")
+ end
+
+ test "selectable_attributes includes common attributes in extensions" do
+ assert_includes(Job.selectable_attributes, "uuid")
+ end
+
+ test "selectable_attributes does not include unexposed attributes" do
+ refute_includes(Job.selectable_attributes, "nodes")
+ end
+
+ test "selectable_attributes on a non-default template" do
+ attr_a = Job.selectable_attributes(:common)
+ assert_includes(attr_a, "uuid")
+ refute_includes(attr_a, "success")
+ end
end
commit 1963df31ffb7e95b72e53a0ec5c891f539b6dadb
Author: Brett Smith <brett at curoverse.com>
Date: Wed Feb 4 15:46:25 2015 -0500
4464: Document API group contents parameters.
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index d4c14cd..e6e1e28 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -23,6 +23,9 @@ Arguments:
table(table table-bordered table-condensed).
|_. Argument |_. Type |_. Description |_. Location |_. Example |
{background:#ccffcc}.|uuid|string|The UUID of the group in question.|path||
+|limit|integer (default 100)|Maximum number of items to return.|query||
+|order|string|Order in which to return matching items. Sort within a resource type by prefixing the attribute with the resource name and a dot.|query|@"collections.modified_at desc"@|
+|filters|array|Conditions for filtering items.|query|@[["uuid", "is_a", "arvados#job"]]@|
h2. create
commit b59b310e23b588c4007af84741d4b94bc9f595f1
Author: Brett Smith <brett at curoverse.com>
Date: Wed Feb 4 15:46:04 2015 -0500
4464: Remove API group contents include_linked parameter.
With Tom's permission.
diff --git a/apps/workbench/app/controllers/projects_controller.rb b/apps/workbench/app/controllers/projects_controller.rb
index abba785..8c2f72e 100644
--- a/apps/workbench/app/controllers/projects_controller.rb
+++ b/apps/workbench/app/controllers/projects_controller.rb
@@ -157,7 +157,7 @@ class ProjectsController < ApplicationController
object.destroy
end
end
- while (objects = @object.contents(include_linked: false)).any?
+ while (objects = @object.contents).any?
objects.each do |object|
object.update_attributes! owner_uuid: current_user.uuid
end
@@ -198,7 +198,6 @@ class ProjectsController < ApplicationController
(val.is_a?(Array) ? val : [val]).each do |type|
objects = @object.contents(order: @order,
limit: @limit,
- include_linked: true,
filters: (@filters - kind_filters + [['uuid', 'is_a', type]]),
)
objects.each do |object|
@@ -236,7 +235,6 @@ class ProjectsController < ApplicationController
else
@objects = @object.contents(order: @order,
limit: @limit,
- include_linked: true,
filters: @filters,
offset: @offset)
@next_page_href = next_page_href(partial: :contents_rows,
diff --git a/apps/workbench/app/controllers/search_controller.rb b/apps/workbench/app/controllers/search_controller.rb
index 9e2ff1b..447f416 100644
--- a/apps/workbench/app/controllers/search_controller.rb
+++ b/apps/workbench/app/controllers/search_controller.rb
@@ -15,8 +15,7 @@ class SearchController < ApplicationController
end
@objects = search_what.contents(limit: @limit,
offset: @offset,
- filters: @filters,
- include_linked: true)
+ filters: @filters)
super
end
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index 478662e..d4c14cd 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -16,17 +16,13 @@ Required arguments are displayed in %{background:#ccffcc}green%.
h2. contents
-Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item) or a "name" link (i.e., a "name" link referencing the item).
+Retrieve a list of items which are associated with the given group by ownership (i.e., the group owns the item).
Arguments:
table(table table-bordered table-condensed).
|_. Argument |_. Type |_. Description |_. Location |_. Example |
{background:#ccffcc}.|uuid|string|The UUID of the group in question.|path||
-|include_linked|boolean|If false, results will only include items whose @owner_uuid@ attribute is the specified group. If true, results will additionally include items for which a "name" link exists.|path|{white-space:nowrap}. @false@ (default)
- at true@|
-
-If @include_linked@ is @true@, the @"links"@ field in the response will contain the "name" links referencing the objects in the @"items"@ field.
h2. create
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index c82ffb4..9d28be1 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -6,11 +6,6 @@ class Arvados::V1::GroupsController < ApplicationController
uuid: {
type: 'string', required: false, default: nil
},
- # include_linked returns name links, which are obsolete, so
- # remove it when clients have been migrated.
- include_linked: {
- type: 'boolean', required: false, default: false
- },
})
end
@@ -35,23 +30,11 @@ class Arvados::V1::GroupsController < ApplicationController
end
def contents
- # Set @objects:
- # include_linked returns name links, which are obsolete, so
- # remove it when clients have been migrated.
- load_searchable_objects(owner_uuid: @object.andand.uuid,
- include_linked: params[:include_linked])
- sql = 'link_class=? and head_uuid in (?)'
- sql_params = ['name', @objects.collect(&:uuid)]
- if @object
- sql += ' and tail_uuid=?'
- sql_params << @object.uuid
- end
- @links = Link.where sql, *sql_params
+ load_searchable_objects(owner_uuid: @object.andand.uuid)
@object_list = {
:kind => "arvados#objectList",
:etag => "",
:self_link => "",
- :links => @links.as_api_response(nil),
:offset => @offset,
:limit => @limit,
:items_available => @items_available,
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index c974076..502c580 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -75,7 +75,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
get :contents, {
id: groups(:aproject).uuid,
format: :json,
- include_linked: true,
}
check_project_contents_response
end
@@ -85,7 +84,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
get :contents, {
id: groups(:aproject).uuid,
format: :json,
- include_linked: true,
}
check_project_contents_response
end
@@ -176,7 +174,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
authorize_with :project_viewer
get :contents, {
format: :json,
- include_linked: false,
filters: [['uuid', 'is_a', 'arvados#specimen']]
}
assert_response :success
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list