This project is archived and is in readonly mode.

#5845 ✓committed
Anatoliy Lysenko

ActiveRecord 3 eager loading fail

Reported by Anatoliy Lysenko | October 20th, 2010 @ 08:23 AM | in 3.0.2

Cascaded eager loading fail when used with joins

After updating my app to Rails 3 I see set of ActiveRecord::ConfigurationError in place when I use cascaded eager loading with joins.
I have following models:
Task has_many :work_logs, has_and_belongs_to_many :tags
WorkLog belongs_to :task
Tag has_and_belongs_to_many :tasks, :join_table => :task_tags
Then code like

 WorkLog.all(:include=>{:task=>:tags},  :joins=>"join users on work_logs.user_id= users.id",  :conditions=>"work_logs.user_id in (select task_users.user_id from task_users)")
raise ActiveRecord::ConfigurationError exception.
When I remove :tags from include or joins or subquery it works. I working on test for this case.
Also I has ActiveRecord::ConfigurationError when use sum with includes and I will make test for it.
Also I has ActiveRecord::ConfigurationError when use joins with cascade includes and count. Failing test for this case is attached.
My first idea was to fix it by changing JoinBase#== method as Ernie Miller suggested in comment to #5124:
from
def ==(other)
  other.class == self.class &&
  other.active_record == active_record &&
  other.table_joins == table_joins
end
to
def ==(other)
  other.class == self.class &&
  other.active_record == active_record
end

Then test pass, and

 WorkLog.all(:include=>{:task=>:tags}, :joins=>"join users on work_logs.user_id= users.id",  :conditions=>"work_logs.user_id in (select task_users.user_id from task_users)")
pass.
But sum with includes still raise ActiveRecord::ConfigurationError.
And
WorkLog.scoped(:include=>:task).all(:include=>{:task=>:tags},  :joins=>"join users on work_logs.user_id= users.id", :conditions=>"work_logs.user_id in (select task_users.user_id from task_users)")
generate weird SQL:

ActiveRecord::StatementInvalid: Mysql2::Error: Unknown column 'tasks_work_logs.id' in 'field list':
SELECT work_logs.id AS t0_r0, work_logs.user_id AS t0_r1, work_logs.task_id AS t0_r2, work_logs.project_id AS t0_r3, work_logs.company_id AS t0_r4, work_logs.customer_id AS t0_r5, work_logs.started_at AS t0_r6, work_logs.duration AS t0_r7, work_logs.body AS t0_r8, work_logs.log_type AS t0_r9, work_logs.paused_duration AS t0_r10, work_logs.comment AS t0_r11, work_logs.exported AS t0_r12, work_logs.approved AS t0_r13, work_logs.access_level_id AS t0_r14, tasks.id AS t1_r0, tasks.name AS t1_r1, tasks.project_id AS t1_r2, tasks.position AS t1_r3, tasks.created_at AS t1_r4, tasks.due_at AS t1_r5, tasks.updated_at AS t1_r6, tasks.completed_at AS t1_r7, tasks.duration AS t1_r8, tasks.hidden AS t1_r9, tasks.milestone_id AS t1_r10, tasks.description AS t1_r11, tasks.company_id AS t1_r12, tasks.priority AS t1_r13, tasks.updated_by_id AS t1_r14, tasks.severity_id AS t1_r15, tasks.type_id AS t1_r16, tasks.task_num AS t1_r17, tasks.status AS t1_r18, tasks.requested_by AS t1_r19, tasks.creator_id AS t1_r20, tasks.notify_emails AS t1_r21, tasks.repeat AS t1_r22, tasks.hide_until AS t1_r23, tasks.scheduled_at AS t1_r24, tasks.scheduled_duration AS t1_r25, tasks.scheduled AS t1_r26, tasks.worked_minutes AS t1_r27, tasks.type AS t1_r28, tasks_work_logs.id AS t2_r0, tasks_work_logs.name AS t2_r1, tasks_work_logs.project_id AS t2_r2, tasks_work_logs.position AS t2_r3, tasks_work_logs.created_at AS t2_r4, tasks_work_logs.due_at AS t2_r5, tasks_work_logs.updated_at AS t2_r6, tasks_work_logs.completed_at AS t2_r7, tasks_work_logs.duration AS t2_r8, tasks_work_logs.hidden AS t2_r9, tasks_work_logs.milestone_id AS t2_r10, tasks_work_logs.description AS t2_r11, tasks_work_logs.company_id AS t2_r12, tasks_work_logs.priority AS t2_r13, tasks_work_logs.updated_by_id AS t2_r14, tasks_work_logs.severity_id AS t2_r15, tasks_work_logs.type_id AS t2_r16, tasks_work_logs.task_num AS t2_r17, tasks_work_logs.status AS t2_r18, tasks_work_logs.requested_by AS t2_r19, tasks_work_logs.creator_id AS t2_r20, tasks_work_logs.notify_emails AS t2_r21, tasks_work_logs.repeat AS t2_r22, tasks_work_logs.hide_until AS t2_r23, tasks_work_logs.scheduled_at AS t2_r24, tasks_work_logs.scheduled_duration AS t2_r25, tasks_work_logs.scheduled AS t2_r26, tasks_work_logs.worked_minutes AS t2_r27, tasks_work_logs.type AS t2_r28, tags.id AS t3_r0, tags.company_id AS t3_r1, tags.name AS t3_r2 FROM work_logs LEFT OUTER JOIN tasks ON tasks.id = work_logs.task_id AND tasks.type = 'Task' LEFT OUTER JOIN task_tags ON task_tags.task_id = tasks.id LEFT OUTER JOIN tags ON tags.id = task_tags.tag_id join users on work_logs.user_id= users.id WHERE (work_logs.user_id in (select task_users.user_id from task_users))

