New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for bidirectional destroy dependencies #18548

Merged
merged 1 commit into from Oct 28, 2015

Conversation

Projects
None yet
@sebjacobs
Copy link
Contributor

sebjacobs commented Jan 16, 2015

Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
destroy would result in an infinite callback loop.

Take the following relationship.

class Content < ActiveRecord::Base
  has_one :content_position, dependent: :destroy
end

class ContentPosition < ActiveRecord::Base
  belongs_to :content, dependent: :destroy
end

Calling Content#destroy or ContentPosition#destroy would result in
an infinite callback loop.

This commit changes the behaviour of ActiveRecord::Callbacks#destroy
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] #13609

end

*Seb Jacobs*

This comment has been minimized.

@vipulnsward

vipulnsward Jan 16, 2015

Member

New CHANGELOG should be at the top of the file

This comment has been minimized.

@sebjacobs
Add support for bidirectional destroy dependencies
Prior to this commit if you defined a bidirectional relationship
between two models with destroy dependencies on both sides, a call to
`destroy` would result in an infinite callback loop.

Take the following relationship.

    class Content < ActiveRecord::Base
      has_one :content_position, dependent: :destroy
    end

    class ContentPosition < ActiveRecord::Base
      belongs_to :content, dependent: :destroy
    end

Calling `Content#destroy` or `ContentPosition#destroy` would result in
an infinite callback loop.

This commit changes the behaviour of `ActiveRecord::Callbacks#destroy`
so that it guards against subsequent callbacks.

Thanks to @zetter for demonstrating the issue with failing tests[1].

[1] #13609

@sebjacobs sebjacobs force-pushed the sebjacobs:support-bidirectional-destroy-dependencies branch to d5bf649 Jan 16, 2015

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Jan 16, 2015

@sebjacobs I'm not sure whether this is a bug in how people are modelling their domain or whether it's a behaviour that needs to work. In both your example and the original issue's example the models shouldn't have the :dependent option on the belongs_to association. Why would you initiate the delete on the Content::Position model? It seems a contrived example.

Similarly the example in the original issue has an address at the other end of a has many through which gets deleted when the link is deleted - this suggests that the address should be a direct has many to the user.

@rafaelfranca @sgrif wdyt?

@pixeltrix

This comment has been minimized.

Copy link
Member

pixeltrix commented Jan 16, 2015

Ignoring the names for the moment and taking your associations, then it's straightforward enough to call @position.content.destroy. I think when deleting object graphs like this you need to have clear entry points for deletion - allowing cascading deletes from what is essentially a leaf node is asking for trouble.

Having said that we should definitely do better at handling it - maybe checking somehow when the association is built?

@sebjacobs

This comment has been minimized.

Copy link
Contributor

sebjacobs commented Jan 16, 2015

@pixeltrix you are correct the example is rather contrived.

Perhaps the example below demonstrates a more realistic use case.

create_table "courses" do |t|
  t.string   "slug"
end

create_table "steps" do |t|
  t.integer  "course_id"
  t.integer  "position"
  t.string   "content_type"
  t.integer  "content_id"
end

create_table "articles" do |t|
  t.string   "title"
  t.string   "short_description"
end

create_table "video_articles" do |t|
  t.string   "title"
  t.string   "short_description"
  t.integer  "video_id"
end

class Course < ActiveRecord::Base
  has_many :steps
end

class Step < ActiveRecord::Base
  belongs_to :course
  belongs_to :content, polymorphic: true, dependent: :destroy
end

module Content
  has_one :step, as: :content, dependent: :destroy
end

class Article < ActiveRecord::Base
  include Content
end

class VideoArticle < ActiveRecord::Base
  include Content
end
@sebjacobs

This comment has been minimized.

Copy link
Contributor

sebjacobs commented Jan 16, 2015

Having said that we should definitely do better at handling it - maybe checking somehow when the association is built?

It might also be worth documenting this issue in the API docs.

@egilburg

This comment has been minimized.

Copy link
Contributor

egilburg commented Jan 16, 2015

Does this problem only exist when both sides use dependent: :destroy, or also when one side uses dependent: :destroy and the other side has a manual destroy before/after hook?

Because I can think of the following example:

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

class Comment < ActiveRecord::Base
  belongs_to :post

  after_destroy do
    post.destroy if post.comments.none?
  end
end

(The idea being, you are modelling a message board post+comment, where the post contains the overall thread subject, but the body of a new user-submitted post is saved as the first comment created at same time as the post itself. Then, either the whole post can get deleted, or all the comments can get deleted, and if they do, the parent post should be implicitly deleted as well.

@daniel-rikowski

This comment has been minimized.

Copy link

daniel-rikowski commented Feb 3, 2015

Is there something I can do to help with this issue? With every new Rails version I have to pull the latest workaround from #13609 😫

@egilburg Yes, the problem is not restricted to dependent: :destroy, but actually affects all after_destroy callbacks. (dependent: :destroy implicitly adds an after_destroy callback) Your code is a prime example for this problem, both callbacks will be called forever, unless one side stops calling destroy. (Which is what @sebjacobs code does, by checking if the callback has been called already)

The fact that everbody is talking about dependent: :destroy is a little bit misleading. @sebjacobs actually fixed a circular callback problem, which just often surfaces when using circular dependencies.

So if this problem should be "fixed in the documentation" it should be done both in the callback and the association sections.

@pixeltrix

I think when deleting object graphs like this you need to have clear entry points for deletion

I can understand your point of view but one could argue the same for circular auto-save associations (and their callbacks!), which are perfecly handled by Rails.
In other words: Why should save callbacks have "loop protection" but destroy callbacks not?

Having said that we should definitely do better at handling it - maybe checking somehow when the association is built?

When first investigating this problem I tried to do that, but it didn't work because the problem is not limited to associations: Even if it were possible, it would still not help with manual after_destroy callbacks.

@Bertg

This comment has been minimized.

Copy link
Contributor

Bertg commented Jun 30, 2015

We are currently in the works of upgrading a Rails 3.2 to Rails 4.1. We encountered this issue.
Does anyone know if this has been dealt with in Rails 4.2?

@stevehanson

This comment has been minimized.

Copy link

stevehanson commented Aug 13, 2015

@Bertg Yes, I seem to be having the issue on Rails 4.2.3

@sgrif sgrif merged commit d5bf649 into rails:master Oct 28, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

sgrif added a commit that referenced this pull request Oct 28, 2015

Merge pull request #18548 from sebjacobs/support-bidirectional-destro…
…y-dependencies

Add support for bidirectional destroy dependencies
@balexand

This comment has been minimized.

Copy link
Contributor

balexand commented Oct 28, 2015

This is great, thanks!

@arthurnn

This comment has been minimized.

Copy link
Member

arthurnn commented Oct 28, 2015

i am glad this was fixed. thanks all

@fernandomm

This comment has been minimized.

Copy link

fernandomm commented Jan 7, 2016

Will this fix be applied in rails 4.2 or only in rails 5?

@NielsKSchjoedt

This comment has been minimized.

Copy link

NielsKSchjoedt commented Jan 26, 2016

I checked the code. It's only changed in rails 5

dnarkiewicz pushed a commit to GSA/voc_admin that referenced this pull request Oct 6, 2017

Juan Alvarado
Remove dependent destroy circular dependency
rails/rails#13609

Having dependent: :destroy on both the question_content and
survey_element for the assetable polymorphic belongs_to causes a
loop in the destroy callbacks for rails 4.0

Removing the dependent :destroy solves the issue for 4.0, but they
should be added back in for the move to rails 4.1 as the issue is fixed
in rails.

rails/rails#18548

andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Jun 10, 2018

Fix `SystemStackError` when paranoid models have a circular `dependen…
…t: :destroy`

- Previously, paranoid models with a circular `dependent: :destroy` would recursively try to delete eachother
- Add a variable to check if the destroy callback has already been
called for that model
- Similar fix to rails/rails#18548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment