[WIP] Add support for collection attributes (attributes=vms.name)#1320
[WIP] Add support for collection attributes (attributes=vms.name)#1320jrafanie wants to merge 1 commit into
Conversation
| # Check for ActiveRecord collections (CollectionProxy, Relation) without triggering queries | ||
| # Both have .klass method, plain Arrays don't | ||
| def collection_association?(obj) | ||
| obj.respond_to?(:klass) | ||
| end |
There was a problem hiding this comment.
This is interesting - I feel like this could be done a better way or there should be a helper method elsewhere that already does this.
There was a problem hiding this comment.
def collection_association?(obj)
obj.respond_to?(:loaded?)
endThis seems more intention revealing and uses a more public API... What do you think?
Note, an inline obj.respond_to?(:loaded?):
if related_obj.respond_to?(:loaded?) && @req.collection_attributes_for(base).any?seems worse than:
if collection_association?(related_obj) && @req.collection_attributes_for(base).any?There was a problem hiding this comment.
Yeah, I was more surprised that we didn't have this method already implemented. I agree on the helper method being more intention revealing.
jrafanie
force-pushed
the
871-support-attributes-on-has-many
branch
3 times, most recently
from
cd6ff1c to
4a1783a
Compare
| return fetch_collection_attributes(type, resource, base, related_obj) | ||
| end | ||
|
|
||
| # Standard virtual attribute handling for single attributes (e.g., hardware.host.name) |
There was a problem hiding this comment.
Note, this method should possibly be renamed as we are now fetching collection_associations or indirect virtual attributes. The comments make this path clearer but it might make sense to make these method calls for clearer intent.
|
|
||
| # TODO: Virtual attributes not yet supported - only physical attributes (database columns) | ||
| # Consider: Support virtual attributes on associations (vms.v_total_snapshots) or expose | ||
| # aggregated virtual attributes on primary collection where they can be properly eager loaded? |
There was a problem hiding this comment.
This is a big one... this adds only vms.name type support, it doesn't add virtual column/attributes on the has many association (so not, vms.v_total_snapshots)
It makes me think we might have to weigh the pros/cons of exposing virtual attributes at the primary collection side (which may not make sense), or adding support for pulling back virtual attribute/columns from these has many associations.
There was a problem hiding this comment.
This is a big one... this adds only vms.name type support, it doesn't add virtual column/attributes on the has many association (so not, vms.v_total_snapshots)
It makes me think we might have to weigh the pros/cons of exposing virtual attributes at the primary collection side (which may not make sense), or adding support for pulling back virtual attribute/columns from these has many associations.
in other words, I don't know that things like
GET /providers?attributes=vms.v_total_snapshots...
would even make sense.
Or even worse, a virtual column on providers themselves:
GET /providers?attributes=v_total_snapshots...
4a1783a to
5eb6e3d
Compare
| if collection_association?(related_obj) && @req.collection_attributes_for(base).any? | ||
| fetch_collection_attributes(type, resource, base, related_obj) | ||
| else | ||
| fetch_standard_virtual_attribute(related_obj, base, attr) |
There was a problem hiding this comment.
I split out the logic into two methods for the conditionals so hopefully it's easier to follow
jrafanie
force-pushed
the
871-support-attributes-on-has-many
branch
3 times, most recently
from
af3940d to
6d730b3
Compare
Enables requesting specific attributes on associations using dot notation (e.g., attributes=vms.name,vms.vendor). Returns array of resources with requested attributes plus href and id. Uses eager loading to prevent N+1 queries, reducing query count from 21 to 15 in test case. Fixes ManageIQ#871
jrafanie
force-pushed
the
871-support-attributes-on-has-many
branch
from
6d730b3 to
2b8ffe5
Compare
|
Checked commit jrafanie@2b8ffe5 with ruby 3.3.10, rubocop 1.86.0, haml-lint 0.73.0, and yamllint 1.37.1 |
| def determine_include_for_find(klass) | ||
| attrs = virtual_attributes_for(klass) do |type, attr_name, attr_base| | ||
| attrs = determine_include_for_find_vattrs(klass) | ||
| collections = determine_include_for_find_collections(klass) |
There was a problem hiding this comment.
This method added the collections to the list of things we need to build the includes for. Since we now have virtuals and collections doing similar things, each was added or moved to methods that return what needs to be included and then loops over them and builds the includes.
|
This pull request is not mergeable. Please rebase and repush. |
|
I think this got conflicted by #1324 |
3 participants
Enables requesting specific attributes on has_many associations using dot notation.
Before: attributes=vms.name returned {"vms": {"name": "Vm"}}
After: Returns array of VMs with requested attributes plus href and id
Uses eager loading to prevent N+1 queries.
Fixes #871