[ARVADOS] updated: 05dee1b692afbd5b424ab974e5b5f0cf566781e7

git at public.curoverse.com git at public.curoverse.com
Mon Jun 16 10:29:04 EDT 2014


Summary of changes:
 services/api/app/models/arvados_model.rb | 24 ++++++++-----
 services/api/app/models/collection.rb    | 61 ++++++++++++++------------------
 services/api/test/fixtures/links.yml     | 14 ++++++++
 services/api/test/unit/job_test.rb       | 17 +++++++--
 4 files changed, 71 insertions(+), 45 deletions(-)

       via  05dee1b692afbd5b424ab974e5b5f0cf566781e7 (commit)
       via  de3cefb72e49061e25b4f31afa845aae515cc4c4 (commit)
       via  d06e01ecf1e8463a78dbed33c243d60d3f6f3d24 (commit)
       via  b5a06a315323e050118cec999676d4a9aff105fa (commit)
      from  51c1f780628da8e0839a127963122ebb1b97020b (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 05dee1b692afbd5b424ab974e5b5f0cf566781e7
Author: Brett Smith <brett at curoverse.com>
Date:   Mon Jun 16 10:29:09 2014 -0400

    2879: Improve SQL for Docker image Collection search.
    
    * Search on Links joined with Collections, to simplify later logic.
    
    * Limit the search to Collections readable by specified users (by
      default, the current user).

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index bd2fb44..29a1f91 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -148,52 +148,52 @@ class Collection < ArvadosModel
     [hash_part, size_part].compact.join '+'
   end
 
-  def self.for_latest_docker_image(search_term, search_tag=nil)
-    base_search = self.joins(:links_via_head).order("links.created_at DESC")
+  def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil)
+    readers ||= [Thread.current[:user]]
+    base_search = Link.
+      readable_by(*readers, table_name: "collections").
+      joins("JOIN collections ON links.head_uuid = collections.uuid").
+      order("links.created_at DESC")
+
     # If the search term is a Collection locator with an associated
     # Docker image hash link, return that Collection.
     coll_matches = base_search.
-      where(uuid: search_term, links: {link_class: 'docker_image_hash'})
-    if coll = coll_matches.first
-      return coll
+      where(link_class: "docker_image_hash", collections: {uuid: search_term})
+    if match = coll_matches.first
+      return find_by_uuid(match.head_uuid)
     end
 
     # Find Collections with matching Docker image repository+tag pairs.
     repo_matches = base_search.
-      where(links: {link_class: 'docker_image_repo+tag',
-                    name: "#{search_term}:#{search_tag || 'latest'}"})
+      where(link_class: "docker_image_repo+tag",
+            name: "#{search_term}:#{search_tag || 'latest'}")
 
     # Find Collections with matching Docker image hashes, unless we're
     # obviously doing a repo+tag search and already found a match that way.
-    if search_tag.nil? or repo_matches.first.nil?
+    if search_tag.nil? or repo_matches.empty?
       hash_matches = base_search.
