Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Fix has_many :through delete with custom foreign keys. Closes #6466.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8043 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
commit 4b639904d7bc4407657767bb963997bc78678d03 1 parent cf9be89
Jeremy Kemper jeremy authored
2  activerecord/CHANGELOG
View
@@ -1,5 +1,7 @@
*SVN*
+* Fix has_many :through delete with custom foreign keys. #6466 [naffis]
+
* Foxy fixtures, from rathole (http://svn.geeksomnia.com/rathole/trunk/README)
- stable, autogenerated IDs
- specify associations (belongs_to, has_one, has_many) by label, not ID
6 activerecord/lib/active_record/associations.rb
View
@@ -49,6 +49,12 @@ def initialize(owner, reflection)
end
end
+ class HasManyThroughCantDissociateNewRecords < ActiveRecordError #:nodoc:
+ def initialize(owner, reflection)
+ super("Cannot dissociate new records through '#{owner.class.name}##{reflection.name}' on '#{reflection.source_reflection.class_name rescue nil}##{reflection.source_reflection.name rescue nil}'. Both records must have an id in order to delete the has_many :through record associating them.")
+ end
+ end
+
class EagerLoadPolymorphicError < ActiveRecordError #:nodoc:
def initialize(reflection)
super("Can not eagerly load the polymorphic association #{reflection.name.inspect}")
22 activerecord/lib/active_record/associations/has_many_through_association.rb
View
@@ -71,18 +71,24 @@ def <<(*records)
def delete(*records)
records = flatten_deeper(records)
records.each { |associate| raise_on_type_mismatch(associate) }
- records.reject! { |associate| @target.delete(associate) if associate.new_record? }
- return if records.empty?
-
- @delete_join_finder ||= "find_all_by_#{@reflection.source_reflection.association_foreign_key}"
+
through = @reflection.through_reflection
- through.klass.transaction do
- records.each do |associate|
- joins = @owner.send(through.name).send(@delete_join_finder, associate.id)
- @owner.send(through.name).delete(joins)
+ raise ActiveRecord::HasManyThroughCantDissociateNewRecords.new(@owner, through) if @owner.new_record?
+
+ load_target
+
+ klass = through.klass
+ klass.transaction do
+ flatten_deeper(records).each do |associate|
+ raise_on_type_mismatch(associate)
+ raise ActiveRecord::HasManyThroughCantDissociateNewRecords.new(@owner, through) unless associate.respond_to?(:new_record?) && !associate.new_record?
+
+ @owner.send(through.name).proxy_target.delete(klass.delete_all(construct_join_attributes(associate)))
@target.delete(associate)
end
end
+
+ self
end
def build(attrs = nil)
18 activerecord/test/associations/join_model_test.rb
View
@@ -9,10 +9,12 @@
require 'fixtures/categorization'
require 'fixtures/vertex'
require 'fixtures/edge'
+require 'fixtures/book'
+require 'fixtures/citation'
class AssociationsJoinModelTest < Test::Unit::TestCase
self.use_transactional_fixtures = false
- fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites, :vertices, :items
+ fixtures :posts, :authors, :categories, :categorizations, :comments, :tags, :taggings, :author_favorites, :vertices, :items, :books
def test_has_many
assert authors(:david).categories.include?(categories(:general))
@@ -476,6 +478,20 @@ def test_adding_to_has_many_through_should_return_self
assert_equal tags, posts(:thinking).tags.push(tags(:general))
end
+ def test_delete_associate_when_deleting_from_has_many_through_with_nonstandard_id
+ count = books(:awdr).references.count
+ references_before = books(:awdr).references
+ book = Book.create!(:name => 'Getting Real')
+ book_awdr = books(:awdr)
+ book_awdr.references << book
+ assert_equal(count + 1, book_awdr.references(true).size)
+
+ assert_nothing_raised { book_awdr.references.delete(book) }
+ assert_equal(count, book_awdr.references.size)
+ assert_equal(count, book_awdr.references(true).size)
+ assert_equal(references_before.sort, book_awdr.references.sort)
+ end
+
def test_delete_associate_when_deleting_from_has_many_through
count = posts(:thinking).tags.count
tags_before = posts(:thinking).tags
4 activerecord/test/fixtures/book.rb
View
@@ -0,0 +1,4 @@
+class Book < ActiveRecord::Base
+ has_many :citations, :foreign_key => 'book1_id'
+ has_many :references, :through => :citations, :source => :reference_of, :uniq => true
+end
7 activerecord/test/fixtures/books.yml
View
@@ -0,0 +1,7 @@
+awdr:
+ id: 1
+ name: "Agile Web Development with Rails"
+
+rfr:
+ id: 2
+ name: "Ruby for Rails"
6 activerecord/test/fixtures/citation.rb
View
@@ -0,0 +1,6 @@
+class Citation < ActiveRecord::Base
+ belongs_to :reference_of, :class_name => "Book", :foreign_key => :book2_id
+
+ belongs_to :book1, :class_name => "Book", :foreign_key => :book1_id
+ belongs_to :book2, :class_name => "Book", :foreign_key => :book2_id
+end
9 activerecord/test/fixtures/db_definitions/schema.rb
View
@@ -290,6 +290,15 @@ def create_table(*args, &block)
t.column :developer_id, :integer, :null=>false
end
+ create_table :books, :force => true do |t|
+ t.column :name, :string
+ end
+
+ create_table :citations, :force => true do |t|
+ t.column :book1_id, :integer
+ t.column :book2_id, :integer
+ end
+
create_table :inept_wizards, :force => true do |t|
t.column :name, :string, :null => false
t.column :city, :string, :null => false
Please sign in to comment.
Something went wrong with that request. Please try again.