AssociationCollection#destroy should only delete join table records

[lighthouse-import]

Imported from Lighthouse. Original ticket at: http://rails.lighthouseapp.com/projects/8994/tickets/2251
Created by Luca Guidi - 2011-02-16 07:20:11 UTC

I opened this ticket as continuation of an off topic discussion in #2146
Martin Andert reported:

I found some unexpected behavior dealing with has_many :through associations. Given the following:

class Post < ActiveRecord::Base
  has_many :taggings
  has_many :tags, :through => :taggings
end

post = Post.first
tag = post.tags.first

Calling post.tags.delete(tag) removes the tagging from the database and leaves the tag intact.
But calling post.tags.destroy(tag) removes the tag from the database (not the tagging).
IMO, I don't expect destroy to delete the record from the database, only the through_association item.

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-03-16 10:45:25 UTC

There is a difference between delete and destroy: the former is for remove associations, and if configured, to destroy the endpoint.

Example:

class Post < ActiveRecord::Base
  has_many :comments, :dependent => :destroy
end

post.destroy # => will also delete orphan comments from database

The latter was designed for always destroy the association endpoint.

class Post < ActiveRecord::Base
  has_many :taggings
  has_many :tags, :through => :taggings
end

post.tags.delete(tag) # => is the proper way to remove the association.
post.tags.destroy(tag) # => should remove __both__ the association and the tag itself.

For your purposes you should use delete, but you raised a good point, there is a bug in destroy: because it leaves an orphan record in taggings table.

I'll write and attach a patch

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-03-16 10:46:04 UTC

Whoops, sorry Pratik

[lighthouse-import]

Imported from Lighthouse.
Comment by Martin Andert - 2009-03-16 11:07:25 UTC

The latter was designed for always destroy the association endpoint.

Okay. I should have read the documenting comments more conscientious.

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-03-16 11:16:04 UTC

The patch is already there ;)

[lighthouse-import]

Imported from Lighthouse.
Comment by Martin Andert - 2009-03-16 11:46:10 UTC

Eloy, wouldn't it be better to do

def destroy(*records)
  delete_records(flatten_deeper(records))
  super
end

instead of

def destroy(*records)
  delete_records(flatten_deeper(records))
  super
end

because there could exist foreign key contraints (on delete cascade) at the endpoint.

And somehow my test case from above still fails. Am I doing anything wrong?

[lighthouse-import]

Imported from Lighthouse.
Comment by Martin Andert - 2009-03-16 11:47:00 UTC

Oh, the second code snippet must be

def destroy(*records)
  super
  delete_records(flatten_deeper(records))
end

Sorry.

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-03-16 12:31:57 UTC

Ok, I changed the order. Previously I was calling super before, just to perform the raise_on_type_mismatch check.
Now I wrapped the operation in a transaction, so if the type check fails, it rollbacks the database status.

Martin, sorry, but it's not clear for me the following statement in your test:

post.people.destroy << person

[lighthouse-import]

Imported from Lighthouse.
Comment by Martin Andert - 2009-03-16 12:45:50 UTC

Oh, that last "<<" should not be there in my test (copy & paste error). Now the test passes. Thank you.

[lighthouse-import]

Imported from Lighthouse.
Comment by Eloy Duran - 2009-03-16 13:49:36 UTC

Applies and runs. +1

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-04-09 08:37:02 UTC

Ping

[lighthouse-import]

Imported from Lighthouse.
Comment by Repository - 2009-05-18 20:36:55 UTC

(from [7a85927]) Ensure HasManyThroughAssociation#destroy delete orphan records [#2251 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/7a85927da21859a6868c3e0ec92267706b0a14bf

[lighthouse-import]

Imported from Lighthouse.
Comment by Repository - 2009-05-18 20:36:56 UTC

(from [cef76c8]) Ensure HasManyThroughAssociation#destroy delete orphan records [#2251 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/cef76c8af4705dc60f85a721e3a14adb99418d33

[lighthouse-import]

Imported from Lighthouse.
Comment by Olly Headey - 2009-05-22 14:46:43 UTC

As of this commit, calling destroy_all on a :has_many :through association raises an ActiveRecord::HasManyThroughCantAssociateThroughHasManyReflection exception. This wasn't the case before this commit.

e.g. Given:

class Company
  has_many :projects, :through => :contacts
end   

Calling company.projects.destroy_all results in:

ActiveRecord::HasManyThroughCantAssociateThroughHasManyReflection: Cannot 
modify association 'MyClass#projects' because the source reflection class 'Project' 
is associated to 'Contact' via :has_many

Calling company.projects.all.destroy_all works, however.

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-05-22 14:49:53 UTC

I'll verify it ASAP

[lighthouse-import]

Imported from Lighthouse.
Comment by Luca Guidi - 2009-05-24 17:17:08 UTC

It works fine for me.
Olly, please look at this gist (http://bit.ly/siWGR) and tell me if I'm missing something.

I vendored Rails, then checked out the this commit (git checkout cef76c8) and tried with the code reported in the gist above.

Everything worked as expected.