Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make direct_vms a relation #19201

Merged
merged 1 commit into from
Sep 12, 2019
Merged

Make direct_vms a relation #19201

merged 1 commit into from
Sep 12, 2019

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Aug 25, 2019

You can preload a relation.
You can not preload a method.

Older code in virtual_attributes and rails was more lenient and would not complain.
Newer versions of the gems are more picky.

Failure/Error: preloader.preload(records, associations, preload_scope)
      
ActiveRecord::AssociationNotFoundError:
  Association named 'direct_vms' was not found on Service; perhaps you misspelled it?

We have 2 options:

  1. Remove the preload.
  2. Convert direct_vms to a virtual_has_many. <== we chose this

LJ was more comfortable converting it to a virtual relation.

@kbrock kbrock force-pushed the direct_vms branch 2 times, most recently from dbfa2c4 to 27a9d11 Compare August 26, 2019 16:13
@NickLaMuro
Copy link
Member

NickLaMuro commented Aug 26, 2019

@kbrock do you mind putting in the steps on how or where you did the test to determine this? I am sure this probably is a bug, but it would be nice to have some proof on how to replicate in the future in case this causes a regression.

Otherwise it looks good to me.

@kbrock
Copy link
Member Author

kbrock commented Sep 3, 2019

The older versions of virtual attributes just ate errors (as we've complained about in a number of PRs)
The newer version of virtual attributes throws an error if you attempt to preload a relation that isn't a valid relation.

@kbrock kbrock added the wip label Sep 4, 2019
@kbrock kbrock changed the title direct_vms is a method [WIP] direct_vms is a method Sep 4, 2019
@kbrock
Copy link
Member Author

kbrock commented Sep 4, 2019

wip: determining if this is indeed a problem

@kbrock kbrock changed the title [WIP] direct_vms is a method direct_vms is a method Sep 4, 2019
@miq-bot miq-bot removed the wip label Sep 4, 2019
@kbrock
Copy link
Member Author

kbrock commented Sep 4, 2019

@NickLaMuro thanks for this comment. I'll change something in virtual attributes.

The definition of Preloader.preload [ref] is pretty straight forward:

Implements the details of eager loading of Active Record associations

preload was made for associations. Service defines direct_vms, but it is not a virtual association, and it does not define a :uses. So the preload will do nothing.

@kbrock
Copy link
Member Author

kbrock commented Sep 4, 2019

via @jrafanie

lets add virtual has many:

virtual_has_many   :direct_vms

A little mixed on this one.
Don't want people using direct_vms, but we have code that 
preloads direct_vms and that only works if direct_vms is an association.

in the end, the preload doesn't do anything but
we felt it was safer to make the preload work
than to just remove it.

(to be honest, I was leaning towards removing it but agree that it would
be a scary proposition for others)
@miq-bot
Copy link
Member

miq-bot commented Sep 4, 2019

Checked commit kbrock@92401ca with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@kbrock kbrock changed the title direct_vms is a method direct_vms now a relation Sep 9, 2019
@kbrock kbrock changed the title direct_vms now a relation Make direct_vms a relation Sep 12, 2019
@jrafanie jrafanie merged commit b8af65c into ManageIQ:master Sep 12, 2019
@jrafanie jrafanie self-requested a review September 12, 2019 18:56
@jrafanie jrafanie self-assigned this Sep 12, 2019
@jrafanie jrafanie added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 12, 2019
@kbrock kbrock deleted the direct_vms branch September 12, 2019 19:16
@chessbyte chessbyte mentioned this pull request Mar 31, 2020
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants