From: Lucas Di Pentima Date: Fri, 11 Dec 2020 15:28:22 +0000 (-0300) Subject: 17152: Moves conditional preserve_version disabling to collection's controller. X-Git-Tag: 2.2.0~185^2~1 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/d5745d536d013a6731e0e6a872abe34d71f0995e 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 --- diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb index 2e7e2f82b0..440ac64016 100644 --- 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 e3802734a0..c2c60ec70e 100644 --- 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 c025394bc1..1ca2dd1dc1 100644 --- 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 86195fba75..73cbad6430 100644 --- 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 70645615a0..916ca09587 100644 --- 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