Fixes to Rbac::Filterer#skip_references #17429
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There are some very infrequent edge cases where `targets` passed into Rbac is a `scope.none`, which is a extended version of `ActiveRecord::Relation` with the `ActiveRecord::NullRelation` module extended on top of it. Part of that module changes `to_sql` to always return `""` instead of the query it would execute (why this is the case is beyond me...). This, as a result, causes an invalid `EXPLAIN` query to be fired if the include_references is going to be up for skipping. To avoid this, check if the scope has inherited the `NullRelation` module, and if it has, don't try to skip and hit the `EXPLAIN`.
MiqExpression has the functionality to split it's filters into SQL based
and non-SQL based when determining if it is `:supported_by_sql`.
For example, if you have an "AND" expression that filters on both a
column value using "=" and a "FIND" search in the MiqExpression, the
"FIND" will not be filterable via SQL, but the "=" clause will (based on
the column being sorted of course.
This means that `MiqExpression#to_sql` could return something like this:
["hosts.name = 'foo'", {:host => {}}, {:supported_by_sql => false}]
Where "hosts.name" would normally be a referenced table/column.
* * *
This change basically updates the check for skip_references so that is
also checks to see if the "exp_sql" is empty, or at least the
"exp_includes" isn't adding anything to the references. This should
avoid most cases where we would see the EXPLAIN get triggered and fail.
|
Note about the backport suggestions: While not required to fix any broken functionality, I think it might make sense to backport into the next release just to clean up the logs a bit. Some of the reports that triggered this are from the Up for debate though. |
|
Checked commits NickLaMuro/manageiq@3baa716~...bb2ab39 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
This was referenced May 16, 2018
|
@miq-bot add_label blocker |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a followup to #17141 which handles a few more edge cases that were found in the specs in
manageiq-ui-classic, in thespec/product/reports_spec.rb.This addresses two edge cases:
ActiveRecord::NullRelationscopesWhen a
.noneis applied to the targets, which under the hood changes theActiveRecord::Relationto aActiveRecord::NullRelation. This causes.to_sqlmethod to change so that it always returns a empty string, which will break theEXPLAINinRbac::Filterer#include_references.The fix here was to just update
#skip_referencesto do some slight introspection into the scope to determine if the.nonehad been applied, and to always return false on the#skip_referencescall in the case.Partially SQL supported
MiqExpressionTurns out that
MiqExpressionobjects have partial support for:supported_by_sql, even though the boolean makes it seem like it is yes or no. This means that if you use an "AND" filters with a SQL supported operator like "=" and a none SQL supported operator like "FIND", theexp_sqlandexp_includesreturned fromMiqExpression#to_sqlcan be non-nil even if options returned is{:supported_by_sql => false}.This change does a simple check against the
exp_sqlandexp_includesif{:supported_by_sql => false}to confirm at least theexp_sqlis false, and if it isn't, that at least their isn't any includes being added by theMiqExpression. This is a bit of a shortcut, but we probably can safely assume that theMiqExpressionwould add it's own includes if theexp_sqlneeded to reference another table, and doing this avoids having to analyze theexp_sqlagainst the main scope to determine that ourselves.Links