[ARVADOS] updated: d10a66b66edc088d33bad52aebd7cac0a6e9ce49

git at public.curoverse.com git at public.curoverse.com
Fri Aug 15 17:11:38 EDT 2014


Summary of changes:
 .../api/app/controllers/application_controller.rb  |   4 +-
 .../arvados/v1/collections_controller.rb           | 184 +++++++++++----------
 .../controllers/arvados/v1/groups_controller.rb    |   6 -
 services/api/app/models/collection.rb              |  29 +++-
 services/api/app/models/job.rb                     |   1 +
 services/api/app/models/locator.rb                 |   9 +-
 services/api/app/models/pipeline_instance.rb       |   1 +
 .../20140811184643_collection_use_regular_uuids.rb |   1 -
 .../20140815171049_add_name_description_columns.rb |  13 ++
 services/api/db/structure.sql                      |  12 +-
 services/api/lib/has_uuid.rb                       |  31 ++--
 services/api/test/fixtures/logs.yml                |   4 +-
 .../arvados/v1/collections_controller_test.rb      |  42 +++--
 .../arvados/v1/groups_controller_test.rb           |   4 +-
 services/api/test/unit/arvados_model_test.rb       |  16 +-
 15 files changed, 210 insertions(+), 147 deletions(-)
 create mode 100644 services/api/db/migrate/20140815171049_add_name_description_columns.rb

       via  d10a66b66edc088d33bad52aebd7cac0a6e9ce49 (commit)
      from  1fc1bc4348ea3a1168f6d7bd3391f2449e30d181 (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 d10a66b66edc088d33bad52aebd7cac0a6e9ce49
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Fri Aug 15 17:11:34 2014 -0400

    3036: Test fixing work in progress.

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 4a1a7d4..bd0fedc 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -104,7 +104,9 @@ class ApplicationController < ActionController::Base
     if e.respond_to? :backtrace and e.backtrace
       logger.error e.backtrace.collect { |x| x + "\n" }.join('')
     end
-    if @object and @object.errors and @object.errors.full_messages and not @object.errors.full_messages.empty?
+    if (@object and @object.respond_to? :errors and
+        @object.errors and @object.errors.full_messages and
+        not @object.errors.full_messages.empty?)
       errors = @object.errors.full_messages
       logger.error errors.inspect
     else
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index a37290c..474a6ba 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -1,9 +1,8 @@
 class Arvados::V1::CollectionsController < ApplicationController
   def create
-    owner_uuid = resource_attrs.delete(:owner_uuid) || current_user.uuid
-    unless current_user.can? write: owner_uuid
-      logger.warn "User #{current_user.andand.uuid} tried to set collection owner_uuid to #{owner_uuid}"
-      raise ArvadosModel::PermissionDeniedError
+    if !resource_attrs[:manifest_text]
+      return send_error("'manifest_text' attribute must be specified",
+                        status: :unprocessable_entity)
     end
 
     # Check permissions on the collection manifest.
@@ -54,6 +53,23 @@ class Arvados::V1::CollectionsController < ApplicationController
     super
   end
 
+  def find_object_by_uuid
+    if loc = Locator.parse(params[:id])
+      loc.strip_hints!
+      if c = Collection.readable_by(*@read_users).where({ portable_data_hash: loc.to_s }).limit(1).first
+        @object = {
+          portable_data_hash: c.portable_data_hash,
+          manifest_text: c.manifest_text,
+          files: c.files,
+          data_size: c.data_size
+        }
+      end
+    else
+      super
+    end
+    true
+  end
+
   def show
     if current_api_client_authorization
       signing_opts = {
@@ -72,7 +88,11 @@ class Arvados::V1::CollectionsController < ApplicationController
         end
       }
     end
-    render json: @object.as_api_response(:with_data)
+    if @object.is_a? Collection
+      render json: @object.as_api_response(:with_data)
+    else
+      render json: @object
+    end
   end
 
   def script_param_edges(visited, sp)
@@ -87,48 +107,75 @@ class Arvados::V1::CollectionsController < ApplicationController
       end
     when String
       return if sp.empty?
-      m = stripped_portable_data_hash(sp)
-      if m
-        generate_provenance_edges(visited, m)
+      if loc = Locator.parse(sp)
+        search_edges(visited, loc.to_s, UP)
       end
     end
   end
 
-  def generate_provenance_edges(visited, uuid)
-    m = stripped_portable_data_hash(uuid)
-    uuid = m if m
+  UP = 1
+  DOWN = 2
+
+  def search_edges(visited, uuid, direction)
+    if uuid.nil? or uuid.empty? or visited[uuid]
+      return
+    end
 
-    if not uuid or uuid.empty? or visited[uuid]
-      return ""
+    if loc = Locator.parse(uuid)
+      loc.strip_hints!
+      return if visited[loc.to_s]
     end
 
     logger.debug "visiting #{uuid}"
 
-    if m
-      # uuid is a collection
-      Collection.readable_by(current_user).where(portable_data_hash: uuid).each do |c|
-        visited[uuid] = c.as_api_response
-        visited[uuid][:files] = []
-        c.files.each do |f|
-          visited[uuid][:files] << f
-        end
+    if loc
+      # uuid is a portable_data_hash
+      if c = Collection.readable_by(*@read_users).where(portable_data_hash: loc.to_s).limit(1).first
+        visited[loc.to_s] = {
+          portable_data_hash: c.portable_data_hash,
+          files: c.files,
+          data_size: c.data_size
+        }
       end
 
-      Job.readable_by(current_user).where(output: uuid).each do |job|
-        generate_provenance_edges(visited, job.uuid)
-      end
+      if direction == UP
+        # Search upstream for jobs where this locator is the output of some job
+        Job.readable_by(*@read_users).where(output: loc.to_s).each do |job|
+          search_edges(visited, job.uuid, UP)
+        end
 
-      Job.readable_by(current_user).where(log: uuid).each do |job|
-        generate_provenance_edges(visited, job.uuid)
-      end
+        Job.readable_by(*@read_users).where(log: loc.to_s).each do |job|
+          search_edges(visited, job.uuid, UP)
+        end
+      elsif direction == DOWN
+        if loc.to_s == "d41d8cd98f00b204e9800998ecf8427e+0"
+          # Special case, don't follow the empty collection.
+          return
+        end
 
+        # Search downstream for jobs where this locator is in script_parameters
+        Job.readable_by(*@read_users).where(["jobs.script_parameters like ?", "%#{loc.to_s}%"]).each do |job|
+          search_edges(visited, job.uuid, DOWN)
+        end
+      end
     else
-      # uuid is something else
+      # uuid is a regular Arvados UUID
       rsc = ArvadosModel::resource_class_for_uuid uuid
       if rsc == Job
-        Job.readable_by(current_user).where(uuid: uuid).each do |job|
+        Job.readable_by(*@read_users).where(uuid: uuid).each do |job|
           visited[uuid] = job.as_api_response
-          script_param_edges(visited, job.script_parameters)
+          if direction == UP
+            # Follow upstream collections referenced in the script parameters
+            script_param_edges(visited, job.script_parameters)
+          elsif direction == DOWN
+            # Follow downstream job output
+            search_edges(visited, job.output, direction)
+          end
+        end
+      elsif rsc == Collection
+        if c = Collection.readable_by(*@read_users).where(uuid: uuid).limit(1).first
+          visited[uuid] = c.as_api_response
+          search_edges(visited, c.portable_data_hash, direction)
         end
       elsif rsc != nil
         rsc.where(uuid: uuid).each do |r|
@@ -137,75 +184,34 @@ class Arvados::V1::CollectionsController < ApplicationController
       end
     end
 
-    Link.readable_by(current_user).
-      where(head_uuid: uuid, link_class: "provenance").
-      each do |link|
-      visited[link.uuid] = link.as_api_response
-      generate_provenance_edges(visited, link.tail_uuid)
+    if direction == UP
+      # Search for provenance links pointing to the current uuid
+      Link.readable_by(*@read_users).
+        where(head_uuid: uuid, link_class: "provenance").
+        each do |link|
+        visited[link.uuid] = link.as_api_response
+        search_edges(visited, link.tail_uuid, direction)
+      end
+    elsif direction == DOWN
+      # Search for provenance links emanating from the current uuid
+      Link.readable_by(current_user).
+        where(tail_uuid: uuid, link_class: "provenance").
+        each do |link|
+        visited[link.uuid] = link.as_api_response
+        search_edges(visited, link.head_uuid, direction)
+      end
     end
   end
 
   def provenance
     visited = {}
-    generate_provenance_edges(visited, @object[:uuid])
+    search_edges(visited, @object[:uuid] || @object[:portable_data_hash], UP)
     render json: visited
   end
 
-  def generate_used_by_edges(visited, uuid)
-    m = stripped_portable_data_hash(uuid)
-    uuid = m if m
-
-    if not uuid or uuid.empty? or visited[uuid]
-      return ""
-    end
-
-    logger.debug "visiting #{uuid}"
-
-    if m
-      # uuid is a collection
-      Collection.readable_by(current_user).where(portable_data_hash: uuid).each do |c|
-        visited[uuid] = c.as_api_response
-        visited[uuid][:files] = []
-        c.files.each do |f|
-          visited[uuid][:files] << f
-        end
-      end
-
-      if uuid == "d41d8cd98f00b204e9800998ecf8427e+0"
-        # special case for empty collection
-        return
-      end
-
-      Job.readable_by(current_user).where(["jobs.script_parameters like ?", "%#{uuid}%"]).each do |job|
-        generate_used_by_edges(visited, job.uuid)
-      end
-
-    else
-      # uuid is something else
-      rsc = ArvadosModel::resource_class_for_uuid uuid
-      if rsc == Job
-        Job.readable_by(current_user).where(uuid: uuid).each do |job|
-          visited[uuid] = job.as_api_response
-          generate_used_by_edges(visited, job.output)
-        end
-      elsif rsc != nil
-        rsc.where(uuid: uuid).each do |r|
-          visited[uuid] = r.as_api_response
-        end
-      end
-    end
-
-    Link.readable_by(current_user).
-      where(tail_uuid: uuid, link_class: "provenance").
-      each do |link|
-      visited[link.uuid] = link.as_api_response
-      generate_used_by_edges(visited, link.head_uuid)
-    end
-  end
-
   def used_by
     visited = {}
-    generate_used_by_edges(visited, @object[:uuid])
+    search_edges(visited, @object[:uuid] || @object[:portable_data_hash], DOWN)
     render json: visited
   end
 
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 17be40a..7ee4e55 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -82,12 +82,6 @@ class Arvados::V1::GroupsController < ApplicationController
         cond_params = []
         conds << "#{klass.table_name}.owner_uuid = ?"
         cond_params << opts[:owner_uuid]
-        if opts[:include_linked]
-          haslink = "#{klass.table_name}.uuid IN (SELECT head_uuid FROM links WHERE link_class=#{klass.sanitize 'name'}"
-          haslink += " AND links.tail_uuid=#{klass.sanitize opts[:owner_uuid]}"
-          haslink += ")"
-          conds << haslink
-        end
         if conds.any?
           cond_sql = '(' + conds.join(') OR (') + ')'
           @objects = @objects.where(cond_sql, *cond_params)
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 72cd0b6..f7d9fe8 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -4,7 +4,7 @@ class Collection < ArvadosModel
   include CommonApiTemplate
 
   before_validation :set_portable_data_hash
-  validate :ensure_manifest_matches_hash
+  validate :ensure_hash_matches_manifest_text
 
   api_accessible :user, extend: :common do |t|
     t.add :data_size
@@ -12,6 +12,7 @@ class Collection < ArvadosModel
     t.add :name
     t.add :description
     t.add :properties
+    t.add :portable_data_hash
   end
 
   api_accessible :with_data, extend: :user do |t|
@@ -19,17 +20,29 @@ class Collection < ArvadosModel
   end
 
   def set_portable_data_hash
-    if portable_data_hash.nil? or portable_data_hash == "" or
-        (manifest_text_changed? and !portable_data_hash_changed?)
-      portable_data_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}"
+    if (self.portable_data_hash.nil? or (self.portable_data_hash == "") or (manifest_text_changed? and !portable_data_hash_changed?))
+      self.portable_data_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}"
+    elsif portable_data_hash_changed?
+      begin
+        loc = Locator.parse!(self.portable_data_hash)
+        loc.strip_hints!
+        self.portable_data_hash = loc.to_s
+      rescue ArgumentError => e
+        errors.add(:portable_data_hash, "#{e}")
+        return false
+      end
     end
     true
   end
 
