This project is archived and is in readonly mode.
ActiveRecord 3 eager loading fail
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':
SELECTid 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.
-
test_cascaded_eager_loading.diff
1.28 KB
-
Anatoliy Lysenko- Tag changed from patch to rails 3.0.0, associations, bug, eager_loading, joindependency, patch
-
Ernie MillerThe 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
- 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
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
Test improved and fix attached.
-
Ari
- 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
- Milestone set to 3.0.2
- Assigned user set to Aaron Patterson
- Importance changed from to Low
-
Anatoliy Lysenko
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
without your fix I get:Category.includes(:posts).includes({:posts=>:comments}).where("posts.id is not null")
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.
-
test_cascaded_eager_loading.diff
2.88 KB
-
-
Ernie Miller
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
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?
-
-
Anatoliy Lysenko
Take a look at mine, is it do the same?
-
Ernie Miller
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
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
And patch from #1860 fail onCategory.includes(:posts, :categorizations).includes({:posts=>:comments}).where("posts.id is not null").all
Category.includes({:posts=>:author}).includes({:posts=>:comments}).where("posts.id is not null").allMy 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
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
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
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
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.
-
fix_duplicate_includes.diff
7.3 KB
-
-
Ernie Miller
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
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
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- 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
Sure, I'll take a look at it right now.
-
-
-
Jon LeightonThis 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
-
fix_5845.diff
1.24 KB
-
-
Ernie Miller
Jon, that's a different patch than the one I did which got applied. Mine doesn't need any extra fixtures.
-
Ernie Miller
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.
-
-