[WIP] Add an option to store record changes

lib/inventory_refresh/inventory_collection.rb

@@ -94,7 +94,7 @@ class InventoryCollection
 
     attr_accessor :attributes_blacklist, :attributes_whitelist
 
-    attr_reader :model_class, :strategy, :custom_save_block, :parent, :internal_attributes, :delete_method, :dependency_attributes, :manager_ref, :create_only, :association, :complete, :update_only, :transitive_dependency_attributes, :check_changed, :arel, :inventory_object_attributes, :name, :saver_strategy, :targeted_scope, :default_values, :targeted_arel, :targeted, :manager_ref_allowed_nil, :use_ar_object, :created_records, :updated_records, :deleted_records, :retention_strategy, :custom_reconnect_block, :batch_extra_attributes, :references_storage, :unconnected_edges, :assert_graph_integrity
+    attr_reader :model_class, :strategy, :custom_save_block, :parent, :internal_attributes, :delete_method, :dependency_attributes, :manager_ref, :create_only, :association, :complete, :update_only, :transitive_dependency_attributes, :check_changed, :arel, :inventory_object_attributes, :name, :saver_strategy, :targeted_scope, :default_values, :targeted_arel, :targeted, :manager_ref_allowed_nil, :use_ar_object, :created_records, :updated_records, :deleted_records, :retention_strategy, :custom_reconnect_block, :batch_extra_attributes, :references_storage, :unconnected_edges, :assert_graph_integrity, :track_record_changes, :record_changes
 
     delegate :<<,
              :build,
@@ -149,7 +149,8 @@ def initialize(properties = {})
                  properties[:update_only],
                  properties[:use_ar_object],
                  properties[:targeted],
-                 properties[:assert_graph_integrity])
+                 properties[:assert_graph_integrity],
+                 properties[:track_record_changes])
 
       init_strategies(properties[:strategy],
                       properties[:saver_strategy],
@@ -214,6 +215,11 @@ def store_deleted_records(records)
       @deleted_records.concat(records_identities(records))
     end
 
+    def store_record_changes(records)
+      records = [records] unless records.respond_to?(:map)
+      @record_changes.merge!(records.map { |r| [record_identity(r)[:id], r.changes.slice(*track_record_changes)] }.to_h)
+    end
+
     # @return [Array<Symbol>] all columns that are part of the best fit unique index
     def unique_index_columns
       return @unique_index_columns if @unique_index_columns
@@ -306,6 +312,10 @@ def base_columns
       @base_columns ||= (unique_index_columns + internal_columns + not_null_columns).uniq
     end
 
+    def track_changed_columns
+      @track_changed_columns ||= track_record_changes? ? track_record_changes : []
+    end
+
     # @param value [Object] Object we want to test
     # @return [Boolean] true is value is kind of InventoryRefresh::InventoryObject
     def inventory_object?(value)

lib/inventory_refresh/inventory_collection/builder.rb

@@ -36,6 +36,7 @@ def self.allowed_properties
           update_only
           use_ar_object
           assert_graph_integrity
+          track_record_changes
         ].to_set
       end
 

lib/inventory_refresh/inventory_collection/helpers/initialize_helper.rb

@@ -177,9 +177,12 @@ def init_ic_relations(dependency_attributes, parent_inventory_collections = nil)
         #        for the batch saver strategy.
         # @param targeted [Boolean] True if the collection is targeted, in that case it will be leveraging :manager_uuids
         #        :parent_inventory_collections and :targeted_arel to save a subgraph of a data.
+        # @param track_record_changes [Boolean/Array] By default false.  If false no changes to the InventoryObject will be stored,
+        #        otherwise changes to properties enumerated in the array will be stored for use by other inventory collections
+        #        Example: :track_record_changes => [:name, :raw_power_state]
         def init_flags(complete, create_only, check_changed,
                        update_only, use_ar_object, targeted,
-                       assert_graph_integrity)
+                       assert_graph_integrity, track_record_changes)
           @complete               = complete.nil? ? true : complete
           @create_only            = create_only.nil? ? false : create_only
           @check_changed          = check_changed.nil? ? true : check_changed
