has_many through self join does not include table alias, so fails

I think I found the offending line. activerecord-2.3.5/lib/active_record/associations/has_many_through_association.rb, line 172, in HasManyThroughAssociation#construct_joins:

      "INNER JOIN %s ON %s.%s = %s.%s %s #{@reflection.options[:joins]} #{custom_joins}" % [

This hardcoded bit of SQL doesn't contain any table alias abilities. Any suggestions how to fix? I'm going to hard code an alias for now, just to get my app working:

      "INNER JOIN %s as temporary_table_alias ON %s.%s = %s.%s %s #{@reflection.options[:joins]} #{custom_joins}" % [

My naive hardcoded sql didn't work. :-(

Here's a simpler schema with no STI:

db/schema.rb:

ActiveRecord::Schema.define(:version => 20100426070714) do

create_table "employees", :force => true do |t|

t.string   "name"
t.integer  "boss_id"
t.datetime "created_at"
t.datetime "updated_at"

end

end

test/fixtures/employees.yml:

alice:
name: CEO alice

bob:
name: CTO bob boss_id: <%= Fixtures.identify(:alice) %>

charlie:
name: junior exec charlie boss_id: <%= Fixtures.identify(:bob) %>

satellite.rb:

class Employee < ActiveRecord::Base
belongs_to :boss, :class_name => "Employee" has_many :reports, :class_name => "Employee", :foreign_key => "boss_id" has_many :reports_of_reports, :through => :reports, :source => :boss end

and test/unit/employee_test.rb:

require 'test_helper'

class EmployeeTest < ActiveSupport::TestCase
def test_reports

alice = Employee.find(:first,:conditions => "name = 'CEO alice'")
expecting = ["CTO bob"]
assert alice.reports.map {|e| e.name} == expecting, 
"alice's reports are #{alice.reports.map {|e| e.name}}, expected #{expecting}"

end

def test_reports_of_reports

alice = Employee.find(:first,:conditions => "name = 'CEO alice'")
expecting = ["junior exec charlie"]
assert alice.reports_of_reports.map {|e| e.name} == ["junior exec charlie"],
"alice's reports' reports are #{alice.reports_of_reports.map {|e| e.name}}, expected #{expecting}"

end end

test_reports passes, but test_reports_of_reports errors with:

1) Error: test_reports_of_reports(EmployeeTest):
ActiveRecord::StatementInvalid: Mysql::Error: Not unique table/alias: 'employees': SELECT employees.* FROM employees INNER JOIN employees ON employees.id = employees.boss_id WHERE ((employees.boss_id = 663665735))

Let's try that again with formatting

Here's a simpler schema with no STI:
db/schema.rb:

ActiveRecord::Schema.define(:version => 20100426070714) do

  create_table "employees", :force => true do |t|
    t.string   "name"
    t.integer  "boss_id"
    t.datetime "created_at"
    t.datetime "updated_at"
  end

end

test/fixtures/employees.yml:

alice:
  name: CEO alice







bob:
  name: CTO bob
  boss_id: <%= Fixtures.identify(:alice) %>







charlie:
  name: junior exec charlie
  boss_id: <%= Fixtures.identify(:bob) %>

satellite.rb:

class Employee < ActiveRecord::Base
  belongs_to :boss, :class_name => "Employee"
  has_many :reports, :class_name => "Employee", :foreign_key => "boss_id"
  has_many :reports_of_reports, :through => :reports, :source => :boss
end

and test/unit/employee_test.rb:

require 'test_helper'

class EmployeeTest < ActiveSupport::TestCase
  def test_reports
    alice = Employee.find(:first,:conditions => "name = 'CEO alice'")
    expecting = ["CTO bob"]
    assert alice.reports.map {|e| e.name} == expecting, 
    "alice's reports are #{alice.reports.map {|e| e.name}}, expected #{expecting}"
  end
  
  def test_reports_of_reports
    alice = Employee.find(:first,:conditions => "name = 'CEO alice'")
    expecting = ["junior exec charlie"]
    assert alice.reports_of_reports.map {|e| e.name} == ["junior exec charlie"],
    "alice's reports' reports are #{alice.reports_of_reports.map {|e| e.name}}, expected #{expecting}"
  end
end

test_reports passes, but test_reports_of_reports errors with:
1) Error: test_reports_of_reports(EmployeeTest): ActiveRecord::StatementInvalid: Mysql::Error: Not unique table/alias: 'employees': SELECT employees.* FROM employees INNER JOIN employees ON employees.id = employees.boss_id WHERE ((employees.boss_id = 663665735))

here's that patch for the unit test

This but is present in rails 2.3.5 as well as edge (3.0.0beta3)

Rather, this bug is present in rails 2.3.5 as well as edge (3.0.0beta3)

