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

+ ActiveRecord::Base#destroy! #6629

Merged
merged 1 commit into from
Jun 6, 2012
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions activerecord/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
## Rails 4.0.0 (unreleased) ##

* Added `#destroy!` which acts like `#destroy` but will raise an
`ActiveRecord::RecordNotDestroyed` exception instead of returning `false`.

*Marc-André Lafortune*

* Allow blocks for `count` with `ActiveRecord::Relation`, to work similar as
`Array#count`:

Expand Down
4 changes: 4 additions & 0 deletions activerecord/lib/active_record/errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ class RecordNotFound < ActiveRecordError
class RecordNotSaved < ActiveRecordError
end

# Raised by ActiveRecord::Base.destroy! when a call to destroy would return false.
class RecordNotDestroyed < ActiveRecordError
end

# Raised when SQL statement cannot be executed by the database (for example, it's often the case for
# MySQL when Ruby driver used is too old).
class StatementInvalid < ActiveRecordError
Expand Down
16 changes: 16 additions & 0 deletions activerecord/lib/active_record/persistence.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ def delete

# Deletes the record in the database and freezes this instance to reflect
# that no changes should be made (since they can't be persisted).
#
# There's a series of callbacks associated with <tt>destroy</tt>. If
# the <tt>before_destroy</tt> callback return +false+ the action is cancelled
# and <tt>destroy</tt> returns +false+. See
# ActiveRecord::Callbacks for further details.
def destroy
raise ReadOnlyRecord if readonly?
destroy_associations
Expand All @@ -130,6 +135,17 @@ def destroy
freeze
end

# Deletes the record in the database and freezes this instance to reflect
# that no changes should be made (since they can't be persisted).
#
# There's a series of callbacks associated with <tt>destroy!</tt>. If
# the <tt>before_destroy</tt> callback return +false+ the action is cancelled
# and <tt>destroy!</tt> raises ActiveRecord::RecordNotDestroyed. See
# ActiveRecord::Callbacks for further details.
def destroy!
destroy || raise(ActiveRecord::RecordNotDestroyed)
end

# Returns an instance of the specified +klass+ with the attributes of the
# current record. This is mostly useful in relation to single-table
# inheritance structures where you want a subclass to appear as the
Expand Down
2 changes: 2 additions & 0 deletions activerecord/test/cases/callbacks_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -426,11 +426,13 @@ def test_before_update_returning_false
def test_before_destroy_returning_false
david = ImmutableDeveloper.find(1)
assert !david.destroy
assert_raise(ActiveRecord::RecordNotDestroyed) { david.destroy! }
assert_not_nil ImmutableDeveloper.find_by_id(1)

someone = CallbackCancellationDeveloper.find(1)
someone.cancel_before_destroy = true
assert !someone.destroy
assert_raise(ActiveRecord::RecordNotDestroyed) { someone.destroy! }

Choose a reason for hiding this comment

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

I think these assertions should go to another test in persistence_test instead, they make more sense there where things like save! are already being tested, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, the test for destroy in persistence_test doesn't test failures. This is only done in callback_test. I added the exact same corresponding tests for destroy!.

Moreover, there is no accessible model that cancels destruction in the callbacks; these are inlined in callback_test.

If you want, I can:

  1. Move some model out of callback_test
  2. Move some destroy testing out of callback_test into persistence_test
  3. Move the corresponding destroy! testing.

These behavior seem pretty intertwined to me (as only callbacks can make destroy return false, unless I'm mistaken), so I think that as long as the test is in either file we're ok.

Thanks

Choose a reason for hiding this comment

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

I don't think it's necessary to do all this moving, I just thought that'd be reasonable to have destroy! closer to save! tests. About callbacks, one could return false from the destroy method itself without touching the callback chain. Thanks!

assert !someone.after_destroy_called
end

Expand Down
7 changes: 7 additions & 0 deletions activerecord/test/cases/persistence_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,13 @@ def test_destroy
assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) }
end

def test_destroy!
topic = Topic.find(1)
assert_equal topic, topic.destroy!, 'topic.destroy! did not return self'
assert topic.frozen?, 'topic not frozen after destroy!'
assert_raise(ActiveRecord::RecordNotFound) { Topic.find(topic.id) }
end

def test_record_not_found_exception
assert_raise(ActiveRecord::RecordNotFound) { Topic.find(99999) }
end
Expand Down