-  def ensure_manifest_matches_hash
-    unless Digest::MD5.hexdigest(manifest_text) == portable_data_hash
-      errors.add(:portable_data_hash, "does not match hash of manifest_text")
-      return false
+  def ensure_hash_matches_manifest_text
+    if manifest_text_changed? or portable_data_hash_changed?
+      computed_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}"
+      unless computed_hash == portable_data_hash
+        logger.debug "(computed) '#{computed_hash}' != '#{portable_data_hash}' (provided)"
+        errors.add(:portable_data_hash, "does not match hash of manifest_text")
+        return false
+      end
     end
     true
   end
diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb
index 039495d..97039db 100644
--- a/services/api/app/models/job.rb
+++ b/services/api/app/models/job.rb
@@ -41,6 +41,7 @@ class Job < ArvadosModel
     t.add :supplied_script_version
     t.add :docker_image_locator
     t.add :name
+    t.add :description
   end
 
   def assert_finished
diff --git a/services/api/app/models/locator.rb b/services/api/app/models/locator.rb
index d01c9dc..39a7aaf 100644
--- a/services/api/app/models/locator.rb
+++ b/services/api/app/models/locator.rb
@@ -35,9 +35,16 @@ class Locator
   # Locator.parse! returns a Locator object parsed from the string tok,
   # raising an ArgumentError if tok cannot be parsed.
   def self.parse!(tok)
