Skip to content
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

Conversation

sebjacobs
Copy link
Contributor

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*

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New CHANGELOG should be at the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vipulnsward fixed in d5bf649.

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] rails#13609
@sebjacobs sebjacobs force-pushed the support-bidirectional-destroy-dependencies branch from 8ed0a36 to d5bf649 Compare January 16, 2015 11:42
@pixeltrix
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link

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
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
Copy link
Contributor

@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
sgrif added a commit that referenced this pull request Oct 28, 2015
…y-dependencies

Add support for bidirectional destroy dependencies
@balexand
Copy link
Contributor

This is great, thanks! ✨

@arthurnn
Copy link
Member

i am glad this was fixed. thanks all

@fernandomm
Copy link

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

@NielsKSchjoedt
Copy link

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
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
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Aug 23, 2019
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Aug 23, 2019
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Aug 23, 2019
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Aug 23, 2019
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Sep 13, 2019
…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
andyklimczak added a commit to andyklimczak/paranoia that referenced this pull request Sep 13, 2019
…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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet