Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

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

  • 0001-Add-has_one-and-has_many-primary_key-option.diff 14.5 KB
• 

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

  • Assigned user set to “Pratik”

• You flagged this item as spam.
  

  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 June 4th, 2008 @ 10:02 PM

  • no changes were found...

  • 0001-Abstract-owner_quoted_id.patch 7.6 KB
• 

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

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

• 

  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.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

  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 June 26th, 2008 @ 03:00 AM

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

  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 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 June 26th, 2008 @ 03:29 AM

  Could you rebase the :primary_key patch against latest master?

• You flagged this item as spam.
  

  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!

  • 0001-primary-key-option.patch 11.7 KB

• You flagged this item as spam.
  

  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 July 7th, 2008 @ 06:07 PM

  It's actually been committed now :)

• 

  Pratik July 14th, 2008 @ 01:47 AM

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

  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?

• You flagged this item as spam.
  

  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 July 28th, 2008 @ 10:22 AM

  Great, thanks!

• You flagged this item as spam.
  

  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

• You flagged this item as spam.
  

  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.

• You flagged this item as spam.
  

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

• You flagged this item as spam.
  

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

  Perfect, this is just what I needed. Thanks!