This project is archived and is in readonly mode.

#323 ✓resolved
Zach Dennis

has_many :through => belongs_to_association bug

Reported by Zach Dennis | June 3rd, 2008 @ 09:05 PM

If you declare a has many association through a belongs_to association it will generate invalid SQL and your database will complain.

For example the below has many association would die.

class Day
   belongs_to :trip
   has_many :expenses, :through => :trip
end

Day.first.expenses # dies without the attached patch. 

# Failing output:
SQLite3::SQLException: no such column: trips.trip_id: SELECT "expenses".* FROM "expenses" INNER JOIN trips ON exp
enses.trip_id = trips.id WHERE (("trips".trip_id = 1) AND (("expenses"."date" = 'today')))

In the context of a has many through association when the through association is a belongs_to, the primary key that needs to be the through table's primary key. This only needs to happen in the context of a has many :through though.

Attached is a patch which fixes the issue. It is a very simple and straightforward patch. Thoughts?

Comments and changes to this ticket

  • Mark Van Holstyn

    Mark Van Holstyn June 3rd, 2008 @ 09:16 PM

    +1 tests all passing and looks like a valid use of the has_many :through

  • Zach Dennis

    Zach Dennis June 3rd, 2008 @ 09:16 PM

    The above output is:

    SQLite3::SQLException: no such column: trips.trip_id: SELECT "expenses".* FROM "expenses" INNER JOIN trips ON expenses.trip_id = trips.id WHERE (("trips".trip_id = 1)
    

    The additional conditions was from testing different variations until I found the source of the problem.

  • Zach Dennis

    Zach Dennis June 3rd, 2008 @ 10:21 PM

    Attached is an updated patch. Rather than relying on fixtures this patch creates what it needs inside the test. This forces the implementation to be be updated as well to be correct.

    Apply the attached patch "has_many_through_belongs_to_association.2.diff" by itself, not on top of the original patch. If you have applied the original patch then reset that and apply the latest patch.

  • Zach Dennis

    Zach Dennis June 4th, 2008 @ 12:05 AM

    Attached is another update to the patch to cover the below scenario:

    class Day
      belongs_to :trip
      has_many :expenses, :through => :trip
    end
    
    # - this would blow up since it has no trip, but it should return an empty array
    # - this patch has this return an empty array if the day doesn't have a trip_id
    Day.new.expenses 
    
  • Gabe da Silveira

    Gabe da Silveira June 4th, 2008 @ 12:58 AM

    +1 Tested on mysql. Approach looks sound.

  • Frederick Cheung

    Frederick Cheung June 9th, 2008 @ 09:06 AM

    I believe you will need to make some changes to that you can eager load such an association.

  • Pratik

    Pratik June 9th, 2008 @ 11:23 AM

    • State changed from “new” to “incomplete”
    • Assigned user set to “Pratik”

    What fred said. Could you add tests for preloading as well ?

    Thanks.

  • Zach Dennis

    Zach Dennis June 13th, 2008 @ 03:18 AM

    I'm working on updating the patch to incorporate a test for eager loading. I think some refactoring will be underway since the way has many through associations work assumes the structure of a has_one or has_many structure, and using a belongs_to changes things around, at least in regard to what table has the primary key to use, what the primary key field is, etc.

    Diligently working on it though,

  • Zach Dennis

    Zach Dennis June 13th, 2008 @ 04:27 AM

    The eager loading test is working, but I need to go back and refactor now that everything is working and I understand the code a little better. Here's the current WIP state of things:

    http://github.com/zdennis/rails/...

    I'll post an updated patch this weekend. Thanks,

  • Zach Dennis

    Zach Dennis June 20th, 2008 @ 03:52 AM

    Attached is an updated patch (which replaces the previous ones, don't use them anymore).

    This adds test for eager loading and introduces a simple strategy object for has many associations so the conditional logic for choosing a primary key name or primary key are limited to a single spot.

    The association preloading could be refactored into using different preload objects which handle preloading records based on the type of association. This would clean up and simplify how preloading is done IMO. I almost went that route until Craig Demyanovich suggested going the strategy route first since it was much simpler and had less impact on the overall preloading code.

  • Zach Dennis

    Zach Dennis July 10th, 2008 @ 08:22 PM

    • Tag set to activerecord, bug, patch

    Ping, any love on this pratik?

  • Pratik

    Pratik July 10th, 2008 @ 08:40 PM

    • State changed from “incomplete” to “open”
  • Glenn Powell

    Glenn Powell August 7th, 2008 @ 04:46 PM

    I believe the 4th patch is out of date. Is anyone working on integrating a fix into the core?

  • Glenn Powell

    Glenn Powell August 11th, 2008 @ 08:15 AM

    This does seem like a fairly important bug to be resolved ASAP. I personally continuously run into a brick wall regarding this, since I opted to bypass the outdated patch and await a core fix (but to no avail). What is the status of this now?

  • Zach Dennis

    Zach Dennis August 11th, 2008 @ 01:30 PM

    I know core is a busy group of folks and when they're going to give this patch a look I'll update it again. All they need to do is ping the ticket.

  • Frederick Cheung

    Frederick Cheung August 11th, 2008 @ 01:57 PM

    Can't hurt to ping the rails-core list.

  • Zach Dennis
  • Zach Dennis

    Zach Dennis August 18th, 2008 @ 09:06 PM

    Here's an updated patch to work against Rails today. Please include.

  • Pratik

    Pratik August 19th, 2008 @ 05:51 PM

    Hey Zach,

    I see a lot of tests are commented in your latest patch. Is that intentional ?

  • Zach Dennis

    Zach Dennis August 19th, 2008 @ 06:29 PM

    Sorry about that. I squashed my several commits into one clean commit. See the latest diff. Thanks,

  • Zach Dennis

    Zach Dennis August 23rd, 2008 @ 03:19 PM

    Pratik,

    Any chance you can a look at this?

  • Pratik

    Pratik August 29th, 2008 @ 12:18 PM

    Hey,

    Sorry for the delay. I don't like this patch a lot. So I've been trying to put this logic inside reflection. Turns out, that'll need me a little more time.

    Thanks.

  • Pratik
  • Repository

    Repository October 4th, 2008 @ 05:51 PM

    • State changed from “open” to “resolved”

    (from [95e1cf4812d4b964d7ab0fdf4bfa31177d27909c]) Fix has_many :through when the source is a belongs_to association. [#323 state:resolved]

    Signed-off-by: Pratik Naik pratiknaik@gmail.com http://github.com/rails/rails/co...

  • Glenn Powell

    Glenn Powell February 2nd, 2009 @ 04:06 PM

    I'm actually still seeing something which I believe is a result of this problem. (I am on 2.2, perhaps it's fixed in 2.3)

    I have this relationship:

    
    class Foo < ActiveRecord::Base
    end
    
    class Bar < ActiveRecord::Base
      belongs_to :foo
    end
    
    class Bat < ActiveRecord::Base
      belongs_to :bar
      has_one :foo, :through => :bar
    end
    

    If I call the through attribute directly, then it works:

    
    >> Bat.first.foo
    => #<Foo id:1>
    

    But if I try to include the :foo association in any find conditions, then I get a similar error to the one above.

    
    >> Bat.all(:include => :foo, :conditions => { 'foos.id' => 1 })
    ActiveRecord::StatementInvalid: Mysql::Error: Unknown column 'bars.bar_id' in 'on clause': SELECT `bats`.`id` AS t0_r0, `bat`.`bar_id` AS t0_r1, `foos`.`id` AS t1_r0 FROM `bats`  LEFT OUTER JOIN `bars` ON (`bats`.`id` = `bars`.`bar_id`)  LEFT OUTER JOIN `foos` ON (`foos`.`id` = `bats`.`foo_id`) WHERE (`foos`.`id` = 1) 
    

    It seems as though the join on condition (bats.id = bars.bar_id) has simply reversed the foreign/primary keys here. So it should be: bats.bar_id = bars.id Correct?

  • Frederick Cheung

    Frederick Cheung February 2nd, 2009 @ 04:13 PM

    I definitely remember a patch not too long ago to do with :include in a similar case - certainly worth checking out 2.3

  • Glenn Powell

    Glenn Powell February 2nd, 2009 @ 06:08 PM

    Yes, now I've gone ahead and tried it out in Rails Edge 2.3, but it still fails with the same error.

    Frederick, do you still have a link to that patch? I'd be interested to check it out.

  • Frederick Cheung

    Frederick Cheung February 2nd, 2009 @ 06:11 PM

    Not off the top of my head. Now that I think about it, was has_one :though that was looking at.

  • Corey

    Corey February 10th, 2009 @ 09:03 PM

    Any update on the eager loading in a single query case of 2.2?

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>

Referenced by

Pages