X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/8f9ba1d6405c397c40de94e155cea0c84120ff0c..bf4193eeaa390cec08bbb8333a53fbc89edfd7f3:/services/api/test/unit/collection_test.rb

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index e8cd21a243..f3b48dbf70 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -3,7 +3,7 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 require 'test_helper'
-require 'sweep_trashed_objects'
+require 'fix_collection_versions_timestamps'
 
 class CollectionTest < ActiveSupport::TestCase
   include DbCurrentTime
@@ -91,19 +91,19 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal 34, c.file_size_total
 
       # Updating the manifest should change file stats
-      c.update_attributes(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt 0:34:foo2.txt\n")
+      c.update(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt 0:34:foo2.txt\n")
       assert c.valid?
       assert_equal 2, c.file_count
       assert_equal 68, c.file_size_total
 
       # Updating file stats and the manifest should use manifest values
-      c.update_attributes(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n", file_count:10, file_size_total: 10)
+      c.update(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n", file_count:10, file_size_total: 10)
       assert c.valid?
       assert_equal 1, c.file_count
       assert_equal 34, c.file_size_total
 
       # Updating just the file stats should be ignored
-      c.update_attributes(file_count: 10, file_size_total: 10)
+      c.update(file_count: 10, file_size_total: 10)
       assert c.valid?
       assert_equal 1, c.file_count
       assert_equal 34, c.file_size_total
@@ -166,7 +166,7 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal 1, c.version
       assert_equal false, c.preserve_version
       # Make a versionable update, it shouldn't create a new version yet
-      c.update_attributes!({'name' => 'bar'})
+      c.update!({'name' => 'bar'})
       c.reload
       assert_equal 'bar', c.name
       assert_equal 1, c.version
@@ -175,21 +175,38 @@ class CollectionTest < ActiveSupport::TestCase
       c.update_column('modified_at', fifteen_min_ago) # Update without validations/callbacks
       c.reload
       assert_equal fifteen_min_ago.to_i, c.modified_at.to_i
-      c.update_attributes!({'name' => 'baz'})
+      c.update!({'name' => 'baz'})
       c.reload
       assert_equal 'baz', c.name
       assert_equal 2, c.version
       # Make another update, no new version should be created
-      c.update_attributes!({'name' => 'foobar'})
+      c.update!({'name' => 'foobar'})
       c.reload
       assert_equal 'foobar', c.name
       assert_equal 2, c.version
+      # Simulate a keep-balance run and trigger a new versionable update
+      # This tests bug #18005
+      assert_nil c.replication_confirmed
+      assert_nil c.replication_confirmed_at
+      # Updates without validations/callbacks
+      c.update_column('modified_at', fifteen_min_ago)
+      c.update_column('replication_confirmed_at', Time.now)
+      c.update_column('replication_confirmed', 2)
+      c.reload
+      assert_equal fifteen_min_ago.to_i, c.modified_at.to_i
+      assert_not_nil c.replication_confirmed_at
+      assert_not_nil c.replication_confirmed
+      # Make the versionable update
+      c.update!({'name' => 'foobarbaz'})
+      c.reload
+      assert_equal 'foobarbaz', c.name
+      assert_equal 3, c.version
     end
   end
 
-  test "preserve_version=false assignment is ignored while being true and not producing a new version" do
+  test "preserve_version updates" do
     Rails.configuration.Collections.CollectionVersioning = true
-    Rails.configuration.Collections.PreserveVersionIfIdle = 3600
+    Rails.configuration.Collections.PreserveVersionIfIdle = -1 # disabled
     act_as_user users(:active) do
       # Set up initial collection
       c = create_collection 'foo', Encoding::US_ASCII
@@ -197,29 +214,62 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal 1, c.version
       assert_equal false, c.preserve_version
       # This update shouldn't produce a new version, as the idle time is not up
-      c.update_attributes!({
-        'name' => 'bar',
-        'preserve_version' => true
+      c.update!({
+        'name' => 'bar'
       })
       c.reload
       assert_equal 1, c.version
       assert_equal 'bar', c.name
+      assert_equal false, c.preserve_version
+      # This update should produce a new version, even if the idle time is not up
+      # and also keep the preserve_version=true flag to persist it.
+      c.update!({
+        'name' => 'baz',
+        'preserve_version' => true
+      })
+      c.reload
+      assert_equal 2, c.version
+      assert_equal 'baz', c.name
       assert_equal true, c.preserve_version
       # Make sure preserve_version is not disabled after being enabled, unless
       # a new version is created.
-      c.update_attributes!({
+      # This is a non-versionable update
+      c.update!({
         'preserve_version' => false,
         'replication_desired' => 2
       })
       c.reload
-      assert_equal 1, c.version
+      assert_equal 2, c.version
       assert_equal 2, c.replication_desired
       assert_equal true, c.preserve_version
-      c.update_attributes!({'name' => 'foobar'})
+      # This is a versionable update
+      c.update!({
+        'preserve_version' => false,
+        'name' => 'foobar'
+      })
       c.reload
-      assert_equal 2, c.version
+      assert_equal 3, c.version
       assert_equal false, c.preserve_version
       assert_equal 'foobar', c.name
+      # Flipping only 'preserve_version' to true doesn't create a new version
+      c.update!({'preserve_version' => true})
+      c.reload
+      assert_equal 3, c.version
+      assert_equal true, c.preserve_version
+    end
+  end
+
+  test "preserve_version updates don't change modified_at timestamp" do
+    act_as_user users(:active) do
+      c = create_collection 'foo', Encoding::US_ASCII
+      assert c.valid?
+      assert_equal false, c.preserve_version
+      modified_at = c.modified_at.to_f
+      c.update!({'preserve_version' => true})
+      c.reload
+      assert_equal true, c.preserve_version
+      assert_equal modified_at, c.modified_at.to_f,
+        'preserve_version updates should not trigger modified_at changes'
     end
   end
 
@@ -235,7 +285,7 @@ class CollectionTest < ActiveSupport::TestCase
         assert_equal 1, c.version
 
         assert_raises(ActiveRecord::RecordInvalid) do
-          c.update_attributes!({
+          c.update!({
             name => new_value
           })
         end
@@ -252,14 +302,14 @@ class CollectionTest < ActiveSupport::TestCase
       assert c.valid?
       assert_equal 1, c.version
       # Make changes so that a new version is created
-      c.update_attributes!({'name' => 'bar'})
+      c.update!({'name' => 'bar'})
       c.reload
       assert_equal 2, c.version
       assert_equal 2, Collection.where(current_version_uuid: c.uuid).count
       new_uuid = 'zzzzz-4zz18-somefakeuuidnow'
       assert_empty Collection.where(uuid: new_uuid)
       # Update UUID on current version, check that both collections point to it
-      c.update_attributes!({'uuid' => new_uuid})
+      c.update!({'uuid' => new_uuid})
       c.reload
       assert_equal new_uuid, c.uuid
       assert_equal 2, Collection.where(current_version_uuid: new_uuid).count
@@ -314,7 +364,7 @@ class CollectionTest < ActiveSupport::TestCase
         # Set up initial collection
         c = create_collection 'foo', Encoding::US_ASCII
         assert c.valid?
-        c.update_attributes!({'properties' => value_1})
+        c.update!({'properties' => value_1})
         c.reload
         assert c.changes.keys.empty?
         c.properties = value_2
@@ -334,8 +384,9 @@ class CollectionTest < ActiveSupport::TestCase
       # Set up initial collection
       c = create_collection 'foo', Encoding::US_ASCII
       assert c.valid?
+      original_version_modified_at = c.modified_at.to_f
       # Make changes so that a new version is created
-      c.update_attributes!({'name' => 'bar'})
+      c.update!({'name' => 'bar'})
       c.reload
       assert_equal 2, c.version
       # Get the old version
@@ -344,14 +395,12 @@ class CollectionTest < ActiveSupport::TestCase
 
       version_creation_datetime = c_old.modified_at.to_f
       assert_equal c.created_at.to_f, c_old.created_at.to_f
-      # Current version is updated just a few milliseconds before the version is
-      # saved on the database.
-      assert_operator c.modified_at.to_f, :<, version_creation_datetime
+      assert_equal original_version_modified_at, version_creation_datetime
 
       # Make update on current version so old version get the attribute synced;
       # its modified_at should not change.
       new_replication = 3
-      c.update_attributes!({'replication_desired' => new_replication})
+      c.update!({'replication_desired' => new_replication})
       c.reload
       assert_equal new_replication, c.replication_desired
       c_old.reload
@@ -361,6 +410,29 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  # Bug #17152 - This test relies on fixtures simulating the problem.
+  test "migration fixing collection versions' modified_at timestamps" do
+    versioned_collection_fixtures = [
+      collections(:w_a_z_file).uuid,
+      collections(:collection_owned_by_active).uuid
+    ]
+    versioned_collection_fixtures.each do |uuid|
+      cols = Collection.where(current_version_uuid: uuid).order(version: :desc)
+      assert_equal cols.size, 2
+      # cols[0] -> head version // cols[1] -> old version
+      assert_operator (cols[0].modified_at.to_f - cols[1].modified_at.to_f), :==, 0
+      assert cols[1].modified_at != cols[1].created_at
+    end
+    fix_collection_versions_timestamps
+    versioned_collection_fixtures.each do |uuid|
+      cols = Collection.where(current_version_uuid: uuid).order(version: :desc)
+      assert_equal cols.size, 2
+      # cols[0] -> head version // cols[1] -> old version
+      assert_operator (cols[0].modified_at.to_f - cols[1].modified_at.to_f), :>, 1
+      assert_operator cols[1].modified_at, :==, cols[1].created_at
+    end
+  end
+
   test "past versions should not be directly updatable" do
     Rails.configuration.Collections.CollectionVersioning = true
     Rails.configuration.Collections.PreserveVersionIfIdle = 0
@@ -369,7 +441,7 @@ class CollectionTest < ActiveSupport::TestCase
       c = create_collection 'foo', Encoding::US_ASCII
       assert c.valid?
       # Make changes so that a new version is created
-      c.update_attributes!({'name' => 'bar'})
+      c.update!({'name' => 'bar'})
       c.reload
       assert_equal 2, c.version
       # Get the old version
@@ -407,7 +479,7 @@ class CollectionTest < ActiveSupport::TestCase
         assert_not_equal first_val, c.attributes[attr]
         # Make changes so that a new version is created and a synced field is
         # updated on both
-        c.update_attributes!({'name' => 'bar', attr => first_val})
+        c.update!({'name' => 'bar', attr => first_val})
         c.reload
         assert_equal 2, c.version
         assert_equal first_val, c.attributes[attr]
@@ -415,7 +487,7 @@ class CollectionTest < ActiveSupport::TestCase
         assert_equal first_val, Collection.where(current_version_uuid: c.uuid, version: 1).first.attributes[attr]
         # Only make an update on the same synced field & check that the previously
         # created version also gets it.
-        c.update_attributes!({attr => second_val})
+        c.update!({attr => second_val})
         c.reload
         assert_equal 2, c.version
         assert_equal second_val, c.attributes[attr]
@@ -453,7 +525,7 @@ class CollectionTest < ActiveSupport::TestCase
 
         # Update attribute and check if version number should be incremented
         old_value = c.attributes[attr]
-        c.update_attributes!({attr => val})
+        c.update!({attr => val})
         assert_equal new_version_expected, c.version == 2
         assert_equal val, c.attributes[attr]
 
@@ -487,11 +559,11 @@ class CollectionTest < ActiveSupport::TestCase
       col2 = create_collection 'bar', Encoding::US_ASCII
       assert col2.valid?
       assert_equal 1, col2.version
-      col2.update_attributes({name: 'baz'})
+      col2.update({name: 'baz'})
       assert_equal 2, col2.version
 
       # Try to make col2 a past version of col1. It shouldn't be possible
-      col2.update_attributes({current_version_uuid: col1.uuid})
+      col2.update({current_version_uuid: col1.uuid})
       assert col2.invalid?
       col2.reload
       assert_not_equal col1.uuid, col2.current_version_uuid
@@ -637,13 +709,26 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test "storage_classes_desired default respects config" do
+    saved = Rails.configuration.DefaultStorageClasses
+    Rails.configuration.DefaultStorageClasses = ["foo"]
+    begin
+      act_as_user users(:active) do
+        c = Collection.create!
+        assert_equal ["foo"], c.storage_classes_desired
+      end
+    ensure
+      Rails.configuration.DefaultStorageClasses = saved
+    end
+  end
+
   test "storage_classes_desired cannot be empty" do
     act_as_user users(:active) do
       c = collections(:collection_owned_by_active)
-      c.update_attributes storage_classes_desired: ["hot"]
+      c.update storage_classes_desired: ["hot"]
       assert_equal ["hot"], c.storage_classes_desired
       assert_raise ArvadosModel::InvalidStateTransitionError do
-        c.update_attributes storage_classes_desired: []
+        c.update storage_classes_desired: []
       end
     end
   end
@@ -651,7 +736,7 @@ class CollectionTest < ActiveSupport::TestCase
   test "storage classes lists should only contain non-empty strings" do
     c = collections(:storage_classes_desired_default_unconfirmed)
     act_as_user users(:admin) do
-      assert c.update_attributes(storage_classes_desired: ["default", "a_string"],
+      assert c.update(storage_classes_desired: ["default", "a_string"],
                                  storage_classes_confirmed: ["another_string"])
       [
         ["storage_classes_desired", ["default", 42]],
@@ -660,7 +745,7 @@ class CollectionTest < ActiveSupport::TestCase
         ["storage_classes_confirmed", [""]],
       ].each do |attr, val|
         assert_raise ArvadosModel::InvalidStateTransitionError do
-          assert c.update_attributes({attr => val})
+          assert c.update({attr => val})
         end
       end
     end
@@ -669,7 +754,7 @@ class CollectionTest < ActiveSupport::TestCase
   test "storage_classes_confirmed* can be set by admin user" do
     c = collections(:storage_classes_desired_default_unconfirmed)
     act_as_user users(:admin) do
-      assert c.update_attributes(storage_classes_confirmed: ["default"],
+      assert c.update(storage_classes_confirmed: ["default"],
                                  storage_classes_confirmed_at: Time.now)
     end
   end
@@ -679,16 +764,16 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:storage_classes_desired_default_unconfirmed)
       # Cannot set just one at a time.
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes storage_classes_confirmed: ["default"]
+        c.update storage_classes_confirmed: ["default"]
       end
       c.reload
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes storage_classes_confirmed_at: Time.now
+        c.update storage_classes_confirmed_at: Time.now
       end
       # Cannot set bot at once, either.
       c.reload
       assert_raise ArvadosModel::PermissionDeniedError do
-        assert c.update_attributes(storage_classes_confirmed: ["default"],
+        assert c.update(storage_classes_confirmed: ["default"],
                                    storage_classes_confirmed_at: Time.now)
       end
     end
@@ -699,15 +784,15 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:storage_classes_desired_default_confirmed_default)
       # Cannot clear just one at a time.
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes storage_classes_confirmed: []
+        c.update storage_classes_confirmed: []
       end
       c.reload
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes storage_classes_confirmed_at: nil
+        c.update storage_classes_confirmed_at: nil
       end
       # Can clear both at once.
       c.reload
-      assert c.update_attributes(storage_classes_confirmed: [],
+      assert c.update(storage_classes_confirmed: [],
                                  storage_classes_confirmed_at: nil)
     end
   end
@@ -717,7 +802,7 @@ class CollectionTest < ActiveSupport::TestCase
       Rails.configuration.Collections.DefaultReplication = 2
       act_as_user users(:active) do
         c = collections(:replication_undesired_unconfirmed)
-        c.update_attributes replication_desired: ask
+        c.update replication_desired: ask
         assert_equal ask, c.replication_desired
       end
     end
@@ -726,7 +811,7 @@ class CollectionTest < ActiveSupport::TestCase
   test "replication_confirmed* can be set by admin user" do
     c = collections(:replication_desired_2_unconfirmed)
     act_as_user users(:admin) do
-      assert c.update_attributes(replication_confirmed: 2,
+      assert c.update(replication_confirmed: 2,
                                  replication_confirmed_at: Time.now)
     end
   end
@@ -736,14 +821,14 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:replication_desired_2_unconfirmed)
       # Cannot set just one at a time.
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes replication_confirmed: 1
+        c.update replication_confirmed: 1
       end
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes replication_confirmed_at: Time.now
+        c.update replication_confirmed_at: Time.now
       end
       # Cannot set both at once, either.
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes(replication_confirmed: 1,
+        c.update(replication_confirmed: 1,
                             replication_confirmed_at: Time.now)
       end
     end
@@ -754,15 +839,15 @@ class CollectionTest < ActiveSupport::TestCase
       c = collections(:replication_desired_2_confirmed_2)
       # Cannot clear just one at a time.
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes replication_confirmed: nil
+        c.update replication_confirmed: nil
       end
       c.reload
       assert_raise ArvadosModel::PermissionDeniedError do
-        c.update_attributes replication_confirmed_at: nil
+        c.update replication_confirmed_at: nil
       end
       # Can clear both at once.
       c.reload
-      assert c.update_attributes(replication_confirmed: nil,
+      assert c.update(replication_confirmed: nil,
                                  replication_confirmed_at: nil)
     end
   end
@@ -770,7 +855,7 @@ class CollectionTest < ActiveSupport::TestCase
   test "clear replication_confirmed* when introducing a new block in manifest" do
     c = collections(:replication_desired_2_confirmed_2)
     act_as_user users(:active) do
-      assert c.update_attributes(manifest_text: collections(:user_agreement).signed_manifest_text)
+      assert c.update(manifest_text: collections(:user_agreement).signed_manifest_text_only_for_tests)
       assert_nil c.replication_confirmed
       assert_nil c.replication_confirmed_at
     end
@@ -779,8 +864,8 @@ class CollectionTest < ActiveSupport::TestCase
   test "don't clear replication_confirmed* when just renaming a file" do
     c = collections(:replication_desired_2_confirmed_2)
     act_as_user users(:active) do
-      new_manifest = c.signed_manifest_text.sub(':bar', ':foo')
-      assert c.update_attributes(manifest_text: new_manifest)
+      new_manifest = c.signed_manifest_text_only_for_tests.sub(':bar', ':foo')
+      assert c.update(manifest_text: new_manifest)
       assert_equal 2, c.replication_confirmed
       assert_not_nil c.replication_confirmed_at
     end
@@ -789,15 +874,15 @@ class CollectionTest < ActiveSupport::TestCase
   test "don't clear replication_confirmed* when just deleting a data block" do
     c = collections(:replication_desired_2_confirmed_2)
     act_as_user users(:active) do
-      new_manifest = c.signed_manifest_text
+      new_manifest = c.signed_manifest_text_only_for_tests
       new_manifest.sub!(/ \S+:bar/, '')
       new_manifest.sub!(/ acbd\S+/, '')
 
       # Confirm that we did just remove a block from the manifest (if
       # not, this test would pass without testing the relevant case):
-      assert_operator new_manifest.length+40, :<, c.signed_manifest_text.length
+      assert_operator new_manifest.length+40, :<, c.signed_manifest_text_only_for_tests.length
 
-      assert c.update_attributes(manifest_text: new_manifest)
+      assert c.update(manifest_text: new_manifest)
       assert_equal 2, c.replication_confirmed
       assert_not_nil c.replication_confirmed_at
     end
@@ -807,9 +892,9 @@ class CollectionTest < ActiveSupport::TestCase
     act_as_user users(:active) do
       t0 = db_current_time
       c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n", name: 'foo')
-      c.update_attributes! trash_at: (t0 + 1.hours)
+      c.update! trash_at: (t0 + 1.hours)
       c.reload
-      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
+      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text_only_for_tests)[1].to_i
       assert_operator sig_exp.to_i, :<=, (t0 + 1.hours).to_i
     end
   end
@@ -819,7 +904,7 @@ class CollectionTest < ActiveSupport::TestCase
       c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n",
                              name: 'foo',
                              trash_at: db_current_time + 1.years)
-      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
+      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text_only_for_tests)[1].to_i
       expect_max_sig_exp = db_current_time.to_i + Rails.configuration.Collections.BlobSigningTTL.to_i
       assert_operator c.trash_at.to_i, :>, expect_max_sig_exp
       assert_operator sig_exp.to_i, :<=, expect_max_sig_exp
@@ -847,7 +932,7 @@ class CollectionTest < ActiveSupport::TestCase
       assert_not_empty c, 'Should be able to find live collection'
 
       # mark collection as expired
-      c.first.update_attributes!(trash_at: Time.new.strftime("%Y-%m-%d"))
+      c.first.update!(trash_at: Time.new.strftime("%Y-%m-%d"))
       c = Collection.readable_by(current_user).where(uuid: uuid)
       assert_empty c, 'Should not be able to find expired collection'
 
@@ -862,7 +947,7 @@ class CollectionTest < ActiveSupport::TestCase
     act_as_user users(:active) do
       t0 = db_current_time
       c = Collection.create!(manifest_text: '', name: 'foo')
-      c.update_attributes! trash_at: (t0 - 2.weeks)
+      c.update! trash_at: (t0 - 2.weeks)
       c.reload
       assert_operator c.trash_at, :>, t0
     end
@@ -917,7 +1002,7 @@ class CollectionTest < ActiveSupport::TestCase
         else
           c = collections(fixture_name)
         end
-        updates_ok = c.update_attributes(updates)
+        updates_ok = c.update(updates)
         expect_valid = expect[:state] != :invalid
         assert_equal expect_valid, updates_ok, c.errors.full_messages.to_s
         case expect[:state]
@@ -954,13 +1039,13 @@ class CollectionTest < ActiveSupport::TestCase
     start = db_current_time
     act_as_user users(:active) do
       c = Collection.create!(manifest_text: '', name: 'foo')
-      c.update_attributes!(trash_at: start + 86400.seconds)
+      c.update!(trash_at: start + 86400.seconds)
       assert_operator c.delete_at, :>=, start + (86400*22).seconds
       assert_operator c.delete_at, :<, start + (86400*22 + 30).seconds
       c.destroy
 
       c = Collection.create!(manifest_text: '', name: 'foo')
-      c.update_attributes!(is_trashed: true)
+      c.update!(is_trashed: true)
       assert_operator c.delete_at, :>=, start + (86400*21).seconds
     end
   end
@@ -972,52 +1057,28 @@ class CollectionTest < ActiveSupport::TestCase
     assert_includes(coll_uuids, collections(:docker_image).uuid)
   end
 
-  test "move collections to trash in SweepTrashedObjects" do
-    c = collections(:trashed_on_next_sweep)
-    refute_empty Collection.where('uuid=? and is_trashed=false', c.uuid)
-    assert_raises(ActiveRecord::RecordNotUnique) do
-      act_as_user users(:active) do
-        Collection.create!(owner_uuid: c.owner_uuid,
-                           name: c.name)
-      end
-    end
-    SweepTrashedObjects.sweep_now
-    c = Collection.where('uuid=? and is_trashed=true', c.uuid).first
-    assert c
+  test "empty names are exempt from name uniqueness" do
     act_as_user users(:active) do
-      assert Collection.create!(owner_uuid: c.owner_uuid,
-                                name: c.name)
-    end
-  end
-
-  test "delete collections in SweepTrashedObjects" do
-    uuid = 'zzzzz-4zz18-3u1p5umicfpqszp' # deleted_on_next_sweep
-    assert_not_empty Collection.where(uuid: uuid)
-    SweepTrashedObjects.sweep_now
-    assert_empty Collection.where(uuid: uuid)
-  end
-
-  test "delete referring links in SweepTrashedObjects" do
-    uuid = collections(:trashed_on_next_sweep).uuid
-    act_as_system_user do
-      Link.create!(head_uuid: uuid,
-                   tail_uuid: system_user_uuid,
-                   link_class: 'whatever',
-                   name: 'something')
+      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
-    past = db_current_time
-    Collection.where(uuid: uuid).
-      update_all(is_trashed: true, trash_at: past, delete_at: past)
-    assert_not_empty Collection.where(uuid: uuid)
-    SweepTrashedObjects.sweep_now
-    assert_empty Collection.where(uuid: uuid)
   end
 
-  test "create collections with default properties" do
-    Rails.configuration.Collections.DefaultProperties = {
-      'default_prop1' => {'value' => 'prop1_value'},
-      'responsible_person_uuid' => {'function' => 'original_owner'}
-    }
+  test "create collections with managed properties" do
+    Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({
+      'default_prop1' => {'Value' => 'prop1_value'},
+      'responsible_person_uuid' => {'Function' => 'original_owner'}
+    })
     # Test collection without initial properties
     act_as_user users(:active) do
       c = create_collection 'foo', Encoding::US_ASCII
@@ -1045,10 +1106,10 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
-  test "update collection with protected default properties" do
-    Rails.configuration.Collections.DefaultProperties = {
-      'default_prop1' => {'value' => 'prop1_value', 'protected' => true},
-    }
+  test "update collection with protected managed properties" do
+    Rails.configuration.Collections.ManagedProperties = ConfigLoader.to_OrderedOptions({
+      'default_prop1' => {'Value' => 'prop1_value', 'Protected' => true},
+    })
     act_as_user users(:active) do
       c = create_collection 'foo', Encoding::US_ASCII
       assert c.valid?
@@ -1057,6 +1118,7 @@ class CollectionTest < ActiveSupport::TestCase
       # Add new property
       c.properties['prop2'] = 'value2'
       c.save!
+      c.reload
       assert_equal 'value2', c.properties['prop2']
       # Try to change protected property's value
       c.properties['default_prop1'] = 'new_value'
@@ -1067,8 +1129,30 @@ class CollectionTest < ActiveSupport::TestCase
       act_as_system_user do
         c.properties['default_prop1'] = 'new_value'
         c.save!
+        c.reload
         assert_equal 'new_value', c.properties['default_prop1']
       end
     end
   end
+
+  test "collection names must be displayable in a filesystem" do
+    set_user_from_auth :active
+    ["", "{SOLIDUS}"].each do |subst|
+      Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
+      c = Collection.create
+      [[nil, true],
+       ["", true],
+       [".", false],
+       ["..", false],
+       ["...", true],
+       ["..z..", true],
+       ["foo/bar", subst != ""],
+       ["../..", subst != ""],
+       ["/", subst != ""],
+      ].each do |name, valid|
+        c.name = name
+        assert_equal valid, c.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+      end
+    end
+  end
 end