Improve references #19127

kbrock · 2019-08-09T15:18:55Z

Overview

use static references() properly. Fix references pass 1 #19295 <== better description of the problem
Use dynamic references() properly Rbac no references array #19318
Use references() sparingly. <== This PR

Use references properly

We pass a hash of relations to include(). We pass that same parameter to references(). But, we should pass an array of table names rails references docs. This is undefined behavior. Rails currently puts all tables into the primary query.

Change references(hash) to references(array). It also introduces a method that converts a hash of relations into an array of table names.

Use references sparingly.

Reports reference tables in 2 ways:

include: the models/columns that are in the SELECT clause.
include_for_find:. the models that are used by the ruby view code.

Only the include portion needs to actually be in the primary query.

Introduce option references to Rbac to allow the caller to distinguish between the 2 scenarios. If references is not passed into Rbac, the code assumes the existing behavior and adds the tables to the references().

This is a remake of #18848 and attempting to address performance issues on both the vm and services page

kbrock · 2019-08-22T18:19:39Z

Rubocop: using name get_include to line up with get_include_or_find
ignoring

app/models/mixins/cloud_tenancy_mixin.rb

lib/rbac.rb

lib/rbac/filterer.rb

spec/lib/rbac/filterer_spec.rb

Fryguy · 2019-08-26T17:56:06Z

Looks good to me.

Fryguy · 2019-08-26T17:58:16Z

Can you also verify that the UI tests pass (since they still have the reports tests there?)

includes are for columns in the query includes_for_find are for the models used by the report views Reports merge the two concepts. But only the miq expression (where) or includes (select) need to be in the references. While includes_for_find only needs to preload the models for use in ruby. This allows each to be accessed in a report.

includes() are used in ruby references() are used by sql Tables used in the select or where clause need to be joined using references, joins, or outer_joins. Up until now, we've treated them as the same. But it is taxing to have so many tables in the query. So they are split apart. :include is used in the select column, so it is referenced miq_expression is used in the where clause, so it is referenced include_for_find is only used to preload models. --- If references are not passed in, it defaults to existing behavior. Meaning, all includes are in references. But if references are passed in, then only those tables are referenced()

:references are now passed for hopefully all rbac that also passes in include_for_find. Since the references are explicitly passed in, we don't want the code second guessing these decisions. Also, since not all tables are passed to references, there is no longer the need to come up with complicated logic to reduce the references passed into ActiveRecord

We are already passing conditions into Rbac.filtered, so the MiqExpression will be applied in rbac. There is no reason to also apply it to the where clause here. The reason for making this change now, is we are changing the way the references from an MiqExpression are added. (instead of using a hash, we're converting into an array) I prefer to make this code introduction in as few places as possible.

miq-bot · 2019-10-04T14:06:22Z

Checked commits kbrock/manageiq@0edb041~...296b73f with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

app/models/miq_report/generator.rb

❗ - Line 87, Col 7 - Naming/AccessorMethodName - Do not prefix reader method names with get_.

…ces" This reverts commit e24682e, reversing changes made to cd215c7.

kbrock added refactoring performance core core/rbac labels Aug 9, 2019

miq-bot added the wip label Aug 9, 2019

kbrock force-pushed the rbac_no_references branch 2 times, most recently from c7f779a to 14b65ba Compare August 9, 2019 16:28

kbrock force-pushed the rbac_no_references branch from 14b65ba to c4a35d4 Compare August 19, 2019 22:08