#4361 has_many through self join does not include table alias, so fails - 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.

#4361 open

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

Reported by Joe Hannon | April 10th, 2010 @ 07:47 AM | in 3.1

Using Rails 2.3.5

I have a has_many :through association where there is only one table. So the owner model, the through model, and the target model are all the same. I've used STI (single table inheritance), but I don't think it's relevant as I'm pretty sure I could replicate the problem without STI as well.

The SQL query generated by a call to the through-association (Person.find(7).babydaddies) generates a SQL query which includes a self-join of the table to itself. The generated query does not alias the tables, and so the self-join query fails. Simply using table aliases in the SQL fixes the problem.

Erroneous SQL, expected SQL, Schema, and model pasted below.

This query is broken:

SELECT DISTINCT `people`.* FROM `people`  INNER JOIN `people` ON `people`.id = `people`.father_id    WHERE ((`people`.mother_id = 7))

it should be something like:

SELECT DISTINCT `dads`.* FROM `people` AS `kids`  INNER JOIN `people` AS `dads` ON `dads`.id = `kids`.father_id    WHERE ((`kids`.mother_id = 7))

db/schema.rb:

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

  create_table "people", :force => true do |t|
    t.string   "name"
    t.string   "sex"
    t.integer  "father_id"
    t.integer  "mother_id"
    t.datetime "created_at"
    t.datetime "updated_at"
  end

end

app/models/person.rb:

class Person < ActiveRecord::Base
  self.inheritance_column = "sex"
  
  belongs_to :father, :class_name => "Person"
  belongs_to :mother, :class_name => "Person"
end

class Male < Person
  has_many :children, :class_name => "Person", :foreign_key => :father_id
  has_many :babymommas, :through => :children, :source => :mother, :uniq => true
end

class Female < Person
  has_many :children, :class_name => "Person", :foreign_key => :mother_id
  has_many :babydaddies, :through => :children, :source => :father, :uniq => true
end

Comments and changes to this ticket

