Merge branch '17152-collection-versions-fixes'
authorLucas Di Pentima <lucas@di-pentima.com.ar>
Wed, 9 Dec 2020 21:31:33 +0000 (18:31 -0300)
committerLucas Di Pentima <lucas@di-pentima.com.ar>
Wed, 9 Dec 2020 21:31:33 +0000 (18:31 -0300)
Refs #17152

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas@di-pentima.com.ar>

services/api/app/models/collection.rb
services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb [new file with mode: 0644]
services/api/db/structure.sql
services/api/lib/fix_collection_versions_timestamps.rb [new file with mode: 0644]
services/api/test/fixtures/collections.yml
services/api/test/unit/collection_test.rb
tools/arvbox/bin/arvbox

index 8b549a71ab4fba348ab9279f456595912fb693db..3637f34e105b1bd66013a7cc0882860dc434034e 100644 (file)
@@ -269,6 +269,7 @@ class Collection < ArvadosModel
         snapshot = self.dup
         snapshot.uuid = nil # Reset UUID so it's created as a new record
         snapshot.created_at = self.created_at
+        snapshot.modified_at = self.modified_at_was
       end
 
       # Restore requested changes on the current version
@@ -294,8 +295,10 @@ class Collection < ArvadosModel
       if snapshot
         snapshot.attributes = self.syncable_updates
         leave_modified_by_user_alone do
-          act_as_system_user do
-            snapshot.save
+          leave_modified_at_alone do
+            act_as_system_user do
+              snapshot.save
+            end
           end
         end
       end
diff --git a/services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb b/services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb
new file mode 100644 (file)
index 0000000..4c56d3d
--- /dev/null
@@ -0,0 +1,17 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'fix_collection_versions_timestamps'
+
+class FixCollectionVersionsTimestamps < ActiveRecord::Migration[5.2]
+  def up
+    # Defined in a function for easy testing.
+    fix_collection_versions_timestamps
+  end
+
+  def down
+    # This migration is not reversible.  However, the results are
+    # backwards compatible.
+  end
+end
index 58c064ac3341ce953d41f83cab0425a0370e5f67..12a28c6c723609bd14b2c20129b8c749695bdc28 100644 (file)
@@ -3186,6 +3186,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20200602141328'),
 ('20200914203202'),
 ('20201103170213'),
-('20201105190435');
+('20201105190435'),
+('20201202174753');
 
 
diff --git a/services/api/lib/fix_collection_versions_timestamps.rb b/services/api/lib/fix_collection_versions_timestamps.rb
new file mode 100644 (file)
index 0000000..61da988
--- /dev/null
@@ -0,0 +1,43 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'set'
+
+include CurrentApiClient
+include ArvadosModelUpdates
+
+def fix_collection_versions_timestamps
+  act_as_system_user do
+    uuids = [].to_set
+    # Get UUIDs from collections with more than 1 version
+    Collection.where(version: 2).find_each(batch_size: 100) do |c|
+      uuids.add(c.current_version_uuid)
+    end
+    uuids.each do |uuid|
+      first_pair = true
+      # All versions of a particular collection get fixed together.
+      ActiveRecord::Base.transaction do
+        Collection.where(current_version_uuid: uuid).order(version: :desc).each_cons(2) do |c1, c2|
+          # Skip if the 2 newest versions' modified_at values are separate enough;
+          # this means that this collection doesn't require fixing, allowing for
+          # migration re-runs in case of transient problems.
+          break if first_pair && (c2.modified_at.to_f - c1.modified_at.to_f) > 1
+          first_pair = false
+          # Fix modified_at timestamps by assigning to N-1's value to N.
+          # Special case: the first version's modified_at will be == to created_at
+          leave_modified_by_user_alone do
+            leave_modified_at_alone do
+              c1.modified_at = c2.modified_at
+              c1.save!(validate: false)
+              if c2.version == 1
+                c2.modified_at = c2.created_at
+                c2.save!(validate: false)
+              end
+            end
+          end
+        end
+      end
+    end
+  end
+end
\ No newline at end of file
index 767f035b88cb824c47de95ef9571c5531c228c23..61bb3f79f8c882565b1ee9bd4ec5806963c1cf53 100644 (file)
@@ -23,8 +23,8 @@ collection_owned_by_active:
   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
+  modified_at: 2014-02-03T18:22:54Z
+  updated_at: 2014-02-03T18:22:54Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
   name: owned_by_active
   version: 2
@@ -43,7 +43,7 @@ collection_owned_by_active_with_file_stats:
   file_count: 1
   file_size_total: 3
   name: owned_by_active_with_file_stats
-  version: 2
+  version: 1
 
 collection_owned_by_active_past_version_1:
   uuid: zzzzz-4zz18-znfnqtbbv4spast
@@ -53,8 +53,8 @@ collection_owned_by_active_past_version_1:
   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-03T15:22:54Z
-  updated_at: 2014-02-03T15:22:54Z
+  modified_at: 2014-02-03T18:22:54Z
+  updated_at: 2014-02-03T18:22:54Z
   manifest_text: ". 37b51d194a7513e45b56f6524f2d51f2+3 0:3:bar\n"
   name: owned_by_active_version_1
   version: 1
@@ -106,8 +106,8 @@ w_a_z_file:
   created_at: 2015-02-09T10:53:38Z
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2015-02-09T10:53:38Z
-  updated_at: 2015-02-09T10:53:38Z
+  modified_at: 2015-02-09T10:55:38Z
+  updated_at: 2015-02-09T10:55:38Z
   manifest_text: ". 4c6c2c0ac8aa0696edd7316a3be5ca3c+5 0:5:w\\040\\141\\040z\n"
   name: "\"w a z\" file"
   version: 2
@@ -120,8 +120,8 @@ w_a_z_file_version_1:
   created_at: 2015-02-09T10:53:38Z
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
-  modified_at: 2015-02-09T10:53:38Z
-  updated_at: 2015-02-09T10:53:38Z
+  modified_at: 2015-02-09T10:55:38Z
+  updated_at: 2015-02-09T10:55:38Z
   manifest_text: ". 4d20280d5e516a0109768d49ab0f3318+3 0:3:waz\n"
   name: "waz file"
   version: 1
index 48cae5afee92622fcda41c2b48e89761f54ac80d..a28893e0119162ea388b698991599e71d74cc3c8 100644 (file)
@@ -4,6 +4,7 @@
 
 require 'test_helper'
 require 'sweep_trashed_objects'
+require 'fix_collection_versions_timestamps'
 
 class CollectionTest < ActiveSupport::TestCase
   include DbCurrentTime
@@ -334,6 +335,7 @@ 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.reload
@@ -344,9 +346,7 @@ 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.
@@ -361,6 +361,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
index a180b43630471f0c92ded944e6e1b8290ece4245..96f3666cda9ee498970ebf0c5ce29badd0fc18d8 100755 (executable)
@@ -205,6 +205,7 @@ run() {
               --publish=8900:8900
               --publish=9000:9000
               --publish=9002:9002
+              --publish=9004:9004
               --publish=25101:25101
               --publish=8001:8001
               --publish=8002:8002