Expose attribute_aliases as attributes #565
Conversation
| # NOTE: CloudObjectStoreObject.alias_attribute :name, :key | ||
| it "finds attributes aliases (without virtual attribute defined)" do | ||
| # if this shows true, find another model/attribute | ||
| expect(model.virtual_attribute?("name")).to eq(false) |
There was a problem hiding this comment.
I'm a little confused what this is trying to show. For CloudObjectStoreObject, both "key" and "name" show up in attribute_names, so I feel like this test would pass without the code changes above. Same goes for Flavor with "cpus" and "cpu_cores"
irb(main):009> Flavor.attribute_names[-2..]
=> ["cpus", "cpu_cores"]
irb(main):010> Flavor.attribute_aliases.keys
=> ["cpus", "cpu_cores"]
There was a problem hiding this comment.
This is for rails 7.1.
irb(main):002:0> Flavor.attribute_names.grep(/cpu/)
=> ["cpu_total_cores", "cpu_cores_per_socket", "cpu_sockets"]
irb(main):003:0> Flavor.attribute_aliases.keys.grep(/cpu/)
=> ["cpus", "cpu_cores"]I think it's related to https://www.github.com/rails/rails/pull/52117 and the issue that caused it: https://www.github.com/rails/rails/issues/52111.
Basically, we can't do virtual_attribute on things that are aliased attributes. We're moving to just keeping alias_attribute and removing the virtual_attribute for aliased attributes. Therefore, we need to change automate to pull in aliased attributes since we can't rely on attributes/attribute_names to have them.
There was a problem hiding this comment.
the idea is to drop virtual_column or virtual_attribute on things that are aliased attributes, like this commit:
There was a problem hiding this comment.
We hack it into attribute_names for virtual columns. This doesn't work with rails 7.1 for aliased attributes because they changed how they work in rails 7.1.
There was a problem hiding this comment.
ok thanks - I couldn't understand the context - I knew it was Rails 7.1, but it didn't make sense.
There was a problem hiding this comment.
@Fryguy @jrafanie Do we want to leverage ar_visible_attributes.rb here?
Maybe introduce hidden_and_non_hidden_attributes or all_attributes?
There was a problem hiding this comment.
something like this? ManageIQ/manageiq#23324
kbrock
force-pushed
the
alias_attribute_access
branch
from
b9aa202 to
6e92c8e
Compare
| @@ -60,7 +60,7 @@ def self.ar_model_associations | |||
|
|
|||
| def self.expose_class_attributes(subclass) | |||
| subclass.class_eval do | |||
| model.attribute_names.each do |attr| | |||
| (model.attribute_names | model.try(:attribute_aliases)&.keys).each do |attr| | |||
There was a problem hiding this comment.
Nice find on this location. Does it make sense to have method that virtual attributes adds that provides this?
I see we have similar code here and ManageIQ/manageiq-api#1278. Perhaps, we could have something in core that extends AR to give us exposed_attribute_names so we can have one place to test that exposed_attribute_names includes virtual attributes, columns, aliased attributes, etc. Perhaps deprecated_attributes are in the same bucket.
There was a problem hiding this comment.
@jrafanie Did you see my comment above?
kbrock
changed the title
|
WIP: introducing more generic method for the concept of attributes and attribute aliases. After that is done, will change this code to leverage that. |
kbrock
force-pushed
the
alias_attribute_access
branch
from
6e92c8e to
bec8203
Compare
|
update:
(hence - un-WIP) |
kbrock
changed the title
kbrock
force-pushed
the
alias_attribute_access
branch
from
bec8203 to
18dff46
Compare
|
Removed radjabov... we'll be doing rails 7.1 in spassky |
Blocked:
TODO:
all_attribute_namesor what ever 23324 implementsBefore
Attributes defined with
alias_attributewere not exposed in automate models.So we either had to edit the service models for each of these attributes, or we had to define virtual attributes for these attributes.
After