Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

[PATCH] Non standard ID column on has_many :through

#2990

Usingthe example code below:

class User < ActiveRecord::Base
has_many :followers has_many :suspects, :through => :followers end

class Connection < ActiveRecord::Base
belongs_to :user belongs_to :suspect, :primary_key => :case_id , :foreign_key => :case_id end

class Suspect < ActiveRecord::Base
belongs_to :connection, :primary_key => :case_id , :foreign_key => :case_id end

The problem is that the belongs_to seems to ignore the :primary key.

If I do

u = User.find(:first)
u.suspects

The SQL generated is:

SELECT suspects.* FROM suspects INNER JOIN connections ON suspects.id = connections.case_id WHERE ((followers.user_id = 1))

However it should be:

SELECT suspects.* FROM suspects INNER JOIN connections ON suspects.case_id = connections.case_id WHERE ((followers.user_id = 1))

This was partially fixed in 2.3.3 but not for has_many :through.

The patch applied to 2.3.3 can be found here: http://github.com/rails/rails/commit/b3ec7b2d03a52e43a4451d522eea7e...

Reported by James Pearson · August 3rd, 2009 @ 10:27 PM

State: resolved
Milestone: none
Assigned to: Aaron Patterson Aaron Patterson
Importance: Medium

Activity

  1. José Valim
    José Valim
    • Tag changed from hasmany, primarykey to bugmash, hasmany, primarykey

    August 8th, 2009 @ 02:43 PM

  2. dira
    dira

    verified on stable

    added tests

    August 9th, 2009 @ 04:45 PM

  3. dira
    dira

    Attached a new patch with tests & implementation. Please ignore the previous patch.

    I'm not sure whether the fix is in the right place or it should be a dedicated method somewhere.

    August 9th, 2009 @ 05:29 PM

  4. dira
    dira
    • Tag changed from bugmash, hasmany, primarykey to bugmash, hasmany, patch, primarykey

    August 9th, 2009 @ 05:30 PM

  5. Rajesh
    Rajesh
    • Tag changed from bugmash, hasmany, patch, primarykey to bugmash, hasmany, primarykey

    verified
    the patch has a failing test case for this case

    August 9th, 2009 @ 06:19 PM

  6. Rajesh
    Rajesh

    the fix also works

    August 9th, 2009 @ 06:28 PM

  7. José Valim
    José Valim

    Dira, do you think we can change your patch to use already existing models?

    August 9th, 2009 @ 07:34 PM

  8. Marshall Huss
    Marshall Huss

    +1 I have had this issue before as well

    August 9th, 2009 @ 08:01 PM

  9. Arthur Zapparoli
    Arthur Zapparoli

    +1 for the idea, -1 for the patch

    Dira: I agree with José Valim, and think this patch needs to be applied using the existing AR models on the test suite.

    Also, I can't see any assertion in your test. And why you added: has_many :posts_by_title, :through => :authorship , :source => :post to Person model?

    August 9th, 2009 @ 08:05 PM

  10. José Valim
    José Valim

    Arthur, could you provide a new patch please?

    August 9th, 2009 @ 09:31 PM

  11. dira
    dira

    @José: Thanks for the feedback; I started on a new patch with the existing models. Also I realized that the original patch was broken (foreign_key & primary_key interchanged). Will submit a new patch tomorrow morning.

    August 9th, 2009 @ 10:49 PM

  12. dira
    dira
    • Tag changed from bugmash, hasmany, primarykey to bugmash, hasmany, patch, primarykey

    Here is the correct patch - with tests and implementation.

    Thank you all for the feedback.

    August 10th, 2009 @ 09:26 AM

  13. dira
    dira

    Here is the correct patch - with tests and implementation.

    Thank you all for the feedback.

    August 10th, 2009 @ 09:27 AM

  14. Szymon Nowak
    Szymon Nowak

    Here are patches for 2-3-stable and master branches, based on dira's last patch. They also fix methods like collection.build/create/delete/clear/<< etc.

    August 13th, 2009 @ 08:20 PM

  15. Szymon Nowak
    Szymon Nowak
    • Assigned user set to Jeremy Kemper
    • Title changed from None standard ID column on has_many :through to [PATCH] Non standard ID column on has_many :through

    August 15th, 2009 @ 04:43 PM

  16. dira
    dira

    the master patch applies cleanly

    the 2.3.stable patch applies cleanly

    September 27th, 2009 @ 11:37 AM

  17. Prem Sichanugrist (sikachu)
    Prem Sichanugrist (sikachu)
    • Tag changed from bugmash, hasmany, patch, primarykey to bugmash, hasmany, patch, primarykey, review
    • State changed from new to open

    January 25th, 2010 @ 05:20 PM

  18. Rizwan Reza
    Rizwan Reza
    • Tag changed from bugmash, hasmany, patch, primarykey, review to hasmany, patch, primarykey, review

    February 12th, 2010 @ 12:46 PM

  19. Prem Sichanugrist (sikachu)
    Prem Sichanugrist (sikachu)
    • Assigned user changed from Jeremy Kemper to José Valim
    • Importance changed from to Medium

    Seems like it has been about a year since there's a patch in this ticket!

    I've confirmed that patch from Szymon still applies on 2-3-stable, and I've update the patch to apply cleanly on master. Can someone please apply this?

    Thank you

    September 10th, 2010 @ 02:59 PM

  20. Prem Sichanugrist (sikachu)
    Prem Sichanugrist (sikachu)
    • Assigned user changed from José Valim to Santiago Pastorino

    Santiago, could you please apply this patch for me?

    Thank you

    September 11th, 2010 @ 09:35 AM

  21. Jon Leighton
    Jon Leighton
    • Assigned user changed from Santiago Pastorino to Aaron Patterson
    • Tag changed from hasmany, patch, primarykey, review to hasmany, patch, primarykey, review, verified

    I have updated Szymon Nowak's patch to current master, and verified that it works correctly.

    Note that two of the tests (test_associate_existing_with_nonstandard_primary_key_on_belongs_to and test_collection_delete_with_nonstandard_primary_key_on_belongs_to) no longer fail in current master due to fixes elsewhere, but I thought it was worth leaving them in for assurance in any case.

    Also note that I have added the method AssociationReflection#association_primary_key. This method is also part of my fix to #2801 and also features in my patch for nested :through associations [#1152], so I'd say it's a good one to have.

    Reassigning to Aaron as he is current the Active Record person.

    December 19th, 2010 @ 04:10 PM

  22. Repository
    Repository
    • State changed from open to resolved

    (from [85683f2a79dbf81130361cb6426786cf6b0d1925]) Fix creation of has_many through records with custom primary_key option on belongs_to [#2990 state:resolved] https://github.com/rails/rails/commit/85683f2a79dbf81130361cb642678...

    December 23rd, 2010 @ 11:20 PM