This project is archived and is in readonly mode.

#505 ✓ wontfix
Luca Guidi

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

Reported by Luca Guidi | June 28th, 2008 @ 10:43 AM | in 2.x

@@@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.

Comments and changes to this ticket

  • Andrew White

    Andrew White June 28th, 2008 @ 03:50 PM

    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.

  • Frederick Cheung

    Frederick Cheung June 28th, 2008 @ 04:29 PM

    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.

  • MatthewRudy

    MatthewRudy June 29th, 2008 @ 01:21 AM

    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!

  • Pratik

    Pratik July 12th, 2008 @ 02:22 PM

    • State changed from “new” to “wontfix”

    What everyone else said.

  • Luca Guidi

    Luca Guidi July 12th, 2008 @ 02:37 PM

    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.

  • Pratik

    Pratik July 12th, 2008 @ 02:52 PM

    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.

  • Luca Guidi

    Luca Guidi November 3rd, 2008 @ 09:19 AM

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Tickets have moved to Github

The new ticket tracker is available at https://github.com/rails/rails/issues

Shared Ticket Bins

Attachments

Pages