@@ -188,6 +191,7 @@ def init_flags(complete, create_only, check_changed,
           @use_ar_object          = use_ar_object || false
           @targeted               = !!targeted
           @assert_graph_integrity = assert_graph_integrity.nil? ? true : assert_graph_integrity
+          @track_record_changes   = process_track_record_changes(track_record_changes)
         end
 
         # @param attributes_blacklist [Array] Attributes we do not want to include into saving. We cannot blacklist an
@@ -390,6 +394,7 @@ def init_changed_records_stats
           @created_records = []
           @updated_records = []
           @deleted_records = []
+          @record_changes  = {}
         end
 
         # Processes passed saver strategy
@@ -450,6 +455,16 @@ def process_retention_strategy(retention_strategy)
                   ":destroy and :archive"
           end
         end
+
+        def process_track_record_changes(track_record_changes)
+          return false unless track_record_changes
+
+          if !track_record_changes.kind_of?(Array)
+            raise "Invalid value for track_record_changes: #{track_record_changes}, allowed values are false or an Array"
+          end
+
+          track_record_changes.map(&:to_s)
+        end
       end
     end
   end

lib/inventory_refresh/inventory_collection/helpers/questions_helper.rb

@@ -87,6 +87,11 @@ def targeted?
           targeted
         end
 
+        # @return [Boolean] if true will store changes to the InventoryObject
+        def track_record_changes?
+          track_record_changes.present?
+        end
+
         # True if processing of this InventoryCollection object would lead to no operations. Then we use this marker to
         # stop processing of the InventoryCollector object very soon, to avoid a lot of unnecessary Db queries, etc.
         #

lib/inventory_refresh/save_collection/saver/base.rb

@@ -24,7 +24,7 @@ def initialize(inventory_collection)
         @arel_primary_key       = @model_class.arel_table[@primary_key]
         @unique_index_keys      = inventory_collection.unique_index_keys
         @unique_index_keys_to_s = inventory_collection.manager_ref_to_cols.map(&:to_s)
-        @select_keys            = [@primary_key] + @unique_index_keys_to_s + internal_columns.map(&:to_s)
+        @select_keys            = [@primary_key] + @unique_index_keys_to_s + internal_columns.map(&:to_s) + track_changed_columns.map(&:to_s)
         @unique_db_primary_keys = Set.new
         @unique_db_indexes      = Set.new
 
@@ -93,6 +93,7 @@ def save_inventory_collection!
                :build_stringified_reference_for_record,
                :resource_version_column,
                :internal_columns,
+               :track_changed_columns,
                :to => :inventory_collection
 
       # Applies serialize method for each relevant attribute, which will cast the value to the right type.

lib/inventory_refresh/save_collection/saver/concurrent_safe_batch.rb

@@ -231,7 +231,9 @@ def update_or_destroy_records!(records_batch_iterator, inventory_objects_index,
                                   # otherwise we would nullify the not sent attributes. Test e.g. on disks in cloud
                                   hash
                                 end
+
               assign_attributes_for_update!(hash_for_update, update_time)
+              store_record_changes!(record, hash_for_update) if inventory_collection.track_record_changes?
 
               hash_for_update[:id] = primary_key_value
               indexed_inventory_objects[index] = inventory_object
@@ -283,6 +285,19 @@ def changed?(_record, _hash, _all_attribute_keys)
         true
       end
 
+      def store_record_changes!(record, hash_for_update)
+        changes = inventory_collection.track_record_changes.map do |col|
+          previous = record_key(record, col)
+          current  = hash_for_update[col.to_sym]
+
+          next if previous == current
+
+          [col, [previous, current]]
+        end.compact
+
+        inventory_collection.record_changes[record_key(record, primary_key)] = changes.to_h
+      end
+
       def db_columns_index(record, pure_sql: false)
         # Incoming values are in SQL string form.
         # TODO(lsmola) unify this behavior with object_index_with_keys method in InventoryCollection

lib/inventory_refresh/save_collection/saver/default.rb

@@ -13,7 +13,9 @@ class Default < InventoryRefresh::SaveCollection::Saver::Base
       #        key value
       def update_record!(record, hash, inventory_object)
         record.assign_attributes(hash.except(:id))
+
         if !inventory_collection.check_changed? || record.changed?
+          inventory_collection.store_record_changes(record) if inventory_collection.track_record_changes?
           record.save
           inventory_collection.store_updated_records(record)
         end

spec/save_inventory/single_inventory_collection_spec.rb

@@ -92,6 +92,23 @@
         {:id => vm2.id, :ems_ref => "vm_ems_ref_2", :name => "vm_changed_name_2", :location => "vm_location_2"}
       )
     end
+
+    describe '#track_record_changes' do
+      it "doesn't track changes for new records" do
+        # Initialize the InventoryCollections
+        @persister.add_collection(:vms) do |builder|
+          builder.add_properties(
+            :model_class => ManageIQ::Providers::CloudManager::Vm
+          )
+        end
+        (1..2).each { |i| @persister.vms.build(vm_data(i)) }
+
+        # Invoke the InventoryCollections saving
+        InventoryRefresh::SaveInventory.save_inventory(@ems, @persister.inventory_collections)
+
+        expect(@persister.vms.record_changes).to be_empty
+      end
+    end
   end
 
   context 'with existing Vms in the DB' do
@@ -193,6 +210,7 @@
         expect(@persister.vms.created_records).to match_array([])
         expect(@persister.vms.updated_records).to match_array([{:id => @vm1.id}])
         expect(@persister.vms.deleted_records).to match_array([{:id => @vm2.id}])
+        expect(@persister.vms.record_changes).to be_empty
 
         # Assert that saved data do miss the deleted VM
         assert_all_records_match_hashes(
@@ -258,6 +276,97 @@
       end
     end
 
+    describe "#track_record_changes" do
+      context 'with VM InventoryCollection track_record_changes not used' do
+        let :changed_data do
+          [
+            vm_data(1).merge(:name            => "vm_changed_name_1",
+                             :location        => "vm_changed_location_1",
+                             :uid_ems         => "uid_ems_changed_1",
+                             :raw_power_state => "raw_power_state_changed_1"),
+            vm_data(2).merge(:name            => "vm_changed_name_2",
+                             :location        => "vm_changed_location_2",
+                             :uid_ems         => "uid_ems_changed_2",
+                             :raw_power_state => "raw_power_state_changed_2"),
+            vm_data(3).merge(:name            => "vm_changed_name_3",
+                             :location        => "vm_changed_location_3",
+                             :uid_ems         => "uid_ems_changed_3",
+                             :raw_power_state => "raw_power_state_changed_3")
+          ]
+        end
+
+        it "doesn't track record changes" do
+          @persister.add_collection(:vms) do |builder|
+            builder.add_properties(
+              :model_class          => ManageIQ::Providers::CloudManager::Vm,
+              :track_record_changes => false
+            )
+          end
+
+          changed_data.each { |vm_data| @persister.vms.build(vm_data) }
+
+          InventoryRefresh::SaveInventory.save_inventory(@ems, @persister.inventory_collections)
+
+          expect(@persister.vms.record_changes).to be_blank
+        end
+      end
+
+      context 'with VM InventoryCollection track_record_changes used' do
+        let :changed_data do
+          [
+            vm_data(1).merge(:name            => "vm_changed_name_1",
+                             :location        => "vm_changed_location_1",
+                             :uid_ems         => "uid_ems_changed_1",
+                             :raw_power_state => "raw_power_state_changed_1"),
+            vm_data(2).merge(:name            => "vm_changed_name_2",
+                             :location        => "vm_changed_location_2",
+                             :uid_ems         => "uid_ems_changed_2",
+                             :raw_power_state => "raw_power_state_changed_2"),
+            vm_data(3).merge(:name            => "vm_changed_name_3",
+                             :location        => "vm_changed_location_3",
+                             :uid_ems         => "uid_ems_changed_3",
+                             :raw_power_state => "raw_power_state_changed_3")
+          ]
+        end
+
+        it 'records changes to the records' do
+          @persister.add_collection(:vms) do |builder|
+            builder.add_properties(
+              :model_class          => ManageIQ::Providers::CloudManager::Vm,
+              :track_record_changes => %i[location raw_power_state]
+            )
+          end
+
+          changed_data.each { |vm_data| @persister.vms.build(vm_data) }
+
+          InventoryRefresh::SaveInventory.save_inventory(@ems, @persister.inventory_collections)
+
+          vm_1_changes = @persister.vms.record_changes[@vm1.id]
+          expect(vm_1_changes).to include(
+            "location"        => ["vm_location_1", "vm_changed_location_1"],
+            "raw_power_state" => ["unknown", "raw_power_state_changed_1"]
+          )
+        end
+
+        it 'ignores changes not in the filter' do
+          @persister.add_collection(:vms) do |builder|
+            builder.add_properties(
+              :model_class          => ManageIQ::Providers::CloudManager::Vm,
+              :track_record_changes => %i[location raw_power_state]
+            )
+          end
+
+          changed_data.each { |vm_data| @persister.vms.build(vm_data) }
+
+          InventoryRefresh::SaveInventory.save_inventory(@ems, @persister.inventory_collections)
+
+          vm_1_changes = @persister.vms.record_changes[@vm1.id]
+
+          expect(vm_1_changes.keys).not_to include("uid_ems", "name")
+        end
+      end
+    end
+
     context 'with VM InventoryCollection blacklist or whitelist used' do
       let :changed_data do
         [
@@ -680,7 +789,7 @@
     end
   end
 
-  %i[default batch].each do |saver_strategy|
+  %i[default batch concurrent_safe_batch].each do |saver_strategy|
     context "testing reconnect logic with saver_strategy: :#{saver_strategy}" do
       it 'reconnects existing VM' do
         # Fill DB with test Vms
@@ -734,5 +843,45 @@
         )
       end
     end
+
+    context "testing track_record_changes with saver_strategy: :#{saver_strategy}" do
+      let :changed_data do
+        [
+          vm_data(1).merge(:name            => "vm_changed_name_1",
+                           :raw_power_state => "raw_power_state_changed_1"),
+          vm_data(2).merge(:name            => "vm_changed_name_2",
+                           :raw_power_state => "raw_power_state_changed_2"),
+        ]
+      end
+
+      context "with existing VMS" do
+        before do
+          # Fill DB with test Vms
+          @vm1 = FactoryBot.create(:vm_cloud, vm_data(1).merge(:ext_management_system => @ems))
+          @vm2 = FactoryBot.create(:vm_cloud, vm_data(2).merge(:ext_management_system => @ems))
+        end
+
+        it "tracks changes to records" do
+          # Initialize the InventoryCollections
+          @persister.add_collection(:vms) do |builder|
+            builder.add_properties(
+              :model_class          => ManageIQ::Providers::CloudManager::Vm,
+              :saver_strategy       => saver_strategy,
+              :track_record_changes => %i[name raw_power_state]
+            )
+          end
+          changed_data.each { |vm_data| @persister.vms.build(vm_data) }
+
+          # Invoke the InventoryCollections saving
+          InventoryRefresh::SaveInventory.save_inventory(@ems, @persister.inventory_collections)
+
+          vm_1_changes = @persister.vms.record_changes[@vm1.id]
+          expect(vm_1_changes).to include(
+            "name"            => ["vm_name_1", "vm_changed_name_1"],
+            "raw_power_state" => ["unknown", "raw_power_state_changed_1"]
+          )
+        end
+      end
+    end
   end
 end