+    if tok.nil? or tok.empty?
+      raise ArgumentError.new "locator is nil or empty"
+    end
+
     m = /^([[:xdigit:]]{32})(\+([[:digit:]]+))?(\+([[:upper:]][[:alnum:]+ at _-]*))?$/.match(tok.strip)
     unless m
-      raise ArgumentError.new "could not parse #{tok}"
+      raise ArgumentError.new "not a valid locator #{tok}"
+    end
+    unless m[2]
+      raise ArgumentError.new "missing size hint on #{tok}"
     end
 
     tokhash, _, toksize, _, trailer = m[1..5]
diff --git a/services/api/app/models/pipeline_instance.rb b/services/api/app/models/pipeline_instance.rb
index 354c892..f2c930c 100644
--- a/services/api/app/models/pipeline_instance.rb
+++ b/services/api/app/models/pipeline_instance.rb
@@ -17,6 +17,7 @@ class PipelineInstance < ArvadosModel
     t.add :pipeline_template_uuid
     t.add :pipeline_template, :if => :pipeline_template
     t.add :name
+    t.add :description
     t.add :components
     t.add :dependencies
     t.add :properties
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 e5b509c..fd9054c 100644
--- a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
+++ b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
@@ -5,7 +5,6 @@ class CollectionUseRegularUuids < ActiveRecord::Migration
     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")
