Merge branch '17152-collection-preserve-version-changes'
authorLucas Di Pentima <lucas@di-pentima.com.ar>
Mon, 14 Dec 2020 21:35:29 +0000 (18:35 -0300)
committerLucas Di Pentima <lucas@di-pentima.com.ar>
Mon, 14 Dec 2020 21:35:29 +0000 (18:35 -0300)
Refs #17152

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

doc/admin/upgrading.html.textile.liquid
doc/api/methods/collections.html.textile.liquid
services/api/app/controllers/arvados/v1/collections_controller.rb
services/api/app/controllers/arvados/v1/schema_controller.rb
services/api/app/models/api_client_authorization.rb
services/api/app/models/collection.rb
services/api/test/fixtures/collections.yml
services/api/test/functional/arvados/v1/collections_controller_test.rb
services/api/test/integration/collections_api_test.rb
services/api/test/unit/collection_test.rb
services/api/test/unit/log_test.rb

index 3f622112e95391d5364be1e16f211729b2c4a150..ac697d87071ce4d31ce59ec5015d93d1f50f8c79 100644 (file)
@@ -35,10 +35,14 @@ TODO: extract this information based on git commit messages and generate changel
 <div class="releasenotes">
 </notextile>
 
-h2(#main). development main (as of 2020-10-28)
+h2(#main). development main (as of 2020-12-10)
 
 "Upgrading from 2.1.0":#v2_1_0
 
+h3. Changes on the collection's @preserve_version@ attribute semantics
+
+The @preserve_version@ attribute on collections was originally designed to allow clients to persist a preexisting collection version. This forced clients to make 2 requests if the intention is to "make this set of changes in a new version that will be kept", so we have changed the semantics to do just that: When passing @preserve_version=true@ along with other collection updates, the current version is persisted and also the newly created one will be persisted on the next update.
+
 h3. Centos7 Python 3 dependency upgraded to python3
 
 Now that Python 3 is part of the base repository in CentOS 7, the Python 3 dependency for Centos7 Arvados packages was changed from SCL rh-python36 to python3.
index 4372cc2f5e7dd17bd2c662bf7224cf6fdd5d4c6f..fd4a36f291ae90641a2e48606af66b35311c0780 100644 (file)
@@ -38,7 +38,7 @@ table(table table-bordered table-condensed).
 |is_trashed|boolean|True if @trash_at@ is in the past, false if not.||
 |current_version_uuid|string|UUID of the collection's current version. On new collections, it'll be equal to the @uuid@ attribute.||
 |version|number|Version number, starting at 1 on new collections. This attribute is read-only.||
-|preserve_version|boolean|When set to true on a current version, it will be saved on the next versionable update.||
+|preserve_version|boolean|When set to true on a current version, it will be persisted. When passing @true@ as part of a bigger update call, both current and newly created versions are persisted.||
 |file_count|number|The total number of files in the collection. This attribute is read-only.||
 |file_size_total|number|The sum of the file sizes in the collection. This attribute is read-only.||
 
index 2e7e2f82b07c024168bd34080f000a91eaea55ac..440ac640169404bc7a0fac4738f55febbd78c0cc 100644 (file)
@@ -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),
index b9aba2726f555883d304fac490f050b6177275b4..9e19397994f2c13abd924ba11a40c88fdccb71ec 100644 (file)
@@ -36,7 +36,7 @@ class Arvados::V1::SchemaController < ApplicationController
         # format is YYYYMMDD, must be fixed width (needs to be lexically
         # sortable), updated manually, may be used by clients to
         # determine availability of API server features.
-        revision: "20200331",
+        revision: "20201210",
         source_version: AppVersion.hash,
         sourceVersion: AppVersion.hash, # source_version should be deprecated in the future
         packageVersion: AppVersion.package_version,
index 6b308a231cb7ede8cf50b949da75a861a46219d3..9290e01a1a7a5b4284580615585d963a5201c386 100644 (file)
@@ -394,7 +394,6 @@ class ApiClientAuthorization < ArvadosModel
   end
 
   def log_update
-
     super unless (saved_changes.keys - UNLOGGED_CHANGES).empty?
   end
 end
index 3637f34e105b1bd66013a7cc0882860dc434034e..4e7b64cf5374fb38003d70790aebd8caee0931fb 100644 (file)
@@ -62,6 +62,8 @@ class Collection < ArvadosModel
     t.add :file_size_total
   end
 
+  UNLOGGED_CHANGES = ['preserve_version', 'updated_at']
+
   after_initialize do
     @signatures_checked = false
     @computed_pdh_for_manifest_text = false
@@ -274,7 +276,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}
@@ -286,7 +288,6 @@ class Collection < ArvadosModel
 
       if should_preserve_version
         self.version += 1
-        self.preserve_version = false
       end
 
       yield
@@ -305,6 +306,12 @@ class Collection < ArvadosModel
     end
   end
 
+  def maybe_update_modified_by_fields
+    if !(self.changes.keys - ['updated_at', 'preserve_version']).empty?
+      super
+    end
+  end
+
   def syncable_updates
     updates = {}
     if self.changes.any?
