Fixup polymorphic issues with MiqReport + Rbac #19804

NickLaMuro · 2020-02-04T19:00:16Z

Alternative to #19778

Takes an "alternative" approach to the above by not trying to fix the issue in Rbac, but instead assume the Rbac implementation is as intended, and callers need to work around it.

In this specific case, change the caller of MiqReport::Generator so we aren't passing in arguments to Rbac that are polymorphic.

Links

Bug/Issue: [Bug] Cannot eagerly load the polymorphic association :resource in performance report #19664
Alternative PR: [WIP] Polymorphic through #19778

NickLaMuro · 2020-02-04T19:01:59Z

app/models/miq_report/generator.rb

+ top_level_rels.uniq.each do |assoc|
+ reflection = db_klass.reflect_on_association(assoc)
+ polymorphic_rels << assoc if reflection && reflection.polymorphic?
+ end


Note: This is basically a reworking of the code that was removed here:

8cc2277b

8cc2277b#diff-8d948055de14ced0e63abf9637a9a788L438-L443

app/models/miq_report/generator.rb

spec/models/miq_report/generator_spec.rb

spec/lib/rbac/filterer_spec.rb

skateman

This fixes my issue with the generation of Utilization charts

We have removed support for filtering includes/references by polymorphic relations: ManageIQ@8cc2277b So it is up to the callers to determine the best way to handle this. Raising a custom error will make it more obvious what needs to be done.

This changes what is passed to Rbac from MiqReport::Generator to avoid passing keys for includes/references that are polymorphic in nature. This means that there might be a bit of a performance degradation on some reports, but for now stability is being favored over performance. Follow up: For the keys that are skipped, a MiqPreload should probably be called to avoid N+1 calls, but unsure where the best for this would be.

miq-bot · 2020-02-06T22:20:55Z

Checked commits NickLaMuro/manageiq@1a4de9c~...66c9ec8 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 3 offenses detected

app/models/miq_report/generator.rb

❗ - Line 102, Col 7 - Naming/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 108, Col 7 - Naming/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 97, Col 38 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

kbrock

This is a great solution. Solves our issue and isn't going backwards.

At a future date I may see about getting away from references and going towards left_joins/preload

jrafanie · 2020-02-07T16:31:30Z

spec/lib/rbac/filterer_spec.rb

- :include_for_find => @include
+ described_class.search :class => "MetricRollup",
+ :include_for_find => @include,
+ :references => @include


It's unclear what's not supported here^: polymorphic reflection in includes OR references OR both. Also, does it make sense to put the @include[:resource] = {} here, so it's more clear what isn't supported. Is that change in the before needed for other tests or specific to this test? If the latter, I'd prefer it be here.

may make sense to trim down this includes.

:includes_for_find => %w[v_derived_storage_used resource], :references => %w[v_derived_storage_used]

The current problem is having a polymorphic in the includes() and anything in the references().

Right, I was commenting that it was unclear what exactly is causing the error to be raised. It sounds like a polymorphic in an includes with anything in references so @kbrock's example would make that clearer to me. That can be done as a followup.

jrafanie · 2020-02-07T16:32:40Z

spec/lib/rbac/filterer_spec.rb

+
+ expect do
+ described_class.search :class => "MetricRollup",
+ :include_for_find => @include


Again, it's unclear if this is raising because the polymorphic is in includes or references or both.

