Simplified SystemStackError prevention in saving and dirtyness testing

* Removed shared state hash being passed around everywhere, and use
  a temporary ivar that is only in scope when the method is being
  executed.
* Added Resource#run_once private method to prevent a method from
  being called more than once.
* Prevent Resource#freeze from being used in the specs

lib/dm-core/associations/many_to_many.rb

@@ -399,7 +399,7 @@ def _create(safe, attributes)
         end
 
         # @api private
-        def _save(safe, resources = {})
+        def _save(safe)
           if @removed.any?
             # delete only intermediaries linked to the removed targets
             return false unless intermediaries.all(via => @removed).send(safe ? :destroy : :destroy!)

lib/dm-core/associations/one_to_many.rb

@@ -256,11 +256,11 @@ def _create(*)
         end
 
         # @api private
-        def _save(safe, resources = {})
+        def _save(safe)
           assert_source_saved 'The source must be saved before saving the collection'
 
           # update removed resources to not reference the source
-          @removed.all? { |resource| resource.destroyed? || resource.__send__(:_save, safe, resources) } && super
+          @removed.all? { |resource| resource.destroyed? || resource.__send__(safe ? :save : :save!) } && super
         end
 
         # @api private

lib/dm-core/collection.rb

@@ -880,7 +880,7 @@ def clean?
     #
     # @api public
     def dirty?
-      _dirty?
+      loaded_entries.any? { |resource| resource.dirty? } || @removed.any?
     end
 
     # Gets a Human-readable representation of this collection,
@@ -1110,20 +1110,10 @@ def _update(dirty_attributes)
     #   Returns true if collection was updated
     #
     # @api private
-    def _save(safe, resources = {})
+    def _save(safe)
       loaded_entries.each { |resource| set_default_attributes(resource) }
       @removed.clear
-      loaded_entries.all? { |resource| resource.destroyed? || resource.__send__(:_save, safe, resources) }
-    end
-
-    # Checks if any resources have unsaved changes (internal)
-    #
-    # @return [Boolean]
-    #  true if the resources have unsaved changed
-    #
-    # @api private
-    def _dirty?(resources = {})
-      loaded_entries.any? { |resource| resource.__send__(:_dirty?, resources) } || @removed.any?
+      loaded_entries.all? { |resource| resource.destroyed? || resource.__send__(safe ? :save : :save!) }
     end
 
     # Returns default values to initialize new Resources in the Collection

lib/dm-core/resource.rb

@@ -142,7 +142,7 @@ def clean?
     #
     # @api public
     def dirty?
-      _dirty?
+      dirty_self? || dirty_parents? || dirty_children?
     end
 
     # Checks if this Resource instance is readonly
@@ -711,9 +711,7 @@ def parent_relationships
       parent_relationships = []
 
       relationships.each_value do |relationship|
-        next unless relationship.respond_to?(:resource_for) && relationship.loaded?(self)
-        next unless relationship.get(self)
-
+        next unless relationship.respond_to?(:resource_for) && relationship.loaded?(self) && relationship.get!(self)
         parent_relationships << relationship
       end
 
@@ -730,11 +728,7 @@ def child_relationships
       child_relationships = []
 
       relationships.each_value do |relationship|
-        next unless relationship.respond_to?(:collection_for) && relationship.loaded?(self)
-
-        association = relationship.get!(self)
-        next unless association.loaded? || association.head.any? || association.tail.any?
-
+        next unless relationship.respond_to?(:collection_for) && relationship.loaded?(self) && relationship.get!(self)
         child_relationships << relationship
       end
 
@@ -830,13 +824,13 @@ def _update
     end
 
     # @api private
-    def _save(safe, resources = {})
-      return resources[object_id] if resources.key?(object_id)
+    def _save(safe)
+      run_once(true) do
+        method = safe ? :save : :save!
+        assert_not_destroyed(method)
 
-      method = safe ? :save : :save!
-      assert_not_destroyed(method)
-
-      save_parents(safe, resources) && save_self(safe, resources) && save_children(safe, resources)
+        save_parents(safe) && save_self(safe) && save_children(safe)
+      end
     end
 
     # Saves the resource
@@ -845,8 +839,8 @@ def _save(safe, resources = {})
     #   true if the resource was successfully saved
     #
     # @api semipublic
-    def save_self(safe, resources)
-      resources[object_id] = if safe
+    def save_self(safe)
+      if safe
         new? ? create_hook : update_hook
       else
         new? ? _create : _update
@@ -855,22 +849,18 @@ def save_self(safe, resources)
 
     # Saves the parent resources
     #
-    # @param [Hash] resources
-    #   resources that have already been saved
-    #
     # @return [Boolean]
     #   true if the parents were successfully saved
     #
     # @api private
-    def save_parents(safe, resources)
-      return resources[object_id] if resources.key?(object_id)
-      resources[object_id] = true
-
-      parent_relationships.all? do |relationship|
-        parent = relationship.get!(self)
+    def save_parents(safe)
+      run_once(true) do
+        parent_relationships.all? do |relationship|
+          parent = relationship.get!(self)
 
-        if parent.__send__(:save_parents, safe, resources) && parent.__send__(:save_self, safe, resources)
-          relationship.set(self, parent)  # set the FK values
+          if parent.__send__(:save_parents, safe) && parent.__send__(:save_self, safe)
+            relationship.set(self, parent)  # set the FK values
+          end
         end
       end
     end
