14484: Improves test coverage and attribute setting performance

Arvados-DCO-1.1-Signed-off-by: Eric Biagiotti <ebiagiotti@veritasgenetics.com>

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index bcf510d88cbf1b4818e73e16816826d364f8b47a..578f25a62fc1eee9a081dfd6a9bac767a44e90d9 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -199,10 +199,15 @@ class Collection < ArvadosModel
   end
 
   def set_file_count_and_total_size
-    if self.manifest_text_changed? || self.file_count_changed? || self.file_size_total_changed?
+    # Only update the file stats if the manifest changed
+    if self.manifest_text_changed?
       m = Keep::Manifest.new(self.manifest_text)
       self.file_size_total = m.files_size
       self.file_count = m.files_count
+    # If the manifest didn't change but the attributes did, ignore the changes
+    elsif self.file_count_changed? || self.file_size_total_changed?
+      self.file_count = self.file_count_was
+      self.file_size_total = self.file_size_total_was
     end
     true
   end

diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 8763f3944471e9a5cad4f4565da2833f988bbdf8..c84e479e48fbe6118a1297a903013addf68e928e 100644 (file)
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -29,6 +29,22 @@ collection_owned_by_active:
   name: owned_by_active
   version: 2
 
+collection_owned_by_active_with_file_stats:
+  uuid: zzzzz-4zz18-fjeod4od92kfj5f
+  current_version_uuid: zzzzz-4zz18-fjeod4od92kfj5f
+  portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  created_at: 2014-02-03T17:22:54Z
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2014-02-03T17:22:54Z
+  updated_at: 2014-02-03T17:22:54Z
+  manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
+  file_count: 1
+  file_size_total: 3
+  name: owned_by_active_with_file_stats
+  version: 2
+
 collection_owned_by_active_past_version_1:
   uuid: zzzzz-4zz18-znfnqtbbv4spast
   current_version_uuid: zzzzz-4zz18-bv31uwvy3neko21

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 ccc875c154d1bd63135c4dd24443f451130a0d56..27d4fee795134f4ea4cfa10e2270e0e4fb5a0eb6 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -958,7 +958,7 @@ EOS
   test "update collection manifest and expect new file stats" do
     authorize_with :active
     post :update, {
-      id: 'zzzzz-4zz18-bv31uwvy3neko21',
+      id: collections(:collection_owned_by_active_with_file_stats).uuid,
       collection: {
         manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n"
       }
@@ -989,10 +989,10 @@ EOS
     ['file_count', 1],
     ['file_size_total', 3]
   ].each do |attribute, val|
-    test "update collection with #{attribute} and expect overwrite" do
+    test "update collection with #{attribute} and expect ignore" do
       authorize_with :active
       post :update, {
-        id: 'zzzzz-4zz18-bv31uwvy3neko21',
+        id: collections(:collection_owned_by_active_with_file_stats).uuid,
         collection: {
           "#{attribute}": 10
         }

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index df487d965d5f8bde1556886a73bdd39904a11a34..8deedee0186ea5bbd87ba6d219d7ef4d47f66314 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -76,24 +76,34 @@ class CollectionTest < ActiveSupport::TestCase
 
   test "file stats cannot be changed unless through manifest change" do
     act_as_system_user do
-      # Changing file stats via an update should ignore and overwrite
+      # Direct changes to file stats should be ignored
       c = Collection.create(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n")
       c.file_count = 6
       c.file_size_total = 30
       assert c.valid?
-      c.reload
       assert_equal 1, c.file_count
       assert_equal 34, c.file_size_total
 
-      # Changing the file stats via manifest change 
-      c = Collection.create(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt 0:34:foo2.txt\n")
-      c.reload
+      # File stats specified on create should be ignored and overwritten
+      c = Collection.create(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 the manifest should change file stats
+      c.update_attributes(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
 
-      # Changing file stats at create will ignore and overwrite
-      c = Collection.create(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e 0:34:foo.txt\n", file_count: 10, file_size_total: 10)
+      # 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)
+      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)
       assert c.valid?
       assert_equal 1, c.file_count
       assert_equal 34, c.file_size_total