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

5 files changed, 102 insertions, 3 deletions

diff --git a/activerecord/CHANGELOG b/activerecord/CHANGELOG
index 0f4fabe293..056c1bf2eb 100644
--- a/activerecord/CHANGELOG
+++ b/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]
diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb
index 9d821d17e9..168719060d 100755
--- a/activerecord/lib/active_record.rb
+++ b/activerecord/lib/active_record.rb
@@ -46,6 +46,7 @@ require 'active_record/acts/tree'
 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 @@ ActiveRecord::Base.class_eval do
   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'
diff --git a/activerecord/lib/active_record/recursion.rb b/activerecord/lib/active_record/recursion.rb
new file mode 100644
index 0000000000..36589238e7
--- /dev/null
+++ b/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
diff --git a/activerecord/lib/active_record/validations.rb b/activerecord/lib/active_record/validations.rb
index 3eedd0093c..d58502e98b 100755
--- a/activerecord/lib/active_record/validations.rb
+++ b/activerecord/lib/active_record/validations.rb
@@ -653,7 +653,7 @@ module ActiveRecord
     # 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.
diff --git a/activerecord/test/callbacks_test.rb b/activerecord/test/callbacks_test.rb
index 391ce5871a..2e3bf54a86 100644
--- a/activerecord/test/callbacks_test.rb
+++ b/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 @@ class CallbackDeveloper < ActiveRecord::Base
   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 @@ class CallbacksTest < Test::Unit::TestCase
       [ :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