[ARVADOS] created: 1.2.0-217-g9fd178164

Git user git at public.curoverse.com
Wed Oct 17 14:52:47 EDT 2018


        at  9fd178164372f1c1ef922f0e890f747b9dc36955 (commit)


commit 9fd178164372f1c1ef922f0e890f747b9dc36955
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Wed Oct 17 15:51:36 2018 -0300

    13561: Raise exception instead of ignoring versioning attributes updates.
    
    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 250dfef36..cedb6e42f 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 :current_versions_always_point_to_self, on: :update
+  validate :versioning_metadata_updates, on: :update
   validate :past_versions_cannot_be_updated, on: :update
   before_save :set_file_names
   around_update :manage_versioning
@@ -242,11 +242,6 @@ class Collection < ArvadosModel
 
       # Restore requested changes on the current version
       changes.keys.each do |attr|
-        if attr == 'version'
-          next
-        elsif attr == 'preserve_version' && changes[attr].last == false
-          next # Ignore false assignment, once true it'll be true until next version
-        end
         self.attributes = {attr => changes[attr].last}
         if attr == 'uuid'
           # Also update the current version reference
@@ -629,11 +624,21 @@ class Collection < ArvadosModel
     end
   end
 
-  def current_versions_always_point_to_self
+  def versioning_metadata_updates
+    valid = true
     if (current_version_uuid_was == uuid_was) && current_version_uuid_changed?
       errors.add(:current_version_uuid, "cannot be updated")
-      false
+      valid = false
+    end
+    if preserve_version_was == true && preserve_version_changed?
+      errors.add(:preserve_version, "cannot be set to false once set to true")
+      valid = false
+    end
+    if version_changed?
+      errors.add(:version, "cannot be updated")
+      valid = false
     end
+    valid
   end
 
   def assign_uuid
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 21450d7a5..95e1940a3 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -137,7 +137,7 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
-  test "preserve_version=false assignment is ignored while being true and not producing a new version" do
+  test "preserve_version=false assignment is invalid while being true" do
     Rails.configuration.collection_versioning = true
     Rails.configuration.preserve_version_if_idle = 3600
     act_as_user users(:active) do
@@ -157,13 +157,13 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal true, c.preserve_version
       # Make sure preserve_version is not disabled after being enabled, unless
       # a new version is created.
-      c.update_attributes!({
-        'preserve_version' => false,
-        'replication_desired' => 2
-      })
+      assert_raises(ActiveRecord::RecordInvalid) do
+        c.update_attributes!({
+          'preserve_version' => false
+        })
+      end
       c.reload
       assert_equal 1, c.version
-      assert_equal 2, c.replication_desired
       assert_equal true, c.preserve_version
       c.update_attributes!({'name' => 'foobar'})
       c.reload
@@ -173,6 +173,26 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  [
+    ['version', 10],
+    ['current_version_uuid', 'zzzzz-4zz18-bv31uwvy3neko21'],
+  ].each do |name, new_value|
+    test "'#{name}' updates on current version collections are not allowed" do
+      act_as_user users(:active) do
+        # Set up initial collection
+        c = create_collection 'foo', Encoding::US_ASCII
+        assert c.valid?
+        assert_equal 1, c.version
+
+        assert_raises(ActiveRecord::RecordInvalid) do
+          c.update_attributes!({
+            name => new_value
+          })
+        end
+      end
+    end
+  end
+
   test "uuid updates on current version make older versions update their pointers" do
     Rails.configuration.collection_versioning = true
     Rails.configuration.preserve_version_if_idle = 0

commit 917cdcde33c5a4ab7075a1d844d6dbe2540ab05a
Author: Lucas Di Pentima <ldipentima at veritasgenetics.com>
Date:   Wed Oct 17 15:05:50 2018 -0300

    13561: Fixes api to get a collection with its past versions.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <ldipentima at veritasgenetics.com>

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 18b88c4d4..5d7a7ae26 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -235,17 +235,4 @@ class Arvados::V1::CollectionsController < ApplicationController
       @select ||= model_class.selectable_attributes - ["manifest_text", "unsigned_manifest_text"]
     end
   end
-
-  def load_filters_param
-    super
-    return if !params[:include_old_versions]
-    @filters = @filters.map do |col, operator, operand|
-      # Replace uuid filters when including past versions
-      if col == 'uuid'
-        ['current_version_uuid', operator, operand]
-      else
-        [col, operator, operand]
-      end
-    end
-  end
 end
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index fdc54894f..f5bed6384 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1197,7 +1197,7 @@ EOS
   test 'can get collection with past versions' do
     authorize_with :active
     get :index, {
-      filters: [['uuid','=',collections(:collection_owned_by_active).uuid]],
+      filters: [['current_version_uuid','=',collections(:collection_owned_by_active).uuid]],
       include_old_versions: true
     }
     assert_response :success

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list