This project is archived and is in readonly mode.

#292 ✓resolved
André Arko

Add has_many :primary_key option

Reported by André Arko | June 1st, 2008 @ 05:09 PM

Has many associations currently force you to use the id of the owner object as the primary key for the association. This patch adds a :primary_key option to has_many that allows you to specify the name of the method that will return the primary key for the association. The new option is tested and documented as an option on the has_many method.

Comments and changes to this ticket

  • Brian Donovan

    Brian Donovan June 1st, 2008 @ 05:14 PM

    There's a good reason for doing this as opposed to setting the primary key for the class, and that's in cases where there is a generated key that is used to join two tables together.

  • Brad Greenlee

    Brad Greenlee June 3rd, 2008 @ 06:07 AM

    Attached patch adds support for :primary_key option for has_one associations as well.

  • Michael Koziarski

    Michael Koziarski June 3rd, 2008 @ 11:43 PM

    • Assigned user set to “Pratik”
  • Brad Greenlee

    Brad Greenlee June 4th, 2008 @ 04:14 PM

    John Hume noted on Rails-Core that :primary_key might be confusing and suggested :alternate_key. I think just :key might suffice. Thoughts?

  • André Arko
  • Pratik

    Pratik June 4th, 2008 @ 10:30 PM

    • State changed from “new” to “invalid”

    I don't believe this is a very common requirement. Please consider making a plugin. And well, if there are enough people asking for this feature, we can always get it in.

    Thanks.

  • Brad Greenlee

    Brad Greenlee June 4th, 2008 @ 10:30 PM

    • State changed from “invalid” to “new”

    I agree that it isn't a very common requirement, but it is fairly core functionality, and monkeypatching it into AR with a plugin is brittle.

  • Brian Donovan

    Brian Donovan June 4th, 2008 @ 07:28 PM

    I think that either :key or :alternate_key would be good.

    Also, I agree with Brad about monkeypatching this into AR. ActiveRecord does a poor job of making itself configurable (via runtime options) and extensible (via code to provide more features). You only have to look at ActiveRecord::Base.find to see that.

    Not only does this patch add a feature that, while you may not need it, seems like something that should be a configuration option, it also abstracts the association's knowledge of how to get the quoted_id, something that is very important if one were to write a plugin to add this functionality.

    As an aside, what is up with Lighthouse's spam filter? It's flagging comments that also change the state of the ticket made by the person who owns the ticket.

  • André Arko

    André Arko June 4th, 2008 @ 10:26 PM

    Other people do want this. Here is a thread from the rubyonrails list where someone wants to know how to set the key on a has_one association:

    http://groups.google.com/group/r...

    Without quoted_id abstracted out, a plugin would be extremely brittle, and it would probably have to be rewritten whenever the rails association code changed.

    I really think this is a good option to have, since it makes associations much more flexible with very little complexity.

  • Brian Donovan

    Brian Donovan June 4th, 2008 @ 10:26 PM

    Can an admin please go to /spams and mark the above comments as ham?

  • Pratik

    Pratik June 4th, 2008 @ 10:26 PM

    I think it's a good idea to make AR code more extensible such that plugins like this are not very brittle.

    Not sure what's up with LH.

  • André Arko

    André Arko June 4th, 2008 @ 10:26 PM

    If you'd like, I could make another patch that just abstracts out quoted_id so that I could just overwrite that in my plugin.

    Are you any more inclined to include this feature since other people have been asking for it on the rails list?

  • Pratik

    Pratik June 4th, 2008 @ 09:17 PM

    I think we can abstract quoted_id to start with. And include the feature in core if more people asks for it.

    Thanks.

  • André Arko

    André Arko June 4th, 2008 @ 10:28 PM

    I've attached a patch that abstracts out @owner.quoted_id to a method named owner_quoted_id. All of the tests still pass. Thanks.

  • André Arko

    André Arko June 25th, 2008 @ 07:23 PM

    • Tag set to activerecord, has_many, patch, tested

    I've provided a patch just like you asked, and waited three weeks. Is it ever going to be applied, so that I can make a plugin out of my earlier patch? Thanks.

  • Jeremy Kemper

    Jeremy Kemper June 26th, 2008 @ 03:00 AM

    • State changed from “new” to “open”
    • Milestone cleared.
  • Jeremy Kemper

    Jeremy Kemper June 26th, 2008 @ 03:04 AM

    Sorry for the wait.

    I think the :primary_key option is great for denormalization and for legacy databases, also.

  • Repository

    Repository June 26th, 2008 @ 03:06 AM

    • State changed from “open” to “committed”

    (from [0b12da44aa35b643b17ab1b61634ff952993e357]) Extract owner_quoted_id so it can be overridden. [#292 state:committed]

    http://github.com/rails/rails/co...

  • Jeremy Kemper

    Jeremy Kemper June 26th, 2008 @ 03:29 AM

    Could you rebase the :primary_key patch against latest master?

  • André Arko

    André Arko July 2nd, 2008 @ 06:32 AM

    Okay, I've rebased my and Brad's patches against master, and included both patches in the attached 0001-primary-key-option.patch. Please let me know if there's anything else you need to get this applied. Thanks!

  • André Arko

    André Arko July 7th, 2008 @ 05:43 PM

    • Assigned user changed from “Pratik” to “Jeremy Kemper”

    Hmm. Should I create a new ticked for the rebased patch, since this ticket is now marked "committed"?

  • Michael Koziarski

    Michael Koziarski July 7th, 2008 @ 06:07 PM

    It's actually been committed now :)

  • Pratik

    Pratik July 14th, 2008 @ 01:47 AM

    • State changed from “committed” to “resolved”
  • Jeremy Kemper

    Jeremy Kemper July 28th, 2008 @ 09:50 AM

    I didn't resolve it earlier since the other patch is still open. indirect, could you open a new ticket and rebase against latest master?

  • André Arko

    André Arko July 28th, 2008 @ 09:58 AM

    Hi Jeremy... all the patches on this ticket have been committed. I rebased a few weeks ago for koz:

    http://github.com/rails/rails/co...

    Thanks for following up, though. :)

  • Jeremy Kemper
  • Shawn

    Shawn August 5th, 2008 @ 10:08 PM

    • Assigned user cleared.

    Could some one add this :primary_key option into belongs_to in a similar fashion? This option is a nice-to-have for has_many, because you can always use finder_sql to workaround it. But it is a must-to-have for belongs_to because there is no way to wrok around it. I had a quick look at the source code and added

    def owner_quoted_id

    if @reflection.options:primary_key]

    quote_value(@owner.send @reflection.options:primary_key]))

    else

    @owner.quoted_id

    end

    end

    into belongs_to.rb. didn't work

  • logan

    logan September 3rd, 2008 @ 08:20 PM

    Just wanted to add some additional motivation for this patch. I'm using uuids in a multi-machine, multi-database configuration with ActiveResource. Since objects are moved between databases on each machine, they can't use the autoincrementing 'id' column, hence uuids. Changing the database primary key to be a uuid has a negative impact on Innodb performance so this patch is the right solution.

    Since all sides of the association are impacted (has_many, belongs_to), the ideal solution would be to specify the primary key as an option on each association declaration, and avoid the finder_sql syntax, which doesn't work on the belongs_to anyway. Of course the same issue exists with has_and_belongs_to_many.

  • André Arko

    André Arko September 3rd, 2008 @ 09:43 PM

    Logan,

    I don't think any additional motivation is needed, since the patch has already been applied to rails. If there are further changes to associations that you would like to make (like adding support for belongs_to :primary_key), a patch would be great. :)

  • Paul Horsfall

    Paul Horsfall October 15th, 2008 @ 04:30 PM

    Perfect, this is just what I needed. Thanks!

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>

Pages