Skip to content

Commit

Permalink
Use OpenStruct over MiqHashStruct
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
NickLaMuro committed Jun 20, 2018
1 parent 28dc337 commit 58a6a6b
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 22 deletions.
6 changes: 3 additions & 3 deletions app/models/miq_host_provision_workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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])}
Expand Down
12 changes: 6 additions & 6 deletions app/models/miq_provision_virt_workflow.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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}
Expand Down Expand Up @@ -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
Expand Down
15 changes: 8 additions & 7 deletions app/models/miq_request_workflow.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
require 'enumerator'
require 'miq-hash_struct'
require 'ostruct'

class MiqRequestWorkflow
include Vmdb::Logging
Expand Down Expand Up @@ -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]]
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion spec/models/miq_provision_virt_workflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 1 addition & 1 deletion spec/models/miq_request_workflow_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 58a6a6b

Please sign in to comment.