Rails Ruby on Rails → 2.3.4

Comments and changes to this ticket

• You flagged this item as spam.
  

  RSL February 26th, 2009 @ 11:37 PM

  Looks like a good well thought-out patch to me. Passes tests, seems like an elegant solution. +1 from me.

• 

  Michael Koziarski March 2nd, 2009 @ 05:30 AM

  This is a nice enhancement that should let us avoid some horribly confusing errors, nice change.

  However I'm wondering why you only did the check on insert_record. Reading from the association would cause surprises too

• You flagged this item as spam.
  

  Jaime Bellmyer March 2nd, 2009 @ 06:48 AM

  Thanks for the feedback! @Michael: I picked the best single point of failure, in my opinion. Creating is where the error occurs, and raising an exception on create means the database will never be corrupted in the first place.

  Of course, I'm open to suggestions if you or anyone else really thinks this aspect should be changed.

• You flagged this item as spam.
  

  Bart ten Brinke March 3rd, 2009 @ 11:29 PM

  On insert seems good enough to me. How could you allready have data in it? If you do, then you have got more serious problems then this patch will solve for you.

  Also if you did it differently, it would probably mean you will be validating all has_and_belongs_to_many relationships on every access, which would not be acceptable. +1 for this one.

• You flagged this item as spam.
  

  Bart ten Brinke March 3rd, 2009 @ 11:29 PM

  On insert seems good enough to me. How could you allready have data in it? If you do, then you have got more serious problems then this patch will solve for you.

  Also if you did it differently, it would probably mean you will be validating all has_and_belongs_to_many relationships on every access, which would not be acceptable. +1 for this one.

• 

  Pratik March 8th, 2009 @ 03:48 PM

  • Assigned user set to “Michael Koziarski”
  • Title changed from “[PATCH] Primary key on HABTM join table now raises an exception” to “Primary key on HABTM join table now raises an exception”
• 

  Michael Koziarski March 9th, 2009 @ 07:49 AM

  This patch doesn't seem to apply cleanly any more, but also it occurs to me that it could be worthwhile to cache the results of this test so we don't degrade performance for everyone's inserts?
  

  Or perhaps have this test only fire in development mode? Or do all the adapters cache the results of the primary_key calls?

• You flagged this item as spam.
  

  Jaime Bellmyer March 9th, 2009 @ 03:39 PM

  I've rebased, and attached the updated patch file, patch-2086b.diff. I'll look into caching today, but I wanted to correct the conflict right away.

  • patch-2086b.diff 7.9 KB

• You flagged this item as spam.
  

  Jaime Bellmyer March 9th, 2009 @ 06:35 PM

  The results of the primary key check are now cached, to save processing time on subsequent inserts. I also added a test which verifies this behavior. I moved the check itself out of insert_record, and into its own has_primary_key? instance method, with result caching:

  
  # activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
  def has_primary_key?
    return @has_primary_key unless @has_primary_key.nil?
    @has_primary_key = (ActiveRecord::Base.connection.supports_primary_key? && 
      ActiveRecord::Base.connection.primary_key(@reflection.options[:join_table]))
  end
  

  The insert_record method now calls this new method:

  
  # activerecord/lib/active_record/associations/has_and_belongs_to_many_association.rb
  def insert_record(record, force = true, validate = true)
    if has_primary_key?
      raise ActiveRecord::ConfigurationError, 
        "Primary key is not allowed in a has_and_belongs_to_many join table (#{@reflection.options[:join_table]})."
    end
  
    # ...
  end
  

  Finally, the test (based on a join table that DOES contain a primary key):

  
  # activerecord/test/cases/associations/habtm_join_table_test.rb
  def test_should_cache_result_of_primary_key_check
    if ActiveRecord::Base.connection.supports_primary_key?
      ActiveRecord::Base.connection.stubs(:primary_key).with('my_books_my_readers').returns(false).once
      weaz = MyReader.create(:name=>'Weaz')
        
      weaz.my_books << MyBook.create(:name=>'Great Expectations')
      weaz.my_books << MyBook.create(:name=>'Greater Expectations')
    end
  end
  

  The updated patch, patch-2086c.diff, has been attached.

  • patch-2086c.diff 11.4 KB

