Rbac no references array #19318
Rbac no references array #19318
Conversation
kbrock
force-pushed
the
rbac_no_references_array
branch
from
0b19a0a
to
f6c55a0
Compare
| @@ -2479,7 +2471,7 @@ def get_rbac_results_for_and_expect_objects(klass, expected_objects) | |||
|
|
|||
| it "adds .references to the scope" do | |||
| allow(subject).to receive(:warn) | |||
| expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"]) | |||
| expect(resulting_scope.references_values).to eq(%w[miq_servers hosts]) | |||
There was a problem hiding this comment.
This is basically the fix:
- expect(resulting_scope.references_values).to eq(["{:miq_server=>{}}", "{:host=>{}}"])
+ expect(resulting_scope.references_values).to eq(%w[miq_servers hosts])Before: there were 2 tables with very ugly names
After: there are 2 tables (with valid names)
There was a problem hiding this comment.
was it working before? Is this where it would reference all?
There was a problem hiding this comment.
it would reference "all"
just looking at it it looks wrong. right?
kbrock
force-pushed
the
rbac_no_references_array
branch
3 times, most recently
from
5e06ee1
to
60a6509
Compare
|
UPDATE: Removed a change to reports as it was tangental. This was going to remove Now, that processing remains. |
lib/rbac/filterer.rb
Outdated
| # Given a nested hash of associations (used by includes) | ||
| # convert into an array of table names (used by references) | ||
| # If given an array of table names, will output the same array | ||
| def self.includes_to_references(klass, inc) |
There was a problem hiding this comment.
is this a function of rbac? Rbac filtering? Or generic enough to be a AR mixin?
lib/rbac/filterer.rb
Outdated
| @@ -411,7 +431,7 @@ def include_references(scope, klass, include_for_find, exp_includes, skip) | |||
|
|
|||
| ref_includes = Hash(include_for_find).merge(Hash(exp_includes)) | |||
| unless polymorphic_include?(klass, ref_includes) | |||
| scope = scope.references(include_for_find).references(exp_includes) | |||
| scope = scope.references(self.class.includes_to_references(klass, include_for_find)).references(self.class.includes_to_references(klass, exp_includes)) | |||
There was a problem hiding this comment.
RE: my comment about where this should be live, it feels like it might make sense to call this like klass.includes_to_references(include_for_find)... so maybe a module added to AR. I could be totally wrong. 😉
kbrock
force-pushed
the
rbac_no_references_array
branch
2 times, most recently
from
007e0f6
to
8d6a13d
Compare
We are now passing an array of table names into references. We were passing a nested hash of relations into references which is not valid. This is undefined behavior, and not guaranteed to work in the future. Currently, tails treats this as "just use references for all". We are introducing includes_to_references. It converts an includes compatible (hash of references) to a references compatible (array of table names)
kbrock
force-pushed
the
rbac_no_references_array
branch
from
8d6a13d
to
254dd75
Compare
|
Checked commit kbrock@254dd75 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
This is part 2 of #19127 (look there or in #19295 for the description)
Before
Rbacis passed a hash of relations for use inincludes(). It passes relations toreferences(), which is not correct.After
The
includes()relation hash is converted to an array of table names. So the table names are passed toreferences(). This lets active record work as it was designed.