From be8fbfd594717232c4dc7c4d16cdf3bde1137ee0 Mon Sep 17 00:00:00 2001 From: Lucas Di Pentima Date: Tue, 25 Sep 2018 18:44:56 -0300 Subject: [PATCH] 13561: Sync selected fields changes with older versions, with tests. Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima --- services/api/app/models/collection.rb | 59 ++++++++++++---- services/api/test/unit/collection_test.rb | 86 +++++++++++++++++++---- 2 files changed, 118 insertions(+), 27 deletions(-) diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index 7e1cd4aff5..61946f82b0 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -220,32 +220,63 @@ class Collection < ArvadosModel end def save! - if !Rails.configuration.collection_versioning || new_record? + if !Rails.configuration.collection_versioning || new_record? || (!self.changes.include?('uuid') && current_version_uuid != uuid) return super end changes = self.changes - versionable_updates = ['manifest_text', 'description', 'properties', 'name'] - if (changes.keys & versionable_updates).empty? - # Updates don't include interesting attributes, don't save a new snapshot. + # Updates that will be synced with older versions + synced_updates = ['uuid', 'owner_uuid', 'delete_at', 'trash_at', 'is_trashed', + 'replication_desired', 'storage_classes_desired'] & changes.keys + # Updates that will produce a new version + versionable_updates = ['manifest_text', 'description', 'properties', 'name'] & changes.keys + + if versionable_updates.empty? && synced_updates.empty? + # Updates don't include interesting attributes, so don't save a new snapshot nor + # sync older versions. return super end - # Create a snapshot of the current collection before saving. - # Does row locking because 'version' is incremented. + + # Does row locking (transaction is implicit) because 'version' + # may be incremented and the older versions synced. + # Note that 'with_lock' reloads the object after locking. with_lock do - # Note that 'with_lock' reloads the object after locking. - # Create a snapshot of the original collection - snapshot = self.dup - snapshot.uuid = nil # Reset UUID so it's created as a new record + # Sync older versions. + if !synced_updates.empty? + updates = {} + synced_updates.each do |attr| + if attr == 'uuid' + # Point old versions to current version's new UUID + updates['current_version_uuid'] = changes[attr].last + else + updates[attr] = changes[attr].last + end + end + Collection.where('current_version_uuid = ? AND uuid != ?', uuid, uuid).each do |c| + c.update_attributes!(updates) + end + # Also update current object just in case a new version will be created, + # as it has to receive the same values for the synced attributes. + self.attributes = updates + end + snapshot = nil + # Make a new version if applicable. + if !versionable_updates.empty? + # Create a snapshot of the original collection + snapshot = self.dup + snapshot.uuid = nil # Reset UUID so it's created as a new record + # Update current version number + self.version += 1 + end # Restore requested changes on the current version changes.keys.each do |attr| next if attr == 'version' self.attributes = {attr => changes[attr].last} end - # Update current version number & save first to avoid index collision - self.version += 1 + # Save current version first to avoid index collision super - # Save the snapshot with previous state - snapshot.save! + # Save the snapshot with previous state (if applicable) + snapshot.andand.save! + return true end end diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb index ee17445a14..e3c2a999cb 100644 --- a/services/api/test/unit/collection_test.rb +++ b/services/api/test/unit/collection_test.rb @@ -106,17 +106,76 @@ class CollectionTest < ActiveSupport::TestCase end end + test "uuid updates on current version make older versions update their pointers" do + Rails.configuration.collection_versioning = true + act_as_system_user do + # Set up initial collection + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + assert_equal 1, c.version + # Make changes so that a new version is created + c.update_attributes!({'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.reload + assert_equal new_uuid, c.uuid + assert_equal 2, Collection.where(current_version_uuid: new_uuid).count + end + end + + [ + ['owner_uuid', 'zzzzz-tpzed-d9tiejq69daie8f', 'zzzzz-tpzed-xurymjxw79nv3jz'], + ['replication_desired', 2, 3], + ['storage_classes_desired', ['hot'], ['archive']], + ['is_trashed', true, false], + ].each do |attr, first_val, second_val| + test "sync #{attr} with older versions" do + Rails.configuration.collection_versioning = true + act_as_system_user do + # Set up initial collection + c = create_collection 'foo', Encoding::US_ASCII + assert c.valid? + assert_equal 1, c.version + 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.reload + assert_equal 2, c.version + assert_equal first_val, c.attributes[attr] + assert_equal 2, Collection.where(current_version_uuid: c.uuid).count + 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.reload + assert_equal 2, c.version + assert_equal second_val, c.attributes[attr] + assert_equal 2, Collection.where(current_version_uuid: c.uuid).count + assert_equal second_val, Collection.where(current_version_uuid: c.uuid, version: 1).first.attributes[attr] + end + end + end + [ - [false, 'name', 'bar'], - [false, 'description', 'The quick brown fox jumps over the lazy dog'], - [false, 'properties', {'new_version' => true}], - [false, 'manifest_text', ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"], - [true, 'name', 'bar'], - [true, 'description', 'The quick brown fox jumps over the lazy dog'], - [true, 'properties', {'new_version' => true}], - [true, 'manifest_text', ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"], - ].each do |versioning, attr, val| - test "update collection #{attr} with versioning #{versioning ? '' : 'not '}enabled" do + [false, 'name', 'bar', false], + [false, 'description', 'The quick brown fox jumps over the lazy dog', false], + [false, 'properties', {'new_version' => true}, false], + [false, 'manifest_text', ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n", false], + [true, 'name', 'bar', true], + [true, 'description', 'The quick brown fox jumps over the lazy dog', true], + [true, 'properties', {'new_version' => true}, true], + [true, 'manifest_text', ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n", true], + # Non-versionable attribute updates shouldn't create new versions + [true, 'replication_desired', 5, false], + [false, 'replication_desired', 5, false], + ].each do |versioning, attr, val, new_version_expected| + test "update #{attr} with versioning #{versioning ? '' : 'not '}enabled should #{new_version_expected ? '' : 'not '}create a new version" do Rails.configuration.collection_versioning = versioning act_as_system_user do # Create initial collection @@ -131,17 +190,18 @@ class CollectionTest < ActiveSupport::TestCase # Update attribute and check if version number should be incremented old_value = c.attributes[attr] c.update_attributes!({attr => val}) - assert_equal versioning, c.version == 2 + assert_equal new_version_expected, c.version == 2 assert_equal val, c.attributes[attr] - if versioning + if versioning && new_version_expected # Search for the snapshot & previous value assert_equal 2, Collection.where(current_version_uuid: c.uuid).count s = Collection.where(current_version_uuid: c.uuid, version: 1).first assert_not_nil s assert_equal old_value, s.attributes[attr] else - # If versioning is disabled, only the current version should exist + # If versioning is disabled or no versionable attribute was updated, + # only the current version should exist assert_equal 1, Collection.where(current_version_uuid: c.uuid).count assert_equal c, Collection.where(current_version_uuid: c.uuid).first end -- 2.30.2