• You flagged this item as spam.
  

  Jaime Bellmyer June 3rd, 2009 @ 06:04 PM

  Is there anything I can/should be doing to move this ticket forward? I really think it's a valuable contribution, and I'd like to see it in Rails.

• 

  Michael Koziarski June 10th, 2009 @ 04:41 AM

  • Milestone changed from “2.x” to “2.3.4”
• 

  CancelProfileIsBroken August 7th, 2009 @ 02:00 PM

  • Tag changed from “activerecord, duplicate, has_and_belongs_to_many, join_table, patch, primary_key” to “activerecord, bugmash, duplicate, has_and_belongs_to_many, join_table, patch, primary_key”

• You flagged this item as spam.
  

  John Trupiano August 8th, 2009 @ 05:15 PM

  • verified
  • still cleanly applies
  • +1, i've seen this bite many people in the past...definitely prefer the more descriptive error message the patch gives us.
  • tests look sufficient
• 

  Dan Pickett August 8th, 2009 @ 07:57 PM

  +1 for the feature but the patch did not apply cleanly to 2-3-stable - I also attempted to apply it to master but no joy.

  I was recently bitten by this last week, so it's great to see this patch! Please confirm that this should apply to 2-3-stable and I'll modify the patch to apply cleanly

• You flagged this item as spam.
  

  Steve St. Martin August 8th, 2009 @ 08:23 PM

  +1 verified applies against 2-3-stable and master, tests look ok

• 

  Jeremy Kemper August 10th, 2009 @ 06:20 AM

  • Tag changed from “activerecord, bugmash, duplicate, has_and_belongs_to_many, join_table, patch, primary_key” to “activerecord, duplicate, has_and_belongs_to_many, join_table, patch, primary_key”
• 

  Repository August 10th, 2009 @ 06:20 AM

  • State changed from “new” to “committed”

  (from [9c1bac0b7fcb627640db6824dca3e6e829a3c3e6]) raises an exception on habtm join table inserts if join table contains a primary key. Caches this check to save time on subsequent inserts.

  [#2086 state:committed]

  Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
  http://github.com/rails/rails/commit/9c1bac0b7fcb627640db6824dca3e6...

• 

  Repository August 10th, 2009 @ 06:20 AM

  (from [9d51f6286680b832b0df5e3ce288575214c1de59]) raises an exception on habtm join table inserts if join table contains a primary key. Caches this check to save time on subsequent inserts.

  [#2086 state:committed]

  Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
  http://github.com/rails/rails/commit/9d51f6286680b832b0df5e3ce28857...

• 

  Jeremy Kemper September 11th, 2009 @ 10:08 PM

  #3128 and #3190 are follow-ups on this feature.

• You flagged this item as spam.
  

  tsenart September 27th, 2009 @ 06:15 PM

  This patch just plain broke my app. Since AR doesn't support natively many-to-many double polymorphic relationships, we have a relationships table. Its migration is as follows:

  class CreateRelationships < ActiveRecord::Migration
  def self.up

  create_table :relationships, :id => false do |t|
    t.column :origin_id,        :integer, :null => false
    t.column :origin_type,      :string,  :null => false
    t.column :destination_id,   :integer, :null => false
    t.column :destination_type, :string,  :null => false
    t.column :position,         :integer, :null => true
  end
  
  add_index :relationships, [:origin_id, :origin_type, :destination_id, :destination_type], 
            :unique => true, 
            :name => 'index_relationship_polymorphs'
  

  end

  def self.down

  drop_table :relationships
  

  end end

  Do you have a solution for me? I need the primary key in this table.