So I think bug is not in JoinBase#== or in JoinDependecy#graft.

One failing test is attached. I working on other tests, will add them asap.

Comments and changes to this ticket

  • Anatoliy Lysenko

    Anatoliy Lysenko October 20th, 2010 @ 08:27 AM

    • Tag changed from “patch” to “rails 3.0.0, associations, bug, eager_loading, joindependency, patch”
  • Ernie Miller

    Ernie Miller October 20th, 2010 @ 01:46 PM

    The text of the ActiveRecord::ConfigurationError would be helpful. I'm assuming it says "Association named tags was not found?"

    Also, since you didn't name your join table with the tables being joined in alphabetical order, you probably should be adding join_table => :task_tags to the Task HABTM.

  • Ari

    Ari October 21st, 2010 @ 02:31 AM

    • Assigned user set to “Marcel Molina”

    Ernie, given that Anatoliy's patch has a test for Rails which makes the bug easily reproducible, are the answers to your questions still important? (I know that Anatoliy will be away from a computer for some time)

  • Ernie Miller

    Ernie Miller October 21st, 2010 @ 02:47 AM

    To be honest, I hadn't seen the patch with test, as I'd quickly
    skimmed the ticket on my iPhone after he sent me a mail asking me to
    take a look at it.

    I'll pull down the test and take a closer look.

  • Ernie Miller

    Ernie Miller October 21st, 2010 @ 03:59 AM

    Test improved and fix attached.

  • Ari

    Ari October 21st, 2010 @ 06:57 AM

    • Assigned user cleared.

    I didn't mean to attach Marcel to this task. I don't know how that happened. Is there someone with commit rights who should be assigned to get this patch into 3.0.2? This bug is stopping the next release of our open source project management system, so Anatoliy and I are keen to see it released.

    Ernie, thanks for you help getting this solved so quickly.

  • Aditya Sanghi

    Aditya Sanghi October 21st, 2010 @ 10:40 AM

    • Milestone set to “3.0.2”
    • Assigned user set to “Aaron Patterson”
    • Importance changed from “” to “Low”
  • Anatoliy Lysenko

    Anatoliy Lysenko October 23rd, 2010 @ 11:31 AM

    Ernie, your fix isn't help me. Yes, it fix my first test. But it doesn't fix issue in my original code. I attached test to illustrate this.
    When I run

     Category.includes(:posts).includes({:posts=>:comments}).where("posts.id is not null")
    without your fix I get:

    2) Failure: test_cascaded_eager_association_loading_with_twice_includes(CascadedEagerLoadingTest) [test/cases/associations/cascaded_eager_loading_test.rb:51]:
    Exception raised:
    <#<ActiveRecord::ConfigurationError: Association named 'comments' was not found; perhaps you misspelled it?>>.

    with your fix:

    1) Failure: test_cascaded_eager_association_loading_with_twice_includes(CascadedEagerLoadingTest) [test/cases/associations/cascaded_eager_loading_test.rb:52]:
    Exception raised:
    <#<ActiveRecord::StatementInvalid: SQLite3::SQLException: no such column: posts_categories.id: SELECT "categories"."id" AS t0_r0, "categories"."name" AS t0_r1, "categories"."type" AS t0_r2, "categories"."categorizations_count" AS t0_r3, "posts"."id" AS t1_r0, "posts"."author_id" AS t1_r1, "posts"."title" AS t1_r2, "posts"."body" AS t1_r3, "posts"."type" AS t1_r4, "posts"."comments_count" AS t1_r5, "posts"."taggings_count" AS t1_r6, "posts_categories"."id" AS t2_r0, "posts_categories"."author_id" AS t2_r1, "posts_categories"."title" AS t2_r2, "posts_categories"."body" AS t2_r3, "posts_categories"."type" AS t2_r4, "posts_categories"."comments_count" AS t2_r5, "posts_categories"."taggings_count" AS t2_r6, "comments"."id" AS t3_r0, "comments"."post_id" AS t3_r1, "comments"."body" AS t3_r2, "comments"."type" AS t3_r3 FROM "categories" LEFT OUTER JOIN "categories_posts" ON "categories_posts"."category_id" = "categories"."id" LEFT OUTER JOIN "posts" ON "posts"."id" = "categories_posts"."post_id" LEFT OUTER JOIN "comments" ON "comments"."post_id" = "posts"."id" WHERE (posts.id is not null)>>

    This code work on Rails 2.3.x but not on Rails 3, 3.0.1, master.

    P.S. I'll add more tests.

  • Ernie Miller

    Ernie Miller October 23rd, 2010 @ 01:45 PM

    So, you say this code works on 2.3.x, but I don't see how identical code could have existed on 2.3.x since #includes is a 3.x method. Your problem, to me, appears to be in your code at this point. What is the intention of calling

    includes(:posts)

    followed by

    includes(:posts => :comments)

    ???

    The same thing is accomplished by a single call to the second one.

    Now, this isn't a case of an existing test failing, so I'm not convinced that what you describe should work is in fact intended behavior, but it's a reasonable assumption since chaining relations should add as few surprises as possible.

    My quick guess as to what's happening: the posts_categories table reference that is failing is a second reference to the same posts table, so it gets the first table alias, which is "#{pluralize(reflection.name)}_#{parent_table_name}#{suffix}" per aliased_table_name_for. Reflection name is :posts, parent table name is categories. It's not actually getting joined though, because a graft is smart enough to recognize it's already been joined once, and so the select portion of the query didn't need to get added.

    For now, the simple workaround is to stop trying to include the same thing twice.

  • Ernie Miller

    Ernie Miller October 23rd, 2010 @ 08:21 PM

    Updated patch attached. This prevents the issue I described above by only allowing associations to be joined once. This is mostly an extract from a similar routine in MetaWhere, but I also needed to consolidate the @associations instance variable as well, to prevent a failure to find the join_part since we delete each one from the duped array once it's found the first time.

    Incidentally, I spent way more time than I should have troubleshooting why I couldn't get a proper count from your supplied test, but the one I did works fine. I believe there's an unrelated bug, possibly around HABTM associations, or there's something I'm not seeing in the fixtures/models, because I always get a count of 0 when I check Category's post association, via joins or includes (with a condition forcing old-style includes). The query shown in debug.log, when run against a fully populated database, returns a count of 3, but AR never does.

    Anyway, this fix passes the tests that are in now, and seems to work properly in one production application I tested it in, as well. Could you let me know if it works properly for you?

  • Ernie Miller

    Ernie Miller October 23rd, 2010 @ 08:22 PM

    Aaaaand attaching the patch. Yay.

  • Anatoliy Lysenko

    Anatoliy Lysenko October 23rd, 2010 @ 09:11 PM

    Take a look at mine, is it do the same?

  • Ernie Miller

    Ernie Miller October 23rd, 2010 @ 09:24 PM

    Anatoliy,

    Yours takes care of the duplicate joins in the first place mentioned (I don't care for the refactor of the two-line build process into one line) but it doesn't address the @associations values, which will lead to ConfigurationErrors when the join_part is already used up by the first time an association is encountered.

  • Anatoliy Lysenko

    Anatoliy Lysenko October 25th, 2010 @ 01:28 PM

    Ernie,
    I doesn't completely understand your last comment. Would be great if you can provide failing test for my patch. I'm newbie and doesn't understand the whole picture, I can only see that one thing fail or pass.
    I review your patch, and patch from #1860 ticket.
    And I can say that my patch is better in some ways.
    I added test to illustrate it in 0003-Fix-eager-loading-of-duplicated-associations.patch.
    Your patch fail on queries like

    Category.includes(:posts, :categorizations).includes({:posts=>:comments}).where("posts.id is not null").all
    And patch from #1860 fail on
    Category.includes({:posts=>:author}).includes({:posts=>:comments}).where("posts.id is not null").all

    My application working now. Thanks! I think we can bother someone from core to accept our patches. Hope this person will review them and set right authors for our commits.

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 02:58 PM

    You're right. I forgot to port over a key part of the metawhere JoinDependency#build code. I'll correct it momentarily, and include details about the @associations stuff I mentioned earlier.

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 03:49 PM

    OK, I squashed all of the commits into one, and am attaching it here. Once again, switching Anatoliy's test against the post association (which always returns 0 results due to situations outlines earlier) for a query that will return results, so we can test the actual results and not just the fact that it doesn't error out.

    Anatoliy, regarding the earlier comment I made about JoinDependency's @associations instance variable:

    If you read through the JoinDependency code, you'll see that JoinDependency#construct deletes join_parts as they are encountered (from a dup of the JD's join_parts). Instantiate passes @associations, which gets the ball rolling. This means that while we're preventing duplicate joins in JD#build, we also need to prevent seeing the same associations twice in @associations while instantiating, or we will trigger the ConfigurationError in this block in construct:

    join_part = join_parts.detect{ |j|
  j.reflection.name.to_s == name.to_s &&
  j.parent_table_name    == parent.class.table_name }
