Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WIP] Add an option to store record changes #115

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

[WIP] Add an option to store record changes #115

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d454709
Add an option to store record changes
agrare Nov 21, 2022
e026a93
Change record_changes to hash
agrare Dec 20, 2022
21b565d
Add a filter to the changes
agrare Dec 20, 2022
87ab409
Track changes in concurrent_safe_batch
agrare Dec 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions lib/inventory_refresh/inventory_collection.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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],
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@record_changes.merge!(records.map { |r| [record_identity(r)[:id], r.changes.slice(*track_record_changes)] }.to_h)
records.each_with_object(@record_changes) { |r, h| h[record_identity(r)[:id]] = , r.changes.slice(*track_record_changes) }

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
Expand Down
1 change: 1 addition & 0 deletions lib/inventory_refresh/inventory_collection/builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ def self.allowed_properties
update_only
use_ar_object
assert_graph_integrity
track_record_changes
].to_set
end

Expand Down
17 changes: 16 additions & 1 deletion lib/inventory_refresh/inventory_collection/helpers/initialize_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Comment on lines +180 to +182
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: I went back and forth a lot on a separate attribute to track the filter plus a boolean to turn on/off, versus combining them all in one attribute. On one hand it is handy to only need one attribute (we have a lot of knobs here already) on the other hand could be confusing to have a boolean/array datatype.

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
Expand All @@ -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
Expand Down Expand Up @@ -390,6 +394,7 @@ def init_changed_records_stats
@created_records = []
@updated_records = []
@deleted_records = []
@record_changes = {}
end

# Processes passed saver strategy
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions lib/inventory_refresh/inventory_collection/helpers/questions_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
#
Expand Down
2 changes: 2 additions & 0 deletions lib/inventory_refresh/save_collection/saver/default.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
91 changes: 91 additions & 0 deletions spec/save_inventory/single_inventory_collection_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,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
[
Expand Down