From 500fb84d69877449954905fa3be0f528e8aed686 Mon Sep 17 00:00:00 2001 From: Beni Cherniavsky-Paskin Date: Tue, 16 Jan 2018 15:17:09 +0200 Subject: [PATCH] Compare decimal columns correctly in batch saver Will allow container_quota_items.quota_* columns to be used in manager_ref. https://bugzilla.redhat.com/show_bug.cgi?id=1504560 --- .../save_collection/saver/base.rb | 17 ++++++-- .../saver/concurrent_safe_batch.rb | 15 +++++-- .../helpers/spec_parsed_data.rb | 15 +++++++ .../save_inventory/init_data_helper.rb | 19 +++++++++ .../save_inventory/saver_strategies_spec.rb | 40 +++++++++++++++++++ 5 files changed, 99 insertions(+), 7 deletions(-) diff --git a/app/models/manager_refresh/save_collection/saver/base.rb b/app/models/manager_refresh/save_collection/saver/base.rb index ad0bd16bc44..6ac6be71113 100644 --- a/app/models/manager_refresh/save_collection/saver/base.rb +++ b/app/models/manager_refresh/save_collection/saver/base.rb @@ -37,21 +37,30 @@ def initialize(inventory_collection) .try(:[], "sql_type") end - @serializable_keys = @model_class.attribute_names.each_with_object({}) do |key, obj| + @serializable_keys = {} + @deserializable_keys = {} + @model_class.attribute_names.each do |key| attribute_type = @model_class.type_for_attribute(key.to_s) pg_type = @pg_types[key.to_sym] if inventory_collection.use_ar_object? # When using AR object, lets make sure we type.serialize(value) every value, so we have a slow but always # working way driven by a configuration - obj[key.to_sym] = attribute_type + @serializable_keys[key.to_sym] = attribute_type + @deserializable_keys[key.to_sym] = attribute_type elsif attribute_type.respond_to?(:coder) || attribute_type.type == :int4range || pg_type == "text[]" || pg_type == "character varying[]" # Identify columns that needs to be encoded by type.serialize(value), it's a costy operations so lets do # do it only for columns we need it for. - obj[key.to_sym] = attribute_type + # TODO: should these set @deserializable_keys too? + @serializable_keys[key.to_sym] = attribute_type + elsif attribute_type.type == :decimal + # Postgres formats decimal columns with fixed number of digits e.g. '0.100' + # Need to parse and let Ruby format the value to have a comparable string. + @serializable_keys[key.to_sym] = attribute_type + @deserializable_keys[key.to_sym] = attribute_type end end end @@ -76,7 +85,7 @@ def save_inventory_collection! attr_reader :unique_index_keys, :unique_index_keys_to_s, :select_keys, :unique_db_primary_keys, :unique_db_indexes, :primary_key, :arel_primary_key, :record_key_method, :pure_sql_records_fetching, :select_keys_indexes, - :batch_size, :batch_size_for_persisting, :model_class, :serializable_keys, :pg_types, :table_name + :batch_size, :batch_size_for_persisting, :model_class, :serializable_keys, :deserializable_keys, :pg_types, :table_name def save!(association) attributes_index = {} diff --git a/app/models/manager_refresh/save_collection/saver/concurrent_safe_batch.rb b/app/models/manager_refresh/save_collection/saver/concurrent_safe_batch.rb index 451e5a71274..2ec399afa71 100644 --- a/app/models/manager_refresh/save_collection/saver/concurrent_safe_batch.rb +++ b/app/models/manager_refresh/save_collection/saver/concurrent_safe_batch.rb @@ -100,15 +100,20 @@ def update_or_destroy_records!(records_batch_iterator, inventory_objects_index, next unless assert_distinct_relation(primary_key_value) + # Incoming values are in SQL string form. # TODO(lsmola) unify this behavior with object_index_with_keys method in InventoryCollection # TODO(lsmola) maybe we can drop the whole pure sql fetching, since everything will be targeted refresh # with streaming refresh? Maybe just metrics and events will not be, but those should be upsert only index = unique_index_keys_to_s.map do |attribute| + value = record_key(record, attribute) if attribute == "timestamp" + # TODO: can this be covered by @deserializable_keys? type = model_class.type_for_attribute(attribute) - type.cast(record_key(record, attribute)).utc.iso8601.to_s + type.cast(value).utc.iso8601.to_s + elsif (type = deserializable_keys[attribute.to_sym]) + type.deserialize(value).to_s else - record_key(record, attribute).to_s + value.to_s end end.join("__") @@ -259,7 +264,11 @@ def map_ids_to_inventory_objects(indexed_inventory_objects, all_attribute_keys, # for every remainders(a last batch in a stream of batches) if !supports_remote_data_timestamp?(all_attribute_keys) || result.count == batch_size_for_persisting result.each do |inserted_record| - key = unique_index_columns.map { |x| inserted_record[x.to_s] } + key = unique_index_columns.map do |x| + value = inserted_record[x.to_s] + type = deserializable_keys[x] + type ? type.deserialize(value) : value + end inventory_object = indexed_inventory_objects[key] inventory_object.id = inserted_record[primary_key] if inventory_object end diff --git a/spec/models/manager_refresh/helpers/spec_parsed_data.rb b/spec/models/manager_refresh/helpers/spec_parsed_data.rb index fbfc7fff11f..61560855004 100644 --- a/spec/models/manager_refresh/helpers/spec_parsed_data.rb +++ b/spec/models/manager_refresh/helpers/spec_parsed_data.rb @@ -1,3 +1,5 @@ +require 'bigdecimal' + module SpecParsedData def vm_data(i, data = {}) { @@ -100,4 +102,17 @@ def network_port_data(i, data = {}) :mac_address => "network_port_mac_#{i}", }.merge(data) end + + def container_quota_items_data(i, data = {}) + { + :quota_desired => BigDecimal("#{i}.#{i}"), + }.merge(data) + end + + def container_quota_items_attrs_data(i, data = {}) + { + :name => "container_quota_items_attrs_#{i}", + :resource_type => "ContainerQuotaItem", + }.merge(data) + end end diff --git a/spec/models/manager_refresh/save_inventory/init_data_helper.rb b/spec/models/manager_refresh/save_inventory/init_data_helper.rb index e70b539ea86..8a6a0da40c7 100644 --- a/spec/models/manager_refresh/save_inventory/init_data_helper.rb +++ b/spec/models/manager_refresh/save_inventory/init_data_helper.rb @@ -56,6 +56,25 @@ def network_ports_init_data(extra_attributes = {}) init_data(network.network_ports(extra_attributes)) end + # Following 2 are fictional, not like this in practice. + def container_quota_items_init_data(extra_attributes = {}) + init_data(extra_attributes).merge( + :model_class => ContainerQuotaItem, + :arel => ContainerQuotaItem.all, + :manager_ref => [:quota_desired], # a decimal column + ) + end + + # Quota items don't even have custom attrs; this is just to have a dependent + # for quota items collection to test their .id are set correctly. + def container_quota_items_attrs_init_data(extra_attributes = {}) + init_data(extra_attributes).merge( + :model_class => CustomAttribute, + :arel => CustomAttribute.where(:resource_type => 'ContainerQuotaItem'), + :manager_ref => [:name], + ) + end + def cloud ManagerRefresh::InventoryCollectionDefault::CloudManager end diff --git a/spec/models/manager_refresh/save_inventory/saver_strategies_spec.rb b/spec/models/manager_refresh/save_inventory/saver_strategies_spec.rb index e18dacbb2c7..7253d0c603c 100644 --- a/spec/models/manager_refresh/save_inventory/saver_strategies_spec.rb +++ b/spec/models/manager_refresh/save_inventory/saver_strategies_spec.rb @@ -171,6 +171,12 @@ ) ) ) + @data[:container_quota_items] = ::ManagerRefresh::InventoryCollection.new( + container_quota_items_init_data(inventory_collection_options(options)) + ) + @data[:container_quota_items_attrs] = ::ManagerRefresh::InventoryCollection.new( + container_quota_items_attrs_init_data(inventory_collection_options(options)) + ) # Parse data for InventoryCollections @network_port_data_1 = network_port_data(1).merge( @@ -211,6 +217,16 @@ @image_data_2 = image_data(2).merge(:name => "image_changed_name_2") @image_data_3 = image_data(3).merge(:name => "image_changed_name_3") + @container_quota_items_data_1 = container_quota_items_data(1) + @container_quota_items_data_2 = container_quota_items_data(2) + + @container_quota_items_attrs_data_1 = container_quota_items_attrs_data(1).merge( + :resource => @data[:container_quota_items].lazy_find(container_quota_items_data(1)[:quota_desired]) + ) + @container_quota_items_attrs_data_2 = container_quota_items_attrs_data(2).merge( + :resource => @data[:container_quota_items].lazy_find(container_quota_items_data(2)[:quota_desired]) + ) + # Fill InventoryCollections with data add_data_to_inventory_collection(@data[:network_ports], @network_port_data_1, @@ -227,6 +243,12 @@ add_data_to_inventory_collection(@data[:miq_templates], @image_data_2, @image_data_3) + add_data_to_inventory_collection(@data[:container_quota_items], + @container_quota_items_data_1, + @container_quota_items_data_2) + add_data_to_inventory_collection(@data[:container_quota_items_attrs], + @container_quota_items_attrs_data_1, + @container_quota_items_attrs_data_2) # Assert data before save expect(@network_port1.device).to eq @vm1 expect(@network_port1.name).to eq "network_port_name_1" @@ -253,6 +275,11 @@ @image2 = MiqTemplate.find(@image2.id) + @quota_item_1 = ContainerQuotaItem.find_by(:quota_desired => container_quota_items_data(1)[:quota_desired]) + @quota_item_2 = ContainerQuotaItem.find_by(:quota_desired => container_quota_items_data(2)[:quota_desired]) + @quota_attr_1 = CustomAttribute.find_by(:name => container_quota_items_attrs_data(1)[:name]) + @quota_attr_2 = CustomAttribute.find_by(:name => container_quota_items_attrs_data(2)[:name]) + # Check ICs stats expect(@data[:vms].created_records).to match_array(record_stats([@vm3, @vm31])) expect(@data[:vms].deleted_records).to match_array(record_stats([@vm1, @vm12, @vm4])) @@ -391,6 +418,19 @@ ) expect(::ManageIQ::Providers::CloudManager::AuthKeyPair.all).to eq([]) + + assert_all_records_match_hashes( + [CustomAttribute.where(:resource_type => 'ContainerQuotaItem')], + { + :id => @quota_attr_1.id, + :name => @quota_attr_1.name, + :resource_id => @quota_item_1.id, + }, { + :id => @quota_attr_2.id, + :name => @quota_attr_2.name, + :resource_id => @quota_item_2.id, + } + ) end end end