This project is archived and is in readonly mode.
JoinAssociation#aliased_table_name_for bug affecting AR find_with_associations?
Reported by Ernie Miller | April 23rd, 2010 @ 04:11 PM | in 3.0.2
While working on a gem, I was making use of the JoinDependency feature where you can supply some existing SQL on initialization to keep aliased_table_name_for from reusing table names in those joins. I encountered a problem in the interaction between aliased_table_name_for, find_with_associations (finder_methods.rb), and build_arel (query_methods.rb)
First: aliased_table_name_for looks at parent#table_joins for this SQL, but therefore only correctly detects table names used outside of the JoinDependency for a top-level join, since this join_sql is not passed down through associations when calling "super(reflection.klass)" in JoinAssociation#initialize.
Second: the order in which build_arel (query_methods.rb) builds a query reverses the "simulated" JoinDependency used to create the outer joins added by find_with_associations, and so ends up reusing column names anyway, regardless of that fact.
This leads to (where Article has_many :comments, has_many :moderations, :through => :comments):
Article.joins(:comments).eager_load(:moderations) ActiveRecord::StatementInvalid: SQLite3::SQLException: ambiguous column name: comments.article_id ...
The fix as I see it involves determining the user-supplied joins in advance of any association joins and supplying that SQL to JoinDependency.new in build_arel to prevent table name collisions.
I have this fixed in my gem at http://github.com/ernie/meta_where/tree/autojoin and would be happy to port the fix to a patch with tests if this is confirmed as a bug.
Comments and changes to this ticket
-
Jeremy Kemper April 23rd, 2010 @ 05:37 PM
- Milestone cleared.
- State changed from “new” to “open”
- Assigned user set to “Pratik”
Indeed! Tricky one; good diagnosis.
-
-
-
Ernie Miller April 24th, 2010 @ 01:44 AM
- Tag changed from “activerecord, eager_loading, find_with_associations” to “activerecord, eager_loading, find_with_associations, patch, tests”
-
-
Ernie Miller April 29th, 2010 @ 04:20 AM
Optimized patch and found a bug in association counts with the old version.
-
Ernie Miller April 29th, 2010 @ 04:21 AM
- Tag changed from “activerecord, eager_loading, find_with_associations, patch, tests” to “activerecord, eager_loading, find_with_associations, patch, tests, updated”
-
Repository April 29th, 2010 @ 04:29 AM
- State changed from “open” to “committed”
(from [e33d304975f5b20b0ba819ab644a2a8f80ff3743]) Fix eager loading of associations causing table name collisions
[#4463 state:committed]
Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/e33d304975f5b20b0ba819ab644a2a... -
Ernie Miller May 22nd, 2010 @ 08:32 PM
Made a silly mistake in the previous patch, checking is_a? instead of class equality. Since JoinAssociation inherits from JoinBase, this can cause a stashed association join to be grafted onto the JoinBase instead of a JoinAssociation in the edge case of multiple-self-joining eager loads. Patch with test included. Sorry about that!
-
Ernie Miller May 23rd, 2010 @ 02:11 PM
- Tag changed from “activerecord, eager_loading, find_with_associations, patch, tests, updated” to “activerecord, eager_loading, find_with_associations, fixed, patch, tests, updated, updated_again”
-
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “Low”
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
Ernie Miller
Jeremy Kemper
Pratik
Attachments
Referenced by
-
4463
JoinAssociation#aliased_table_name_for bug affecting AR find_with_associations?
[#4463 state:committed]
-
4679
Multiple self-referencing eager loads don't join properly
This is from #4463 -- decided it's probably better to cre...