Skip to content
This repository has been archived by the owner on Dec 12, 2021. It is now read-only.

load_collection? triggers sql query on inherited resources #813

Closed
wants to merge 1 commit into from
Closed

load_collection? triggers sql query on inherited resources #813

wants to merge 1 commit into from

Conversation

tmikoss
Copy link

@tmikoss tmikoss commented Jan 29, 2013

When resource_base is a ActiveRecord::Associations::CollectionProxy (common case - inherited resources) resource_base.respond_to? will actually execute the query, due to it being overwritten here.

This presents a problem when working with big data sets + pagination, since all the records will be loaded in memory regardless of how the collection is paginated afterwards.

Rails version: 3.2.11
CanCan version: 1.6.8

@tmikoss
Copy link
Author

tmikoss commented Jan 29, 2013

Will need some version-dependent guards around this code, since the referenced code is different prior to 3.1, and has changed again in current master.

@tmikoss
Copy link
Author

tmikoss commented Jan 29, 2013

This should fix #398

@natebird
Copy link

natebird commented Feb 8, 2013

Can you write up a pull request for the different versions of Rails? Then Ryan can nail down a fix for various versions and decide CanCan v.x will support which implementations or if we can to handle the version cases inline.

@@ -74,7 +74,11 @@ def load_instance?
end

def load_collection?
resource_base.respond_to?(:accessible_by) && !current_ability.has_block?(authorization_action, resource_class)
if @options[:through]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be something like: if @options[:through] && parent_resource instead to avoid raising when resource_base resolves to the resource_class when @options[:shallow] is true?

@pdf
Copy link

pdf commented Dec 8, 2013

I like the end.method syntax better here because it avoids duplication, but I think simply checking for resource_base.respond_to? :proxy_association as I did in #729 is the right way of fixing this. I've been using that in production for about a year.

@tmikoss tmikoss closed this Aug 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants