17152: Moves conditional preserve_version disabling to collection's controller.

There was a bug that flipped preserve_version to false after creating a new
version even though the update request included preserve_version=true.
This happened because the logic was at model level, and was incorrect because
at that level we cannot know if the client included the attribute on its
request.

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

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 2e7e2f82b07c024168bd34080f000a91eaea55ac..440ac640169404bc7a0fac4738f55febbd78c0cc 100644 (file)
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -43,6 +43,14 @@ class Arvados::V1::CollectionsController < ApplicationController
     super
   end
 
+  def update
+    # preserve_version should be disabled unless explicitly asked otherwise.
+    if !resource_attrs[:preserve_version]
+      resource_attrs[:preserve_version] = false
+    end
+    super
+  end
+
   def find_objects_for_index
     opts = {
       include_trash: params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name),

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index e3802734a07580c70bf1921c64071c71dc43980b..c2c60ec70e0202038151206eb1ace452fe1d1268 100644 (file)
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -278,7 +278,7 @@ class Collection < ArvadosModel
 
       # Restore requested changes on the current version
       changes.keys.each do |attr|
-        if attr == 'preserve_version' && changes[attr].last == false
+        if attr == 'preserve_version' && changes[attr].last == false && !should_preserve_version
           next # Ignore false assignment, once true it'll be true until next version
         end
         self.attributes = {attr => changes[attr].last}
@@ -290,9 +290,6 @@ class Collection < ArvadosModel
 
       if should_preserve_version
         self.version += 1
-        if !changes.keys.include?('preserve_version')
-          self.preserve_version = false
-        end
       end
 
       yield

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 c025394bc1f33616684be72861862126642d95c4..1ca2dd1dc109857e6987fdaa32caad2b04a52f8b 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1145,7 +1145,7 @@ EOS
   end
 
   [:admin, :active].each do |user|
-    test "get trashed collection via filters and #{user} user" do
+    test "get trashed collection via filters and #{user} user without including its past versions" do
       uuid = 'zzzzz-4zz18-mto52zx1s7sn3ih' # expired_collection
       authorize_with user
       get :index, params: {

diff --git a/services/api/test/integration/collections_api_test.rb b/services/api/test/integration/collections_api_test.rb
index 86195fba750877af031abc92d451570a567ac096..73cbad64303391e82ef593d7a9cffc080ae6084f 100644 (file)
--- a/services/api/test/integration/collections_api_test.rb
+++ b/services/api/test/integration/collections_api_test.rb
@@ -495,4 +495,82 @@ class CollectionsApiTest < ActionDispatch::IntegrationTest
     assert_equal Hash, json_response['properties'].class, 'Collection properties attribute should be of type hash'
     assert_equal 'value_1', json_response['properties']['property_1']
   end
+
+  test "update collection with versioning enabled and using preserve_version" do
+    Rails.configuration.Collections.CollectionVersioning = true
+    Rails.configuration.Collections.PreserveVersionIfIdle = -1 # Disable auto versioning
+
+    signed_manifest = Collection.sign_manifest(". bad42fa702ae3ea7d888fef11b46f450+44 0:44:my_test_file.txt\n", api_token(:active))
+    post "/arvados/v1/collections",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection',
+          manifest_text: signed_manifest,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_not_nil json_response['uuid']
+    assert_equal 1, json_response['version']
+    assert_equal false, json_response['preserve_version']
+
+    # Versionable update including preserve_version=true should create a new
+    # version that will also be persisted.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v2',
+          preserve_version: true,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 2, json_response['version']
+    assert_equal true, json_response['preserve_version']
+
+    # 2nd versionable update including preserve_version=true should create a new
+    # version that will also be persisted.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v3',
+          preserve_version: true,
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 3, json_response['version']
+    assert_equal true, json_response['preserve_version']
+
+    # 3rd versionable update without including preserve_version should create a new
+    # version that will have its preserve_version attr reset to false.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v4',
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 4, json_response['version']
+    assert_equal false, json_response['preserve_version']
+
+    # 4th versionable update without including preserve_version=true should NOT
+    # create a new version.
+    put "/arvados/v1/collections/#{json_response['uuid']}",
+      params: {
+        format: :json,
+        collection: {
+          name: 'Test collection v5?',
+        }.to_json,
+      },
+      headers: auth(:active)
+    assert_response 200
+    assert_equal 4, json_response['version']
+    assert_equal false, json_response['preserve_version']
+  end
 end

diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 70645615a062d3a967de2af32e60ef67ff2f50d2..916ca095872db7a3a80d59799654dc32504e1f2b 100644 (file)
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -190,7 +190,7 @@ class CollectionTest < ActiveSupport::TestCase
 
   test "preserve_version updates" do
     Rails.configuration.Collections.CollectionVersioning = true
-    Rails.configuration.Collections.PreserveVersionIfIdle = 3600
+    Rails.configuration.Collections.PreserveVersionIfIdle = -1 # disabled
     act_as_user users(:active) do
       # Set up initial collection
       c = create_collection 'foo', Encoding::US_ASCII
@@ -227,7 +227,10 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal 2, c.replication_desired
       assert_equal true, c.preserve_version
       # This is a versionable update
-      c.update_attributes!({'name' => 'foobar'})
+      c.update_attributes!({
+        'preserve_version' => false,
+        'name' => 'foobar'
+      })
       c.reload
       assert_equal 3, c.version
       assert_equal false, c.preserve_version