-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Add support for bidirectional destroy dependencies #18548
Conversation
end | ||
|
||
*Seb Jacobs* | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
8ed0a36
to
d5bf649
Compare
@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 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? |
Ignoring the names for the moment and taking your associations, then it's straightforward enough to call Having said that we should definitely do better at handling it - maybe checking somehow when the association is built? |
@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 |
It might also be worth documenting this issue in the API docs. |
Does this problem only exist when both sides use 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. |
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 The fact that everbody is talking about So if this problem should be "fixed in the documentation" it should be done both in the callback and the association sections.
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.
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 |
We are currently in the works of upgrading a Rails 3.2 to Rails 4.1. We encountered this issue. |
@Bertg Yes, I seem to be having the issue on Rails 4.2.3 |
…y-dependencies Add support for bidirectional destroy dependencies
This is great, thanks! ✨ |
i am glad this was fixed. thanks all |
Will this fix be applied in rails 4.2 or only in rails 5? |
I checked the code. It's only changed in rails 5 |
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
…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
…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
…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
…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
…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
…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
…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
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.
Calling
Content#destroy
orContentPosition#destroy
would result inan 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