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

[WIP] Polymorphic through #19778

Closed
wants to merge 10 commits into from
Closed

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Jan 29, 2020

Fix queries that have virtual attributes and polymorphic queries

This gets away from references().includes() and uses preload for those models we want to reference from ruby and left_joins for those models we want to have in our where and select clauses

it is not valid to references() a polymorphic column

Our understanding was not quite right before.
we did want to preload/include the column, just not use references
references force all includes from preload() to eager_load()
eager_load is the one that generates all the columns aliased
without references(), includes() use preload()

if we includes() a polymorphic, then it converts over to
eager_load() and blows up.

Now we're avoiding references(), getting better sql, and avoiding
eager_load()
@miq-bot
Copy link
Member

miq-bot commented Feb 5, 2020

Checked commits kbrock/manageiq@eeb8c62~...d7cf4fd with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
12 files checked, 4 offenses detected

lib/rbac/filterer.rb

  • ⚠️ - Line 370, Col 29 - Lint/UnusedMethodArgument - Unused method argument - klass. If it's necessary, use _ or _klass as an argument name to indicate that it won't be used.

spec/models/miq_report_spec.rb

@kbrock
Copy link
Member Author

kbrock commented Feb 7, 2020

defering to #19778

@kbrock kbrock closed this Feb 7, 2020
@kbrock kbrock deleted the polymorphic_through branch February 7, 2020 17:17
@kbrock
Copy link
Member Author

kbrock commented Apr 15, 2020

Actually think it was actually fixed by #19804

(still wish we could use join instead of references though)

@kbrock
Copy link
Member Author

kbrock commented Jun 1, 2020

commenting to explain why this was closed:

everything worked with this except for includes and counts

You would think that calling a count would not deal with the includes. It is a preload. And since we are not bringing back the model to then use the preload, it would be pointless to bring them back. So it seems right to ignore the includes statement.

But for counts, the includes are automatically converted to references, which is not what we want and sometimes creates syntax errors. To make this work we would need to add special logic around our counts to ensure that the includes (that do not have references) are removed.

I thought we would also need to add logic to make sure that we don't have a references to a table that is in a joins. But it turned out to remove all the code that used references and includes. Of course I don't totally rule out that there is a problem, hence I am adding this note.

still wish we could use left joins instead of references.

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.

[Bug] Cannot eagerly load the polymorphic association :resource in performance report
3 participants