-        where("links.link_class = ? and links.name LIKE ?",
+        where("link_class = ? and name LIKE ?",
               "docker_image_hash", "#{search_term}%")
     else
       hash_matches = nil
     end
 
     # Select the image that was created most recently from both repo
-    # and hash matches.
-    latest_image = nil
+    # and hash matches.  Note that the SQL search order and fallback
+    # timestamp values are chosen so that if image timestamps are
+    # missing, we use the image with the newest link.
+    latest_image_link = nil
     latest_image_timestamp = "1900-01-01T00:00:00Z"
     [repo_matches, hash_matches].compact.each do |search_result|
-      search_result.find_each do |coll|
-        coll_latest_timestamp = "1900-01-01T00:00:00Z"
-
-        coll.links_via_head.find_each do |link|
-          this_link_timestamp = link.properties["image_timestamp"]
-          if (this_link_timestamp.andand > coll_latest_timestamp)
-            coll_latest_timestamp = this_link_timestamp
-          end
-        end
-
-        if coll_latest_timestamp > latest_image_timestamp
-          latest_image = coll
-          latest_image_timestamp = coll_latest_timestamp
+      search_result.find_each do |link|
+        link_timestamp = link.properties.fetch("image_timestamp",
+                                               "1900-01-01T00:00:01Z")
+        if link_timestamp > latest_image_timestamp
+          latest_image_link = link
+          latest_image_timestamp = link_timestamp
         end
       end
     end
-    latest_image
+    latest_image_link.nil? ? nil : find_by_uuid(latest_image_link.head_uuid)
   end
 end
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index d5496a6..0655785 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -441,6 +441,20 @@ bug2931_link_with_null_head_uuid:
   head_uuid: ~
   properties: {}
 
+active_user_permission_to_docker_image_collection:
+  uuid: zzzzz-o0j2j-dp1d8395ldqw33s
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-000000000000000
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  link_class: permission
+  name: can_read
+  head_uuid: fa3c1a9cb6783f85f2ecda037e07b8c3+167
+  properties: {}
+
 docker_image_collection_hash:
   uuid: zzzzz-o0j2j-dockercollhasha
   owner_uuid: zzzzz-tpzed-000000000000000
diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb
index 4f3e8f3..5f53b2a 100644
--- a/services/api/test/unit/job_test.rb
+++ b/services/api/test/unit/job_test.rb
@@ -3,6 +3,10 @@ require 'test_helper'
 class JobTest < ActiveSupport::TestCase
   BAD_COLLECTION = "#{'f' * 32}+0"
 
+  setup do
+    set_user_from_auth :active
+  end
+
   test "Job without Docker image doesn't get locator" do
     job = Job.new
     assert job.valid?

commit de3cefb72e49061e25b4f31afa845aae515cc4c4
Author: Brett Smith <brett at curoverse.com>
Date:   Mon Jun 16 10:27:38 2014 -0400

    2879: Let ArvadosModel.readable_by callers specify table name.
    
    This makes it possible to use readable_by in queries with arbitrary
    joins.

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 95fd055..2df6686 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -91,6 +91,13 @@ class ArvadosModel < ActiveRecord::Base
     # Get rid of troublesome nils
     users_list.compact!
 
+    # Load optional keyword arguments, if they exist.
+    if users_list.last.is_a? Hash
+      kwargs = users_list.pop
+    else
+      kwargs = {}
+    end
+
     # Check if any of the users are admin.  If so, we're done.
     if users_list.select { |u| u.is_admin }.empty?
 
@@ -101,6 +108,7 @@ class ArvadosModel < ActiveRecord::Base
         collect { |uuid| sanitize(uuid) }.join(', ')
       sql_conds = []
       sql_params = []
+      sql_table = kwargs.fetch(:table_name, table_name)
       or_object_uuid = ''
 
       # This row is owned by a member of users_list, or owned by a group
@@ -113,25 +121,25 @@ class ArvadosModel < ActiveRecord::Base
       # to this row, or to the owner of this row (see join() below).
       permitted_uuids = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (#{sanitized_uuid_list}))"
 
-      sql_conds += ["#{table_name}.owner_uuid in (?)",
-                    "#{table_name}.uuid in (?)",
-                    "#{table_name}.uuid IN #{permitted_uuids}"]
+      sql_conds += ["#{sql_table}.owner_uuid in (?)",
+                    "#{sql_table}.uuid in (?)",
+                    "#{sql_table}.uuid IN #{permitted_uuids}"]
       sql_params += [uuid_list, user_uuids]
 
-      if self == Link and users_list.any?
+      if sql_table == "links" and users_list.any?
         # This row is a 'permission' or 'resources' link class
         # The uuid for a member of users_list is referenced in either the head
         # or tail of the link
-        sql_conds += ["(#{table_name}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND (#{table_name}.head_uuid IN (?) OR #{table_name}.tail_uuid IN (?)))"]
+        sql_conds += ["(#{sql_table}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND (#{sql_table}.head_uuid IN (?) OR #{sql_table}.tail_uuid IN (?)))"]
         sql_params += [user_uuids, user_uuids]
       end
 
-      if self == Log and users_list.any?
+      if sql_table == "logs" and users_list.any?
         # Link head points to the object described by this row
-        sql_conds += ["#{table_name}.object_uuid IN #{permitted_uuids}"]
+        sql_conds += ["#{sql_table}.object_uuid IN #{permitted_uuids}"]
 
         # This object described by this row is owned by this user, or owned by a group readable by this user
-        sql_conds += ["#{table_name}.object_owner_uuid in (?)"]
+        sql_conds += ["#{sql_table}.object_owner_uuid in (?)"]
         sql_params += [uuid_list]
       end
 

commit d06e01ecf1e8463a78dbed33c243d60d3f6f3d24
Author: Brett Smith <brett at curoverse.com>
Date:   Mon Jun 16 09:17:30 2014 -0400

    2879: Generalize Job docker_image_locator protection tests.
    
    Refs #2879 - the test for MassAssignmentError is not passing for
    Peter.  I want to try this approach to see if a more general test is
    all we need.

diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb
index 2da1e22..4f3e8f3 100644
--- a/services/api/test/unit/job_test.rb
+++ b/services/api/test/unit/job_test.rb
@@ -84,15 +84,22 @@ class JobTest < ActiveSupport::TestCase
   end
 
   test "can't create Job with Docker image locator" do
-    assert_raises(ActiveModel::MassAssignmentSecurity::Error) do
-      Job.new(docker_image_locator: BAD_COLLECTION)
+    begin
+      job = Job.new(docker_image_locator: BAD_COLLECTION)
+    rescue ActiveModel::MassAssignmentSecurity::Error
+      # Test passes - expected attribute protection
+    else
+      assert_nil job.docker_image_locator
     end
   end
 
   test "can't assign Docker image locator to Job" do
     job = Job.new
-    assert_raises(NoMethodError) do
+    begin
       Job.docker_image_locator = BAD_COLLECTION
+    rescue NoMethodError
+      # Test passes - expected attribute protection
     end
+    assert_nil job.docker_image_locator
   end
 end

commit b5a06a315323e050118cec999676d4a9aff105fa
Author: Brett Smith <brett at curoverse.com>
Date:   Fri Jun 13 16:16:27 2014 -0400

    2879: Revert previous commit.
    
    This was an effort to better match Docker's behavior.  But it turns
    out `docker run` always looks for an image tagged 'latest' if you
    don't specify, so the previous behavior is actually closer.

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 898127b..bd2fb44 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -158,17 +158,10 @@ class Collection < ArvadosModel
       return coll
     end
 
-    # Find Collections with a matching Docker image repository,
-    # plus the tag pair if provided.
-    if search_tag.nil?
-      repo_matches = base_search.
-        where(links: {link_class: 'docker_image_repository',
-                      name: search_term})
-    else
-      repo_matches = base_search.
-        where(links: {link_class: 'docker_image_repo+tag',
-                      name: "#{search_term}:#{search_tag || 'latest'}"})
-    end
+    # Find Collections with matching Docker image repository+tag pairs.
+    repo_matches = base_search.
+      where(links: {link_class: 'docker_image_repo+tag',
+                    name: "#{search_term}:#{search_tag || 'latest'}"})
 
     # Find Collections with matching Docker image hashes, unless we're
     # obviously doing a repo+tag search and already found a match that way.

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list