update attribute_builder for rails change #17996

kbrock · 2018-09-19T01:16:21Z

rails 5.0.7 [changed] the way attribute_builder works.
This updates attribute_builder to work accordingly

The [old code] used a block. The [new code] uses an array - We are now modifying the array instead of passing the block.

Also, 5.0.0 changed the way dirty worked.
Think the change to forgetting_assignment introduced this problem for
all rails users, but it is subtle and would only be noticed by
people who use lots of non-database attributes (e.g.: us)
The bug remains in 5.1.0 but the code changes significantly in 5.2

This fixes an issue seen by #17912

Thnx @gmcculloug and @d-m-u for tracking down the bug and the pairing. Looks like it was introduced in rails 5.0.7
/cc @NickLaMuro @jrafanie you guys saw this original patch.

The Attribute change should go into rails proper, but they won't patch anything before 5.2 (we're on 5.0.7 right now), and the code has changed so much that I'm not sure it is even necessary.

kbrock · 2018-09-19T01:17:37Z

/cc @gtanzillo This does work with virtual_attributes - and I know you and I have merged some code with that. so just keeping you in the loop

d-m-u

Thanks!

d-m-u · 2018-09-19T13:47:20Z

Tested with the changes in #17912, works like a beaut.

jrafanie · 2018-09-19T16:57:25Z

lib/extensions/ar_virtual.rb

-          _default_attributes[name].dup
-        end
+    def attributes_builder # :nodoc:
+      unless defined?(@attributes_builder) && @attributes_builder


What is this saying? defined and not nil? Why change this from @ivar ||= ? It seems unrelated.

Plus, I mentally convert unless to if !xxx so && requires me to think whereas the original feels easier to understand.

Are you concerned with setting this to false and not memoizing false properly?

