Permalink
Browse files

Destroy respects optimistic locking.

Now works with :dependent => :destroy and includes unit tests for that
case.  Also includes better error messages when updating/deleting stale
objects.

[#1966 state:committed]

Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
  • Loading branch information...
1 parent aa401bd commit ce5af2fefe0e447669fa8b7031f07558bfc84f4a @cghawthorne cghawthorne committed with jeremy Feb 22, 2010
View
2 activerecord/CHANGELOG
@@ -1,5 +1,7 @@
*2.3.6 (pending)*
+* Destroy uses optimistic locking. If lock_version on the record you're destroying doesn't match lock_version in the database, a StaleObjectError is raised. #1966 [Curtis Hawthorne]
+
* To prefix the table names of all models in a module, define self.table_name_prefix on the module. #4032 [Andrew White]
* Association inverses for belongs_to, has_one, and has_many. Optimization to reduce database queries. #3533 [Murray Steele]
View
11 activerecord/lib/active_record/associations.rb
@@ -1419,7 +1419,16 @@ def configure_dependency_for_has_many(reflection, extra_conditions = nil)
when :destroy
method_name = "has_many_dependent_destroy_for_#{reflection.name}".to_sym
define_method(method_name) do
- send(reflection.name).each { |o| o.destroy }
+ send(reflection.name).each do |o|
+ # No point in executing the counter update since we're going to destroy the parent anyway
+ counter_method = ('belongs_to_counter_cache_before_destroy_for_' + self.class.name.downcase).to_sym
+ if(o.respond_to? counter_method) then
+ class << o
+ self
+ end.send(:define_method, counter_method, Proc.new {})
+ end
+ o.destroy
+ end
end
before_destroy method_name
when :delete_all
View
35 activerecord/lib/active_record/locking/optimistic.rb
@@ -23,6 +23,16 @@ module Locking
# p2.first_name = "should fail"
# p2.save # Raises a ActiveRecord::StaleObjectError
#
+ # Optimistic locking will also check for stale data when objects are destroyed. Example:
+ #
+ # p1 = Person.find(1)
+ # p2 = Person.find(1)
+ #
+ # p1.first_name = "Michael"
+ # p1.save
+ #
+ # p2.destroy # Raises a ActiveRecord::StaleObjectError
+ #
# You're then responsible for dealing with the conflict by rescuing the exception and either rolling back, merging,
# or otherwise apply the business logic needed to resolve the conflict.
#
@@ -39,6 +49,7 @@ def self.included(base) #:nodoc:
base.lock_optimistically = true
base.alias_method_chain :update, :lock
+ base.alias_method_chain :destroy, :lock
base.alias_method_chain :attributes_from_column_definition, :lock
class << base
@@ -86,7 +97,7 @@ def update_with_lock(attribute_names = @attributes.keys) #:nodoc:
end_sql
unless affected_rows == 1
- raise ActiveRecord::StaleObjectError, "Attempted to update a stale object"
+ raise ActiveRecord::StaleObjectError, "Attempted to update a stale object: #{self.class.name}"
end
affected_rows
@@ -98,6 +109,28 @@ def update_with_lock(attribute_names = @attributes.keys) #:nodoc:
end
end
+ def destroy_with_lock #:nodoc:
+ return destroy_without_lock unless locking_enabled?
+
+ unless new_record?
+ lock_col = self.class.locking_column
+ previous_value = send(lock_col).to_i
+
+ affected_rows = connection.delete(
+ "DELETE FROM #{self.class.quoted_table_name} " +
+ "WHERE #{connection.quote_column_name(self.class.primary_key)} = #{quoted_id} " +
+ "AND #{self.class.quoted_locking_column} = #{quote_value(previous_value)}",
+ "#{self.class.name} Destroy"
+ )
+
+ unless affected_rows == 1
+ raise ActiveRecord::StaleObjectError, "Attempted to delete a stale object: #{self.class.name}"
+ end
+ end
+
+ freeze
+ end
+
module ClassMethods
DEFAULT_LOCKING_COLUMN = 'lock_version'
View
51 activerecord/test/cases/locking_test.rb
@@ -38,6 +38,25 @@ def test_lock_existing
assert_raise(ActiveRecord::StaleObjectError) { p2.save! }
end
+ # See Lighthouse ticket #1966
+ def test_lock_destroy
+ p1 = Person.find(1)
+ p2 = Person.find(1)
+ assert_equal 0, p1.lock_version
+ assert_equal 0, p2.lock_version
+
+ p1.first_name = 'stu'
+ p1.save!
+ assert_equal 1, p1.lock_version
+ assert_equal 0, p2.lock_version
+
+ assert_raises(ActiveRecord::StaleObjectError) { p2.destroy }
+
+ assert p1.destroy
+ assert_equal true, p1.frozen?
+ assert_raises(ActiveRecord::RecordNotFound) { Person.find(1) }
+ end
+
def test_lock_repeating
p1 = Person.find(1)
p2 = Person.find(1)
@@ -150,6 +169,32 @@ def test_readonly_attributes
end
end
end
+
+ # See Lighthouse ticket #1966
+ def test_destroy_dependents
+ # Establish dependent relationship between People and LegacyThing
+ add_counter_column_to(Person, 'legacy_things_count')
+ LegacyThing.connection.add_column LegacyThing.table_name, 'person_id', :integer
+ LegacyThing.reset_column_information
+ LegacyThing.class_eval do
+ belongs_to :person, :counter_cache => true
+ end
+ Person.class_eval do
+ has_many :legacy_things, :dependent => :destroy
+ end
+
+ # Make sure that counter incrementing doesn't cause problems
+ p1 = Person.new(:first_name => 'fjord')
+ p1.save!
+ t = LegacyThing.new(:person => p1)
+ t.save!
+ p1.reload
+ assert_equal 1, p1.legacy_things_count
+ assert p1.destroy
+ assert_equal true, p1.frozen?
+ assert_raises(ActiveRecord::RecordNotFound) { Person.find(p1.id) }
+ assert_raises(ActiveRecord::RecordNotFound) { LegacyThing.find(t.id) }
+ end
def test_quote_table_name
ref = references(:michael_magician)
@@ -168,11 +213,11 @@ def test_update_without_attributes_does_not_only_update_lock_version
private
- def add_counter_column_to(model)
- model.connection.add_column model.table_name, :test_count, :integer, :null => false, :default => 0
+ def add_counter_column_to(model, col='test_count')
+ model.connection.add_column model.table_name, col, :integer, :null => false, :default => 0
model.reset_column_information
# OpenBase does not set a value to existing rows when adding a not null default column
- model.update_all(:test_count => 0) if current_adapter?(:OpenBaseAdapter)
+ model.update_all(col => 0) if current_adapter?(:OpenBaseAdapter)
end
def remove_counter_column_from(model)

0 comments on commit ce5af2f

Please sign in to comment.