From 58a6a6bd0ee6935ad3deb4d9c639fbfe11f73066 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Tue, 24 Apr 2018 16:32:38 -0500 Subject: [PATCH] Use OpenStruct over MiqHashStruct This is almost a straight 1 for 1 conversion, and performs much better than our implementation: MiqHashStruct. * * * There is a small portion of this that I had to make a change on, and I _think_ it is correct, but here is the explaination for it: There was a chunk of code in this model that silently did nothing when it was an MiqHashStruct because of how often it relied on method_missing to function: field_values[:values].each do |tz| if tz[1].to_i_with_method == val.to_i_with_method # Save [value, description] for timezones array init_values[field_name] = [val, tz[0]] end end If `tz` in the above was a MiqHashStruct, and when `tz[1]` is called, it hit's `method_missing` with `:[]` as the method argument. This of course will almost never have a match in the underlying `@hash` in the MiqHashStruct, return nil and not blow up at the very least. When it is an OpenStruct, this fails with an error since `1` can't be converted to a symbol properly. Since the `tz` object can also potentially be an `Array`, I added a simple check to the code: field_values[:values].each do |tz| next unless tz.kind_of?(Array) # changed this line if tz[1].to_i_with_method == val.to_i_with_method # Save [value, description] for timezones array init_values[field_name] = [val, tz[0]] end end That allowed this to work when it should, and skip over when it wouldn't. --- app/models/miq_host_provision_workflow.rb | 6 +++--- app/models/miq_provision_virt_workflow.rb | 12 ++++++------ app/models/miq_request_workflow.rb | 15 ++++++++------- .../cloud_manager/provision_workflow_spec.rb | 8 ++++---- spec/models/miq_provision_virt_workflow_spec.rb | 2 +- spec/models/miq_request_workflow_spec.rb | 2 +- 6 files changed, 23 insertions(+), 22 deletions(-) diff --git a/app/models/miq_host_provision_workflow.rb b/app/models/miq_host_provision_workflow.rb index 706439d42a5..0b2615cf8a7 100644 --- a/app/models/miq_host_provision_workflow.rb +++ b/app/models/miq_host_provision_workflow.rb @@ -207,15 +207,15 @@ def ws_find_matching_ci(allowed_method, keys, match_str, klass) end def self.from_ws(*args) - # Move optional arguments into the MiqHashStruct object + # Move optional arguments into the OpenStruct object prov_args = args[0, 6] - prov_options = MiqHashStruct.new(:values => args[6], :ems_custom_attributes => args[7], :miq_custom_attributes => args[8]) + prov_options = OpenStruct.new(:values => args[6], :ems_custom_attributes => args[7], :miq_custom_attributes => args[8]) prov_args << prov_options MiqHostProvisionWorkflow.from_ws_ver_1_x(*prov_args) end def self.from_ws_ver_1_x(version, user, template_fields, vm_fields, requester, tags, options) - options = MiqHashStruct.new if options.nil? + options = OpenStruct.new if options.nil? _log.warn("Web-service host provisioning starting with interface version <#{version}> by requester <#{user.userid}>") init_options = {:use_pre_dialog => false, :request_type => request_type(parse_ws_string(template_fields)[:request_type])} diff --git a/app/models/miq_provision_virt_workflow.rb b/app/models/miq_provision_virt_workflow.rb index da43cd00b7a..914f3537597 100644 --- a/app/models/miq_provision_virt_workflow.rb +++ b/app/models/miq_provision_virt_workflow.rb @@ -724,8 +724,8 @@ def self.from_ws(*args) version = args.first.to_f return from_ws_ver_1_0(*args) if version == 1.0 - # Move optional arguments into the MiqHashStruct object - prov_options = MiqHashStruct.new( + # Move optional arguments into the OpenStruct object + prov_options = OpenStruct.new( :values => args[6], :ems_custom_attributes => args[7], :miq_custom_attributes => args[8], @@ -938,7 +938,7 @@ def ws_customize_fields(values, _fields, data) end def self.from_ws_ver_1_x(version, user, template_fields, vm_fields, requester, tags, options) - options = MiqHashStruct.new if options.nil? + options = OpenStruct.new if options.nil? _log.warn("Web-service provisioning starting with interface version <#{version}> by requester <#{user.userid}>") init_options = {:use_pre_dialog => false, :request_type => request_type(parse_ws_string(template_fields)[:request_type]), :initial_pass => true} @@ -1045,11 +1045,11 @@ def create_hash_struct_from_vm_or_template(vm_or_template, options) :v_total_snapshots => vm_or_template.v_total_snapshots, :evm_object_class => :Vm} data_hash[:cloud_tenant] = vm_or_template.cloud_tenant if vm_or_template.respond_to?(:cloud_tenant) - hash_struct = MiqHashStruct.new(data_hash) - hash_struct.operating_system = MiqHashStruct.new( + hash_struct = OpenStruct.new(data_hash) + hash_struct.operating_system = OpenStruct.new( :product_name => vm_or_template.operating_system.product_name ) if vm_or_template.operating_system - hash_struct.ext_management_system = MiqHashStruct.new( + hash_struct.ext_management_system = OpenStruct.new( :name => vm_or_template.ext_management_system.name ) if vm_or_template.ext_management_system if options[:include_datacenter] == true diff --git a/app/models/miq_request_workflow.rb b/app/models/miq_request_workflow.rb index 89cc31bc443..ae4d3452414 100644 --- a/app/models/miq_request_workflow.rb +++ b/app/models/miq_request_workflow.rb @@ -1,5 +1,5 @@ require 'enumerator' -require 'miq-hash_struct' +require 'ostruct' class MiqRequestWorkflow include Vmdb::Logging @@ -113,6 +113,7 @@ def init_from_dialog(init_values) init_values[field_name] = [val, field_values[:values][val]] else field_values[:values].each do |tz| + next unless tz.kind_of?(Array) if tz[1].to_i_with_method == val.to_i_with_method # Save [value, description] for timezones array init_values[field_name] = [val, tz[0]] @@ -641,7 +642,7 @@ def tag_symbol end def build_ci_hash_struct(ci, props) - nh = MiqHashStruct.new(:id => ci.id, :evm_object_class => ci.class.base_class.name.to_sym) + nh = OpenStruct.new(:id => ci.id, :evm_object_class => ci.class.base_class.name.to_sym) props.each { |p| nh.send("#{p}=", ci.send(p)) } nh end @@ -843,7 +844,7 @@ def find_classes_under_ci(item, klass) def load_ems_node(item, log_header) @ems_xml_nodes ||= {} - klass_name = if item.kind_of?(MiqHashStruct) + klass_name = if item.kind_of?(OpenStruct) Object.const_get(item.evm_object_class) else item.class.base_class @@ -855,7 +856,7 @@ def load_ems_node(item, log_header) def ems_has_clusters? found = each_ems_metadata(nil, EmsCluster) { |ci| break(ci) } - return found.evm_object_class == :EmsCluster if found.kind_of?(MiqHashStruct) + return found.evm_object_class == :EmsCluster if found.kind_of?(OpenStruct) false end @@ -1023,7 +1024,7 @@ def customization_spec_to_hash_struct(ci) def load_ar_obj(ci) return load_ar_objs(ci) if ci.kind_of?(Array) - return ci unless ci.kind_of?(MiqHashStruct) + return ci unless ci.kind_of?(OpenStruct) ci.evm_object_class.to_s.camelize.constantize.find_by(:id => ci.id) end @@ -1243,7 +1244,7 @@ def set_ws_field_value(values, key, data, dialog_name, dlg_fields) field_values = dlg_field[:values] _log.info("processing key <#{dialog_name}:#{key}(#{data_type})> with values <#{field_values.inspect}>") if field_values.present? - result = if field_values.first.kind_of?(MiqHashStruct) + result = if field_values.first.kind_of?(OpenStruct) found = field_values.detect { |v| v.id == set_value } [found.id, found.name] if found elsif data_type == :array_integer @@ -1284,7 +1285,7 @@ def set_ws_field_value_by_display_name(values, key, data, dialog_name, dlg_field field_values = dlg_field[:values] _log.info("processing key <#{dialog_name}:#{key}(#{data_type})> with values <#{field_values.inspect}>") if field_values.present? - result = if field_values.first.kind_of?(MiqHashStruct) + result = if field_values.first.kind_of?(OpenStruct) found = field_values.detect { |v| v.send(obj_key).to_s.downcase == find_value } [found.id, found.send(obj_key)] if found else diff --git a/spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb b/spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb index 57fb7d70d6c..2be9fb928c8 100644 --- a/spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb +++ b/spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb @@ -25,8 +25,8 @@ context "with allowed customization templates" do it "#allowed_customization_templates" do - expect(workflow.allowed_customization_templates.first).to be_a(MiqHashStruct) - expect(sysprep_workflow.allowed_customization_templates.first).to be_a(MiqHashStruct) + expect(workflow.allowed_customization_templates.first).to be_a(OpenStruct) + expect(sysprep_workflow.allowed_customization_templates.first).to be_a(OpenStruct) end it "should retrieve cloud-init templates when cloning" do @@ -35,7 +35,7 @@ result = workflow.allowed_customization_templates(options) customization_template = workflow.instance_variable_get(:@values)[:customization_template_script] - template_hash = result.first.instance_variable_get(:@hash) + template_hash = result.first.instance_variable_get(:@table) expect(customization_template).to eq cloud_init_template.script expect(template_hash).to be_a(Hash) @@ -53,7 +53,7 @@ result = sysprep_workflow.allowed_customization_templates(options) customization_template = sysprep_workflow.instance_variable_get(:@values)[:customization_template_script] - template_hash = result.first.instance_variable_get(:@hash) + template_hash = result.first.instance_variable_get(:@table) expect(customization_template).to eq sysprep_template.script expect(template_hash).to be_a(Hash) diff --git a/spec/models/miq_provision_virt_workflow_spec.rb b/spec/models/miq_provision_virt_workflow_spec.rb index 230dfce52a6..95163464f71 100644 --- a/spec/models/miq_provision_virt_workflow_spec.rb +++ b/spec/models/miq_provision_virt_workflow_spec.rb @@ -262,7 +262,7 @@ host1 = FactoryGirl.create(:host_vmware, :ems_id => ems.id) src_vm = FactoryGirl.create(:vm_vmware, :host => host1, :ems_id => ems.id) allow(workflow).to receive(:source_vm_rbac_filter).and_return([src_vm]) - expect(workflow.ws_find_template_or_vm("", "VMWARE", "asdf-adsf", "asdfadfasdf")).to be_a(MiqHashStruct) + expect(workflow.ws_find_template_or_vm("", "VMWARE", "asdf-adsf", "asdfadfasdf")).to be_a(OpenStruct) end end diff --git a/spec/models/miq_request_workflow_spec.rb b/spec/models/miq_request_workflow_spec.rb index ae3c49c461f..96f22cdd26e 100644 --- a/spec/models/miq_request_workflow_spec.rb +++ b/spec/models/miq_request_workflow_spec.rb @@ -625,7 +625,7 @@ let(:storage) { FactoryGirl.create(:storage) } it 'filters out storage_clusters not in same ems' do - allow(workflow).to receive(:get_source_and_targets).and_return(:ems => MiqHashStruct.new(:id => ems.id)) + allow(workflow).to receive(:get_source_and_targets).and_return(:ems => OpenStruct.new(:id => ems.id)) storage_cluster1 = FactoryGirl.create(:storage_cluster, :name => 'test_storage_cluster1', :ems_id => ems.id) storage_cluster2 = FactoryGirl.create(:storage_cluster, :name => 'test_storage_cluster2', :ems_id => ems.id + 1) storage_cluster1.add_child(storage)