[ARVADOS] updated: 1.3.0-2836-ga6327d5c0

[Git user]

Summary of changes:
 services/api/app/models/arvados_model.rb     | 15 +++++++++++++++
 services/api/test/unit/arvados_model_test.rb | 25 +++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

       via  a6327d5c07da90adfc3356067d12281c36b3bfb9 (commit)
       via  9f3c3500b06fe1d02155d37be77fe78f8ffbd3d0 (commit)
      from  571e9ea468feaa22ccf56b9585a36d6d84f1b8bd (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 a6327d5c07da90adfc3356067d12281c36b3bfb9
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date:   Tue Jul 28 16:16:29 2020 -0300

    16470: Fixes false unpersisted status when retrieving a record with audit logs.
    
    The cleanest solution I came up with is to flag the instance when it's
    retrieved from the database, and reset any changes after stashing its
    state on the log_start_state callback.
    Haven't found a way to read the serialized attributes without making
    them appear as changed, and I think it isn't possible because the
    attributes have to be unserialized before the read operation, and thus
    the dirty state machinery would assume the attribute may be modified.
    This solution isn't ideal, but I think it's acceptable as it doesn't
    make additional database requests.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 80ea0c0b7..c3e1ff42a 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -16,6 +16,7 @@ class ArvadosModel < ApplicationRecord
   include DbCurrentTime
   extend RecordFilters
 
+  after_find :schedule_restoring_changes
   after_initialize :log_start_state
   before_save :ensure_permission_to_save
   before_save :ensure_owner_uuid_is_permitted
@@ -834,10 +835,24 @@ class ArvadosModel < ApplicationRecord
              Rails.configuration.AuditLogs.MaxDeleteBatch.to_i > 0)
   end
 
+  def schedule_restoring_changes
+    # This will be checked at log_start_state, to reset any (virtual) changes
+    # produced by the act of reading a serialized attribute.
+    @fresh_from_database = true
+  end
+
   def log_start_state
     if is_audit_logging_enabled?
       @old_attributes = Marshal.load(Marshal.dump(attributes))
       @old_logged_attributes = Marshal.load(Marshal.dump(logged_attributes))
+      if @fresh_from_database
+        # This instance was created from reading a database record. Attributes
+        # haven't been changed, but those serialized attributes will be reported
+        # as unpersisted, so we restore them to avoid issues with lock!() and
+        # with_lock().
+        restore_attributes
+        @fresh_from_database = nil
+      end
     end
   end
 

commit 9f3c3500b06fe1d02155d37be77fe78f8ffbd3d0
Author: Lucas Di Pentima <lucas at di-pentima.com.ar>
Date:   Tue Jul 28 13:55:50 2020 -0300

    16470: Adds ArvadosModel test exposing a bug with audit logs.
    
    When audit logs are enabled, fetching objects from models with serialized
    attributes (for example: User or ContainerRequest) return an unpersisted
    instance even if reload() is called on it.
    This is a problem because from Rails 5.2, lock!() and with_lock() will raise
    an exception when called on unpersisted instances.
    
    Arvados-DCO-1.1-Signed-off-by: Lucas Di Pentima <lucas at di-pentima.com.ar>

diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb
index c1db8c8b5..64f780713 100644
--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@@ -295,4 +295,29 @@ class ArvadosModelTest < ActiveSupport::TestCase
     c.reload
     assert_equal({'foo' => 'bar'}, c.properties)
   end
+
+  test 'serialized attributes dirty tracking with audit log settings' do
+    Rails.configuration.AuditLogs.MaxDeleteBatch = 1000
+    set_user_from_auth :admin
+    [false, true].each do |auditlogs_enabled|
+      if auditlogs_enabled
+        Rails.configuration.AuditLogs.MaxAge = 3600
+      else
+        Rails.configuration.AuditLogs.MaxAge = 0
+      end
+      [
+        User.find_by_uuid(users(:active).uuid),
+        ContainerRequest.find_by_uuid(container_requests(:queued).uuid),
+        Container.find_by_uuid(containers(:queued).uuid),
+        PipelineInstance.find_by_uuid(pipeline_instances(:has_component_with_completed_jobs).uuid),
+        PipelineTemplate.find_by_uuid(pipeline_templates(:two_part).uuid),
+        Job.find_by_uuid(jobs(:running).uuid)
+      ].each do |obj|
+        assert_not(obj.class.serialized_attributes.empty?,
+          "#{obj.class} model doesn't have serialized attributes")
+        # obj shouldn't have changed since it's just retrieved from the database
+        assert_not(obj.changed?, "#{obj.class} model's attribute(s) appear as changed: '#{obj.changes.keys.join(',')}' with audit logs #{auditlogs_enabled ? '': 'not '}enabled.")
+      end
+    end
+  end
 end

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


hooks/post-receive
--