[ARVADOS] updated: cc6f3141d4e55c8adab282762bad9a8643336346

git at public.curoverse.com git at public.curoverse.com
Thu Aug 14 17:07:18 EDT 2014


Summary of changes:
 .../arvados/v1/collections_controller.rb           |  5 +--
 services/api/app/models/arvados_model.rb           |  4 +--
 services/api/app/models/collection.rb              |  6 ++--
 services/api/app/models/job.rb                     |  3 +-
 .../20140811184643_collection_use_regular_uuids.rb | 19 +++++-----
 services/api/lib/has_uuid.rb                       | 41 +++++++++++++++++++---
 services/api/test/fixtures/collections.yml         |  2 +-
 .../arvados/v1/collections_controller_test.rb      |  6 ++--
 services/api/test/unit/arvados_model_test.rb       |  3 +-
 services/api/test/unit/job_test.rb                 | 14 ++++----
 10 files changed, 65 insertions(+), 38 deletions(-)

       via  cc6f3141d4e55c8adab282762bad9a8643336346 (commit)
      from  22457e13e9c758f2c4a38326fa181607997bf023 (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 cc6f3141d4e55c8adab282762bad9a8643336346
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 14 17:07:12 2014 -0400

    3036: Split out logic to determine if a user is actually allowed to set the
    uuid into validate_uuid.  Fix docker/collection/job tests to use
    portable_data_hash instead of uuid where appropriate.

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 0ed5fd0..a37290c 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -51,10 +51,7 @@ class Arvados::V1::CollectionsController < ApplicationController
       end
     }
 
-    # Save the collection with the stripped manifest.
-    @object = model_class.new resource_attrs
-    @object.save!
-    show
+    super
   end
 
   def show
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 5c23674..7f1f1e1 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -355,8 +355,6 @@ class ArvadosModel < ActiveRecord::Base
     []
   end
 
-  @@UUID_REGEX = /^[0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15}$/
-
   @@prefixes_hash = nil
   def self.uuid_prefixes
     unless @@prefixes_hash
@@ -424,7 +422,7 @@ class ArvadosModel < ActiveRecord::Base
     resource_class = nil
 
     Rails.application.eager_load!
-    uuid.match @@UUID_REGEX do |re|
+    uuid.match HasUuid::UUID_REGEX do |re|
       return uuid_prefixes[re[1]] if uuid_prefixes[re[1]]
     end
 
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index c05cac5..b451ad0 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -129,10 +129,10 @@ class Collection < ArvadosModel
     # that looks like a Docker image, return it.
     if loc = Locator.parse(search_term)
       loc.strip_hints!
-      coll_match = readable_by(*readers).where(uuid: loc.to_s).first
+      coll_match = readable_by(*readers).where(portable_data_hash: loc.to_s).first
       if coll_match and (coll_match.files.size == 1) and
           (coll_match.files[0][1] =~ /^[0-9A-Fa-f]{64}\.tar$/)
-        return [loc.to_s]
+        return [find_by_portable_data_hash(loc.to_s).uuid]
       end
     end
 
@@ -144,7 +144,7 @@ class Collection < ArvadosModel
     # If that didn't work, find Collections with matching Docker image hashes.
     if matches.empty?
       matches = base_search.
-        where("link_class = ? and name LIKE ?",
+        where("link_class = ? and links.name LIKE ?",
               "docker_image_hash", "#{search_term}%")
     end
 
diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb
index fc445ae..039495d 100644
--- a/services/api/app/models/job.rb
+++ b/services/api/app/models/job.rb
@@ -40,6 +40,7 @@ class Job < ArvadosModel
     t.add :repository
     t.add :supplied_script_version
     t.add :docker_image_locator
+    t.add :name
   end
 
   def assert_finished
@@ -118,7 +119,7 @@ class Job < ArvadosModel
       self.docker_image_locator = nil
       true
     elsif coll = Collection.for_latest_docker_image(image_search, image_tag)
-      self.docker_image_locator = coll.uuid
+      self.docker_image_locator = coll.portable_data_hash
       true
     else
       errors.add(:docker_image_locator, "not found for #{image_search}")
diff --git a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
index 6ad5b86..e5b509c 100644
--- a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
+++ b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
@@ -2,9 +2,10 @@ class CollectionUseRegularUuids < ActiveRecord::Migration
   def up
     add_column :collections, :name, :string
     add_column :collections, :description, :string
-    add_column :collections, :properties, :string
+    add_column :collections, :properties, :text
     add_column :collections, :expire_time, :date
     remove_column :collections, :locator
+    add_column :jobs, :name, :string
 
     say_with_time "Step 1. Move manifest hashes into portable_data_hash field" do
       ActiveRecord::Base.connection.execute("update collections set portable_data_hash=uuid, uuid=null")
@@ -89,7 +90,7 @@ where tail_uuid like '________________________________+%' and link_class='permis
 }
     end
 
