[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