`load_collection?` triggers expensive superfluous query #817

Open
scomma opened this Issue Feb 1, 2013 · 3 comments

Comments

Projects
None yet
2 participants

scomma commented Feb 1, 2013

This took me a while to track down.

I have two models

Account (has_many :conversations)
Conversation (belongs_to :account)

and

class ConversationsController < ApplicationController
  load_resource :account
  load_resource through: :account
end

and a standard nested resources route.

The action is :index.

The first load_resource goes down without issues, populating @account.

The second one, within CanCan::ControllerResource#load_resource, we come across this

        elsif load_collection?
          self.collection_instance ||= load_collection

which when calls this

    def load_collection?
      resource_base.respond_to?(:accessible_by) && !current_ability.has_block?(authorization_action, resource_class)
    end

translates to @account.conversations.respond_to?(:accessible_by), which in turn generates this very expensive query

  Conversation Load (16.5ms)  SELECT `conversations`.* FROM `conversations` WHERE `conversations`.`account_id` = 1

:rage1:

The actual call to @account.conversations.accessible_by doesn't produce any query.

Rails 3.2.9 & CanCan 1.6.8.

scomma commented Apr 25, 2013

Am I the only one who has this problem? This is a critical bug from a performance perspective.

Additional observation: any call to a method of a has_many relationship that ActiveRecord::Relation doesn't know about (i.e. not where, limit, ...) will trigger an implied .all on the relation. Even to a method that doesn't exist.

[37] pry(main)> Account.first.conversations.limit(10).asdfoesudoaeu
  Account Load (0.1ms)  SELECT `accounts`.* FROM `accounts` LIMIT 1
  Conversation Load (0.6ms)  SELECT `conversations`.* FROM `conversations` WHERE `conversations`.`account_id` = 6 LIMIT 10
NoMethodError: undefined method `asdfoesudoaeu' for #<ActiveRecord::Relation:0x00000006c99108>
from /home/alpha/.rbenv/versions/1.9.3-p385/lib/ruby/gems/1.9.1/gems/activerecord-3.2.13/lib/active_record/relation/delegation.rb:45:in `method_missing'
[38] pry(main)> Account.first.conversations.asdfoesudoaeu
  Account Load (0.1ms)  SELECT `accounts`.* FROM `accounts` LIMIT 1
  Conversation Load (0.4ms)  SELECT `conversations`.* FROM `conversations` WHERE `conversations`.`account_id` = 6
NoMethodError: undefined method `asdfoesudoaeu' for #<ActiveRecord::Relation:0x00000006d98298>
from /home/alpha/.rbenv/versions/1.9.3-p385/lib/ruby/gems/1.9.1/gems/activerecord-3.2.13/lib/active_record/relation/delegation.rb:45:in `method_missing'

scomma commented Apr 25, 2013

I think I've found a fix. In controller_resource.rb,

@options[:singleton] ? resource_class : parent_resource.send(@options[:through_association] || name.to_s.pluralize)

if we could append .scoped to the chain, we could prevent ActiveRecord from freaking out and fetching all the records every time we call .respond_to?.

I just need someone to verify that this issue is reproducible.

See also #398 and linked issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment