after_destroy isn't called on has_many through #7618

Closed
heaven opened this Issue Sep 12, 2012 · 23 comments

Comments

Projects
None yet
@heaven

heaven commented Sep 12, 2012

Here is the case:

class Project < ActiveRecord::Base
  has_many :object_tags, :as => :taggable, :dependent => :destroy
  has_many :tags, :through => :object_tags
end

class Tag < ActiveRecord::Base
  has_many :object_tags, :dependent => :destroy
end

class ObjectTag < ActiveRecord::Base
  belongs_to :taggable, :polymorphic => true
  belongs_to :tag

  after_create :update_taggable_score
  after_destroy :update_taggable_score
end

The problem is in that the ObjectTag.after_destroy is never called when I am updating Project and unassigning tags. And only being called on Project/Tag .destroy or when manually calling project.tags.destroy(id).

Why not to call destroy_all instead of delete_all? Or how to handle this in any other way? And why then after_create is being called? Something is wrong with this.

UPD:

Here is the ProjectsController:

def update
  begin
    @project.update_attributes!(params[:project])
  rescue
    render :edit
  else
    flash[:success] = "Project has been updated successfully."
    redirect_to admin_project_path(@project)
  end
end

Should I manually synchronize project tags and remove each unassigned tag manually? I have 15 tagged models so performing this in each controller doesn't looks good.

@EricR

This comment has been minimized.

Show comment Hide comment
@EricR

EricR Sep 12, 2012

I might be wrong, but this seems to me like a technicality rather than a bug... You shouldn't call delete_all, because that strategy intentionally doesn't run the associated callback(s). It's also the default strategy that gets used when deleting associated records in a has_many relationship. I'm not sure about destroy_all. You can see the docs here: http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html (Deleting from Associations)

How about Project.object_tags.each(&:destroy). Does that work?

EricR commented Sep 12, 2012

I might be wrong, but this seems to me like a technicality rather than a bug... You shouldn't call delete_all, because that strategy intentionally doesn't run the associated callback(s). It's also the default strategy that gets used when deleting associated records in a has_many relationship. I'm not sure about destroy_all. You can see the docs here: http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html (Deleting from Associations)

How about Project.object_tags.each(&:destroy). Does that work?

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Sep 12, 2012

How about Project.object_tags.each(&:destroy)

Hi, the problem is in that I don't need to remove all object_tags, see http://oi46.tinypic.com/axymt.jpg
So user can add or remove tags and in controller I simply do @profile.update_attributes(params[:profile]). All other ways to solve this require some specific code, that should be repeated in all actions where I add or remove tags (in addition I have many tagged models).

Project.object_tags.each(&:destroy) of course works, as well as @project.tags.destroy(tag_id) or @project.tags.destroy_all, but this is not a solution. The problem is in that rails uses delete_all instead of destroy_all when I am unassigning tags from @project (when doing @project.update_attributes) and this is weird.

UPD:
To avoid callbacks I would simply change the association type from has_many :through to HABTM

heaven commented Sep 12, 2012

How about Project.object_tags.each(&:destroy)

Hi, the problem is in that I don't need to remove all object_tags, see http://oi46.tinypic.com/axymt.jpg
So user can add or remove tags and in controller I simply do @profile.update_attributes(params[:profile]). All other ways to solve this require some specific code, that should be repeated in all actions where I add or remove tags (in addition I have many tagged models).

Project.object_tags.each(&:destroy) of course works, as well as @project.tags.destroy(tag_id) or @project.tags.destroy_all, but this is not a solution. The problem is in that rails uses delete_all instead of destroy_all when I am unassigning tags from @project (when doing @project.update_attributes) and this is weird.

UPD:
To avoid callbacks I would simply change the association type from has_many :through to HABTM

@ashrafuzzaman

This comment has been minimized.

Show comment Hide comment
@ashrafuzzaman

ashrafuzzaman Sep 14, 2012

