virtual_delegate: make delegation more powerful

[kbrock]

BLOCKED by:

• VirtualDelegate: simplify virtual_attribute portion of virtual_delegate #10754 (2 commits focused on the virtual_attribute portion)

background

We liberally use virtual_attribute in our model definitions.
The :arel option allows us to define SQL that will use the attributes within queries.
This arel option is very important. It allows screens to only download 20 records for the screen rather than having to download every record and pruning in ruby.

The SQL used by :arel is tricky to construct, so helper methods like virtual_delegate automatically generate it.

This PR adds 2 enhancements to virtual_delegate by duplicating and modifying ActiveSupport's own delegate method:

1. allow :to to specify a name and description (e.g.: :to => "hardware.cpu_sockets")
2. add :default option, so nil references return something other than nil.

before

The method Vm#num_cpu is defined as an attribute with ruby method definition.

virtual_attribute :num_cpu, :integer, :uses => :hardware

def num_cpu
  hardware.nil? ? 0 : hardware.cpu_sockets
end

Pages that sort by num_cpu bring back every Vm and corresponding Hardware records into memory. Then 20 of those records are picked and displayed on the screen.

Vm.order(:num_cpu) # invalid
Vm.where(:num_cpu =>2) # invalid

after virtual_delegate

virtual_delegate :num_cpu, :to => :hardware, :allow_nil => true

Defines the attribute with a sql component and the delegate ruby method:

virtual_attribute :num_cpu, :integer, :uses => :hardware, :arel => ...

def num_cpu
  hardware.try(:num_cpu)
end

Issues:

vm.num_cpu # => error, undefined method hardware.num_cpu

We wanted num_cpu to call hardware.cpu_sockets.

after improving :to option

virtual_delegate :num_cpu, :to => "hardware.cpu_sockets"

Defines the attribute with a component and the delegate ruby method:

virtual_attribute :num_cpu, :integer, :uses => :hardware, :arel => ...

def num_cpu
  hardware.try(:cpu_sockets)
end

Good:

It will also define a virtual_attribute with arel properly so it is valid in SQL:

vm.num_cpu # => 2      # valid

Vm.order(:num_cpu)     # valid
Vm.where(:num_cpu =>2) # valid

These generate the following SQL:

SELECT "vms"."*"
FROM "vms"
ORDER BY (SELECT "hardware"."cpu_sockets" FROM "hardware" WHERE "hardware"."vm_id" = "vms"."id")

SELECT "vms"."*"
FROM "vms"
WHERE (SELECT "hardware"."cpu_sockets" FROM "hardware" WHERE "hardware"."vm_id" = "vms"."id") = 2

Issue:

vm.hardware = nil
vm.num_cpu # => nil #Invalid. want 0

We want num_cpu to default to 0.

after adding :default option

virtual_delegate :num_cpu, :to => "hardware.cpu_sockets", :allow_nil => true, :default => 0

Defines the attribute with a component and the delegate ruby method:

def num_cpu
  hardware.try(:cpu_sockets) || 0
end

Good:

vm.hardware = nil
vm.num_cpu # => 0

The code works as it did before, only now, on the vms page, are able to sort by num_cpus - so it goes from 49s to 12s (10k vms)

[NickLaMuro]

Seems like we can remove the uses of :allow_nil => true since we are no longer deferring to delegate:

https://github.com/ManageIQ/manageiq/blob/ba69b49e/app/models/vm_or_template.rb#L183-L187

[NickLaMuro]

This is no longer a valid comment since the new changes.

[NickLaMuro]

From your description (as of 2:02 PM CDT, Aug 17th)

virtual_delegate defines the arel, and also defines the ruby method / accessor.

I think it is important in the Before/After section to note that we were previously using activesupport's delegate method, and now we are including the relevant part of that in virtual_delegate, and allowing nil by default.

[NickLaMuro]

:default here is going to be a bit tricky...

If you want it to be a static string, you will have to define it as:

virtual_delegate :name, :to => :host, :prefix => true, :default => '"N/A"'

Which is fine, because it allows :default to be another method, but that is not very obvious to the user/developer this is being done. Is there an easier way to make this more obvious or user friendly? Perhaps use two different keys: :default_value and :default_method (:default_value would do an .inspect on the value)?

[kbrock]

I like inspect. don't want a method

very good find - thanks

[NickLaMuro]

I think the code changes here are fine aside from the few things that I brought up. Shouldn't cause any regressions as far as I can tell, and if so, they would show up in vm_or_template.

I do have a few follow up questions though:

• Do you have some use cases where you see the :default and :name being helpful? Might be helpful for those who haven't spent hours trying to understand ar_virtual where this could be used.
• We should probably get some generic tests wrapped around this stuff, and definitely a future effort. Arguably tests from vm_or_template should cover this not causing a regression, but something to consider since this is a very dense lib we have here.

[Fryguy]

extract_options! is designed to always give you a hash, so the || {} is not needed.

[1] pry(main)> require 'active_support/all'
=> true
[2] pry(main)> [].extract_options!
=> {}
[3] pry(main)> [{:a => 1}].extract_options!
=> {:a=>1}
[4] pry(main)> [:foo, {:a => 1}].extract_options!
=> {:a=>1}

[Fryguy]

@kbrock I like the signature...very clear.

Even if that were not the case, the current delegate returns a nil when hardware.nil? instead of 0.

Can you show this example in the after case? I assume it returns 0 because a default was specified. What happens if no default is specified.

[NickLaMuro]

These two lines are here only to be used by the define_delgate method. Would it be possible to just call these within the define_delgate method itself? You might have/want to update the first line if you want to provide an accurate location for errors, because you would now be farther down the stack:

location = caller_locations(2,1).first

https://ruby-doc.org/core-2.1.0/Module.html#method-i-module_eval
https://ruby-doc.org/core-2.1.0/Kernel.html#method-i-caller_locations

[kbrock]

@NickLaMuro I didn't have much success with caller_locations(2,1).first, but I'll try again. it sure would make it easier to read and closer to delegate

[miq-bot]

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

[kbrock]

fixed line number, and multiple "." detection, and extracted virtual_attribtue work and all other comments I could do

[chessbyte]

LGTM 👍 -- @Fryguy any concerns before merging?

[miq-bot]

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

[miq-bot]

Checked commits kbrock/manageiq@7ca5654~...6e4a129 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 3 offenses detected

lib/extensions/ar_virtual.rb

• ❕ - Line 128, Col 5 - Metrics/MethodLength - Method has too many lines. [29/25]
• ❕ - Line 153, Col 121 - Metrics/LineLength - Line is too long. [135/120]
• ❕ - Line 80, Col 121 - Metrics/LineLength - Line is too long. [172/120]

[Fryguy]

I'm not thrilled with the copying of the define_delegate method, but I can't think of another way to do it, so will merge.

[kbrock]

@Fryguy we will find a way to get rid of the delegate copy.