[ARVADOS] created: 84c0cf9978b38a8643815345797775a4d8a91c64

git at public.curoverse.com git at public.curoverse.com
Thu Feb 5 12:23:25 EST 2015


        at  84c0cf9978b38a8643815345797775a4d8a91c64 (commit)


commit 84c0cf9978b38a8643815345797775a4d8a91c64
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 a9e07ecb8ff1ca421e9dbf34ce7042e2f880d72c
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 734ded9afee344d294a3363a9a67b9628cba81f1
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