Fix virtual attribute selection #15387
Conversation
| @@ -166,7 +166,9 @@ def virtual_attribute_search(resource, attribute) | |||
| if resource.class < ApplicationRecord | |||
| # is relation in 'attribute' variable plural in the model class (from 'resource.class') ? | |||
| if [:has_many, :has_and_belongs_to_many].include?(resource.class.reflection_with_virtual(attribute).try(:macro)) | |||
| Rbac.filtered(resource.public_send(attribute)) | |||
| resource_attr = resource.public_send(attribute) | |||
| return resource_attr if resource_attr.kind_of?(Array) && resource_attr.first.kind_of?(String) | |||
There was a problem hiding this comment.
it is because Service#power_states is :virtual_has_many relation what is a custom method and I think that it can return any data type In general, so I think the condition have to be built on fact what we can pass to RBAC,
we can pass to Rbac.filtered(Vm):
- model (
Rbac.filtered(Vm)) - relation, scope (Vm.with_ownership.class
Vm::ActiveRecord_Relation < ActiveRecord::Relation) - the array of ids
- the array of ActiveRecord objects
and I think that only point 4 is is relevant for this case.
@jntullo what do you think about it?
|
🌮 💃 ❤️ |
spec/requests/api/services_spec.rb
Outdated
|
|
||
| expected = { | ||
| "resources" => [ | ||
| a_hash_including("power_states" => %w(on on)) |
There was a problem hiding this comment.
I think these values come from the service's vms (defined somewhere above) - based on the default values from the factory. I think it could be a good idea to specify the raw power state of the vms when they are built, so they are independent of those default values
| @@ -166,7 +166,9 @@ def virtual_attribute_search(resource, attribute) | |||
| if resource.class < ApplicationRecord | |||
| # is relation in 'attribute' variable plural in the model class (from 'resource.class') ? | |||
| if [:has_many, :has_and_belongs_to_many].include?(resource.class.reflection_with_virtual(attribute).try(:macro)) | |||
| Rbac.filtered(resource.public_send(attribute)) | |||
| resource_attr = resource.public_send(attribute) | |||
| return resource_attr unless resource_attr.first.kind_of?(ApplicationRecord) | |||
There was a problem hiding this comment.
I'm having a hard time trying to parse this mentally - perhaps a comment would help here? If I understand this correctly (I may not) - I'm concerned whether Model#some_attribute is guaranteed to respond to first?
There was a problem hiding this comment.
it is under if with condition is it a plural relation? so I think it has to be guaranteed. Or rather add try(:first)?
There was a problem hiding this comment.
@lpichler could this be a problem with RBAC? Or a problem with this being virtual? If it's virtual but can't be treated like a "real" relation, it seems like something is wrong there :P
|
Checked commit jntullo@4713a61 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
I love this pr. SUPER love this pr. 🌮 🎉 |
|
This LGTM!! Thanks @jntullo for fixing this. 👍 |
|
@miq-bot add_label fine/yes |
|
BZ for backport: https://bugzilla.redhat.com/show_bug.cgi?id=1466114 |
Fix virtual attribute selection (cherry picked from commit e92afb1) https://bugzilla.redhat.com/show_bug.cgi?id=1468294
|
Fine backport details: |
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1461939
When expanding a virtual attribute that is an array of values (ie,
power_states => ["ok","ok"]on a service), it is currently failing with the error:This is due to these values being passed to
Rbac.filterer, which is looking foridof object. This fix avoids sending the value of the virtual attribute to filterer.@miq-bot add_label api, bug
@miq-bot assign @abellotti
cc: @AllenBW