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

Does not fail replacing children when destroy of a child fails. #12812

Closed
arturopie opened this issue Nov 8, 2013 · 4 comments · Fixed by #13042
Closed

Does not fail replacing children when destroy of a child fails. #12812

arturopie opened this issue Nov 8, 2013 · 4 comments · Fixed by #13042

Comments

@arturopie
Copy link
Contributor

When using a has many association with options autosave: true and dependent: :destroy, and destroying a child fails, replacing all the children will not fail. In fact, it will end up merging new children with the children to which destroy failed

Here is an example test:

class Post < ActiveRecord::Base
  has_many :comments, autosave: true, dependent: :destroy

  attr_accessible :comments
end

class Comment < ActiveRecord::Base
  before_destroy :fail

  def fail
    self.errors.add(:base, "Comment can not be deleted")
    return false
  end
end

class AutosaveTest < ActiveSupport::TestCase
  def test_autosave_replace
    post = Post.create! comments: [Comment.create!]
    orig_comments = post.comments.dup

    post.comments = [Comment.create!]

    assert_equal orig_comments, Post.find(post.id).comments
  end
end

Post.find(post.id).comments will end up returning 2 comments, instead of only one.
I'm using rails 3.2.15

@rafaelfranca
Copy link
Member

Does this works on Rails 4.0.1? Rails 3.2 is not supported anymore

@arturopie
Copy link
Contributor Author

I confirm this is an issue in Rails 4.0.1 as well

@brianstorti
Copy link
Contributor

Do you guys think that raising an ActiveRecord::RecordNotDestroyed when I try to replace the children is enough?

fluxusfrequency pushed a commit to fluxusfrequency/rails that referenced this issue Dec 4, 2013
Fixes rails#12812
Raise `ActiveRecord::RecordNotDestroyed` when a child marked with
`dependent: destroy` can't be destroyed.

The following code:
```ruby
class Post < ActiveRecord::Base
  has_many :comments, dependent: :destroy
end

class Comment < ActiveRecord::Base
  before_destroy do
    return false
  end
end

post = Post.create!(comments: [Comment.create!])
post.comments = [Comment.create!]
````

would result in a `post` with two `comments`.
With this commit, the same code would raise a `RecordNotDestroyed`
exception, keeping the `post` with the same `comment`.
naiyt added a commit to naiyt/rails that referenced this issue Apr 15, 2015
This is a proposal for a possibly better solution to commit rails#13042.
Currently, Rails will raise ActiveRecord::RecordNotDestroyed if you call
destroy on a model, but that model has children with dependent: :destroy
set, and those models fail to be destroyed. This causes issues if you
need to perform some action in the parent's before_destroy regardless of
whether the children fail to be destroyed.

This switches back to calling destroy instead of destroy! on the
children. The original issue (rails#12812) is resolved in this commit by only
raising an ActiveRecord::RecordNotSaved exception when you are replacing
a dependent: :destroy child, and not when calling destroy on the parent.
naiyt added a commit to naiyt/rails that referenced this issue Apr 15, 2015
This is a proposal for a possibly better solution to commit rails#13042.
Currently, Rails will raise ActiveRecord::RecordNotDestroyed if you call
destroy on a model, but that model has children with dependent: :destroy
set, and those models fail to be destroyed. This causes issues if you
need to perform some action in the parent's before_destroy regardless of
whether the children fail to be destroyed.

This switches back to calling destroy instead of destroy! on the
children. The original issue (rails#12812) is resolved in this commit by only
raising an ActiveRecord::RecordNotSaved exception when you are replacing
a dependent: :destroy child, and not when calling destroy on the parent.
@albertopriore
Copy link

Do you have any update on this issue?

naiyt added a commit to naiyt/rails that referenced this issue Mar 1, 2016
This is a proposal for a possibly better solution to commit rails#13042.
Currently, Rails will raise ActiveRecord::RecordNotDestroyed if you call
destroy on a model, but that model has children with dependent: :destroy
set, and those models fail to be destroyed. This causes issues if you
need to perform some action in the parent's before_destroy regardless of
whether the children fail to be destroyed.

This switches back to calling destroy instead of destroy! on the
children. The original issue (rails#12812) is resolved in this commit by only
raising an ActiveRecord::RecordNotSaved exception when you are replacing
a dependent: :destroy child, and not when calling destroy on the parent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants