Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone #18079

jrafanie · 2018-10-10T18:32:19Z

Note, the before_save was calling remove_invalid_resource on create
which didn't make sense because it was trying to find service_resources
with orphaned resources, which can't happen unless the service_template
itself has been saved. Because of this, it makes sense to change this
to before_update instead of adding r.persisted? checks in the
remove_invalid_resource method.

The force reload parameter was removed from associations in:
rails/rails@09cac8c

It was previously deprecated here:
rails/rails@6eae366

From that commit:

For collections:
@user.posts.reload # Instead of @user.posts(true)

For singular associations:
@user.reload.profile # Instead of @user.profile(true)

Extracted from #18076

NickLaMuro

Putting this as "Request changes" for now, but am going to say I will be easily convinced to change my mind if you can prove I am wrong (very possible).

At least want a discussion around this since the commit I have provided has show some reasonable doubt if this is the right change.

NickLaMuro · 2018-10-10T19:32:23Z

app/models/service_template_ansible_tower.rb

@@ -24,7 +24,7 @@ def self.create_catalog_item(options, _auth_user = nil)

  def remove_invalid_resource
    # remove the resource from both memory and table
-    service_resources.to_a.delete_if { |r| r.destroy unless r.resource(true) }
+    service_resources.to_a.delete_if { |r| r.destroy unless r.resource.reload }


So I think you have this backwards... Or, at least I think you do based on what I think this is a better link for the "change commit" (compared to what you have in the description):

rails/rails@6eae366

Since this is a singular association, I think you want r.reload.resource to clear the objects "association cache", as it were, and then load the object again and confirm nothing is returned from the DB.

The downside I see with this is that it could be more SQL queries then what you have here (one to reload the r, and one to reload the r.resource again), but I think this is then in the spirit of what was trying to be done here. Possibly there is another internal API to just clear the association cache, but unsure. Also unsure if a reload will mess with the r.destroy.

This is why I asked you @NickLaMuro. I think you're absolutely right.

While you raise good questions about the number of sql queries, etc., I don't think this is ever called often enough to cause performance concern.

@jrafanie that is where I would ask the original author of service_template_ansible_tower.rb about this, since I am just guessing what the code is doing based on context (jedi mind trick). Couldn't tell you which is supposed to be "more right".

jrafanie · 2018-10-11T16:08:37Z

At least want a discussion around this since the commit I have provided has show some reasonable doubt if this is the right change.

I double checked the original commit and it's just trying to make sure the backing resource for a service resource is gone before deleting it. You were absolutely right.

I updated the commit and description.

Note, the before_save was calling remove_invalid_resource on create which didn't make sense because it was trying to find service_resources with orphaned resources, which can't happen unless the service_template itself has been saved. Because of this, it makes sense to change this to before_update instead of adding r.persisted? checks in the remove_invalid_resource method. The force reload parameter was removed from associations in: rails/rails@09cac8c67af It was previously deprecated here: rails/rails@6eae366 From that commit: For collections: @user.posts.reload # Instead of @user.posts(true) For singular associations: @user.reload.profile # Instead of @user.profile(true) Extracted from ManageIQ#18076

jrafanie · 2018-10-11T18:51:10Z

app/models/service_template_ansible_tower.rb

@@ -24,7 +24,7 @@ def self.create_catalog_item(options, _auth_user = nil)

  def remove_invalid_resource
    # remove the resource from both memory and table
-    service_resources.to_a.delete_if { |r| r.destroy unless r.resource(true) }


@bzwei Can you review this change? I needed to remove the .resource(true) to force reload of the association because .resource doesn't accept any parameters in rails 5.1. The alternative of .reload.resource causes issues because this before_save is called before creates and updates and blows up on creates because it's not yet persisted. I changed this to a before_update, to simplify the code based on the comment you added.

I understand the s/\](true\)/reload

curious:

Do we want to change from delete_if to each? think the to_a negates any real side effects.

yeah, I think this needs to be cleanedup and possibly moved up as you suggested elsewhere. I'm not sure why we need to delete_if on an array we don't retain but that seems besides the point of the PR. I agree, it's needs further refinement.

NickLaMuro

Mostly removing my ❌ here. Will let Bill and crew be the final judge, but looks good from my perspective.

bzwei · 2018-10-11T20:46:16Z

@jrafanie LGTM. Thanks for the very detailed explanation.

miq-bot · 2018-10-12T17:34:43Z

Checked commit jrafanie@3f4f8c6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

app/models/service_template_ansible_tower.rb

