17152: Adds migration to fix collection versions' modified_at timestamps.

Also, with test.

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

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
+++ b/services/api/db/migrate/20201202174753_fix_collection_versions_timestamps.rb
@@ -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

diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 58c064ac3341ce953d41f83cab0425a0370e5f67..12a28c6c723609bd14b2c20129b8c749695bdc28 100644 (file)
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -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
+++ b/services/api/lib/fix_collection_versions_timestamps.rb
@@ -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

diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 767f035b88cb824c47de95ef9571c5531c228c23..61bb3f79f8c882565b1ee9bd4ec5806963c1cf53 100644 (file)
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -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

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 666d41f0eebe2e0fd407b6014150f6ce0b28e168..a28893e0119162ea388b698991599e71d74cc3c8 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -4,6 +4,7 @@
 
 require 'test_helper'
 require 'sweep_trashed_objects'
+require 'fix_collection_versions_timestamps'
 
 class CollectionTest < ActiveSupport::TestCase
   include DbCurrentTime
@@ -360,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