#2251 AssociationCollection#destroy should only delete join table records - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#2251 open

AssociationCollection#destroy should only delete join table records

Reported by Luca Guidi | March 16th, 2009 @ 10:34 AM | in 2.3.10

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.

failing_test.diff 1.2 KB

Comments and changes to this ticket

Pratik March 16th, 2009 @ 10:37 AM
- Assigned user set to “Pratik”
You flagged this item as spam.
Luca Guidi March 16th, 2009 @ 10:45 AM
- Assigned user cleared.
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
You flagged this item as spam.
Luca Guidi March 16th, 2009 @ 10:46 AM
- Assigned user set to “Pratik”
Whoops, sorry Pratik
You flagged this item as spam.
Martin Andert March 16th, 2009 @ 11:07 AM
The latter was designed for always destroy the association endpoint.

Okay. I should have read the documenting comments more conscientious.
You flagged this item as spam.
Luca Guidi March 16th, 2009 @ 11:16 AM
- Tag changed from “activerecord, associations, destroy, has_many_through” to “activerecord, associations, destroy, has_many_through, patch, tested”
The patch is already there ;)
- 0001-Ensure-HasManyThroughAssociation-destroy-delete-orph.patch 2.8 KB
You flagged this item as spam.
Martin Andert March 16th, 2009 @ 11:46 AM
- Tag changed from “activerecord, associations, destroy, has_many_through, patch, tested” to “activerecord, associations, destroy, has_many_through”
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?
You flagged this item as spam.
Martin Andert March 16th, 2009 @ 11:47 AM
Oh, the second code snippet must be
```
def destroy(*records)
  super
  delete_records(flatten_deeper(records))
end
```
Sorry.
You flagged this item as spam.
Luca Guidi March 16th, 2009 @ 12:31 PM
- Tag changed from “activerecord, associations, destroy, has_many_through” to “activerecord, associations, destroy, has_many_through, patch”
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: @@@ruby post.people.destroy << person
- 0001-Ensure-HasManyThroughAssociation-destroy-delete-orph.patch 2.8 KB
You flagged this item as spam.
Martin Andert March 16th, 2009 @ 12:45 PM
Oh, that last "<<" should not be there in my test (copy & paste error). Now the test passes. Thank you.
Eloy Duran March 16th, 2009 @ 01:49 PM
Applies and runs. +1
You flagged this item as spam.
Luca Guidi April 9th, 2009 @ 09:37 AM
Ping
Repository May 18th, 2009 @ 09:36 PM
- State changed from “new” to “resolved”
(from [7a85927da21859a6868c3e0ec92267706b0a14bf]) Ensure HasManyThroughAssociation#destroy delete orphan records [#2251 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/7a85927da21859a6868c3e0ec92267...
Repository May 18th, 2009 @ 09:36 PM
(from [cef76c8af4705dc60f85a721e3a14adb99418d33]) Ensure HasManyThroughAssociation#destroy delete orphan records [#2251 state:resolved]

Signed-off-by: Pratik Naik pratiknaik@gmail.com
http://github.com/rails/rails/commit/cef76c8af4705dc60f85a721e3a14a...
You flagged this item as spam.
Olly Headey May 22nd, 2009 @ 03:46 PM
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.
You flagged this item as spam.
Luca Guidi May 22nd, 2009 @ 03:49 PM
I'll verify it ASAP
You flagged this item as spam.
Luca Guidi May 24th, 2009 @ 06:17 PM
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 cef76c8af4705dc60f85a721e3a14adb99418d33) and tried with the code reported in the gist above.

Everything worked as expected.

Olly Headey June 1st, 2009 @ 10:09 PM

I've created a new gist which demonstrates the problem with the latest 2-3-stable code.

http://gist.github.com/121794

Here's how I created the test project

rails destroy_all_test
cd destroy_all_test/vendor
git clone git://github.com/rails/rails.git
cd rails
git checkout -b 2-3-stable origin/2-3-stable

If you then copy in the files from the gist and run

rake test

You'll see this failure:

/opt/ruby-enterprise-1.8.6-20090421/bin/ruby -I"lib:test" "/opt/ruby-enterprise-1.8.6-20090421/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader.rb" "test/unit/company_test.rb" "test/unit/contact_test.rb" "test/unit/project_test.rb" 
Loaded suite /opt/ruby-enterprise-1.8.6-20090421/lib/ruby/gems/1.8/gems/rake-0.8.7/lib/rake/rake_test_loader
Started
E..
Finished in 0.054827 seconds.

  1) Error:
test_destroy_all(CompanyTest):
ActiveRecord::HasManyThroughCantAssociateThroughHasManyReflection: Cannot modify association 'Company#projects' because the source reflection class 'Project' is associated to 'Contact' via :has_many.
    /test/unit/company_test.rb:8:in `test_destroy_all'

3 tests, 2 assertions, 0 failures, 1 errors

As you can see I'm running Ruby Enterprise Edition 1.8.6

You flagged this item as spam.
Luca Guidi June 3rd, 2009 @ 11:08 AM
I gave a look at the Olly's gist, I don't know if it's a proper way to use an hmt relation, please look at the Contact class, it uses both belongs_to and has_many macros.

I always thought about hmt as an extended habtm relation.
```
class Feed < ActiveRecord::Base
  has_many :subscriptions
  has_many :subscribers, :through => :subscriptions, :source => :user
end

class Subscription < ActiveRecord::Base
  belongs_to :subscription
  belongs_to :user
end

class User < ActiveRecord::Base
  has_many :subscriptions
  has_many :feeds, :through => :subscriptions
end
```
Look at how Subscription uses belongs_to, it acts as a bridge between the other two models.

The exampled posted by Olly breaks this pattern, just because a Company instance tries to delete records which are property of Contact.
This is the reason we didn't found the bug until now.

I'll take care about this issue.
You flagged this item as spam.
Olly Headey June 3rd, 2009 @ 02:09 PM
Thanks for looking at this. I do think my example model is fairly typical of a has_many :through situation.

A Company has_many Contacts.
A Contact has_many Projects.
Therefore, a Company has_many Projects through Contacts.

Looking forward to the fix!
You flagged this item as spam.
Luca Guidi June 3rd, 2009 @ 02:36 PM
I found a contradictory behavior, if look at [has_many_association_test.rb:858](http://github.com/rails/rails/blob/971e2438d98326c994ec6d3ef8e37b7e868ed6e2/activerecord/test/cases/associations/has_many_associations_test.rb#L858) the hmt association should raise an exception if tries to modify the endpoint collection.

According to this test the this isn't a bug, but I found something else: [has_many_through_association_test.rb:104](http://github.com/rails/rails/blob/971e2438d98326c994ec6d3ef8e37b7e868ed6e2/activerecord/test/cases/associations/has_many_through_associations_test.rb#L104). As you can see this test endorses an opposite policy.

Why hmt is tested twice in the above files? Why it misbehaves, and works both with opposite guidelines?

First: there is a TODO in [has_many_through_association.rb:100](http://github.com/rails/rails/blob/e9a75451236119e1db3e5d7cc7703637d048c7f8/activerecord/lib/active_record/associations/has_many_through_association.rb#L100): TODO: revist this to allow it for deletion, supposing dependent option is supported

Second: the strange thing is the first test case uses Article, Post and Comment classes in the same way are used by Olly, otherwise Post, Person and Reader (second case) are associated like my example above (with the bridge model).

What's the correct behavior?
You flagged this item as spam.
Will Bryant June 4th, 2009 @ 10:36 AM
Yeah, I'm hitting the destroy_all issue too - it certainly works fine in a 2.2 app. I see no reason why it should delete the records it goes through; for example, when we have customers who have projects and they have tasks, and when I destroy_all tasks for a customer, there's no reason to expect the projects should be destroyed too.
Michael Koziarski June 8th, 2009 @ 03:55 AM
- Milestone changed from “2.x” to “2.3.3”
Michael Koziarski June 8th, 2009 @ 03:56 AM
- State changed from “resolved” to “open”
reopening to block 2.3.3
You flagged this item as spam.
Luca Guidi June 8th, 2009 @ 10:02 AM
Ok, I believe we should focus our attention on the nature of hmt.

I think it was introduced as an habmt on steroids association, look at my latest example above, Subscription is a join model, so if an user is being destroyed I expect the related subscriptions should be destroyed too.

In the other hand hmt is actually used as shortcut to avoid the classic train-wreck syntax, as Will has suggested, destroying tasks for a customer's project shouldn't destroy the project too.

In other words, we have different use cases for the same association kind, both are right, and if we satisfy only one of them, we could break existing code.

So my suggestion is to use the ignored dependent option for hmt (http://bit.ly/XNfuG look at the Supported Options section), and destroy records according that value.

If you agree I will take care about the related patch.
Michael Koziarski June 9th, 2009 @ 09:06 AM
- Milestone changed from “2.3.3” to “2.3.4”
OK, I've reverted this from 2-3-stable until we resolve the discussions here. It's still in master.

FWIW I don't consider HMT a join model on steroids. It's merely a convenient accessor, the 'real' association is from user to membership, not user to accounts. The accounts accessor is just for convinience.
You flagged this item as spam.
Luca Guidi June 10th, 2009 @ 10:04 AM
Ok, if hmt is only a convenient accessor, we should revert my original patch from master too. Please look at the description of this ticket (Post, Tagging and Tag), in that case the associated taggings shouldn't be destroyed, even if users could expect so.

Is it right, or am I missing something?
Michael Koziarski June 26th, 2009 @ 06:37 AM
Having thought some more about this I'm not entirely sure what I think should happen...

Basically the hmt exists as a convinient accessor but calling destroy_all is unclear...
For the case of has_many tags through taggings, it's 'obvious' we want to destroy all the taggings. Without the tags they're useless.

However we also support has_many tasks through projects, and for that case destroying the projects is 'obviously' wrong.

I think one option could be only destroying when the :through class has belongs_to associations on either side...

On the other hand perhaps it's safer just to revert this from master too.

Anyone else had any thoughts on the matter?
You flagged this item as spam.
Will Bryant July 3rd, 2009 @ 12:39 AM
I think that's a great idea - if Task.belongs_to :project, :dependent => destroy, then customer.tasks.destroy_all should destroy all projects too.

Otherwise - if Task doesn't belong_to :project or if Task.belongs_to :project without :dependent => :destroy, then customer.tasks.destroy_all shouldn't destroy projects too.

That makes it nice and consistent with what should happen if you call destroy on each Task you've loaded individually, with or without using the has_many :through.
Michael Koziarski July 3rd, 2009 @ 01:06 AM
OK, I'm in agreement with will here. Can someone upload a patch to do
this, and then I think we're good to go.

(or does it do this already?)

Luca Guidi July 6th, 2009 @ 11:26 AM

Will, I don't agree: :dependent => :destroy was designed for the opposite use case:

class task < ActiveRecord::Base
  belongs_to :project, :dependent => :destroy
end

project.destroy # => all the associated tasks will be destroyed

Using customer.tasks.destroy_all I don't expect the owner project is being destroyed too.

My idea is to use dependent as flag for hmt:

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







class Company < ActiveRecord::Base
  has_many :projects, :through => :contacts
end

post.tags.destroy_all        # => destroy taggings too
company.projects.destroy_all # => doesn't destroy contacts

As already said that option is actually ignored for hmt ( http://bit.ly/XNfuG ), so we can use it.

You flagged this item as spam.
jamesw August 8th, 2009 @ 07:18 PM
I'm not sure if this ticket is related to the ticket I just filed
https://rails.lighthouseapp.com/projects/8994/tickets/3007-accepts_...

But whether or not it is some comments here have concerned me so I thought I'd take the opportunity to clear something up as far as expected behaviour is concerned on has_many :through

In a normal database scenario it is quite legitimate to have joins to grandchildren that are NOT a many to many join on the join table.

A grandparent has_many parents which in turn have many children
If I wanted to make sure that a product could exist for only one category and a category could only exist for one catalogue I would have a foreign key in my category table for the catalogue and I would have a foreign key in my products table for my category. but I would not have any other foreign keys to make up a many to many join. Yes there is an argument that a category should be able to exist in more than one catalogue and by association a product can also belong in more than one catalogue thus you have a many to many

Many to many joins are cumbersome and are usually avoided in other development environments and used only as a last resort.

Thus my interpretation of the HABTM functionality offered by rails was to meet the many to many join requirement and I can and do use has_many :through for quite a few non many to many joins and I was rather surprised to read here that has_many :through is not really for the purpose I thought it was for and this could partly explain the problem I'm having with accepts_nested_attributes as outlined in my ticket.

If this is the case (and I hope I read the posts wrong) then I have no solution for multi level nested tables that are not a many to many join which seems like totaly madness to me.

It seems that if I can define table relationships in whatever way I want to define them to meet my customers requirements then functionality such as accepts_nested_attributes and has_many through should work consistently and raise errors if I don't have all the foreign keys defined for a many to many and a "different" option should be allowed or accepts_nested_attributes should be intelligent enough to work out that a particular join table being used for a has_many :through is wither a HABTM model or not depending on the foreign keys.

Of course I could have totally misunderstood what I was reading here as a lot of it went straight over the top of my head and I hope that I haven't got this totally wrong.
Jeremy Kemper September 11th, 2009 @ 11:04 PM
- Milestone changed from “2.3.4” to “2.3.6”
[milestone:id#50064 bulk edit command]
Rizwan Reza May 16th, 2010 @ 02:41 AM
- Tag changed from “activerecord, associations, destroy, has_many_through, patch” to “activerecord, associations, bugmash, destroy, has_many_through, patch”
[bulk edit]
Jeremy Kemper May 23rd, 2010 @ 05:54 PM
- Milestone changed from “2.3.6” to “2.3.7”
[bulk edit]
Jeremy Kemper May 24th, 2010 @ 09:40 AM
- Milestone changed from “2.3.7” to “2.3.8”
[bulk edit]
Jeremy Kemper May 25th, 2010 @ 11:45 PM
- Milestone changed from “2.3.8” to “2.3.9”
[bulk edit]
Jeremy Kemper August 30th, 2010 @ 02:28 AM
- Milestone changed from “2.3.9” to “2.3.10”
- Importance changed from “” to “Low”
[bulk edit]