Give this patch a shot -- let me know if it does the trick.

Ernie's patch fixes the issue. Has many through self joins are now working in the unit test, as well as my example schemata above. Ernie's patch also fixes a couple mistakes in my unit test, which now passes with Ernie's patch.

I'm extremely grateful to Ernie for his patch; this bug was blocking me, and I wasn't making much headway trying to fix it myself. Thanks, Ernie!

+1 is what I'm trying to say

+1 for fixing this ASAP.

Aaron, my original unit test had a mistake in it, so it will fail. Ernie's patch includes the table aliasing fix, but also includes a fix for my unit test. Did you also include ernie's patch?

I found the patch to apply cleanly a few days ago to both 3.0.3 and HEAD. I guess I will look again to make sure.

@Joe Yes I applied both patches. Make sure it works against 3-0-stable please. Thanks!

@Aaron Oh yes, I see your commit now, including Ernie's patch.

Anyway, I'm not sure what you mean about it not applying cleanly. I've tried it on 3-0-stable and the patch applies cleanly, and the unit test passes for me. Am I looking at the wrong version? Commit 9254750223f1ff65a3dd70f3e0eefbf0f40f02c6?

@Joe Here is a video of what happens when I try to apply Ernie's patch:

http://www.youtube.com/watch?v=aVegs3Uc5U0

I think I'm applying the right thing.

@Aaron Yeah, it looks like you're applying the right patch. Am I applying the patch to the right codebase?

Here's what I'm doing (sorry, no music):

git clone https://github.com/rails/rails.git rails_30stable
cd rails_30stable
git checkout origin/3-0-stable -b 3-0-stable #Switched to a new branch '3-0-stable'
patch -p1 < ../../../hmt_self_join_alias.diff

patching file activerecord/test/cases/associations/has_many_through_associations_test.rb
Hunk #1 succeeded at 456 with fuzz 2 (offset 73 lines).
patching file activerecord/test/models/person.rb
Hunk #1 succeeded at 12 (offset 2 lines).
patching file activerecord/lib/active_record/associations/through_association_scope.rb
Hunk #1 succeeded at 16 (offset 1 line).
Hunk #2 succeeded at 62 (offset 1 line).
Hunk #3 succeeded at 77 (offset 1 line).
patching file activerecord/test/cases/associations/has_many_through_associations_test.rb
Hunk #1 succeeded at 460 (offset 73 lines).
patching file activerecord/test/models/person.rb
Hunk #1 succeeded at 12 (offset 2 lines).

Maybe I'm not using git correctly to get the 3-0-stable branch?

I'm not too sure of my git usage, so I tried directly downloading from github. Still works fine for me. See screengrab:

http://twitpic.com/3g2uf2

O HAI LIGHTHOUSE!

Do I need to update this patch, then? I honestly don't remember even creating it, and after reading the patch I put together have deduced that I did it in some sort of haze induced by reading Rails association code.

Aaron, your comment "I've been making lots of changes to the associations in master so I ported this patch to master" evokes a simultaneous sense of pity and awe. /salute, good sir.

@Joe Yes, I can use patch, but then the commit history will have my name. I'd prefer to keep the credit where credit is due. I'd like to use git am to apply the patch so that everyone gets proper credit. Can you make this patch work with git am? You should be able to just use the patch command like you're doing, but also follow the contributor guidelines once you've made sure everything works locally.

@Ernie Together, we can clean this code up so that people don't feel like scooping their eyes out with a spoon while reading it!

On master, I'm trying to get the association code to generate SQL AST nodes rather than build strings. This should give us speed increases as there will be fewer calls to the AR connection. The other benefit is that we might be able to clean this code up more easily.

@Aaron That'd be a huge win. The association code feels like one of the last bastions of String-y filth in ActiveRecord. It must be exterminated.

Ok Aaron. I think I've fixed the patch to work on 3-0-stable. The merge conflict was trivial (the patch file was expecting the method above HasManyThroughAssociationsTest#test_has_many_association_through_a_has_many_association_to_self to be the last method in the class, which is no longer the case), but it took me some screwing around to figure out how to make git do what I wanted, so please sanity check the new commit file. But I've applied it cleanly to a new checkout using git-am, and I think it looks correct, preserves commit attributions, etc.

See attached.

Attachment hmt_self_join_alias_30stable_safe.diff

PS is it weird that patch can apply the diff cleanly, but git-am cannot?

I redid the new patch so that it puts the new method at the top of the conflict, instead of below it, so that it matches the commit you made to master. Probably git would be smart enough to merge it in even if the new method were in a different place, but just in case...

So disregard hmt_self_join_alias_30stable_safe.diff and use hmt_self_join_alias_30stable_safe2.diff instead.