Replace "some" default_value_for usage #22901

jrafanie · 2024-02-15T22:03:22Z

tldr; I think these are mostly safe changes that gets us closer to not depending on default_value_for. There's more to do but we can tackle that later if we really want to not use it anymore.

From research in upgrading default_value_for, it seems reasonable to replace it and not have to deal with getting this gem to work with rails as we upgrade. Currently, the gem's master branch supports rails 7 but hasn't been released so we need to ride master to work with rails 7 or fork it.

I've found the attributes API is mostly a replacement but it is different than default_value_for in the following ways:

You can't override the getter/setter methods for the attribute in the same way you could with this gem. I think this revolves around how it's hard to detect if the super method's return is nil ({} serialize as hash attributes) explicitly or coming from the defaults from rails. I suspect there's a way to do it correctly but it's less obvious.
The default value block format doesn't yield the instance, so you can't derive defaults based on the current instance. We definitely use this and the remaining usages are those.
There doesn't seem to be a way to set multiple default values all at once. You have to set them individually. I don't think we use this feature.
Default values make new objects look changed, unlike default_value_for.
Attributes API default value doesn't support allow_nil: false so if a nil is provided as a value for an attribute, the default value will not be used. NOTE: I don't see much usage of this.

EDIT:

This PR avoids the more complicated changes where we used a block and the corresponding instance in the block. The remaining ones in core should probably be done in a followup PR. Either way, we can choose to keep default_value_for, but I thought it made sense to switch to vanilla rails for the more basic cases. This is what remains in core (note, there's also usage in providers/plugins):

app/models/miq_provision_request.rb:  default_value_for(:source_id)    { |r| r.get_option(:src_vm_id) || r.get_option(:source_id) }
app/models/miq_request.rb:  default_value_for(:message)       { |r| "#{r.class::TASK_DESCRIPTION} - Request Created" }
app/models/miq_request.rb:  default_value_for(:request_type)  { |r| r.request_types.first }
app/models/miq_retire_request.rb:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
app/models/service_reconfigure_request.rb:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
app/models/service_template.rb:  default_value_for(:generic_subtype) { |st| 'custom' if st.prov_type == 'generic' }
app/models/service_template_provision_request.rb:  default_value_for(:source_id)    { |r| r.get_option(:src_id) }
app/models/vm_retire_request.rb:  default_value_for(:source_id)    { |r| r.get_option(:src_ids) }

jrafanie · 2024-02-16T21:31:43Z

app/models/blacklisted_event.rb

  after_create      :audit_creation
+  after_destroy     :reload_all_server_settings, :audit_deletion
+  after_save        :reload_all_server_settings


the cop wanted these sorted by their order... it's only the after_validation that's new

kbrock · 2024-02-19T19:46:50Z

app/models/time_profile.rb

@@ -75,7 +73,7 @@ def tz_or_default(default_tz = DEFAULT_TZ)
  end

  def days
-    profile[:days]
+    profile.fetch(:days, ALL_DAYS)


attribute :days, :default => ALL_DAYS

Is there a reason to not just set the default on this attribute?

the default_value overriding what comes back from the database (vs creating the object) is a little suspect. Glad we are getting rid of this.

Do we want to db migrate profiles and set ALL_DAYS for time profiles with no :days in the hash?

I believe I tried that and existing tests failed because it's expecting this hash structure on the getter since it's done on the setter. I 100% agree with the idea of migrating this out and simplifying it. I don't know why it was done this way.

the default_value overriding what comes back from the database (vs creating the object) is a little suspect. Glad we are getting rid of this.

Good point. Let me check into it. most of the defaults were to set it if it's not set. We probably don't have good tests if I was able to ignore the value in the database. I think you found a bug.

Ok, I took a look. I think this is fine for now. I agree, we should stop storing the days/hours in the profile column.

kbrock · 2024-02-19T20:02:50Z

Still think using default for the database (in a migration) is a good alternative for these.

But keeping this in ruby does have its advantages

kbrock · 2024-02-19T22:26:02Z

app/models/blacklisted_event.rb

-  def enabled=(value)
-    return if enabled == value
-
-    super
-    $audit_log.info("Blacklisted event [#{event_name}] for provider [#{provider_model}] with ID [#{ems_id}] had enabled changed to #{value} by user [#{self.class.current_userid}]")
+  def log_enabling
+    $audit_log.info("Blacklisted event [#{event_name}] for provider [#{provider_model}] with ID [#{ems_id}] had enabled changed to #{enabled} by user [#{self.class.current_userid}]")


I really like the new solution here

Do we need this audit trail?

Can you add new_record? here? (maybe put a byebug in this method and create a new record - see if it goes in here. I worry seeding will produce a slew of these)

yeah, I'll check

Ok, it has the same exact behavior as before.

Seed with empty db... messages showing it was created, see audit_creation, then, became enabled. I'll add a check to change the behavior to only do it unless new_record?. I agree, it was a bug before.

pushed change to skip if new_record? and added tests

kbrock · 2024-02-19T22:46:12Z

app/models/endpoint.rb

@@ -3,7 +3,7 @@
 class Endpoint < ApplicationRecord
  belongs_to :resource, :polymorphic => true

-  default_value_for :verify_ssl, OpenSSL::SSL::VERIFY_PEER
+  attribute :verify_ssl, :default => OpenSSL::SSL::VERIFY_PEER


FYI: OpenSSL::SSL::VERIFY_PEER == 1

Note, defaults applied to new objects count as "changed", so the test needs to be changed to do 2 queries.

miq-bot · 2024-02-20T18:55:48Z

Checked commits jrafanie/manageiq@5b5e927~...48e1183 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
51 files checked, 3 offenses detected

spec/lib/extensions/ar_yaml_spec.rb

❗ - Line 13, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.
❗ - Line 21, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.
❗ - Line 28, Col 144 - Style/ColonMethodCall - Do not use :: for method calls.

jrafanie · 2024-02-20T19:08:25Z

@kbrock pushed minor changes. I think you raise good questions for followup. I think the changes here are minimized to avoid changing behavior without using default_value_for.

kbrock · 2024-02-20T20:51:39Z

spec/lib/uniqueness_when_changed_validator_spec.rb

@@ -3,7 +3,7 @@
  describe "#uniqueness_when_changed" do
    it "queries for a new record" do
      alert = FactoryBot.build(:miq_alert)
-      expect { alert.valid? }.to make_database_queries(:count => 1)
+      expect { alert.valid? }.to make_database_queries(:count => 2)


sigh

Not thrilled about getting a changed event for a default value.
I had thought that this changed the base metadata (so it acted as if the database specified the default value), but this suggests that is not true.

haha, yup... default_value_for masked changes... we can do the same but I was hoping we didn't need to.

…value_for" This reverts commit c773b2b, reversing changes made to bb72015.

Revert "Merge pull request #22901 from jrafanie/drop_default_value_for"

Revert "Merge pull request #22901 from jrafanie/drop_default_value_for" (cherry picked from commit 340776c)

Replace simple default_value_for with attribute defaults

5b5e927

jrafanie added the wip label Feb 15, 2024

jrafanie requested review from agrare, Fryguy and kbrock as code owners February 15, 2024 22:03

jrafanie added 3 commits February 16, 2024 16:29

Use after_validation if attribute changed to log a message

d53eac8

Replace default_value_for with simple fetch defaults

149c54a

We already serializes these as a hash, no need for default_value_for

631488f

jrafanie force-pushed the drop_default_value_for branch from 0aa80d1 to 39c3e5f Compare February 16, 2024 21:30

jrafanie commented Feb 16, 2024

View reviewed changes

jrafanie changed the title ~~[WIP] Replace default_value_for usage~~ Replace default_value_for usage Feb 16, 2024

jrafanie changed the title ~~Replace default_value_for usage~~ Replace "some" default_value_for usage Feb 16, 2024

jrafanie added technical debt and removed wip labels Feb 16, 2024

jrafanie mentioned this pull request Feb 16, 2024

Upgrade to rails 7.0 #22052

Closed

kbrock reviewed Feb 19, 2024

View reviewed changes

kbrock self-assigned this Feb 19, 2024

kbrock reviewed Feb 19, 2024

View reviewed changes

jrafanie force-pushed the drop_default_value_for branch 2 times, most recently from 3ece4e3 to e3efcc0 Compare February 20, 2024 18:35

jrafanie added 4 commits February 20, 2024 13:51

Reimplement default_value_for as custom getters

6b2b6f9

Switch default_value_for to attributes API

a4c42ec

Note, defaults applied to new objects count as "changed", so the test needs to be changed to do 2 queries.

Switch default_value_for to attribute default with basic block

c12546c

Fix bug, don't log enabled on creation

48e1183

jrafanie force-pushed the drop_default_value_for branch from e3efcc0 to 48e1183 Compare February 20, 2024 18:51

kbrock reviewed Feb 20, 2024

View reviewed changes

kbrock merged commit c773b2b into ManageIQ:master Feb 20, 2024
8 checks passed

kbrock added the rails7 label Feb 20, 2024

jrafanie deleted the drop_default_value_for branch February 20, 2024 21:07

This was referenced Feb 28, 2024

Workaround attribute default triggering changes ManageIQ/manageiq-providers-awx#27

Closed

Workaround attribute default triggering changes ManageIQ/manageiq-providers-ansible_tower#306

Closed

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Feb 29, 2024

Revert "Merge pull request ManageIQ#22901 from jrafanie/drop_default_…

6a3ef60

…value_for" This reverts commit c773b2b, reversing changes made to bb72015.

jrafanie mentioned this pull request Feb 29, 2024

Revert "Merge pull request #22901 from jrafanie/drop_default_value_for" #22922

Merged

agrare added a commit that referenced this pull request Feb 29, 2024

Merge pull request #22922 from jrafanie/revert_22901

340776c

Revert "Merge pull request #22901 from jrafanie/drop_default_value_for"

Fryguy pushed a commit that referenced this pull request Mar 1, 2024

Merge pull request #22922 from jrafanie/revert_22901

9acc4b2

Revert "Merge pull request #22901 from jrafanie/drop_default_value_for" (cherry picked from commit 340776c)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace "some" default_value_for usage #22901

Replace "some" default_value_for usage #22901

jrafanie commented Feb 15, 2024 •

edited

Loading

jrafanie Feb 16, 2024

kbrock Feb 19, 2024 •

edited

Loading

jrafanie Feb 19, 2024

jrafanie Feb 20, 2024

kbrock commented Feb 19, 2024

kbrock Feb 19, 2024

jrafanie Feb 19, 2024

jrafanie Feb 20, 2024

jrafanie Feb 20, 2024 •

edited

Loading

kbrock Feb 19, 2024

miq-bot commented Feb 20, 2024

jrafanie commented Feb 20, 2024

kbrock Feb 20, 2024 •

edited

Loading

jrafanie Feb 20, 2024

Replace "some" default_value_for usage #22901

Replace "some" default_value_for usage #22901

Conversation

jrafanie commented Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

kbrock Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 20, 2024

jrafanie commented Feb 20, 2024

kbrock Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Feb 15, 2024 •

edited

Loading

kbrock Feb 19, 2024 •

edited

Loading

jrafanie Feb 20, 2024 •

edited

Loading

kbrock Feb 20, 2024 •

edited

Loading