@@ -359,6 +366,7 @@ class Collection < ArvadosModel
 
     idle_threshold = Rails.configuration.Collections.PreserveVersionIfIdle
     if !self.preserve_version_was &&
+      !self.preserve_version &&
       (idle_threshold < 0 ||
         (idle_threshold > 0 && self.modified_at_was > db_current_time-idle_threshold.seconds))
       return false
@@ -742,4 +750,8 @@ class Collection < ArvadosModel
     self.current_version_uuid ||= self.uuid
     true
   end
+
+  def log_update
+    super unless (saved_changes.keys - UNLOGGED_CHANGES).empty?
+  end
 end
index 61bb3f79f8c882565b1ee9bd4ec5806963c1cf53..1f2eab73afedd748070086e8083f8bccc2256af8 100644 (file)
@@ -21,7 +21,7 @@ collection_owned_by_active:
   portable_data_hash: fa7aeb5140e2848d39b416daeef4ffc5+45
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   created_at: 2014-02-03T17:22:54Z
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_client_uuid: zzzzz-ozdt8-teyxzyd8qllg11h
   modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
   modified_at: 2014-02-03T18:22:54Z
   updated_at: 2014-02-03T18:22:54Z
index c025394bc1f33616684be72861862126642d95c4..1ca2dd1dc109857e6987fdaa32caad2b04a52f8b 100644 (file)
@@ -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: {
index 86195fba750877af031abc92d451570a567ac096..73cbad64303391e82ef593d7a9cffc080ae6084f 100644 (file)
@@ -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
index a28893e0119162ea388b698991599e71d74cc3c8..916ca095872db7a3a80d59799654dc32504e1f2b 100644 (file)
@@ -188,9 +188,9 @@ 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 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
@@ -199,28 +199,61 @@ class CollectionTest < ActiveSupport::TestCase
       assert_equal false, c.preserve_version
       # This update shouldn't produce a new version, as the idle time is not up
       c.update_attributes!({
-        'name' => 'bar',
-        'preserve_version' => true
+        'name' => 'bar'
       })
       c.reload
       assert_equal 1, c.version
       assert_equal 'bar', c.name
+      assert_equal false, c.preserve_version
+      # This update should produce a new version, even if the idle time is not up
+      # and also keep the preserve_version=true flag to persist it.
+      c.update_attributes!({
+        'name' => 'baz',
+        'preserve_version' => true
+      })
+      c.reload
+      assert_equal 2, c.version
+      assert_equal 'baz', c.name
       assert_equal true, c.preserve_version
       # Make sure preserve_version is not disabled after being enabled, unless
       # a new version is created.
+      # This is a non-versionable update
       c.update_attributes!({
         'preserve_version' => false,
         'replication_desired' => 2
       })
       c.reload
-      assert_equal 1, c.version
+      assert_equal 2, c.version
       assert_equal 2, c.replication_desired
       assert_equal true, c.preserve_version
-      c.update_attributes!({'name' => 'foobar'})
+      # This is a versionable update
+      c.update_attributes!({
+        'preserve_version' => false,
+        'name' => 'foobar'
+      })
       c.reload
-      assert_equal 2, c.version
+      assert_equal 3, c.version
       assert_equal false, c.preserve_version
       assert_equal 'foobar', c.name
+      # Flipping only 'preserve_version' to true doesn't create a new version
+      c.update_attributes!({'preserve_version' => true})
+      c.reload
+      assert_equal 3, c.version
+      assert_equal true, c.preserve_version
+    end
+  end
+
+  test "preserve_version updates don't change modified_at timestamp" do
+    act_as_user users(:active) do
+      c = create_collection 'foo', Encoding::US_ASCII
+      assert c.valid?
+      assert_equal false, c.preserve_version
+      modified_at = c.modified_at.to_f
+      c.update_attributes!({'preserve_version' => true})
+      c.reload
+      assert_equal true, c.preserve_version
+      assert_equal modified_at, c.modified_at.to_f,
+        'preserve_version updates should not trigger modified_at changes'
     end
   end
 
index 76d78f9eaa6cf14ebd83b8b7b138e3f94c69dff5..66c8c8d923d06f9f745f0c393f29ff99ce605f84 100644 (file)
@@ -228,6 +228,20 @@ class LogTest < ActiveSupport::TestCase
     assert_logged(auth, :update)
   end
 
+  test "don't log changes only to Collection.preserve_version" do
+    set_user_from_auth :admin_trustedclient
+    col = collections(:collection_owned_by_active)
+    start_log_count = get_logs_about(col).size
+    assert_equal false, col.preserve_version
+    col.preserve_version = true
+    col.save!
+    assert_equal(start_log_count, get_logs_about(col).size,
+                 "log count changed after updating Collection.preserve_version")
+    col.name = 'updated by admin'
+    col.save!
+    assert_logged(col, :update)
+  end
+
   test "token isn't included in ApiClientAuthorization logs" do
     set_user_from_auth :admin_trustedclient
     auth = ApiClientAuthorization.new