diff --git a/services/api/db/migrate/20140815171049_add_name_description_columns.rb b/services/api/db/migrate/20140815171049_add_name_description_columns.rb
new file mode 100644
index 0000000..c0c8f6a
--- /dev/null
+++ b/services/api/db/migrate/20140815171049_add_name_description_columns.rb
@@ -0,0 +1,13 @@
+class AddNameDescriptionColumns < ActiveRecord::Migration
+  def up
+    add_column :jobs, :name, :string
+    add_column :jobs, :description, :text
+    add_column :pipeline_instances, :description, :text
+  end
+
+  def down
+    remove_column :jobs, :name
+    remove_column :jobs, :description
+    remove_column :pipeline_instances, :description
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index b31825e..4b32d38 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -3,7 +3,6 @@
 --
 
 SET statement_timeout = 0;
-SET lock_timeout = 0;
 SET client_encoding = 'UTF8';
 SET standard_conforming_strings = on;
 SET check_function_bodies = false;
@@ -430,7 +429,9 @@ CREATE TABLE jobs (
     repository character varying(255),
     output_is_persistent boolean DEFAULT false NOT NULL,
     supplied_script_version character varying(255),
-    docker_image_locator character varying(255)
+    docker_image_locator character varying(255),
+    name character varying(255),
+    description text
 );
 
 
@@ -679,7 +680,8 @@ CREATE TABLE pipeline_instances (
     updated_at timestamp without time zone NOT NULL,
     properties text,
     state character varying(255),
-    components_summary text
+    components_summary text,
+    description text
 );
 
 
@@ -1981,4 +1983,6 @@ INSERT INTO schema_migrations (version) VALUES ('20140709172343');
 
 INSERT INTO schema_migrations (version) VALUES ('20140714184006');
 
-INSERT INTO schema_migrations (version) VALUES ('20140811184643');
\ No newline at end of file
+INSERT INTO schema_migrations (version) VALUES ('20140811184643');
+
+INSERT INTO schema_migrations (version) VALUES ('20140815171049');
\ No newline at end of file
diff --git a/services/api/lib/has_uuid.rb b/services/api/lib/has_uuid.rb
index 9f1bbf4..df175f8 100644
--- a/services/api/lib/has_uuid.rb
+++ b/services/api/lib/has_uuid.rb
@@ -31,36 +31,33 @@ module HasUuid
 
   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
