#2801 incorrect sql from has_many :through - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2801 ✓resolved

incorrect sql from has_many :through

Reported by Wildgoose | June 13th, 2009 @ 08:23 PM | in 3.0.5

It would appear that AR is generating incorrect SQL for a :through relationship if the middle relationship is a "belongs_to"

It's not really clear from the code that this can ever have worked, so would appreciate some advice.

The end result is that I end up with the error (formatted for clarity):

Mysql::Error: Unknown column 'users.user_id' in 'on clause':
SELECT count(DISTINCT `fleet_agents`.id) AS count_all 
FROM `fleet_agents`  
LEFT OUTER JOIN `users` ON (`fleet_agents`.`id` = `users`.`user_id`)  
LEFT OUTER JOIN `position_mappings` ON (`position_mappings`.`user_id` = `users`.`id`)

Given:

class FleetAgent < ActiveRecord::Base
  belongs_to :user
  has_many :position_mappings, :through => :user
end

Trigger the error with:

FleetAgent.count(:include=> [:position_mappings])

Basically the first "LEFT OUTER JOIN" clause has it's keys and tables mixed up. Its a belongs to, so should be "fleet_agents.user_id = users.id"

The relevant chunk of code seems to be associations.rb :2051

                  " #{join_type} %s ON (%s.%s = %s.%s%s%s%s) " % [
                    table_alias_for(through_reflection.klass.table_name, aliased_join_table_name),
                    connection.quote_table_name(parent.aliased_table_name),
                    connection.quote_column_name(parent.primary_key),
                    connection.quote_table_name(aliased_join_table_name),
                    connection.quote_column_name(jt_foreign_key),
                    jt_as_extra, jt_source_extra, jt_sti_extra
                  ] +
                  ... second clause is correct ...

Here we can see "parent.aliased_table_name . parent.primary_key", which is never going to be a good pairing for a belongs_to relationship. In fact it seems hardwired for a "has_many" interim relationship?

The problem is now highlighted, but as someone who doesn't stare into the rails internals too often I'm struggling to propose the correct solution.

For example looking at the code bath for a normal "belongs_to" and things are completely different there.. In my case reversing the keys is the solution, but that's clearly a coincidence in naming and obviously the reflections need adapting somehow. Also the return value obviously needs to become specific to whether it's a belongs_to or has_many (I'm not even sure how to see what kind of a relationship it is?)

Can someone help take this a bit further and propose a fix?

Comments and changes to this ticket

Jeremy Kemper May 4th, 2010 @ 06:48 PM
- Milestone changed from “2.x” to “3.x”
[bulk edit]
Dan Pickett May 9th, 2010 @ 07:05 PM
- Tag changed from “:through, :through_conditions, has_many, has_many_through” to “:through, :through_conditions, bugmash, has_many, has_many_through”
Can a bugmasher verify this behavior with a failing test case in master?
Neeraj Singh May 9th, 2010 @ 10:44 PM
I am not able to reproduce this issue in rails edge.
You flagged this item as spam.
Diego Algorta May 15th, 2010 @ 10:00 PM
-1 not reproducible

This ticket is not fully coherent to me. What are you wanting to get with FleetAgent.count(:include=> [:position_mappings])
which would be any different from a plain FleetAgent.count ?

Enrico Bianco May 16th, 2010 @ 12:35 AM

+1 verified in current master.

Here are my models:

class User < ActiveRecord::Base
  has_many :position_mappings
end

class PositionMapping < ActiveRecord::Base
  belongs_to :user
end

class FleetAgent < ActiveRecord::Base
  belongs_to :user
  has_many :position_mappings, :through => :user
end

And here's the error:

ruby-1.8.7-p249 > FleetAgent.count(:include => [:position_mappings])
ActiveRecord::StatementInvalid: Mysql::Error: Unknown column 'users.user_id' in 'on clause': SELECT     COUNT(DISTINCT fleet_agents.id) AS count_id FROM       fleet_agents LEFT OUTER JOIN users ON fleet_agents.id = users.user_id LEFT OUTER JOIN position_mappings ON position_mappings.user_id = users.id

But, strangely, the error doesn't happen when calling for actual models instead of counting:
(Note: I didn't actually write any records into the DB)

ruby-1.8.7-p249 > FleetAgent.all(:include => [:position_mappings])
 => []

But why would one want to count FleetAgents and include the position_mappings relation in it? To create conditions based on the position_mappings for the count?

You flagged this item as spam.
Wildgoose May 17th, 2010 @ 08:08 PM
Yes, haven't repro'd this with latest rails, but I think the original issue only occurred once you tried to set any kind of conditions on the relation, it's not the relation itself that see's the problem. So .count is just one way to repro it, but also doing any kind of filter I think should also cause the same issue?

I think if you glance at the code I referenced in the original error report (and presumably factor in that it might have moved line since), then the problem was fairly easy to see - there are several code paths through that section and quite clearly you can see that several types of association should be handled, but there is code only for I think "has_many" associations?

I think if you look at the code the problem should be quite apparent? The challenge to code a fix is that I wasn't sure what other code paths enter this hunk and hence what the full and proper fix should look like. It's somewhat abstract at this point what the various variables point to in terms of different types of relationships and I think it would take someone a little more familiar with what else uses this code to do a full fix (ie we should cover all types of relationship here). I don't think the change is hard or a big change, it's simply that the code appears to be used by several other (unknown) code paths and the solution looks a bit subtle?

From memory it's only a problem when you apply a relationship which is probably why not so many people bang into this?
Rohit Arondekar July 4th, 2010 @ 03:17 AM
- Importance changed from “” to “”
Any updates here? Is this still an issue on master?
Jon Leighton December 16th, 2010 @ 10:31 AM
- Assigned user set to “Aaron Patterson”
- Tag changed from “:through, :through_conditions, bugmash, has_many, has_many_through” to “:through, :through_conditions, bugmash, has_many, has_many_through, patch”
I can confirm this. It affects queries involving joins, so while the above example using count doesn't quite seem to make sense, a query in the form Foo.joins(:bar) would equally have this problem.

Attached is a fix, with tests.
- 2461_has_many_through_join_quoting.diff 5.5 KB
Jon Leighton December 19th, 2010 @ 04:41 PM
Just realised the file name of that patch says 2461. This is a mistake, but rest assured the patch is definitely concerned with this ticket and not a different one.
Repository December 20th, 2010 @ 10:05 PM
- State changed from “new” to “resolved”
(from [b8153fd5a18441567f787a33ca882acb3bb5088a]) Fix problem where wrong keys are used in JoinAssociation when an association goes :through a belongs_to [#2801 state:resolved] https://github.com/rails/rails/commit/b8153fd5a18441567f787a33ca882...
Jon Leighton December 21st, 2010 @ 09:01 PM
- Milestone cleared.
- State changed from “resolved” to “open”
I've seen about 3 other bug reports about this issue, so I've ported the fix to 3-0-stable too, as it would be good to get in 3.0.4. Thanks.
- 2801_3-0-stable_port.diff 6.7 KB
Repository December 23rd, 2010 @ 11:31 PM
- State changed from “open” to “resolved”
(from [e4b384222c303414c42d92c67acdc87385c49c92]) Fix problem where wrong keys are used in JoinAssociation when an association goes :through a belongs_to [#2801 state:resolved] https://github.com/rails/rails/commit/e4b384222c303414c42d92c67acdc...
Santiago Pastorino February 12th, 2011 @ 09:58 PM
- Milestone set to “3.0.5”
[bulk edit]