Heaven,
I am facing the same issue and it hurts :(
If you change the association to HABTM then how can we handle the after_destroy on ObjectTag?

Heaven,
I am facing the same issue and it hurts :(
If you change the association to HABTM then how can we handle the after_destroy on ObjectTag?

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Sep 14, 2012

@ashrafuzzaman Hi, I meant that those who don't need any callback could switch to HABTM, but now I am using has many through especially because I need those callbacks, unfortunately only the after_create one works.

heaven commented Sep 14, 2012

@ashrafuzzaman Hi, I meant that those who don't need any callback could switch to HABTM, but now I am using has many through especially because I need those callbacks, unfortunately only the after_create one works.

@masterkain

This comment has been minimized.

Show comment Hide comment
@masterkain

masterkain Sep 14, 2012

Contributor

It's a problem I have since Rails 3. When it will be fixed it will break a lot of apps.

Contributor

masterkain commented Sep 14, 2012

It's a problem I have since Rails 3. When it will be fixed it will break a lot of apps.

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Sep 20, 2012

If it will be fixed it will help to avoid a lot of ugly hacks (at least for me) :)

heaven commented Sep 20, 2012

If it will be fixed it will help to avoid a lot of ugly hacks (at least for me) :)

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Nov 22, 2012

Hi, is there any chance it will be fixed soon? How can it break existing apps? Perhaps there could be a compromise in introducing this as an additional option for the has many through association? E.g. :destroy => true or something.

heaven commented Nov 22, 2012

Hi, is there any chance it will be fixed soon? How can it break existing apps? Perhaps there could be a compromise in introducing this as an additional option for the has many through association? E.g. :destroy => true or something.

@hungryzi

This comment has been minimized.

Show comment Hide comment
@hungryzi

hungryzi Dec 26, 2012

I had a similar situation earlier today. Digging in to ActiveRecord associations/has_many_through_association#delete_records, I saw that the default strategy of has_many_through is :delete_all. It only calls :destroy_all when we explicitly specify dependent: :destroy on the association.

In your case, the following modification should work. Removing a tag from a project will then destroy the object_tag without destroying the tag itself.

class Project < ActiveRecord::Base
  has_many :object_tags, :as => :taggable, :dependent => :destroy
  has_many :tags, :through => :object_tags, :dependent => :destroy
end

Please let me know if that works for you, I don't have time to try it out.

I had a similar situation earlier today. Digging in to ActiveRecord associations/has_many_through_association#delete_records, I saw that the default strategy of has_many_through is :delete_all. It only calls :destroy_all when we explicitly specify dependent: :destroy on the association.

In your case, the following modification should work. Removing a tag from a project will then destroy the object_tag without destroying the tag itself.

class Project < ActiveRecord::Base
  has_many :object_tags, :as => :taggable, :dependent => :destroy
  has_many :tags, :through => :object_tags, :dependent => :destroy
end

Please let me know if that works for you, I don't have time to try it out.

@heaven

This comment has been minimized.

Show comment Hide comment
@heaven

heaven Dec 26, 2012

@hungryzi Hi, yes, thank you, I am also found the same and this is the exact way how I solved the problem.

Btw, no need to specify :dependent => :destroy on has_many :object_tags. So this works:

  has_many :object_tags, :as => :taggable
  has_many :tags, :through => :object_tags, :dependent => :destroy

But frankly saying it's not very obvious, I always tough that the "dependent" option, specified on has_many :tags, should cause tags to be destroyed, and if I will need to only destroy the association I could add :dependent => :destroy to has_many :object_tags (if I need to remove the relation between taggable record and tag).

So, right now dependent => :destroy specified on has_many :tags only force rails to use destroy instead of delete when removing :object_tags and this is not a habitual behavior for the :dependent option.

heaven commented Dec 26, 2012

@hungryzi Hi, yes, thank you, I am also found the same and this is the exact way how I solved the problem.

Btw, no need to specify :dependent => :destroy on has_many :object_tags. So this works:

  has_many :object_tags, :as => :taggable
  has_many :tags, :through => :object_tags, :dependent => :destroy