@@ -881,25 +871,20 @@ def save_parents(safe, resources)
     #   true if the children were successfully saved
     #
     # @api private
-    def save_children(safe, resources)
+    def save_children(safe)
       child_collections.all? do |collection|
-        collection.send(:_save, safe, resources)
+        collection.send(safe ? :save : :save!)
       end
     end
 
-    # @api private
-    def _dirty?(resources = {})
-      dirty_self?(resources) || dirty_parents?(resources) || dirty_children?(resources)
-    end
-
     # Checks if the resource has unsaved changes
     #
     # @return [Boolean]
     #  true if the resource has unsaged changes
     #
     # @api private
-    def dirty_self?(resources)
-      resources[object_id] = if original_attributes.any?
+    def dirty_self?
+      if original_attributes.any?
         true
       elsif new?
         !model.serial.nil? || properties.any? { |property| property.default? }
@@ -914,11 +899,11 @@ def dirty_self?(resources)
     #  true if the parents have unsaved changes
     #
     # @api private
-    def dirty_parents?(resources)
-      return resources[object_id] if resources.key?(object_id)
-
-      parent_resources.any? do |parent|
-        parent.__send__(:dirty_self?, resources) || parent.__send__(:dirty_parents?, resources)
+    def dirty_parents?
+      run_once(false) do
+        parent_resources.any? do |parent|
+          parent.__send__(:dirty_self?) || parent.__send__(:dirty_parents?)
+        end
       end
     end
 
@@ -931,8 +916,8 @@ def dirty_parents?(resources)
     #  true if the children have unsaved changes
     #
     # @api private
-    def dirty_children?(resources)
-      child_collections.any? { |children| children.send(:_dirty?, resources) }
+    def dirty_children?
+      child_collections.any? { |children| children.dirty? }
     end
 
     # Return true if +other+'s is equivalent or equal to +self+'s
@@ -993,5 +978,33 @@ def assert_not_destroyed(method)
         raise PersistenceError, "#{model}##{method} cannot be called on a destroyed resource"
       end
     end
+
+    # Prevent a method from being in the stack more than once
+    #
+    # The purpose of this method is to prevent SystemStackError from
+    # being thrown from methods from encountering infinite recursion
+    # when called on resources having circular dependencies.
+    #
+    # @param [Object] default
+    #   default return value
+    #
+    # @yield The block of code to run once
+    #
+    # @return [Object]
+    #   block return value
+    #
+    # @api private
+    def run_once(default)
+      caller_method = caller(1).first[/`([^'?!]+)[?!]?'/, 1]
+      sentinel      = "@__#{caller_method}_sentinel"
+      return instance_variable_get(sentinel) if instance_variable_defined?(sentinel)
+
+      begin
+        instance_variable_set(sentinel, default)
+        yield
+      ensure
+        remove_instance_variable(sentinel)
+      end
+    end
   end # module Resource
 end # module DataMapper

spec/public/shared/collection_shared_spec.rb

@@ -396,7 +396,7 @@
 
       describe 'on a limited collection' do
         before :all do
-          @other   = @articles.create.freeze
+          @other   = @articles.create
           @limited = @articles.all(:limit => 1)
 
           @return = @limited.send(method)

spec/public/shared/resource_shared_spec.rb

@@ -763,35 +763,38 @@
       describe 'with dirty resources in a has relationship' do
         before :all do
           rescue_if 'TODO: fix for one to one association', !@user.respond_to?(:comments) do
-            @initial_comments = @user.comments.size
-            @first_comment    = @user.comments.create(:body => "DM is great!")
-            @second_comment   = @comment_model.create(:user => @user, :body => "is it really?")
+            @first_comment  = @user.comments.create(:body => 'DM is great!')
+            @second_comment = @comment_model.create(:user => @user, :body => 'is it really?')
 
-            @first_comment.body  = "It still has rough edges"
-            @second_comment.body = "But these cool specs help fixing that"
+            @first_comment.body  = 'It still has rough edges'
+            @second_comment.body = 'But these cool specs help fixing that'
             @second_comment.user = @user_model.create(:name => 'dkubb')
-            @return              = @user.__send__(method)
+
+            @return = @user.__send__(method)
           end
         end
 
-        it 'should save the dirty resources' do
+        it 'should return true' do
           pending_if !@user.respond_to?(:comments) do
             @return.should be_true
           end
         end
 
+        it 'should not be dirty' do
+          @user.should_not be_dirty
+        end
+
         it 'should have saved the first child resource' do
           pending_if !@user.respond_to?(:comments) do
-            @first_comment.should_not be_dirty
+            @first_comment.model.get(*@first_comment.key).body.should == 'It still has rough edges'
           end
         end
 
         it 'should not have saved the second child resource' do
           pending_if !@user.respond_to?(:comments) do
-            @second_comment.should be_dirty
+            @second_comment.model.get(*@second_comment.key).body.should == 'is it really?'
           end
         end
-
       end
 
       describe 'with a new dependency' do

spec/spec_helper.rb

@@ -119,6 +119,7 @@
 module RemoveSend
   def self.included(model)
     model.send(:undef_method, :send)
+    model.send(:undef_method, :freeze)
   end
 
   DataMapper::Model.append_inclusions self