yea, this problem is that no references is passed and it fell back to legacy support (when we didn't pass references, we used includes_for_find for both)

jrafanie · 2020-02-07T16:33:37Z

spec/lib/rbac/filterer_spec.rb

+
+ expect do
+ described_class.search :class => "MetricRollup",
+ :include_for_find => @include,


This helps clarify it but is passing just the polymorphic in the references also supported?

To avoid a 📖 explaining the Rails internals, the short answer is that .references does not work without .includes, per the docs:

https://api.rubyonrails.org/v5.1.7/classes/ActiveRecord/QueryMethods.html#method-i-references

So doing a test by itself would be pointless.

ActiveRecord::Base.includes, on the otherhand, under the hood will internally do a polymorphic check and decides whether or not to do one or multiple queries (the "one query" will do some joins as a result):

MetricRollup.inclMetricRollup.includes(:time_profile => {}) # MetricRollup Load (2.2ms) SELECT "metric_rollups".* FROM "metric_rollups" # MetricRollup Inst Including Associations (0.1ms - 1rows) # TimeProfile Load (0.3ms) SELECT "time_profiles".* FROM "time_profiles" WHERE "time_profiles"."id" = 22000000000118 # TimeProfile Inst Including Associations (0.2ms - 1rows) MetricRollup.includes(:time_profile => {}).references(:time_profiles) # SQL (2.3ms) SELECT "metric_rollups"."id" AS t0_r0, # "metric_rollups"."timestamp" AS t0_r1, # "metric_rollups"."capture_interval" AS t0_r2, # "metric_rollups"."resource_type" AS t0_r3, # ... # "time_profiles"."id" AS t1_r0, # "time_profiles"."description" AS t1_r1, # "time_profiles"."profile_type" AS t1_r2, # "time_profiles"."profile_key" AS t1_r3, # LEFT OUTER JOIN "time_profiles" ON "time_profiles"."id" = "metric_rollups"."time_profile_id" # MetricRollup Inst Including Associations (0.2ms - 1rows)

Guess this is a book...

But if you want a book, you can read the commit where I added this spec in the first place: NickLaMuro@03284cf

lol. yup 📖 says you need all references() values in the includes()`. ;)

jrafanie · 2020-02-07T16:47:58Z

spec/models/miq_report_spec.rb

+ :db => "VimPerformanceDaily",
+ :col_order => %w[timestamp cpu_usagemhz_rate_average max_derived_cpu_available
+ resource.derived_memory_used_low_over_time_period]
+ )


Are we testing that it runs without raising? Should we be doing that or checking the results of generate_table?

I didn't write this spec... @kbrock ?

yea, this is testing that it doesn't puke.

jrafanie · 2020-02-07T16:50:58Z

lib/rbac/filterer.rb

@@ -319,6 +321,8 @@ def search(options = {})
 attrs[:auth_count] = auth_count unless options[:skip_counts]

 return targets, attrs
+ rescue ActiveRecord::EagerLoadPolymorphicError
+ raise Rbac::PolymorphicError


Does the exception message include the polymorphic that attempted to be eagerly loaded?

This is just a nice to have... hopefully if it blows up, it will tell you which polymorphic is at fault.

jrafanie · 2020-02-07T16:53:25Z

LGTM, I just had a few questions above.

jrafanie · 2020-02-07T18:51:21Z

Removed comment in description implying this is WIP.

skateman · 2020-02-07T18:53:04Z

YAY!

NickLaMuro commented Feb 4, 2020

View reviewed changes

miq-bot added the wip label Feb 4, 2020

NickLaMuro force-pushed the fix_issue_19664_v2 branch 2 times, most recently from 2fa1e6c to 91ff9a6 Compare February 4, 2020 19:50

kbrock added 2 commits February 4, 2020 16:41

Add Test to show issue with polymorphic in tests

1a4de9c

add tests for report get_includes

b6ea190

NickLaMuro force-pushed the fix_issue_19664_v2 branch from 91ff9a6 to 6f23bdc Compare February 4, 2020 22:41

kbrock reviewed Feb 5, 2020

View reviewed changes

NickLaMuro force-pushed the fix_issue_19664_v2 branch 2 times, most recently from b545018 to 98af3e7 Compare February 5, 2020 23:09

skateman approved these changes Feb 6, 2020

View reviewed changes

NickLaMuro force-pushed the fix_issue_19664_v2 branch from 98af3e7 to 7a94f10 Compare February 6, 2020 17:13

NickLaMuro changed the title ~~[WIP] Fixup polymorphic issues with MiqReport + Rbac~~ Fixup polymorphic issues with MiqReport + Rbac Feb 6, 2020

NickLaMuro added 2 commits February 6, 2020 16:16

NickLaMuro force-pushed the fix_issue_19664_v2 branch from 7a94f10 to 66c9ec8 Compare February 6, 2020 22:16

miq-bot removed the wip label Feb 6, 2020

kbrock approved these changes Feb 7, 2020

View reviewed changes

kbrock added bug core/rbac core/reporting labels Feb 7, 2020

jrafanie reviewed Feb 7, 2020

View reviewed changes

jrafanie approved these changes Feb 7, 2020

View reviewed changes

jrafanie merged commit c6a25c0 into ManageIQ:master Feb 7, 2020

jrafanie self-assigned this Feb 7, 2020

jrafanie added this to the Sprint 130 Ending Feb 17, 2020 milestone Feb 7, 2020

NickLaMuro mentioned this pull request Feb 13, 2020

[Bug] Cannot eagerly load the polymorphic association :resource in performance report #19664

Closed

kbrock mentioned this pull request Apr 15, 2020

[WIP] Polymorphic through #19778

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixup polymorphic issues with MiqReport + Rbac #19804

Fixup polymorphic issues with MiqReport + Rbac #19804

NickLaMuro commented Feb 4, 2020 •

edited by jrafanie

Loading

NickLaMuro Feb 4, 2020

skateman left a comment

miq-bot commented Feb 6, 2020

kbrock left a comment

jrafanie Feb 7, 2020

kbrock Feb 7, 2020 •

edited

Loading

jrafanie Feb 7, 2020

jrafanie Feb 7, 2020

kbrock Feb 7, 2020

jrafanie Feb 7, 2020

NickLaMuro Feb 7, 2020

kbrock Feb 7, 2020

jrafanie Feb 7, 2020

NickLaMuro Feb 7, 2020

kbrock Feb 7, 2020

jrafanie Feb 7, 2020 •

edited

Loading

jrafanie Feb 7, 2020

jrafanie commented Feb 7, 2020

jrafanie commented Feb 7, 2020 •

edited

Loading

skateman commented Feb 7, 2020

Fixup polymorphic issues with MiqReport + Rbac #19804

Fixup polymorphic issues with MiqReport + Rbac #19804

Conversation

NickLaMuro commented Feb 4, 2020 • edited by jrafanie Loading

Links

Choose a reason for hiding this comment

skateman left a comment

Choose a reason for hiding this comment

miq-bot commented Feb 6, 2020

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 7, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Feb 7, 2020

jrafanie commented Feb 7, 2020 • edited Loading

skateman commented Feb 7, 2020

NickLaMuro commented Feb 4, 2020 •

edited by jrafanie

Loading

kbrock Feb 7, 2020 •

edited

Loading

jrafanie Feb 7, 2020 •

edited

Loading

jrafanie commented Feb 7, 2020 •

edited

Loading