But frankly saying it's not very obvious, I always tough that the "dependent" option, specified on has_many :tags, should cause tags to be destroyed, and if I will need to only destroy the association I could add :dependent => :destroy to has_many :object_tags (if I need to remove the relation between taggable record and tag).

So, right now dependent => :destroy specified on has_many :tags only force rails to use destroy instead of delete when removing :object_tags and this is not a habitual behavior for the :dependent option.

@elado

This comment has been minimized.

Show comment Hide comment
@elado

elado Feb 15, 2013

Since after_create is always called but after_destroy isn't, I also find it as an inconsistency bug.

The only solution I have found is to hook an after_remove on the has_many and trigger what I need.

elado commented Feb 15, 2013

Since after_create is always called but after_destroy isn't, I also find it as an inconsistency bug.

The only solution I have found is to hook an after_remove on the has_many and trigger what I need.

@readysetawesome

This comment has been minimized.

Show comment Hide comment
@readysetawesome

readysetawesome May 31, 2013

+1 for doing something about the inconsistency between after_create and after_destroy on join models. It wasted time for my team this week.

+1 for doing something about the inconsistency between after_create and after_destroy on join models. It wasted time for my team this week.

@neerajdotname

This comment has been minimized.

Show comment Hide comment
@neerajdotname

neerajdotname Jun 2, 2013

Member

I can take a look if someone can create executable test case following this pattern. https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb

Member

neerajdotname commented Jun 2, 2013

I can take a look if someone can create executable test case following this pattern. https://github.com/rails/rails/blob/master/guides/bug_report_templates/active_record_master.rb

@KensoDev

This comment has been minimized.

Show comment Hide comment
@KensoDev

KensoDev Aug 30, 2013

Contributor

This is what the remove_records method looks like today.
https://gist.github.com/KensoDev/6387722

From what I see, the possible fix for this is just add the callback before_destroy and after_destroy on the records like this
https://gist.github.com/KensoDev/6387831

Is this a viable fix?

If so, I can add tests around this and push the fix. LMK

Contributor

KensoDev commented Aug 30, 2013

This is what the remove_records method looks like today.
https://gist.github.com/KensoDev/6387722

From what I see, the possible fix for this is just add the callback before_destroy and after_destroy on the records like this
https://gist.github.com/KensoDev/6387831

Is this a viable fix?

If so, I can add tests around this and push the fix. LMK

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Oct 13, 2013

Member

Could you open a PR so we can discuss the solution. Thanks

Member

rafaelfranca commented Oct 13, 2013

Could you open a PR so we can discuss the solution. Thanks

@jerefrer

This comment has been minimized.

Show comment Hide comment
@jerefrer

jerefrer Nov 4, 2013

+1 for what @heaven said, I had the exact same expectation about where to put the :dependent => :destroy.
Took me some time to find this issue and make it work the way I intended.

jerefrer commented Nov 4, 2013

+1 for what @heaven said, I had the exact same expectation about where to put the :dependent => :destroy.
Took me some time to find this issue and make it work the way I intended.

@rahul100885

This comment has been minimized.

Show comment Hide comment
@rahul100885

rahul100885 Dec 4, 2013

Contributor

Facing same issue..
any progress on this?

Contributor

rahul100885 commented Dec 4, 2013

Facing same issue..
any progress on this?

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 4, 2014

Member

Guys I'm considering this issue as stale and I'll close in a month or so. If you still having this issue please provide an example application or an executable issue.

Member

rafaelfranca commented Apr 4, 2014

Guys I'm considering this issue as stale and I'll close in a month or so. If you still having this issue please provide an example application or an executable issue.

@rafaelfranca rafaelfranca added the stale label Apr 4, 2014

@jerefrer

This comment has been minimized.

Show comment Hide comment
@jerefrer

jerefrer Apr 5, 2014

Well it's not really an issue but more of an inconsistency in the framework I would say
Read @heaven reply again : this is not a habitual behavior for the :dependent option.

I really hope this get fixed soon and avoid losing time to more people.

jerefrer commented Apr 5, 2014

