14287: Make collection name "" equivalent to null.

Regardless of whether name is set/updated to "" or null, it is exempt
from the unique-name-in-project constraint, and returned as "" in API
responses.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

diff --git a/apps/workbench/app/helpers/application_helper.rb b/apps/workbench/app/helpers/application_helper.rb
index 83123b26c35c469e3efad7ec2833679f55445a39..6352916b00b14560af0683aa9594b6daf7ce20ca 100644 (file)
--- a/apps/workbench/app/helpers/application_helper.rb
+++ b/apps/workbench/app/helpers/application_helper.rb
@@ -359,8 +359,8 @@ module ApplicationHelper
           display_value = link.name
         elsif value_info[:link_name]
           display_value = value_info[:link_name]
-        elsif value_info[:selection_name]
-          display_value = value_info[:selection_name]
+        elsif (sn = value_info[:selection_name]) && sn != ""
+          display_value = sn
         end
       end
       if (attr == :components) and (subattr.size > 2)

diff --git a/apps/workbench/test/integration/pipeline_instances_test.rb b/apps/workbench/test/integration/pipeline_instances_test.rb
index 801609fbb6747f74ee52d0e0bc38b9d7a57ae12b..adfd62bd8e04c73767ed2756a442ee15442691ec 100644 (file)
--- a/apps/workbench/test/integration/pipeline_instances_test.rb
+++ b/apps/workbench/test/integration/pipeline_instances_test.rb
@@ -393,6 +393,7 @@ class PipelineInstancesTest < ActionDispatch::IntegrationTest
   def create_and_run_pipeline_in_aproject in_aproject, template_name, collection_fixture, choose_file=false
     # collection in aproject to be used as input
     collection = api_fixture('collections', collection_fixture)
+    collection['name'] ||= '' # API response is "" even if fixture attr is null
 
     # create a pipeline instance
     find('.btn', text: 'Run a process').click
@@ -421,7 +422,7 @@ class PipelineInstancesTest < ActionDispatch::IntegrationTest
 
       if collection_fixture == 'foo_collection_in_aproject'
         first('span', text: 'foo_tag').click
-      elsif collection['name']
+      elsif collection['name'] != ''
         first('span', text: "#{collection['name']}").click
       else
         collection_uuid = collection['uuid']

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 775ebdb49486861d73f20b97ba562d77656de765..f8cca5f0749b88846a54b9a5ccb63faa5b097c0e 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -26,6 +26,7 @@ class Collection < ArvadosModel
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
+  before_validation :name_null_if_empty
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
   validate :ensure_storage_classes_contain_non_empty_strings
@@ -36,7 +37,7 @@ class Collection < ArvadosModel
   around_update :manage_versioning, unless: :is_past_version?
 
   api_accessible :user, extend: :common do |t|
-    t.add :name
+    t.add lambda { |x| x.name || "" }, as: :name
     t.add :description
     t.add :properties
     t.add :portable_data_hash
@@ -75,6 +76,7 @@ class Collection < ArvadosModel
                 # correct timestamp in signed_manifest_text.
                 'manifest_text' => ['manifest_text', 'trash_at', 'is_trashed'],
                 'unsigned_manifest_text' => ['manifest_text'],
+                'name' => ['name'],
                 )
   end
 
@@ -193,6 +195,12 @@ class Collection < ArvadosModel
     end
   end
 
+  def name_null_if_empty
+    if name == ""
+      self.name = nil
+    end
+  end
+
   def set_file_names
     if self.manifest_text_changed?
       self.file_names = manifest_files

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 08d5b1fb72cb9544ba8ae651e0936826462703d3..e75ad5d7731ebf9c88f5f1946852f22f5f7abf12 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -1012,4 +1012,21 @@ class CollectionTest < ActiveSupport::TestCase
     SweepTrashedObjects.sweep_now
     assert_empty Collection.where(uuid: uuid)
   end
+
+  test "empty names are exempt from name uniqueness" do
+    act_as_user users(:active) do
+      c1 = Collection.new(name: nil, manifest_text: '', owner_uuid: groups(:aproject).uuid)
+      assert c1.save
+      c2 = Collection.new(name: '', manifest_text: '', owner_uuid: groups(:aproject).uuid)
+      assert c2.save
+      c3 = Collection.new(name: '', manifest_text: '', owner_uuid: groups(:aproject).uuid)
+      assert c3.save
+      c4 = Collection.new(name: 'c4', manifest_text: '', owner_uuid: groups(:aproject).uuid)
+      assert c4.save
+      c5 = Collection.new(name: 'c4', manifest_text: '', owner_uuid: groups(:aproject).uuid)
+      assert_raises(ActiveRecord::RecordNotUnique) do
+        c5.save
+      end
+    end
+  end
 end