+      if current_user.andand.is_admin and self.uuid.is_a?(String)
+        if (re = self.uuid.match HasUuid::UUID_REGEX)
+          if re[1] == self.class.uuid_prefix
+            return true
           else
-            self.errors.add(:uuid, "'#{self.uuid}' is not a valid Arvados UUID")
+            self.errors.add(:uuid, "Matched uuid type '#{re[1]}', expected '#{self.class.uuid_prefix}'")
             return false
           end
-        elsif self.uuid.nil? or self.uuid == ""
-          return true
         else
-          self.errors.add(:uuid, "Not permitted to specify uuid")
+          self.errors.add(:uuid, "'#{self.uuid}' is not a valid Arvados UUID")
           return false
         end
       else
-        self.errors.add(:uuid, "Not permitted to change uuid")
+        if self.new_record?
+          self.errors.add(:uuid, "Not permitted to specify uuid")
+        else
+          self.errors.add(:uuid, "Not permitted to change uuid")
+        end
         return false
       end
+    else
+      return true
     end
-
-    true
   end
 
   def assign_uuid
-    if self.respond_to_uuid? and self.uuid.nil? or self.uuid == ""
+    if self.respond_to_uuid? and self.uuid.nil? or self.uuid.empty?
       self.uuid = self.class.generate_uuid
     end
     true
diff --git a/services/api/test/fixtures/logs.yml b/services/api/test/fixtures/logs.yml
index d95177a..c737bb1 100644
--- a/services/api/test/fixtures/logs.yml
+++ b/services/api/test/fixtures/logs.yml
@@ -24,7 +24,7 @@ log4: # foo collection added, readable by active through link
   id: 4
   uuid: zzzzz-xxxxx-pshmckwoma00004
   owner_uuid: zzzzz-tpzed-000000000000000 # system user
-  object_uuid: 1f4b0bc7583c2a7f9102c395f4ffc5e3+45 # foo file
+  object_uuid: 4n8aq-4zz18-znfnqtbbv4spc3w # foo file
   object_owner_uuid: zzzzz-tpzed-000000000000000 # system user
   event_at: <%= 4.minute.ago.to_s(:db) %>
 
@@ -32,6 +32,6 @@ log5: # baz collection added, readable by active and spectator through group 'al
   id: 5
   uuid: zzzzz-xxxxx-pshmckwoma00005
   owner_uuid: zzzzz-tpzed-000000000000000 # system user
-  object_uuid: ea10d51bcf88862dbcc36eb292017dfd+45 # baz file
+  object_uuid: 4n8aq-4zz18-y9vne9npefyxh8g # baz file
   object_owner_uuid: zzzzz-tpzed-000000000000000 # system user
   event_at: <%= 5.minute.ago.to_s(:db) %>
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 e6834e0..30559a5 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -84,7 +84,7 @@ EOS
     assert_nil assigns(:objects)
 
     get :show, {
-      id: test_collection[:uuid]
+      id: test_collection[:portable_data_hash]
     }
     assert_response :success
     assert_not_nil assigns(:object)
@@ -128,12 +128,12 @@ EOS
       collection: {
         owner_uuid: 'zzzzz-j7d0g-rew6elm53kancon',
         manifest_text: manifest_text,
-        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102"
+        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
       }
     }
     assert_response :success
     resp = JSON.parse(@response.body)
-    assert_equal 'zzzzz-tpzed-000000000000000', resp['owner_uuid']
+    assert_equal 'zzzzz-j7d0g-rew6elm53kancon', resp['owner_uuid']
   end
 
   test "create with owner_uuid set to group i can_manage" do
@@ -142,30 +142,44 @@ EOS
     manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
     post :create, {
       collection: {
-        owner_uuid: 'zzzzz-j7d0g-8ulrifv67tve5sx',
+        owner_uuid: groups(:system_owned_group).uuid,
         manifest_text: manifest_text,
-        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102"
+        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
       }
     }
     assert_response :success
     resp = JSON.parse(@response.body)
-    assert_equal 'zzzzz-tpzed-000000000000000', resp['owner_uuid']
+    assert_equal 'zzzzz-j7d0g-8ulrifv67tve5sx', resp['owner_uuid']
   end
 