Well it's not really an issue but more of an inconsistency in the framework I would say
Read @heaven reply again : this is not a habitual behavior for the :dependent option.

I really hope this get fixed soon and avoid losing time to more people.

@rafaelfranca

This comment has been minimized.

Show comment Hide comment
@rafaelfranca

rafaelfranca Apr 5, 2014

Member

So it sound more like a feature request and we don't accept feature requests in the issue tracker. The behavior can be changed, we only need a PR or someone on the core team wanting to work on this.

I'm closing this issue now.

Member

rafaelfranca commented Apr 5, 2014

So it sound more like a feature request and we don't accept feature requests in the issue tracker. The behavior can be changed, we only need a PR or someone on the core team wanting to work on this.

I'm closing this issue now.

@rafaelfranca rafaelfranca removed the stale label Apr 23, 2014

@nanaya

This comment has been minimized.

Show comment Hide comment
@nanaya

nanaya Apr 25, 2014

Contributor

I wrote a test case showing behavior difference between explicit destroy and implicit destroy (by reassigning) here https://github.com/edogawaconan/rails-hmt-after_destroy

Considering http://guides.rubyonrails.org/association_basics.html actually mentioned this suggestion

You should use has_many :through if you need validations, callbacks, or extra attributes on the join model.

I'd personally call this a bug instead of feature request.

Contributor

nanaya commented Apr 25, 2014

I wrote a test case showing behavior difference between explicit destroy and implicit destroy (by reassigning) here https://github.com/edogawaconan/rails-hmt-after_destroy

Considering http://guides.rubyonrails.org/association_basics.html actually mentioned this suggestion

You should use has_many :through if you need validations, callbacks, or extra attributes on the join model.

I'd personally call this a bug instead of feature request.

@gerep

This comment has been minimized.

Show comment Hide comment
@gerep

gerep Jun 24, 2014

I'm with the same problem running Rails 3.2.16.

@rafaelfranca I think you could re-open this issue since the docs are really clear about it and now you have an example from @edogawaconan to test it, just as you requested

rafaelfranca commented on Apr 4

If you still having this issue please provide an example application or an executable issue.

gerep commented Jun 24, 2014

I'm with the same problem running Rails 3.2.16.

@rafaelfranca I think you could re-open this issue since the docs are really clear about it and now you have an example from @edogawaconan to test it, just as you requested

rafaelfranca commented on Apr 4

If you still having this issue please provide an example application or an executable issue.

@sgrif

This comment has been minimized.

Show comment Hide comment
@sgrif

sgrif Jun 24, 2014

Member

@gerep Rails 3.2 is no longer supported. Can you reproduce the issue on 4.1 or master? If so, please open a new issue.

Member

sgrif commented Jun 24, 2014

@gerep Rails 3.2 is no longer supported. Can you reproduce the issue on 4.1 or master? If so, please open a new issue.

@sgrif sgrif locked and limited conversation to collaborators Jun 24, 2014

fiedl added a commit to fiedl/wingolfsplattform that referenced this issue Oct 14, 2014

Fixing association callbacks for delete_cache.
issue: rails/rails#7618

The issue is circumvented in:
vendor/engines/your_platform/app/models/active_record_associations_patches.rb

trello: https://trello.com/c/cOa9EpLf/677-veranstaltungen

kardeiz added a commit to kardeiz/docrails that referenced this issue Jun 4, 2015

Update association_basics.md
Added a warning for behavior when `:dependent` option is used in conjunction with `:through` option. Text is taken from http://apidock.com/rails/ActiveRecord/Associations/ClassMethods/has_many. I was recently confused by this behavior and overlooked the description in the docs. 

It might be nice to include this warning in the "Methods Added by `has_many`" section as well, but this is where I would have looked first.

For additional context, see:

* rails/rails#7618
* http://stackoverflow.com/questions/30629680/rails-isnt-running-destroy-callbacks-for-has-many-through-join-model

Zequez added a commit to Zequez/FactorioMods that referenced this issue Jul 17, 2015

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.