raise(ConfigurationError, "No such association") if join_part.nil?

    Because while join_parts still only has one reference to the association (which has been deleted on the first encounter of the association), @associations could have multiple such references. That's why I altered @associations to start as an empty hash and build it up while we go through the #build process, ensuring the hash matches the actual join parts.

    Hope that helps explain.

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 03:53 PM

    One other note - the associations you were using weren't producing any actual results, which is why you weren't seeing the bugs I mentioned... This is why it's important to always test the results of a query and not just that it doesn't raise an exception.

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 04:14 PM

    Speaking of which, I forgot to re-add those instantiation checks in my tests after working through some other issues. They're back in, now.

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 04:39 PM

    Ignore my foolishness above regarding the "unrelated bug" in habtm associations. I missed that the categories_posts fixture wasn't loaded in this test, which explains the missing associated records.

  • Anatoliy Lysenko

    Anatoliy Lysenko October 25th, 2010 @ 04:51 PM

    Thanks for explanation. You're right.
    Only one thing, in find_join_association you check class of the first argument but you always call it with reflection. Why you do so?

  • Ernie Miller

    Ernie Miller October 25th, 2010 @ 04:58 PM

    I'm checking the class of the variable being used in the right hand side of the comparison -- the left hand side is always a JoinAssociation, so I'm comparing the reflection name, a symbol, to a symbol, in the first case. In the second case, I compare the reflection itself. Since this is an extraction from MetaWhere it actually supports symbols (for use by the MW builder as it's tying nested where conditions into the association tables as aliased by the joindependency. Technically, in this patch, we could opt to just drop the Symbol, String case and assume a joinassociation on both sides.

    Before I spend any more time on this though, I'll wait to hear from someone on the core team.

  • Aaron Patterson

    Aaron Patterson October 30th, 2010 @ 04:29 PM

    • State changed from “new” to “open”

    @Ernie I've applied your patch to master. Can you backport this to 3-0-stable and I'll apply it there too?

  • Ernie Miller

    Ernie Miller October 30th, 2010 @ 04:30 PM

    Sure, I'll take a look at it right now.

  • Ernie Miller

    Ernie Miller October 30th, 2010 @ 05:24 PM

    Here you go!

  • Aaron Patterson

    Aaron Patterson October 30th, 2010 @ 07:21 PM

    • State changed from “open” to “committed”

    Thanks, I've applied this!

  • Jon Leighton

    Jon Leighton October 31st, 2010 @ 11:18 AM

    This tests in this commit were failing for me. Which is due to a missing fixtures declaration in the tests.

    I've attached a patch which fixes it.

    Cheers

  • Jon Leighton

    Jon Leighton October 31st, 2010 @ 11:19 AM

    • no changes were found...
  • Ernie Miller

    Ernie Miller October 31st, 2010 @ 12:07 PM

    Jon, that's a different patch than the one I did which got applied. Mine doesn't need any extra fixtures.

  • Ernie Miller

    Ernie Miller October 31st, 2010 @ 12:12 PM

    Never mind. You're right. Just checked and 3-0-stable doesn't have the categories fixture that master does. Odd that I didn't get any test failures.

  • Ryan Bigg

    Ryan Bigg November 8th, 2010 @ 01:49 AM

    • Tag cleared.

    Automatic cleanup of spam.

  • bingbing

    bingbing March 26th, 2011 @ 01:38 AM

Create your profile

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

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

People watching this ticket

Attachments

Referenced by

Pages