Skip to content
Browse files

Added ActiveRecord::Recursion to guard against recursive calls to #save

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@1411 5ecf4fe2-1ee6-0310-87b1-e25e094e27de
  • Loading branch information...
1 parent 76e4c1a commit e0537acaeb6c432db844ff835120de7aabb1e39b @jamis jamis committed Jun 13, 2005
View
2 activerecord/CHANGELOG
@@ -1,5 +1,7 @@
*SVN*
+* Added ActiveRecord::Recursion for guarding against recursive saves
+
* Fixed sanitized conditions for has_many finder method. #1281 [jackc@hylesanderson.com, pragdave, Tobias Luetke]
* Comprehensive PostgreSQL schema support. Use the optional schema_search_path directive in database.yml to give a comma-separated list of schemas to search for your tables. This allows you, for example, to have tables in a shared schema without having to use a custom table name. See http://www.postgresql.org/docs/8.0/interactive/ddl-schemas.html to learn more. #827 [dave@cherryville.org]
View
2 activerecord/lib/active_record.rb
@@ -46,6 +46,7 @@
require 'active_record/acts/nested_set'
require 'active_record/locking'
require 'active_record/migration'
+require 'active_record/recursion'
ActiveRecord::Base.class_eval do
include ActiveRecord::Validations
@@ -59,6 +60,7 @@
include ActiveRecord::Acts::Tree
include ActiveRecord::Acts::List
include ActiveRecord::Acts::NestedSet
+ include ActiveRecord::Recursion # must go last!
end
require 'active_record/connection_adapters/mysql_adapter'
View
60 activerecord/lib/active_record/recursion.rb
@@ -0,0 +1,60 @@
+module ActiveRecord
+ # Wraps a guard around #save to make sure that recursive calls don't actually
+ # invoke save multiple times. Recursive calls to save can occur quite
+ # easily, and unintentionally. Consider the following case:
+ #
+ # class Project < ActiveRecord::Base
+ # has_and_belongs_to_many :people
+ # after_create :grant_access_to_admins
+ #
+ # def grant_access_to_admins
+ # Person.admins.each do |admin|
+ # admin.projects.push_with_attributes(self, "access_level" => 42)
+ # end
+ # end
+ # end
+ #
+ # class Person < ActiveRecord::Base
+ # has_and_belongs_to_many :projects
+ # ...
+ # end
+ #
+ # teddy = Person.find_by_name("teddy")
+ # project = Project.new :name => "sumo wrestling"
+ # project.people << teddy
+ # project.save!
+ #
+ # The #push_with_attributes causes +self+ (the project) to be saved again,
+ # even though we're already in the midst of doing a save. This results in
+ # "teddy" _not_ being added to the project's people list, because the
+ # recursive call resets the new-record status and thus ignores any
+ # non-new records in the collection.
+ #
+ # Thus, the need for a recursive guard on save.
+ module Recursion
+ def self.append_features(base) # :nodoc:
+ super
+
+ base.class_eval do
+ alias_method :save_without_recursive_guard, :save
+ alias_method :save, :save_with_recursive_guard
+ end
+ end
+
+ # Wrap the save call with a sentinel that prevents saves from occuring if
+ # a save is already in progress.
+ def save_with_recursive_guard(*args)
+ critical = Thread.critical
+ Thread.critical = true
+ old_save_state = @currently_saving_record
+ return true if @currently_saving_record
+ @currently_saving_record = true
+ Thread.critical = critical
+
+ save_without_recursive_guard(*args)
+ ensure
+ Thread.critical = critical
+ @currently_saving_record = old_save_state
+ end
+ end
+end
View
2 activerecord/lib/active_record/validations.rb
@@ -653,7 +653,7 @@ def save_with_validation(perform_validation = true)
# Attempts to save the record just like Base.save but will raise a RecordInvalid exception instead of returning false
# if the record is not valid.
def save!
- valid? ? save_without_validation : raise(RecordInvalid)
+ valid? ? save(false) : raise(RecordInvalid)
end
# Updates a single attribute and saves the record without going through the normal validation procedure.
View
39 activerecord/test/callbacks_test.rb
@@ -1,9 +1,9 @@
require 'abstract_unit'
class CallbackDeveloper < ActiveRecord::Base
- class << self
- def table_name; 'developers' end
+ set_table_name 'developers'
+ class << self
def callback_string(callback_method)
"history << [#{callback_method.to_sym.inspect}, :string]"
end
@@ -49,6 +49,27 @@ def after_find
end
end
+class RecursiveCallbackDeveloper < ActiveRecord::Base
+ set_table_name 'developers'
+
+ before_save :on_before_save
+ after_save :on_after_save
+
+ attr_reader :on_before_save_called, :on_after_save_called
+
+ def on_before_save
+ @on_before_save_called ||= 0
+ @on_before_save_called += 1
+ save unless @on_before_save_called > 1
+ end
+
+ def on_after_save
+ @on_after_save_called ||= 0
+ @on_after_save_called += 1
+ save unless @on_after_save_called > 1
+ end
+end
+
class CallbacksTest < Test::Unit::TestCase
fixtures :developers
@@ -283,4 +304,18 @@ def test_zzz_callback_returning_false # must be run last since we modify Callbac
[ :before_validation, :returning_false ]
], david.history
end
+
+ def test_save_not_called_recursively
+ david = RecursiveCallbackDeveloper.find(1)
+ david.save
+ assert_equal 1, david.on_before_save_called
+ assert_equal 1, david.on_after_save_called
+ end
+
+ def test_save_bang_not_called_recursively
+ david = RecursiveCallbackDeveloper.find(1)
+ david.save!
+ assert_equal 1, david.on_before_save_called
+ assert_equal 1, david.on_after_save_called
+ end
end

0 comments on commit e0537ac

Please sign in to comment.
Something went wrong with that request. Please try again.