1
0
Fork 0
mirror of https://github.com/rails/rails.git synced 2022-11-09 12:12:34 -05:00

destroy shouldn't raise when child associations fail to save

Deep down in the association internals, we're calling `destroy!` rather
than `destroy` when handling things like `dependent` or autosave
association callbacks. Unfortunately, due to the structure of the code
(e.g. it uses callbacks for everything), it's nearly impossible to pass
whether to call `destroy` or `destroy!` down to where we actually need
it.

As such, we have to do some legwork to handle this. Since the callbacks
are what actually raise the exception, we need to rescue it in
`ActiveRecord::Callbacks`, rather than `ActiveRecord::Persistence` where
it matters. (As an aside, if this code wasn't so callback heavy, it
would handling this would likely be as simple as changing `destroy` to
call `destroy!` instead of the other way around).

Since we don't want to lose the exception when `destroy!` is called (in
particular, we don't want the value of the `record` field to change to
the parent class), we have to do some additional legwork to hold onto it
where we can use it.

Again, all of this is ugly and there is definitely a better way to do
this. However, barring a much more significant re-architecting for what
I consider to be a reletively minor improvement, I'm willing to take
this small hit to the flow of this code (begrudgingly).
This commit is contained in:
Sean Griffin 2015-07-24 09:13:20 -06:00
parent cc214cff7e
commit d937a1175f
4 changed files with 48 additions and 1 deletions

View file

@ -1,3 +1,10 @@
* Don't raise an error if an association failed to destroy when `destroy` was
called on the parent (as opposed to `destroy!`).
Fixes #20991.
*Sean Griffin*
* ActiveRecord::RecordNotFound modified to store model name, primary_key and
id of the caller model. It allows the catcher of this exception to make
a better decision to what to do with it. For example consider this simple

View file

@ -290,6 +290,9 @@ module ActiveRecord
def destroy #:nodoc:
_run_destroy_callbacks { super }
rescue RecordNotDestroyed => e
@_association_destroy_exception = e
false
end
def touch(*) #:nodoc:

View file

@ -193,7 +193,7 @@ module ActiveRecord
# and #destroy! raises ActiveRecord::RecordNotDestroyed.
# See ActiveRecord::Callbacks for further details.
def destroy!
destroy || raise(RecordNotDestroyed.new("Failed to destroy the record", self))
destroy || _raise_record_not_destroyed
end
# Returns an instance of the specified +klass+ with the attributes of the
@ -548,5 +548,12 @@ module ActiveRecord
def verify_readonly_attribute(name)
raise ActiveRecordError, "#{name} is marked as readonly" if self.class.readonly_attributes.include?(name)
end
def _raise_record_not_destroyed
@_association_destroy_exception ||= nil
raise @_association_destroy_exception || RecordNotDestroyed.new("Failed to destroy the record", self)
ensure
@_association_destroy_exception = nil
end
end
end

View file

@ -2278,4 +2278,34 @@ class HasManyAssociationsTest < ActiveRecord::TestCase
assert_deprecated { company.clients_of_firm(true) }
end
class AuthorWithErrorDestroyingAssociation < ActiveRecord::Base
self.table_name = "authors"
has_many :posts_with_error_destroying,
class_name: "PostWithErrorDestroying",
foreign_key: :author_id,
dependent: :destroy
end
class PostWithErrorDestroying < ActiveRecord::Base
self.table_name = "posts"
self.inheritance_column = nil
before_destroy -> { throw :abort }
end
def test_destroy_does_not_raise_when_association_errors_on_destroy
assert_no_difference "AuthorWithErrorDestroyingAssociation.count" do
author = AuthorWithErrorDestroyingAssociation.first
assert_not author.destroy
end
end
def test_destroy_with_bang_bubbles_errors_from_associations
error = assert_raises ActiveRecord::RecordNotDestroyed do
AuthorWithErrorDestroyingAssociation.first.destroy!
end
assert_instance_of PostWithErrorDestroying, error.record
end
end