-    say_with_time "Step 5. Migrate collection -> collection provenance links to jobs" do
+    say_with_time "Step 7. Migrate collection -> collection provenance links to jobs" do
       from_clause = %{
 from links
 where head_uuid like '________________________________+%' and tail_uuid like '________________________________+%' and links.link_class = 'provenance'
@@ -115,7 +116,7 @@ values (#{ActiveRecord::Base.connection.quote newuuid},
       ActiveRecord::Base.connection.execute "delete from links where links.uuid in (select links.uuid #{from_clause})"
     end
 
-    say_with_time "Step 7. Migrate remaining links with head_uuid pointing to collections" do
+    say_with_time "Step 8. Migrate remaining links with head_uuid pointing to collections" do
       from_clause = %{
 from links inner join collections on links.head_uuid=portable_data_hash
 where collections.uuid is not null
@@ -142,7 +143,11 @@ values (#{ActiveRecord::Base.connection.quote Link.generate_uuid},
       ActiveRecord::Base.connection.execute "delete from links where links.uuid in (select links.uuid #{from_clause})"
     end
 
-    say_with_time "Step 8. Validate links table" do
+    say_with_time "Step 9. Delete any remaining name links" do
+      ActiveRecord::Base.connection.execute("delete from links where link_class='name'")
+    end
+
+    say_with_time "Step 10. Validate links table" do
       links = ActiveRecord::Base.connection.select_all %{
 select links.uuid, head_uuid, tail_uuid, link_class, name
 from links
@@ -156,10 +161,6 @@ where head_uuid like '________________________________+%' or tail_uuid like '___
   end
 
   def down
-    #remove_column :collections, :name
-    #remove_column :collections, :description
-    #remove_column :collections, :properties
-    #remove_column :collections, :expire_time
-
+    # Not gonna happen.
   end
 end
diff --git a/services/api/lib/has_uuid.rb b/services/api/lib/has_uuid.rb
index 481d27a..9f1bbf4 100644
--- a/services/api/lib/has_uuid.rb
+++ b/services/api/lib/has_uuid.rb
@@ -1,7 +1,10 @@
 module HasUuid
 
+  UUID_REGEX = /^[0-9a-z]{5}-([0-9a-z]{5})-[0-9a-z]{15}$/
+
   def self.included(base)
     base.extend(ClassMethods)
+    base.validate :validate_uuid
     base.before_create :assign_uuid
     base.before_destroy :destroy_permission_links
     base.has_many :links_via_head, class_name: 'Link', foreign_key: :head_uuid, primary_key: :uuid, conditions: "not (link_class = 'permission')", dependent: :restrict
@@ -26,13 +29,41 @@ module HasUuid
     self.respond_to? :uuid
   end
 
+  def validate_uuid
+    if self.respond_to_uuid? and self.uuid_changed?
+      if self.uuid_was.nil? or self.uuid_was == ""
+        if self.uuid.is_a?(String) and self.uuid.length > 0 and current_user.andand.is_admin
+          if (re = self.uuid.match HasUuid::UUID_REGEX)
+            if re[1] == self.class.uuid_prefix
+              return true
+            else
+              self.errors.add(:uuid, "Matched uuid type '#{re[1]}', expected '#{self.class.uuid_prefix}'")
+              return false
+            end
+          else
+            self.errors.add(:uuid, "'#{self.uuid}' is not a valid Arvados UUID")
+            return false
+          end
+        elsif self.uuid.nil? or self.uuid == ""
+          return true
+        else
+          self.errors.add(:uuid, "Not permitted to specify uuid")
+          return false
+        end
+      else
+        self.errors.add(:uuid, "Not permitted to change uuid")
+        return false
+      end
+    end
+
+    true
+  end
+
   def assign_uuid
-    return true if !self.respond_to_uuid?
-    if (uuid.is_a?(String) and uuid.length>0 and
-        current_user and current_user.is_admin)
-      return true
+    if self.respond_to_uuid? and self.uuid.nil? or self.uuid == ""
+      self.uuid = self.class.generate_uuid
     end
-    self.uuid = self.class.generate_uuid
+    true
   end
 
   def destroy_permission_links
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 2421ab0..5296eb8 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -21,7 +21,7 @@ foo_file:
   updated_at: 2014-02-03T17:22:54Z
   manifest_text: ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n"
   name: foo_file
-z
+
 bar_file:
   uuid: 4n8aq-4zz18-ehbhgtheo8909or
   portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index e4bbd5c..9592d2a 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -220,7 +220,7 @@ EOS
     assert_response :success
     assert_not_nil assigns(:object)
     resp = JSON.parse(@response.body)
-    assert_equal "d30fe8ae534397864cb96c544f4cf102+47", resp['uuid']
+    assert_equal "d30fe8ae534397864cb96c544f4cf102+47", resp['portable_data_hash']
   end
 
   test "get full provenance for baz file" do
@@ -311,7 +311,7 @@ EOS
       assert_response :success
       assert_not_nil assigns(:object)
       resp = JSON.parse(@response.body)
-      assert_equal manifest_uuid, resp['uuid']
+      assert_equal manifest_uuid, resp['portable_data_hash']
       assert_equal 48, resp['data_size']
       # All of the locators in the output must be signed.
       resp['manifest_text'].lines.each do |entry|
@@ -359,7 +359,7 @@ EOS
     assert_response :success
     assert_not_nil assigns(:object)
     resp = JSON.parse(@response.body)
-    assert_equal manifest_uuid, resp['uuid']
+    assert_equal manifest_uuid, resp['portable_data_hash']
     assert_equal 48, resp['data_size']
     # All of the locators in the output must be signed.
     resp['manifest_text'].lines.each do |entry|
diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb
index 85d07a5..e52dfcd 100644
--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@@ -12,8 +12,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
     set_user_from_auth :active_trustedclient
     want_uuid = Specimen.generate_uuid
     a = create_with_attrs(uuid: want_uuid)
-    assert_not_equal want_uuid, a.uuid, "Non-admin should not assign uuid."
-    assert a.uuid.length==27, "Auto assigned uuid length is wrong."
+    assert_nil a, "Non-admin should not assign uuid."
   end
 
   test 'admin can assign valid uuid' do
diff --git a/services/api/test/unit/job_test.rb b/services/api/test/unit/job_test.rb
index 73a676f..357e010 100644
--- a/services/api/test/unit/job_test.rb
+++ b/services/api/test/unit/job_test.rb
@@ -27,14 +27,14 @@ class JobTest < ActiveSupport::TestCase
 
   { 'name' => [:links, :docker_image_collection_repository, :name],
     'hash' => [:links, :docker_image_collection_hash, :name],
-    'locator' => [:collections, :docker_image, :uuid],
+    'locator' => [:collections, :docker_image, :portable_data_hash],
   }.each_pair do |spec_type, (fixture_type, fixture_name, fixture_attr)|
     test "Job initialized with Docker image #{spec_type} gets locator" do
       image_spec = send(fixture_type, fixture_name).send(fixture_attr)
       job = Job.new job_attrs(runtime_constraints:
                               {'docker_image' => image_spec})
       assert job.valid?, job.errors.full_messages.to_s
-      assert_equal(collections(:docker_image).uuid, job.docker_image_locator)
+      assert_equal(collections(:docker_image).portable_data_hash, job.docker_image_locator)
     end
 
     test "Job modified with Docker image #{spec_type} gets locator" do
@@ -44,12 +44,12 @@ class JobTest < ActiveSupport::TestCase
       image_spec = send(fixture_type, fixture_name).send(fixture_attr)
       job.runtime_constraints['docker_image'] = image_spec
       assert job.valid?, job.errors.full_messages.to_s
-      assert_equal(collections(:docker_image).uuid, job.docker_image_locator)
+      assert_equal(collections(:docker_image).portable_data_hash, job.docker_image_locator)
     end
   end
 
   test "removing a Docker runtime constraint removes the locator" do
-    image_locator = collections(:docker_image).uuid
+    image_locator = collections(:docker_image).portable_data_hash
     job = Job.new job_attrs(runtime_constraints:
                             {'docker_image' => image_locator})
     assert job.valid?, job.errors.full_messages.to_s
@@ -66,7 +66,7 @@ class JobTest < ActiveSupport::TestCase
                             {'docker_image' => image_repo,
                               'docker_image_tag' => image_tag})
     assert job.valid?, job.errors.full_messages.to_s
-    assert_equal(collections(:docker_image).uuid, job.docker_image_locator)
+    assert_equal(collections(:docker_image).portable_data_hash, job.docker_image_locator)
   end
 
   test "can't locate a Docker image with a nonexistent tag" do
@@ -83,7 +83,7 @@ class JobTest < ActiveSupport::TestCase
     job = Job.new job_attrs(runtime_constraints:
                             {'docker_image' => image_hash})
     assert job.valid?, job.errors.full_messages.to_s + " with partial hash #{image_hash}"
-    assert_equal(collections(:docker_image).uuid, job.docker_image_locator)
+    assert_equal(collections(:docker_image).portable_data_hash, job.docker_image_locator)
   end
 
   { 'name' => 'arvados_test_nonexistent',
@@ -104,7 +104,7 @@ class JobTest < ActiveSupport::TestCase
   end
 
   test "can create Job with Docker image Collection without Docker links" do
-    image_uuid = collections(:unlinked_docker_image).uuid
+    image_uuid = collections(:unlinked_docker_image).portable_data_hash
     job = Job.new job_attrs(runtime_constraints: {"docker_image" => image_uuid})
     assert(job.valid?, "Job created with unlinked Docker image was invalid")
     assert_equal(image_uuid, job.docker_image_locator)

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list