This is likely because we tend to straight copy upstream and patch a part of it (so this code may actually be upstream code.

@jrafanie this is taken directly from https://github.com/rails/rails/blob/5-0-stable/activerecord/lib/active_record/model_schema.rb#L334-L340

jrafanie · 2018-09-19T16:58:42Z

lib/extensions/ar_virtual.rb

@@ -400,12 +400,15 @@ def virtual_attribute_names
      end
    end

-    def attributes_builder
-      @attributes_builder ||= ::ActiveRecord::AttributeSet::Builder.new(attribute_types, primary_key) do |name|
-        unless columns_hash.key?(name) || virtual_attribute?(name)


unless || 🏃 🙀

@jrafanie this was by me, and yes, it was intentional. This is patching over the original Rails method that only had unless columns_hash.key(name) at the time, and for easier comparison with our || virtual_attribute?(name) addition, I left it as the less pretty unless || you speak of.

In less words: #dealWithIt™

Also... #13302 (comment)

haha, yeah, now I remember. I only say it because I literally have to make mental truth tables with unless || so they might save real estate but I can't grok them easily.

jrafanie · 2018-09-19T17:05:04Z

lib/extensions/ar_virtual.rb

-    def attributes_builder
-      @attributes_builder ||= ::ActiveRecord::AttributeSet::Builder.new(attribute_types, primary_key) do |name|
-        unless columns_hash.key?(name) || virtual_attribute?(name)
-          _default_attributes[name].dup


I don't understand what this block was doing before. Before, we created a Builder object with the attribute_types and primary_key, did some dup'ing of attributes in the block. Now, we're filtering out the primary_key and the virtual attributes and creating the Builder with all attributes minus these two categories of attributes.

So, why are we filtering primary_key when we didn't before? Why are we now passing other non-virtual attributes to the Builder when we didn't before?

What was here previously was exactly what was in the Rails codebase, with the addition of || virtual_attribute?(name) (for our purposes).

I think what @kbrock did here was just do the same for the new changes that are now in 5.0.7, which has a bug fix that we need. Maybe some links in the description would help clear some of the confusion?

Sure. I can't follow what was happening before and why we changed how we instantiated the Builder objects without looking at the rails internals.

The link that @d-m-u was passing around yesterday might help provide some clarity/context:

https://gitter.im/ManageIQ/manageiq/core/archives/2017/01/05

Though I personally prefer the ?at=[TIMESTAMP] link since there is more discussion the next day that the previous link doesn't show:

https://gitter.im/ManageIQ/manageiq/core?at=586e8f5b61e516c157900687

I'd love to just merge this but I have no idea why these changes are needed. I understand the issue but am totally lost as to why the builder instantiation was changed. 🤷‍♂️ I don't get why primary key was removed. Did we pull down a new version of the method from rails 5.0.7? Did we patch it on top of that? It's unclear.

@jrafanie the old code is based upon rails 5.0.0 - 5.0.6 [ref]
But the method changed [here].

I updated it to match rails 5.0.7 implementation of this method. [ref]

jrafanie · 2018-09-19T17:07:04Z

lib/extensions/ar_virtual.rb

+  class Attribute
+    def with_value_from_database(value)
+      # self.class.from_database(name, value, type)
+      initialized? ? self.class.from_database(name, value, type) : self


Wow, why? Please provide a two line comment explaining why we need this. I'm not sure of the why here.

@kbrock your comment about says this is no longer needed for 5.2, can you explain why this is needed for 5.0 and 5.1? You mention a bug... can you provide summary and link here?

Also, do we need to remove this with 5.2? Do we need a warning or something if this code is run against 5.2?

This all changed in 5.2 - so yes, we will need to remove it.

I didn't know the proper way to warn. snippet reference?

If we want to clean this up later, we can warn. If we just want to patch it for 5.0 and 5.1, you can just put a guard clause `return if Rails::VERSION >= "5.2.0" or something like that...

If you want to just do this in a followup, I'm fine with that

jrafanie · 2018-09-19T17:07:55Z

Is this something we have tests around? Can we add them if not? It feels like @d-m-u's issue should be easy to wrap a test around.

Fryguy · 2018-09-20T18:59:57Z

a minimal bit of code that shows the issue:

irb(main):001:0> s = Service.last
=> #<Service id: 364, name: "Test", description: nil, guid: "cc25b8a7-d495-402c-bfae-ed6bdb911e7b", type: nil, service_template_id: nil, options: {}, display: false, created_at: "2018-09-17 20:07:45", updated_at: "2018-09-18 12:40:32", evm_owner_id: nil, miq_group_id: 1, retired: false, retires_on: nil, retirement_warn: nil, retirement_last_warn: nil, retirement_state: nil, retirement_requester: nil, tenant_id: 1, ancestry: nil, initiator: "user">
irb(main):002:0> s.attributes
=> {"id"=>364, "name"=>"Test", "description"=>nil, "guid"=>"cc25b8a7-d495-402c-bfae-ed6bdb911e7b", "type"=>nil, "service_template_id"=>nil, "options"=>{}, "display"=>false, "created_at"=>Mon, 17 Sep 2018 20:07:45 UTC +00:00, "updated_at"=>Tue, 18 Sep 2018 12:40:32 UTC +00:00, "evm_owner_id"=>nil, "miq_group_id"=>1, "retired"=>false, "retires_on"=>nil, "retirement_warn"=>nil, "retirement_last_warn"=>nil, "retirement_state"=>nil, "retirement_requester"=>nil, "tenant_id"=>1, "ancestry"=>nil, "initiator"=>"user"}
irb(main):003:0> s.update_attributes(:display => true)
=> true
irb(main):004:0> s.attributes
=> {"id"=>364, "name"=>"Test", "description"=>nil, "guid"=>"cc25b8a7-d495-402c-bfae-ed6bdb911e7b", "type"=>nil, "service_template_id"=>nil, "options"=>{}, "display"=>true, "created_at"=>Mon, 17 Sep 2018 20:07:45 UTC +00:00, "updated_at"=>Tue, 18 Sep 2018 12:55:52 UTC +00:00, "evm_owner_id"=>nil, "miq_group_id"=>1, "retired"=>false, "retires_on"=>nil, "retirement_warn"=>nil, "retirement_last_warn"=>nil, "retirement_state"=>nil, "retirement_requester"=>nil, "tenant_id"=>1, "ancestry"=>nil, "initiator"=>"user", "href_slug"=>nil, "region_number"=>nil, "region_description"=>nil, "owned_by_current_user"=>nil, "owned_by_current_ldap_group"=>nil, "custom_1"=>nil, "custom_2"=>nil, "custom_3"=>nil, "custom_4"=>nil, "custom_5"=>nil, "custom_6"=>nil, "custom_7"=>nil, "custom_8"=>nil, "custom_9"=>nil, "cpu_usagemhz_rate_average_avg_over_time_period"=>nil, "cpu_usagemhz_rate_average_low_over_time_period"=>nil, "cpu_usagemhz_rate_average_high_over_time_period"=>nil, "cpu_usage_rate_average_avg_over_time_period"=>nil, "cpu_usage_rate_average_low_over_time_period"=>nil, "cpu_usage_rate_average_high_over_time_period"=>nil, "mem_usage_absolute_average_avg_over_time_period"=>nil, "mem_usage_absolute_average_low_over_time_period"=>nil, "mem_usage_absolute_average_high_over_time_period"=>nil, "derived_memory_used_avg_over_time_period"=>nil, "derived_memory_used_low_over_time_period"=>nil, "derived_memory_used_high_over_time_period"=>nil, "max_cpu_usage_rate_average_avg_over_time_period"=>nil, "max_cpu_usage_rate_average_low_over_time_period"=>nil, "max_cpu_usage_rate_average_high_over_time_period"=>nil, "max_mem_usage_absolute_average_avg_over_time_period"=>nil, "max_mem_usage_absolute_average_low_over_time_period"=>nil, "max_mem_usage_absolute_average_high_over_time_period"=>nil, "max_cpu_usage_rate_average_avg_over_time_period_without_overhead"=>nil, "max_cpu_usage_rate_average_low_over_time_period_without_overhead"=>nil, "max_cpu_usage_rate_average_high_over_time_period_without_overhead"=>nil, "max_mem_usage_absolute_average_avg_over_time_period_without_overhead"=>nil, "max_mem_usage_absolute_average_low_over_time_period_without_overhead"=>nil, "max_mem_usage_absolute_average_high_over_time_period_without_overhead"=>nil, "aggregate_direct_vm_cpus"=>nil, "aggregate_direct_vm_memory"=>nil, "aggregate_direct_vm_disk_count"=>nil, "aggregate_direct_vm_disk_space_allocated"=>nil, "aggregate_direct_vm_disk_space_used"=>nil, "aggregate_direct_vm_memory_on_disk"=>nil, "aggregate_all_vm_cpus"=>nil, "aggregate_all_vm_memory"=>nil, "aggregate_all_vm_disk_count"=>nil, "aggregate_all_vm_disk_space_allocated"=>nil, "aggregate_all_vm_disk_space_used"=>nil, "aggregate_all_vm_memory_on_disk"=>nil, "v_total_vms"=>nil, "has_parent"=>nil, "power_state"=>nil, "power_status"=>nil, "service_id"=>nil, "evm_owner_email"=>nil, "evm_owner_name"=>nil, "evm_owner_userid"=>nil, "owning_ldap_group"=>nil}

Fryguy · 2018-09-20T19:07:47Z

Better one:

[1] pry(main)> z = Zone.first
[2] pry(main)> z.attributes
=> {"id"=>21000000000001,
 "name"=>"default",
 "description"=>"Default Zone",
 "created_on"=>Fri, 04 May 2018 22:05:45 UTC +00:00,
 "updated_on"=>Fri, 04 May 2018 22:05:45 UTC +00:00,
 "settings"=>{},
 "log_file_depot_id"=>nil}
[3] pry(main)> z.save!
[4] pry(main)> z.attributes
=> {"id"=>21000000000001,
 "name"=>"default",
 "description"=>"Default Zone",
 "created_on"=>Fri, 04 May 2018 22:05:45 UTC +00:00,
 "updated_on"=>Fri, 04 May 2018 22:05:45 UTC +00:00,
 "settings"=>{},
 "log_file_depot_id"=>nil,
 "href_slug"=>nil,
 "region_number"=>nil,
 "region_description"=>nil,
 "authentication_status"=>nil,
 "cpu_usagemhz_rate_average_avg_over_time_period"=>nil,
 "cpu_usagemhz_rate_average_low_over_time_period"=>nil,
 "cpu_usagemhz_rate_average_high_over_time_period"=>nil,
 "cpu_usage_rate_average_avg_over_time_period"=>nil,
 "cpu_usage_rate_average_low_over_time_period"=>nil,
 "cpu_usage_rate_average_high_over_time_period"=>nil,
 "mem_usage_absolute_average_avg_over_time_period"=>nil,
 "mem_usage_absolute_average_low_over_time_period"=>nil,
 "mem_usage_absolute_average_high_over_time_period"=>nil,
 "derived_memory_used_avg_over_time_period"=>nil,
 "derived_memory_used_low_over_time_period"=>nil,
 "derived_memory_used_high_over_time_period"=>nil,
 "max_cpu_usage_rate_average_avg_over_time_period"=>nil,
 "max_cpu_usage_rate_average_low_over_time_period"=>nil,
 "max_cpu_usage_rate_average_high_over_time_period"=>nil,
 "max_mem_usage_absolute_average_avg_over_time_period"=>nil,
 "max_mem_usage_absolute_average_low_over_time_period"=>nil,
 "max_mem_usage_absolute_average_high_over_time_period"=>nil,
 "max_cpu_usage_rate_average_avg_over_time_period_without_overhead"=>nil,
 "max_cpu_usage_rate_average_low_over_time_period_without_overhead"=>nil,
 "max_cpu_usage_rate_average_high_over_time_period_without_overhead"=>nil,
 "max_mem_usage_absolute_average_avg_over_time_period_without_overhead"=>nil,
 "max_mem_usage_absolute_average_low_over_time_period_without_overhead"=>nil,
 "max_mem_usage_absolute_average_high_over_time_period_without_overhead"=>nil,
 "aggregate_cpu_speed"=>nil,
 "aggregate_cpu_total_cores"=>nil,
 "aggregate_physical_cpus"=>nil,
 "aggregate_memory"=>nil,
 "aggregate_vm_cpus"=>nil,
 "aggregate_vm_memory"=>nil,
 "aggregate_disk_capacity"=>nil}

rails 5.0.7 changed the way attribute_builder works This updates attribute_builder to work accordingly Also, 5.0.0 changed the way dirty worked. Think the change to forgetting_assignment introduced this problem for all rails versions, but it is subtle and would only be noticed by people who use lots of non-database attributes (e.g.: us) It remains in 5.1.0 but the code changes significantly in 5.2

miq-bot · 2018-09-25T16:22:32Z

Checked commit kbrock@a687c22 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

kbrock · 2018-09-25T16:29:29Z

Thanks for review all.

Added a test

Added references to the code before / after. I'll move them into the description.

jrafanie

Looks good. Let's do the rails version check in a followup PR. We "can't touch this" branch right now, so will wait for @Fryguy to hammer out the new release branch and give the go ahead and then we'll merge.

jrafanie · 2018-09-25T19:10:50Z

@kbrock @d-m-u I'll let you decide what branches this needs to go back to... and create the bugzillas for it. 🙇

kbrock · 2018-09-25T19:40:49Z

I just work here - I defer to @d-m-u to decide which version to use

d-m-u · 2018-09-25T19:51:41Z

@miq-bot add_label hammer/yes
¯_(ツ)_/¯

gmcculloug · 2018-09-25T21:14:01Z

I agree with @d-m-u that the hammer branch would be preferred. We are covered by the work-around in PR #17912 here app/models/service_retire_task.rb:41.

However, If this change gets back-ported we could also remove the work-around on hammer with PR #18019.

update attribute_builder for rails change (cherry picked from commit 677bf46)

simaishi · 2018-09-26T12:58:40Z

Hammer backport details:

$ git log -1
commit c6d4787b9a957ab752102e7ae5ba7b9c6a99b6f1
Author: Joe Rafaniello <[email protected]>
Date:   Tue Sep 25 15:09:24 2018 -0400

    Merge pull request #17996 from kbrock/virtual_attributes_after_save
    
    update attribute_builder for rails change
    
    (cherry picked from commit 677bf4601627a588c95c33efe47f76ea6998f0ef)

kbrock added bug rails5 core labels Sep 19, 2018

d-m-u approved these changes Sep 19, 2018

View reviewed changes

d-m-u mentioned this pull request Sep 19, 2018

Adds task to subservices for correct retirement completion #17912

Merged

jrafanie reviewed Sep 19, 2018

View reviewed changes

kbrock force-pushed the virtual_attributes_after_save branch from 37f1365 to a687c22 Compare September 25, 2018 16:17

jrafanie approved these changes Sep 25, 2018

View reviewed changes

jrafanie merged commit 677bf46 into ManageIQ:master Sep 25, 2018

jrafanie added this to the Sprint 96 Ending Oct 8, 2018 milestone Sep 25, 2018

jrafanie self-assigned this Sep 25, 2018

d-m-u mentioned this pull request Sep 25, 2018

Remove todo with virtual attr inclusion in attr list #18019

Merged

kbrock deleted the virtual_attributes_after_save branch September 25, 2018 19:40

miq-bot added the hammer/yes label Sep 25, 2018

simaishi pushed a commit that referenced this pull request Sep 26, 2018

Merge pull request #17996 from kbrock/virtual_attributes_after_save

c6d4787

update attribute_builder for rails change (cherry picked from commit 677bf46)

simaishi added hammer/backported and removed hammer/yes labels Sep 26, 2018

d-m-u mentioned this pull request Mar 22, 2019

@attributes between Rails 5.0.0 and 5.0.1 includes virtual attributes #17993

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update attribute_builder for rails change #17996

update attribute_builder for rails change #17996

kbrock commented Sep 19, 2018 •

edited

Loading

kbrock commented Sep 19, 2018

d-m-u left a comment

d-m-u commented Sep 19, 2018

jrafanie Sep 19, 2018

jrafanie Sep 19, 2018

Fryguy Sep 20, 2018

kbrock Sep 25, 2018

jrafanie Sep 19, 2018

NickLaMuro Sep 19, 2018

NickLaMuro Sep 19, 2018

jrafanie Sep 19, 2018

jrafanie Sep 19, 2018

NickLaMuro Sep 19, 2018

jrafanie Sep 19, 2018

NickLaMuro Sep 19, 2018

jrafanie Sep 20, 2018

kbrock Sep 25, 2018 •

edited

Loading

jrafanie Sep 19, 2018

jrafanie Sep 24, 2018

jrafanie Sep 24, 2018

kbrock Sep 25, 2018

jrafanie Sep 25, 2018

jrafanie Sep 25, 2018

jrafanie commented Sep 19, 2018

Fryguy commented Sep 20, 2018

Fryguy commented Sep 20, 2018 •

edited

Loading

miq-bot commented Sep 25, 2018

kbrock commented Sep 25, 2018

jrafanie left a comment

jrafanie commented Sep 25, 2018

kbrock commented Sep 25, 2018

d-m-u commented Sep 25, 2018

gmcculloug commented Sep 25, 2018

simaishi commented Sep 26, 2018

update attribute_builder for rails change #17996

update attribute_builder for rails change #17996

Conversation

kbrock commented Sep 19, 2018 • edited Loading

kbrock commented Sep 19, 2018

d-m-u left a comment

Choose a reason for hiding this comment

d-m-u commented Sep 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Sep 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Sep 19, 2018

Fryguy commented Sep 20, 2018

Fryguy commented Sep 20, 2018 • edited Loading

miq-bot commented Sep 25, 2018

kbrock commented Sep 25, 2018

jrafanie left a comment

Choose a reason for hiding this comment

jrafanie commented Sep 25, 2018

kbrock commented Sep 25, 2018

d-m-u commented Sep 25, 2018

gmcculloug commented Sep 25, 2018

simaishi commented Sep 26, 2018

kbrock commented Sep 19, 2018 •

edited

Loading

kbrock Sep 25, 2018 •

edited

Loading

Fryguy commented Sep 20, 2018 •

edited

Loading