-  test "create with owner_uuid set to group with no can_manage permission" do
+  test "create with owner_uuid fails on group with can_read permission" do
     permit_unsigned_manifests
     authorize_with :active
     manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
     post :create, {
       collection: {
-        owner_uuid: 'zzzzz-j7d0g-it30l961gq3t0oi',
+        owner_uuid: groups(:all_users).uuid,
         manifest_text: manifest_text,
-        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102"
+        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
       }
     }
     assert_response 403
   end
 
+  test "create with owner_uuid fails on group with no permission" do
+    permit_unsigned_manifests
+    authorize_with :active
+    manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
+    post :create, {
+      collection: {
+        owner_uuid: groups(:public).uuid,
+        manifest_text: manifest_text,
+        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
+      }
+    }
+    assert_response 422
+  end
+
   test "admin create with owner_uuid set to group with no permission" do
     permit_unsigned_manifests
     authorize_with :admin
@@ -174,7 +188,7 @@ EOS
       collection: {
         owner_uuid: 'zzzzz-j7d0g-it30l961gq3t0oi',
         manifest_text: manifest_text,
-        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102"
+        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47"
       }
     }
     assert_response :success
@@ -187,7 +201,7 @@ EOS
       collection: <<-EOS
       {
         "manifest_text":". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n",\
-        "portable_data_hash":"d30fe8ae534397864cb96c544f4cf102"\
+        "portable_data_hash":"d30fe8ae534397864cb96c544f4cf102+47"\
       }
       EOS
     }
@@ -201,7 +215,7 @@ EOS
       collection: <<-EOS
       {
         "manifest_text":". d41d8cd98f00b204e9800998ecf8427e 0:0:bar.txt\n",\
-        "portable_data_hash":"d30fe8ae534397864cb96c544f4cf102"\
+        "portable_data_hash":"d30fe8ae534397864cb96c544f4cf102+47"\
       }
       EOS
     }
@@ -261,8 +275,8 @@ EOS
       where: { any: ['contains', '7f9102c395f4ffc5e3'] }
     }
     assert_response :success
-    found = assigns(:objects).collect(&:uuid)
-    assert_equal 1, found.count
+    found = assigns(:objects).collect(&:portable_data_hash)
+    assert_equal 2, found.count
     assert_equal true, !!found.index('1f4b0bc7583c2a7f9102c395f4ffc5e3+45')
   end
 
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 217809a..276037f 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -262,7 +262,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
   end
 
   test 'get group-owned objects with include_linked' do
-    expected_uuid = specimens(:in_aproject_linked_from_asubproject).uuid
+    expected_uuid = specimens(:in_asubproject).uuid
     authorize_with :active
     get :contents, {
       id: groups(:asubproject).uuid,
@@ -273,7 +273,7 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     uuids = json_response['items'].collect { |i| i['uuid'] }
     assert_includes uuids, expected_uuid, "Did not get #{expected_uuid}"
 
-    expected_name = links(:specimen_is_in_two_projects).name
+    expected_name = specimens(:in_asubproject).name
     found_specimen_name = false
     assert(json_response['links'].any?,
            "Expected a non-empty array of links in response")
diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb
index e52dfcd..e47aa3b 100644
--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@@ -23,11 +23,23 @@ class ArvadosModelTest < ActiveSupport::TestCase
     assert a.uuid.length==27, "Auto assigned uuid length is wrong."
   end
 
+  test 'admin cannot assign uuid with wrong object type' do
+    set_user_from_auth :admin_trustedclient
+    want_uuid = Human.generate_uuid
+    a = create_with_attrs(uuid: want_uuid)
+    assert_nil a, "Admin should not be able to assign invalid uuid."
+  end
+
+  test 'admin cannot assign badly formed uuid' do
+    set_user_from_auth :admin_trustedclient
+    a = create_with_attrs(uuid: "ntoheunthaoesunhasoeuhtnsaoeunhtsth")
+    assert_nil a, "Admin should not be able to assign invalid uuid."
+  end
+
   test 'admin cannot assign empty uuid' do
     set_user_from_auth :admin_trustedclient
     a = create_with_attrs(uuid: "")
-    assert_not_equal "", a.uuid, "Admin should not assign empty uuid."
-    assert a.uuid.length==27, "Auto assigned uuid length is wrong."
+    assert_nil a, "Admin cannot assign empty uuid."
   end
 
   [ {:a => 'foo'},

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list