[ARVADOS] updated: 1.2.0-165-gc8b76b1d7
Git user
git at public.curoverse.com
Thu Oct 11 12:18:11 EDT 2018
Summary of changes:
services/api/app/models/collection.rb | 56 +++++++++++++++++--------------
services/api/test/unit/collection_test.rb | 44 +++++++++++++++++-------
2 files changed, 62 insertions(+), 38 deletions(-)
via c8b76b1d730d68fe9cb89ec4a619ce2f5a515531 (commit)
via 7182964d1d475f77a4ae72424c06ebb447b7a14f (commit)
from 1499536dae11776694d8cd99747a1d8625903b26 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit c8b76b1d730d68fe9cb89ec4a619ce2f5a515531
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date: Thu Oct 11 13:17:47 2018 -0300
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 at veritasgenetics.com>
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 1cf0a401c..250dfef36 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 d8cb469d6..21450d7a5 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
commit 7182964d1d475f77a4ae72424c06ebb447b7a14f
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date: Thu Oct 11 11:09:37 2018 -0300
13561: Fix validation & update related test.
Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 4163d314a..1cf0a401c 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -27,7 +27,7 @@ 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 :old_versions_cannot_be_updated, 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? }
@@ -622,11 +622,12 @@ class Collection < ArvadosModel
end
end
- def old_versions_cannot_be_updated
+ def past_versions_cannot_be_updated
# We check for the '_was' values just in case the update operation
# includes a change on current_version_uuid or uuid.
if current_version_uuid_was != uuid_was
- raise ArvadosModel::PermissionDeniedError.new("previous versions cannot be updated")
+ errors.add(:base, "past versions cannot be updated")
+ false
end
end
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index b0c225dda..d8cb469d6 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -245,25 +245,18 @@ class CollectionTest < ActiveSupport::TestCase
c_old = Collection.where(current_version_uuid: c.uuid, version: 1).first
assert_not_nil c_old
# With collection versioning still being enabled, try to update
- assert_raises ArvadosModel::PermissionDeniedError do
- c_old.update_attributes(name: 'this was foo')
- end
+ c_old.name = 'this was foo'
+ assert c_old.invalid?
c_old.reload
- assert_equal 'foo', c_old.name
# Try to fool the validator attempting to make c_old to look like a
# current version, it should also fail.
- assert_raises ArvadosModel::PermissionDeniedError do
- c_old.update_attributes(current_version_uuid: c_old.uuid)
- end
+ c_old.current_version_uuid = c_old.uuid
+ assert c_old.invalid?
c_old.reload
- assert_equal c.uuid, c_old.current_version_uuid
# Now disable collection versioning, it should behave the same way
Rails.configuration.collection_versioning = false
- assert_raises ArvadosModel::PermissionDeniedError do
- c_old.update_attributes(name: 'this was foo')
- end
- c_old.reload
- assert_equal 'foo', c_old.name
+ c_old.name = 'this was foo'
+ assert c_old.invalid?
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list