Primary key on HABTM join table now raises an exception

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

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

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.

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.

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.

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?

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.

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.

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.

• 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

+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

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

(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 Primary key on HABTM join table now raises an exception state:committed]

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

#3128 Check for primary key in habtm when the association is defined and #3190 HABTM no longer works when using multiple databases are follow-ups on this feature.

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.