[wip] use built in default values via rails attribute

[d-m-u]

miq policy defaults are well tested already so this is the first of many if it gets approval

Keenan caught it a couple days ago on September 22, 2020 11:50 AM

@kbrock @Fryguy could I get opinions, please?

@miq-bot add_label refactoring

it's work in progress until i'm sure that our use cases can sufficiently be satisfied

[chessbyte]

@d-m-u if this gets approved, let's create an epic to replace default_value_for with attribute :default so that we can see all the refactorings from a single issue

[miq-bot]

Checked commit d-m-u@f64226b with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 👍

[Fryguy]

cc @jrafanie, keep me honest/complete here...

So, the use cases for default_value_for are (see also their README):

class MyModel < ActiveRecord::Base
  default_value_for :foo, "bar"
end

• When you use .new, then an attribute already has its value present

  MyModel.new.foo
  # => "bar"

• If you give the value to .new, it will use it instead

  MyModel.new(:foo => "xxx").foo
  # => "xxx"

• Once a record is saved with a value, that value is not changed (including nil)

  MyModel.create!(:foo => nil)
  MyMode.first.foo
  # => nil

• It allows procs/blocks to be defined for attributes

  e.g.

  manageiq/app/models/miq_retire_request.rb

  Line 6 in a4c1e11

  default_value_for(:source_id) { |r| r.get_option(:src_id) }

  example list via grep

  $ ag-miq "default_value_for.+{"
  /Users/jfrey/dev/manageiq-consumption/app/models/manageiq/showback/data_rollup.rb:19:    default_value_for :data, {}
  /Users/jfrey/dev/manageiq-consumption/app/models/manageiq/showback/data_rollup.rb:22:    default_value_for :context, {}
  /Users/jfrey/dev/manageiq-consumption/app/models/manageiq/showback/data_view.rb:19:    default_value_for :data_snapshot, {}
  /Users/jfrey/dev/manageiq-consumption/app/models/manageiq/showback/data_view.rb:20:    default_value_for :context_snapshot, {}
  /Users/jfrey/dev/manageiq-consumption/app/models/manageiq/showback/rate.rb:20:    default_value_for :screener, { }
  /Users/jfrey/dev/manageiq/app/models/service_template_provision_request.rb:19:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
  /Users/jfrey/dev/manageiq/app/models/miq_server.rb:33:  default_value_for(:zone) { Zone.default_zone }
  /Users/jfrey/dev/manageiq/app/models/vm_retire_request.rb:6:  default_value_for(:source_id)    { |r| r.get_option(:src_ids) }
  /Users/jfrey/dev/manageiq/app/models/miq_provision_request.rb:20:  default_value_for(:src_vm_id)    { |r| r.get_option(:src_vm_id) }
  /Users/jfrey/dev/manageiq/app/models/miq_request.rb:24:  default_value_for(:message)       { |r| "#{r.class::TASK_DESCRIPTION} - Request Created" }
  /Users/jfrey/dev/manageiq/app/models/miq_request.rb:25:  default_value_for :options,       {}
  /Users/jfrey/dev/manageiq/app/models/miq_request.rb:27:  default_value_for(:request_type)  { |r| r.request_types.first }
  /Users/jfrey/dev/manageiq/app/models/miq_ae_method.rb:8:  default_value_for(:embedded_methods) { [] }
  /Users/jfrey/dev/manageiq/app/models/service_template.rb:64:  default_value_for(:generic_subtype) { |st| 'custom' if st.prov_type == 'generic' }
  /Users/jfrey/dev/manageiq/app/models/service_reconfigure_request.rb:11:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
  /Users/jfrey/dev/manageiq/app/models/miq_group.rb:36:  default_value_for(:sequence) { next_sequence }
  /Users/jfrey/dev/manageiq/app/models/dialog_field.rb:32:  default_value_for(:visible) { true }
  /Users/jfrey/dev/manageiq/app/models/notification.rb:19:  default_value_for(:options) { Hash.new }
  /Users/jfrey/dev/manageiq/app/models/miq_schedule.rb:47:  default_value_for(:zone_id) { MiqServer.my_server.zone_id }
  /Users/jfrey/dev/manageiq/app/models/miq_retire_request.rb:6:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
  /Users/jfrey/dev/manageiq/app/models/miq_request_task.rb:16:  default_value_for :phase_context, {}
  /Users/jfrey/dev/manageiq/app/models/miq_request_task.rb:17:  default_value_for :options,       {}
  /Users/jfrey/dev/manageiq/app/models/user.rb:54:  default_value_for(:settings) { Hash.new }
  /Users/jfrey/dev/manageiq/lib/uuid_mixin.rb:4:    default_value_for(:guid) { SecureRandom.uuid }
  /Users/jfrey/dev/miq_bot/app/models/branch.rb:11:  default_value_for(:commits_list) { [] }
  

• Also don't forget this ;)

  manageiq/app/models/miq_database.rb

  Line 17 in a4c1e11

  default_values REGISTRATION_DEFAULT_VALUES

• There are cases where the default value is defined in subclasses. For example:

  /Users/jfrey/dev/manageiq/app/models/manageiq/providers/cloud_manager/template.rb:2:  default_value_for :cloud, true
  /Users/jfrey/dev/manageiq/app/models/manageiq/providers/cloud_manager/vm.rb:29:  default_value_for :cloud, true
  

If all of the above still hold, then I'm 100% on board with dropping default_value_for

[d-m-u]

Here are the first three cases:

2.5.5 :001 > MiqPolicy.new.mode
 => "control"
2.5.5 :002 > MiqPolicy.new(:mode => 'something_else').mode
 => "something_else"
2.5.5 :003 > MiqPolicy.create!(:mode => 'compliance', :description => 'foo')
 => #<MiqPolicy id: 11, name: "a1eafa54-9501-4d1c-8120-5f4befb42e7a", description: "foo", created_on: "2020-09-24 16:29:36", updated_on: "2020-09-24 16:29:36", expression: nil, towhat: "Vm", guid: "a1eafa54-9501-4d1c-8120-5f4befb42e7a", created_by: nil, updated_by: nil, notes: nil, active: true, mode: "compliance", read_only: nil>
2.5.5 :004 > MiqPolicy.last.mode
 => "compliance"

with

#default_value_for(:settings) { Hash.new }
attribute :settings, :text,  :default => proc { Hash.new }

2.5.5 :001 > User.new.settings
 => {}

[jrafanie]

I agree. If the cases @Fryguy mentioned pass and the tests pass, including cross-repo, I'd be good with dropping default_value_for in favor of using attribute default.

[d-m-u]

Closing: the attribute type is required, and we're going to run into issues with anything like the source_id column that Jason mentioned as the ActiveRecord type for source_id is an integer and this will have overflow issues. The workaround for this is defining a custom type of the right size which I think isn't viable as it is too much overhead for this change.