❗ - Line 27, Col 54 - Rails/Blank - Use if r.reload.resource.blank? instead of unless r.reload.resource.present?.

jrafanie · 2018-10-12T21:06:27Z

❗️ - Line 27, Col 54 - Rails/Blank - Use if r.reload.resource.blank? instead of unless r.reload.resource.present?.

Sorry bot, I disagree here. It reads more like English the way it is.

kbrock

Looking good.

Almost feels like the remove_invalid_resource stuff belongs higher up in the chain for ServiceTemplate. Kinda feels like it belongs as a :dependent => :destroy, though that would possibly be anywhere.

kbrock · 2018-10-12T21:19:17Z

app/models/service_template_ansible_tower.rb

@@ -24,7 +24,7 @@ def self.create_catalog_item(options, _auth_user = nil)

  def remove_invalid_resource
    # remove the resource from both memory and table
-    service_resources.to_a.delete_if { |r| r.destroy unless r.resource(true) }


I understand the s/\](true\)/reload

curious:

Do we want to change from delete_if to each? think the to_a negates any real side effects.

kbrock · 2018-10-12T21:19:57Z

app/models/service_template_ansible_tower.rb

@@ -1,7 +1,7 @@
 class ServiceTemplateAnsibleTower < ServiceTemplate
  include ServiceConfigurationMixin

-  before_save :remove_invalid_resource


jrafanie requested review from kbrock and NickLaMuro October 10, 2018 18:32

jrafanie added rails5.1 hammer/no labels Oct 10, 2018

NickLaMuro suggested changes Oct 10, 2018

View reviewed changes

jrafanie force-pushed the rails_5_1_association_reload_was_removed branch from bc4b079 to 6ac1637 Compare October 11, 2018 16:05

jrafanie changed the title ~~Rails 5.0/5.1 Use assoc.reload, assoc(true) is gone~~ Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone Oct 11, 2018

jrafanie force-pushed the rails_5_1_association_reload_was_removed branch from 6ac1637 to 3f4f8c6 Compare October 11, 2018 18:46

jrafanie commented Oct 11, 2018

View reviewed changes

NickLaMuro approved these changes Oct 11, 2018

View reviewed changes

kbrock reviewed Oct 12, 2018

View reviewed changes

kbrock merged commit a93cc4d into ManageIQ:master Oct 12, 2018

kbrock self-assigned this Oct 12, 2018

kbrock added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 12, 2018

jrafanie deleted the rails_5_1_association_reload_was_removed branch December 14, 2018 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone #18079

Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone #18079

jrafanie commented Oct 10, 2018 •

edited

Loading

NickLaMuro left a comment

NickLaMuro Oct 10, 2018

jrafanie Oct 10, 2018

NickLaMuro Oct 10, 2018 •

edited

Loading

jrafanie commented Oct 11, 2018 •

edited

Loading

jrafanie Oct 11, 2018 •

edited

Loading

kbrock Oct 12, 2018

jrafanie Oct 12, 2018

NickLaMuro left a comment

bzwei commented Oct 11, 2018

miq-bot commented Oct 12, 2018 •

edited by jrafanie

Loading

jrafanie commented Oct 12, 2018

kbrock left a comment

kbrock Oct 12, 2018

kbrock Oct 12, 2018

Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone #18079

Rails 5.0/5.1 Use o.reload.assoc, assoc(true) is gone #18079

Conversation

jrafanie commented Oct 10, 2018 • edited Loading

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Oct 10, 2018

Choose a reason for hiding this comment

jrafanie Oct 10, 2018

Choose a reason for hiding this comment

NickLaMuro Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

jrafanie commented Oct 11, 2018 • edited Loading

jrafanie Oct 11, 2018 • edited Loading

Choose a reason for hiding this comment

kbrock Oct 12, 2018

Choose a reason for hiding this comment

jrafanie Oct 12, 2018

Choose a reason for hiding this comment

NickLaMuro left a comment

Choose a reason for hiding this comment

bzwei commented Oct 11, 2018

miq-bot commented Oct 12, 2018 • edited by jrafanie Loading

jrafanie commented Oct 12, 2018

kbrock left a comment

Choose a reason for hiding this comment

kbrock Oct 12, 2018

Choose a reason for hiding this comment

kbrock Oct 12, 2018

Choose a reason for hiding this comment

jrafanie commented Oct 10, 2018 •

edited

Loading

NickLaMuro Oct 10, 2018 •

edited

Loading

jrafanie commented Oct 11, 2018 •

edited

Loading

jrafanie Oct 11, 2018 •

edited

Loading

miq-bot commented Oct 12, 2018 •

edited by jrafanie

Loading