Vm rollup defaults (ruby solution)

[kbrock]

Fixes #14393 [Alternate to #14448 ]

Introduced by #14285, the default value for Vm#provisioned_storage became nil when there is no hardware record. This PR changes the default back to 0.

Details

When delegating a column to another model, and the other model does not exist in the database, the SQL for the attribute comes back as a null. So the attribute looks like it is coming back from SQL, but in truth, it is just a null. Since there is no value, the default needs to be returned.

[NickLaMuro]

@kbrock as I have stated in private previously, I am not a big fan of changing the "default default behavior" since we are already using it in other places and this might cause issues relying on that previous behavior.

What I would prefer to see is either:

1. Confirmation that the 10ish virtual_delegates that already use a :default are un-affected by this (not sure how you could actually do that though). With that, then further documentation around virtual_delegates that this is the new behavior.
2. Make this an opt-in feature with an additional flag to trigger this functionality:

   virtual_delegate :allocated_disk_storage, :used_disk_storage, :provisioned_storage,
                    :to => :hardware, :allow_nil => true, :default => 0, :uses => {:hardware => :disks},
                    :use_default_for_nil_sql => true

# lib/extensions/ar_virtual.rb

         method_def = <<-METHOD
            def #{method_name}(#{definition})
 -            return self[:#{method_name}] if has_attribute?(:#{method_name})
 +            return self[:#{method_name}]#{default if use_default_for_nil_sql} if has_attribute?(:#{method_name})
              _ = #{to}
              if !_.nil? || nil.respond_to?(:#{method})
                _.#{method}(#{definition})
             end#{default}
           end
         METHOD

The latter makes it so the default behavior is explicit to the user of both the person assigning or modifying the virtual_delegate in the model, but also anyone who is hacking on virtual_delegate in the future.

[miq-bot]

Checked commits kbrock/manageiq@9dd7669~...0071a20 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

[kbrock]

I compared the current virtual_delegate code with the original code.
All cases returned the default when there was no target. (e.g. Vm delegating to a null Hardware returns the default value for the attribute, typically a 0)

I believe that the current code on master is wrong for these values. Making this change fixes these methods.

What @NickLaMuro said is totally true. The recently written code may assume that the attributes will return a nil instead of a 0. This behavior may be "wrong", but it is the way the recent code has been written. So "fixing" the delegates may still introduce bugs. Again, this is unquestionably true.

I just feel it is less risky to change the way default works than to leave it as is.

[kbrock]

Here is the reference to the diffs: https://gist.github.com/kbrock/c08919f7ec51fd7d399bc6f237462804

I think they speak for themselves and make this a no brainer.

To mitigate the risk Nick mentioned, I'm going through the references to all these variables and looking for any code like || that would handle a 0 differently than a nil.

[gtanzillo]

Thanks @NickLaMuro for you comments and @kbrock for putting together the gist backing up the explanation for the change. I'm comfortable with this change 👍