Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

Eager loading doesn't respect :limit option of the loaded association

#505

@@@ruby class Category < ActiveRecord::Base has_and_belongs_to_many :movies has_and_belongs_to_many :most_recent_movies, :class_name => 'Movie', :order => 'created_at DESC', :limit => 10 end

class Movie < ActiveRecord::Base has_and_belongs_to_many :categories end

Category.find(1).most_recent_movies # => returns 10 movies Category.find(1, :include => :most_recent_movies).most_recent_movies # => returns all the associated movies



I attached a patch to solve this issue.

Reported by Luca Guidi · June 28th, 2008 @ 10:43 AM

State: wontfix
Milestone: 2.x
Assigned to: nobody
Importance: Medium

Activity

  1. Andrew White
    Andrew White

    On the surface this appears to fix things but what you have to remember is that the eager loading may be for more than one parent so if you just limit the query the result will be incorrect unless the associated records are the same for each parent.

    For example if you do a Category.find(:all, :include => :most_recent_movies) then limiting the query will not give you want you want. The only way to fix it is to discard any extra records after they have been preloaded.

    However in your example there may be irregular distribution of movies between categories so I wouldn't recommend eager loading - you may end up loading all of the movie records just to fill the sparser categories.

    The best way to proceed I think would be to patch the preloading code so that extra records are discarded and add a warning in the documentation for eager loading of limited associations.

    June 28th, 2008 @ 03:50 PM

  2. Frederick Cheung
    Frederick Cheung

    Eager loading and limits on the association don't really match. As Andrew says, your patch will break most of the time when you're loading (in your example) more than one category.

    Limiting after load is a bit messy (and sort of creates the illusion that we can eager load such associations efficiently). We already do that for has_one though. If you do go down that road, add_preloaded_records_to_collection is probably the place to enforce that.

    June 28th, 2008 @ 04:29 PM

  3. MatthewRudy
    MatthewRudy

    and then one could argue the following;

    class A
      has_many :bs, :limit => 2
    end
    
    >> a.bs
    => [#< B:1 >, #<B:2 >]
    >> a.bs.create()
    (irb):10:in `irb_binding': limit of association :bs exceeded (HasManyLimitException)
    
    or
    
    >> a.bs.create()
    => #< B:new >
    >> a.bs
    => [#< B:2 >, #< B:new >]
    

    but then which object do you push off...

    and what if you actually wanted to maintain a limit, but allowed a create, so it'd be a hard remove of one of them...

    I dunno...

    just drunkenly (without drinking anything 'cept tea) imagining the craziness that could be involved!

    June 29th, 2008 @ 01:21 AM

  4. Pratik
    Pratik
    • State changed from new to wontfix

    What everyone else said.

    July 12th, 2008 @ 02:22 PM

  5. Luca Guidi
    Luca Guidi

    I tried to figure out how to fix it, but an unnecessary complexity would be added to the code.

    I agree to wontfix status.

    It would be nice to specify this behavior into the documentation, in order to avoid unexpected results.

    July 12th, 2008 @ 02:37 PM

  6. Pratik
    Pratik

    Hey jodosha,

    If you message me (lifo) at github, I'll add you to docrails. So that you can commit the docpatch yourself anytime you wish.

    Thanks.

    July 12th, 2008 @ 02:52 PM

  7. Luca Guidi
    Luca Guidi

    This is just a reminder: the case was documented in http://github.com/lifo/docrails/...

    November 3rd, 2008 @ 09:19 AM