You flagged this item as spam.
Joe Hannon April 10th, 2010 @ 08:34 AM
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}" % [
```
You flagged this item as spam.
Joe Hannon April 10th, 2010 @ 08:34 AM
My naive hardcoded sql didn't work. :-(
You flagged this item as spam.
Joe Hannon April 26th, 2010 @ 04:50 PM
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))

Joe Hannon April 26th, 2010 @ 04:55 PM

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))

Joe Hannon May 2nd, 2010 @ 08:27 PM

Tag set to “patch, unit”

I've attached a patch which adds a unit test to rails/activerecord/test/cases/associations/has_many_through_associations_test.rb:

  def test_has_many_association_through_a_has_many_association_to_self
    sarah = Person.create!(:first_name => 'Sarah', :primary_contact_id => people(:susan).id, :gender => 'F', :number1_fan_id => 1)
    john = Person.create!(:first_name => 'John', :primary_contact_id => sarah.id, :gender => 'M', :number1_fan_id => 1)
    assert_equal sarah.agents, [people(:john)]
    assert_equal people(:susan).agents_of_agents, [people(:john)]
  end

and rails/activerecord/test/models/person.rb:

  has_many :agents_of_agents, :through => :agents, :source => :primary_contact

You flagged this item as spam.
Joe Hannon May 3rd, 2010 @ 12:27 AM
here's that patch for the unit test
- has_many_through_self.diff 2.1 KB
You flagged this item as spam.
Joe Hannon May 3rd, 2010 @ 06:00 PM
This but is present in rails 2.3.5 as well as edge (3.0.0beta3)
You flagged this item as spam.
Joe Hannon May 3rd, 2010 @ 06:00 PM
Rather, this bug is present in rails 2.3.5 as well as edge (3.0.0beta3)
You flagged this item as spam.
Ernie Miller May 4th, 2010 @ 01:50 AM
Give this patch a shot -- let me know if it does the trick.
- hmt_self_join_alias.diff 6.6 KB
You flagged this item as spam.
Ernie Miller May 4th, 2010 @ 03:30 AM
- Tag changed from “patch, unit” to “has_many_through_association, patch, tests, unit”
You flagged this item as spam.
Joe Hannon May 4th, 2010 @ 04:59 PM
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!
You flagged this item as spam.
Joe Hannon May 4th, 2010 @ 05:05 PM
+1 is what I'm trying to say
You flagged this item as spam.
felipekk June 23rd, 2010 @ 10:22 PM
+1 for fixing this ASAP.
Ryan Bigg December 11th, 2010 @ 11:49 PM
- State changed from “new” to “verified”
- Importance changed from “” to “Low”
This patch has been around for too long and needs accepting. Applies fine to master.
Aaron Patterson December 12th, 2010 @ 07:47 PM
- Milestone cleared.
- Assigned user set to “Aaron Patterson”
Aaron Patterson December 15th, 2010 @ 12:33 AM
- Milestone set to “3.1”
- State changed from “verified” to “resolved”
I've been making lots of changes to the associations in master so I ported this patch to master.

The patch doesn't apply cleanly to 3-0-stable, so I tried porting it, but the tests fail. If someone makes sure this applies cleanly and the tests pass on 3-0-stable, I'll apply there.

I'm marking this resolved for 3.1. If someone ports the patch, please create a new ticket for 3.0.4 and assign to me. Thanks!
Repository December 15th, 2010 @ 12:34 AM
- State changed from “resolved” to “open”
(from [00c893d3b86be3a141c21ed065e085adbb26062a]) add test which fails for has_many through self join [#4361 state:open] https://github.com/rails/rails/commit/00c893d3b86be3a141c21ed065e08...
You flagged this item as spam.
Joe Hannon December 15th, 2010 @ 12:58 AM
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.
Aaron Patterson December 15th, 2010 @ 01:01 AM
@Joe Yes I applied both patches. Make sure it works against 3-0-stable please. Thanks!
You flagged this item as spam.
Joe Hannon December 15th, 2010 @ 06:16 AM
@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?
Aaron Patterson December 15th, 2010 @ 06:51 AM
@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.
You flagged this item as spam.
Joe Hannon December 15th, 2010 @ 07:35 AM
@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?
You flagged this item as spam.
Joe Hannon December 15th, 2010 @ 07:41 AM
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
You flagged this item as spam.
Ernie Miller December 15th, 2010 @ 02:19 PM
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.
Aaron Patterson December 15th, 2010 @ 04:45 PM
@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.
You flagged this item as spam.
Ernie Miller December 15th, 2010 @ 04:49 PM
@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.
You flagged this item as spam.
Joe Hannon December 18th, 2010 @ 01:48 AM
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.
You flagged this item as spam.
Joe Hannon December 18th, 2010 @ 01:50 AM
Attachment hmt_self_join_alias_30stable_safe.diff
You flagged this item as spam.
Joe Hannon December 18th, 2010 @ 02:00 AM
PS is it weird that patch can apply the diff cleanly, but git-am cannot?
You flagged this item as spam.
Joe Hannon December 18th, 2010 @ 05:22 AM
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.
- hmt_self_join_alias_30stable_safe2.diff 6.7 KB

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>

Shared Ticket Bins (Sort)

↓↑ drag 455 Open Bugs
↓↑ drag 0 Recent Open Tickets
↓↑ drag 87 Open Patches
↓↑ drag 0 Open Doc Patches
↓↑ drag 542 Stale Tickets
↓↑ drag 3 Verified Patches
↓↑ drag 87 Stale Patches
↓↑ drag 0 Yesterday's Tickets
↓↑ drag 0 Two Days Ago
↓↑ drag 0 This Week
↓↑ drag 0 Last Week
↓↑ drag 7 Bugmash
↓↑ drag 0 Bugmash review queue
↓↑ drag 5 Rails 3 High Priority Tickets

People watching this ticket

Attachments

Referenced by

4361 has_many through self join does not include table alias, so fails (from [00c893d3b86be3a141c21ed065e085adbb26062a]) add tes...

Rails Ruby on Rails → 3.1

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

Comments and changes to this ticket

Joe Hannon April 10th, 2010 @ 08:34 AM

Joe Hannon April 10th, 2010 @ 08:34 AM

Joe Hannon April 26th, 2010 @ 04:50 PM

Joe Hannon April 26th, 2010 @ 04:55 PM

Joe Hannon May 2nd, 2010 @ 08:27 PM

Joe Hannon May 3rd, 2010 @ 12:27 AM

Joe Hannon May 3rd, 2010 @ 06:00 PM

Joe Hannon May 3rd, 2010 @ 06:00 PM

Ernie Miller May 4th, 2010 @ 01:50 AM

Ernie Miller May 4th, 2010 @ 03:30 AM

Joe Hannon May 4th, 2010 @ 04:59 PM

Joe Hannon May 4th, 2010 @ 05:05 PM

felipekk June 23rd, 2010 @ 10:22 PM

Ryan Bigg December 11th, 2010 @ 11:49 PM

Aaron Patterson December 12th, 2010 @ 07:47 PM

Aaron Patterson December 15th, 2010 @ 12:33 AM

Repository December 15th, 2010 @ 12:34 AM

Joe Hannon December 15th, 2010 @ 12:58 AM

Aaron Patterson December 15th, 2010 @ 01:01 AM

Joe Hannon December 15th, 2010 @ 06:16 AM

Aaron Patterson December 15th, 2010 @ 06:51 AM

Joe Hannon December 15th, 2010 @ 07:35 AM

Joe Hannon December 15th, 2010 @ 07:41 AM

Ernie Miller December 15th, 2010 @ 02:19 PM

Aaron Patterson December 15th, 2010 @ 04:45 PM

Ernie Miller December 15th, 2010 @ 04:49 PM

Joe Hannon December 18th, 2010 @ 01:48 AM

Joe Hannon December 18th, 2010 @ 01:50 AM

Joe Hannon December 18th, 2010 @ 02:00 AM

Joe Hannon December 18th, 2010 @ 05:22 AM

Create your profile

Shared Ticket Bins (Sort)

People watching this ticket

Attachments

Tags

Referenced by

Pages

Rails Ruby on Rails → 3.1

Keyword searching

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

Comments and changes to this ticket

Create your profile

Shared Ticket Bins (Sort)

People watching this ticket

Attachments

Tags

Referenced by

Pages