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.

with_scope should merge complex includes

#1860

with_scope' fails to merge complex includes. Take this code:

Article.with_scope(:find => { :include => :comments }) do
  Article.with_scope(:find => { :include => { :comments => :users } }) do
    Article.find(:all)
  end
end

The includes in effect are [:comments, { :comments => :users }] instead of [{ :comments => :users }] which might result in an unnecessary join.

Responsible for this behaviour is the code in ActiveRecord::Base::merge_includes - it should be able to recognize such complex includes and merge them accordingly.

Reported by Anselm Helbig · February 3rd, 2009 @ 03:46 PM

State: open
Milestone: 3.0.6
Assigned to: Ryan Bigg Ryan Bigg
Importance: Low

Activity

  1. Anselm Helbig
    Anselm Helbig

    February 3rd, 2009 @ 04:45 PM

  2. Brian Rose
    Brian Rose
    • Tag set to activerecord, include, joins
    • Assigned user set to Ryan Bigg

    Unable to replicate under Rails 3.0. The following code only produces three queries (one for each table):

    Article.includes(:comments => :user).includes(:comments).all

    April 12th, 2010 @ 11:51 PM

  3. Ryan Bigg
    Ryan Bigg
    • State changed from new to resolved
    • Milestone cleared.

    I agree, this is more than likely fixed in Rails 3.

    April 13th, 2010 @ 12:39 AM

  4. Ryan Bigg
    Ryan Bigg
    • Tag cleared.
    • Importance changed from to Low

    Automatic cleanup of spam.

    October 11th, 2010 @ 12:11 PM

  5. Brian Underwood
    Brian Underwood

    I think that this is still happening in Rails 3. It've been struggling with this for a while. Could it be re-opened?

    It shows up when you use a where/order which causes Rails to create a join query rather than using separate queries for the eager loading.

    Here's an example in my app that is failing:
    Change.includes(:user).includes(:user => :regions).where('users.deleted_at IS NULL').all

    The application uses Postgres, so I get the following error:

    ActiveRecord::StatementInvalid: PGError: ERROR: missing FROM-clause entry for table "users_changes"
    LINE 1: ..." AS t1_r18, "users"."email_signature" AS t1_r19, "users_cha...

    Of course the "users_changes" table doesn't exist as "user" is a has_many association on the Change model.

    In the SQL query, the "changes", "users", and "regions" tables are used among the select fields. "changes", "users", "regions_users", and "regions" are all joined in the FROM clause (see attachment for the full error including the SQL query and backtrace)

    Using Rails 3.0.0 and RVM on OS 10.6 with ruby version "ruby 1.8.7 (2010-08-16 patchlevel 302) [i686-darwin10.4.0]"

    October 13th, 2010 @ 10:11 PM

  6. Brian Underwood
    Brian Underwood

    Perhaps not surprisingly, Change.includes(:user).includes(:user => :regions).where('users.deleted_at IS NULL').size works, probably because there are no fields specified in the SELECT clause but rather a COUNT(DISTINCT "changes"."id"). Here is the query that in generates:

    SQL (51.7ms) SELECT COUNT(DISTINCT "changes"."id") AS count_id FROM "changes" LEFT OUTER JOIN "users" ON "users"."id" = "changes"."user_id" LEFT OUTER JOIN "regions_users" ON "regions_users"."user_id" = "users"."id" LEFT OUTER JOIN "regions" ON "regions"."id" = "regions_users"."region_id" WHERE (users.deleted_at IS NULL)

    October 13th, 2010 @ 10:36 PM

  7. Brian Underwood
    Brian Underwood

    Digging around a bit myself, it looks like the "includes_values" instance_variable is getting set to [:user, {:user=>:regions}]. Is that right?

    I found that variable being used in ActiveRecord::FinderMethods#find_with_associations

    Searching for the string "includes_values", the only place that I see it being assigned is in ActiveRecord::QueryMethods#includes

    Am I on the right track (I'm also about to get off the train and my connection is slow)?

    October 13th, 2010 @ 10:44 PM

  8. Brian Underwood
    Brian Underwood

    I've done some more digging and I've created a patch which seems to work. Please have a look.

    October 14th, 2010 @ 12:29 AM

  9. Brian Underwood
    Brian Underwood

    I've made a better patch including tests. Please have a look and thanks!

    October 14th, 2010 @ 02:36 PM

  10. Jeremy Kemper
    Jeremy Kemper
    • Milestone set to 3.0.2

    October 15th, 2010 @ 11:01 PM

  11. Brian Underwood
    Brian Underwood

    Just curious: what happens to this ticket now? Should it be marked as open again? Does the milestone changing to 3.0.2 mean that it will go in, or that it targeted for that version? Thanks!

    October 18th, 2010 @ 02:23 PM

  12. Brian Underwood
    Brian Underwood

    Sorry for being so persistent. It's my first patch and I'm a bit caught up in the excitement of open source code in action ;)

    October 18th, 2010 @ 02:25 PM

  13. Aditya Sanghi
    Aditya Sanghi
    • State changed from resolved to open
    • Tag set to activerelation, active_record, patch

    Hi Brian,

    No it doesnt mean that explicitly, i think that was just a bulk update.

    Adding a patch tag to the ticket and marking it open for now since you can reproduce the problem on Rails 3. Someone will need to validate that the patch applies and comment/code review etc. It may then end up in Rails.

    I've not validated your patch but maybe i'll give it a shot as soon as i get the time.

    I understand your enthusiasm, i was in a similar boat a few weeks ago! Keep it up!

    October 18th, 2010 @ 02:37 PM

  14. Anatoliy Lysenko
    Anatoliy Lysenko

    Hi Brian,
    Аccidentally I and Ernie Miller fix this error when we work on #5845.
    We have two patches, hope you'll review them both.
    Unfortunately I dislike your patch, it fail because loop isn't right way to work with associations.
    Here I added test that fail with you patch:
    https://rails.lighthouseapp.com/projects/8994/tickets/5845/a/735636...
    Your patch is good for [:price_estimates,{:price_estimates => :pirate} where loop is good enough. But it fail on [{:posts=>:author}, {:posts=>:comments}] where you should use recursion, like in JoinDependecy#build.

    October 25th, 2010 @ 01:44 PM

  15. Brian Underwood
    Brian Underwood

    Thanks! I'm just glad to see that it's going to be fixed ;) I've got my patch frozen into our version of 3.0.1 in our app, but I look forward to having your patch in soon (and being able to merge hashes in includes)!

    October 25th, 2010 @ 06:38 PM

  16. Jeff Kreeftmeijer
    Jeff Kreeftmeijer
    • Tag cleared.

    Automatic cleanup of spam.

    November 1st, 2010 @ 05:06 PM

  17. Santiago Pastorino
    Santiago Pastorino
    • Milestone cleared.

    November 15th, 2010 @ 09:04 PM

  18. Santiago Pastorino
    Santiago Pastorino
    • Milestone set to 3.0.5

    February 12th, 2011 @ 09:58 PM

  19. Santiago Pastorino
    Santiago Pastorino
    • Milestone changed from 3.0.5 to 3.0.6

    February 27th, 2011 @ 03:15 AM