From e0537acaeb6c432db844ff835120de7aabb1e39b Mon Sep 17 00:00:00 2001 From: Jamis Buck Date: Mon, 13 Jun 2005 10:52:53 +0000 Subject: 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 --- activerecord/CHANGELOG | 2 + activerecord/lib/active_record.rb | 2 + activerecord/lib/active_record/recursion.rb | 60 +++++++++++++++++++++++++++ activerecord/lib/active_record/validations.rb | 2 +- activerecord/test/callbacks_test.rb | 39 ++++++++++++++++- 5 files changed, 102 insertions(+), 3 deletions(-) create mode 100644 activerecord/lib/active_record/recursion.rb (limited to 'activerecord') 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 -- cgit v1.2.3