From c8b76b1d730d68fe9cb89ec4a619ce2f5a515531 Mon Sep 17 00:00:00 2001
From: Lucas Di Pentima <ldipentima@veritasgenetics.com>
Date: Thu, 11 Oct 2018 13:17:47 -0300
Subject: [PATCH] 13561: Move versioning code inside locking block.

Also, don't allow current_version_uuid updates on current versions.

Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima@veritasgenetics.com>
---
 services/api/app/models/collection.rb     | 49 +++++++++++++----------
 services/api/test/unit/collection_test.rb | 25 ++++++++++++
 2 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 1cf0a401ce..250dfef368 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -27,11 +27,10 @@ class Collection < ArvadosModel
   validate :ensure_pdh_matches_manifest_text
   validate :ensure_storage_classes_desired_is_not_empty
   validate :ensure_storage_classes_contain_non_empty_strings
+  validate :current_versions_always_point_to_self, on: :update
   validate :past_versions_cannot_be_updated, on: :update
   before_save :set_file_names
-  around_update :prepare_for_versioning
-  after_update :save_old_version, if: Proc.new { |c| c.should_preserve_version? }
-  after_update :sync_past_versions, if: Proc.new { |c| c.syncable_updates?(self.changes.keys) }
+  around_update :manage_versioning
 
   api_accessible :user, extend: :common do |t|
     t.add :name
@@ -226,16 +225,19 @@ class Collection < ArvadosModel
     ['current_version_uuid']
   end
 
-  def prepare_for_versioning
+  def manage_versioning
+    should_preserve_version = should_preserve_version? # Time sensitive, cache value
+    return(yield) unless (should_preserve_version || syncable_updates.any?)
+
     # Put aside the changes because with_lock forces a record reload
     changes = self.changes
-    @snapshot = nil
+    snapshot = nil
     with_lock do
       # Copy the original state to save it as old version
-      if versionable_updates?(changes.keys) && should_preserve_version?
-        @snapshot = self.dup
-        @snapshot.uuid = nil # Reset UUID so it's created as a new record
-        @snapshot.created_at = self.created_at
+      if should_preserve_version
+        snapshot = self.dup
+        snapshot.uuid = nil # Reset UUID so it's created as a new record
+        snapshot.created_at = self.created_at
       end
 
       # Restore requested changes on the current version
@@ -252,11 +254,18 @@ class Collection < ArvadosModel
         end
       end
 
-      if versionable_updates?(changes.keys) && should_preserve_version?
+      if should_preserve_version
         self.version += 1
         self.preserve_version = false
       end
+
       yield
+
+      sync_past_versions if syncable_updates.any?
+      if snapshot
+        snapshot.attributes = self.syncable_updates
+        snapshot.save
+      end
     end
   end
 
@@ -293,19 +302,8 @@ class Collection < ArvadosModel
     ['uuid', 'owner_uuid', 'delete_at', 'trash_at', 'is_trashed', 'replication_desired', 'storage_classes_desired']
   end
 
-  def syncable_updates?(attrs)
-    (self.syncable_attrs & attrs).any?
-  end
-
-  def save_old_version
-    if @snapshot && should_preserve_version?
-      @snapshot.attributes = self.syncable_updates
-      @snapshot.save
-    end
-  end
-
   def should_preserve_version?
-    return false if !Rails.configuration.collection_versioning
+    return false unless (Rails.configuration.collection_versioning && versionable_updates?(self.changes.keys))
 
     idle_threshold = Rails.configuration.preserve_version_if_idle
     if !self.preserve_version_was &&
@@ -631,6 +629,13 @@ class Collection < ArvadosModel
     end
   end
 
+  def current_versions_always_point_to_self
+    if (current_version_uuid_was == uuid_was) && current_version_uuid_changed?
+      errors.add(:current_version_uuid, "cannot be updated")
+      false
+    end
+  end
+
   def assign_uuid
     super
     self.current_version_uuid ||= self.uuid
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index d8cb469d67..21450d7a55 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -343,6 +343,31 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test 'current_version_uuid is ignored during update' do
+    Rails.configuration.collection_versioning = true
+    Rails.configuration.preserve_version_if_idle = 0
+    act_as_user users(:active) do
+      # Create 1st collection
+      col1 = create_collection 'foo', Encoding::US_ASCII
+      assert col1.valid?
+      assert_equal 1, col1.version
+
+      # Create 2nd collection, update it so it becomes version:2
+      # (to avoid unique index violation)
+      col2 = create_collection 'bar', Encoding::US_ASCII
+      assert col2.valid?
+      assert_equal 1, col2.version
+      col2.update_attributes({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})
+      assert col2.invalid?
+      col2.reload
+      assert_not_equal col1.uuid, col2.current_version_uuid
+    end
+  end
+
   test 'with versioning enabled, simultaneous updates increment version correctly' do
     Rails.configuration.collection_versioning = true
     Rails.configuration.preserve_version_if_idle = 0
-- 
2.30.2