Fix MONGOID-5198 Calling children_changed? on a deep cyclical data structure will cause semi-infinite looping

Author: p

lib/mongoid/association/embedded/embeds_many/proxy.rb

@@ -72,7 +72,7 @@ def build(attributes = {}, type = nil)
             doc.apply_post_processed_defaults
             yield(doc) if block_given?
             doc.run_callbacks(:build) { doc }
-            _base._reset_memoized_children!
+            _base._reset_memoized_descendants!
             doc
           end
 

lib/mongoid/association/embedded/embeds_one/proxy.rb

@@ -31,7 +31,7 @@ def initialize(base, target, association)
               characterize_one(_target)
               bind_one
               characterize_one(_target)
-              _base._reset_memoized_children!
+              _base._reset_memoized_descendants!
               _target.save if persistable?
             end
           end

lib/mongoid/atomic.rb

@@ -120,7 +120,7 @@ def atomic_updates(_use_indexes = false)
       process_flagged_destroys
       mods = Modifiers.new
       generate_atomic_updates(mods, self)
-      _children.each do |child|
+      _descendants.each do |child|
         child.process_flagged_destroys
         generate_atomic_updates(mods, child)
       end

lib/mongoid/changeable.rb

@@ -28,8 +28,7 @@ def changed?
 
     # Have any children (embedded documents) of this document changed?
     #
-    # @example Have any children changed?
-    #   model.children_changed?
+    # @note This intentionally only considers children and not descendants.
     #
     # @return [ true, false ] If any children have changed.
     def children_changed?
@@ -81,7 +80,7 @@ def move_changes
     # @example Handle post persistence.
     #   document.post_persist
     def post_persist
-      reset_persisted_children
+      reset_persisted_descendants
       move_changes
     end
 

lib/mongoid/persistable/creatable.rb

@@ -83,7 +83,7 @@ def insert_as_root
       # @return [ true ] true.
       def post_process_insert
         self.new_record = false
-        flag_children_persisted
+        flag_descendants_persisted
         true
       end
 

lib/mongoid/traversable.rb

@@ -115,49 +115,82 @@ def self.get_discriminator_mapping(value)
       end
     end
 
-    # Get all child +Documents+ to this +Document+, going n levels deep if
-    # necessary. This is used when calling update persistence operations from
+    # Get all child +Documents+ to this +Document+
+    #
+    # @return [ Array<Document> ] All child documents in the hierarchy.
+    #
+    # @api private
+    def _children
+      @__children ||= collect_children
+    end
+
+    # Get all descendant +Documents+ of this +Document+ recursively.
+    # This is used when calling update persistence operations from
     # the root document, where changes in the entire tree need to be
     # determined. Note that persistence from the embedded documents will
     # always be preferred, since they are optimized calls... This operation
     # can get expensive in domains with large hierarchies.
     #
-    # @example Get all the document's children.
-    #   person._children
+    # @return [ Array<Document> ] All descendant documents in the hierarchy.
     #
-    # @return [ Array<Document> ] All child documents in the hierarchy.
-    def _children
-      @__children ||= collect_children
+    # @api private
+    def _descendants
+      @__descendants ||= collect_descendants
     end
 
     # Collect all the children of this document.
     #
-    # @example Collect all the children.
-    #   document.collect_children
-    #
     # @return [ Array<Document> ] The children.
+    #
+    # @api private
     def collect_children
       children = []
       embedded_relations.each_pair do |name, association|
         without_autobuild do
           child = send(name)
-          Array.wrap(child).each do |doc|
-            children.push(doc)
-            children.concat(doc._children)
-          end if child
+          if child
+            children += Array.wrap(child)
+          end
         end
       end
       children
     end
 
-    # Marks all children as being persisted.
+    # Collect all the descendants of this document.
     #
-    # @example Flag all the children.
-    #   document.flag_children_persisted
+    # @return [ Array<Document> ] The descendants.
+    #
+    # @api private
+    def collect_descendants
+      children = []
+      to_expand = []
+      expanded = {}
+      embedded_relations.each_pair do |name, association|
+        without_autobuild do
+          child = send(name)
+          if child
+            to_expand += Array.wrap(child)
+          end
+        end
+      end
+      until to_expand.empty?
+        expanding = to_expand
+        to_expand = []
+        expanding.each do |child|
+          next if expanded[child]
+          expanded[child] = true
+          children << child
+          to_expand += child._children
+        end
+      end
+      children
+    end
+
+    # Marks all descendants as being persisted.
     #
-    # @return [ Array<Document> ] The flagged children.
-    def flag_children_persisted
-      _children.each do |child|
+    # @return [ Array<Document> ] The flagged descendants.
+    def flag_descendants_persisted
+      _descendants.each do |child|
         child.new_record = false
       end
     end
@@ -204,33 +237,28 @@ def remove_child(child)
       end
     end
 
-    # After children are persisted we can call this to move all their changes
-    # and flag them as persisted in one call.
+    # After descendants are persisted we can call this to move all their
+    # changes and flag them as persisted in one call.
     #
-    # @example Reset the children.
-    #   document.reset_persisted_children
-    #
-    # @return [ Array<Document> ] The children.
-    def reset_persisted_children
-      _children.each do |child|
+    # @return [ Array<Document> ] The descendants.
+    def reset_persisted_descendants
+      _descendants.each do |child|
         child.move_changes
         child.new_record = false
       end
-      _reset_memoized_children!
+      _reset_memoized_descendants!
     end
 
-    # Resets the memoized children on the object. Called internally when an
+    # Resets the memoized descendants on the object. Called internally when an
     # embedded array changes size.
     #
-    # @api semiprivate
-    #
-    # @example Reset the memoized children.
-    #   document._reset_memoized_children!
-    #
     # @return [ nil ] nil.
-    def _reset_memoized_children!
-      _parent._reset_memoized_children! if _parent
+    #
+    # @api private
+    def _reset_memoized_descendants!
+      _parent._reset_memoized_descendants! if _parent
       @__children = nil
+      @__descendants = nil
     end
 
     # Return the root document in the object graph. If the current document

spec/integration/associations/embedded_dirty_spec.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+require_relative '../../mongoid/association/embedded/embeds_many_models'
+require_relative '../../mongoid/association/embedded/embeds_one_models'
+
+describe 'embedded associations' do
+  describe 'dirty tracking' do
+    context 'when association is cyclic' do
+      before do
+        # create deeply nested record
+        a = EmmOuter.create(level: 0)
+        level = 1
+        iter = a.inners.create(level: level)
+        loop do
+          iter.friends.create(level: (level += 1))
+          iter = iter.friends[0]
+          break if level == 40
+        end
+      end
+
+      let(:subject) { EmmOuter.first }
+
+      it 'performs dirty tracking efficiently' do
+        subject.changed?.should be false
+      end
+    end
+  end
+end

spec/mongoid/association/embedded/embeds_many_models.rb

@@ -51,3 +51,19 @@ class EmmProduct
 
   field :name, type: String
 end
+
+class EmmInner
+  include Mongoid::Document
+
+  embeds_many :friends, :class_name => self.name, :cyclic => true
+  embedded_in :parent, :class_name => self.name, :cyclic => true
+
+  field :level, :type => Integer
+end
+
+class EmmOuter
+  include Mongoid::Document
+  embeds_many :inners, class_name: 'EmmInner'
+
+  field :level, :type => Integer
+end

spec/mongoid/traversable_spec.rb

@@ -62,8 +62,72 @@
         expect(person._children).to include(address)
       end
 
+      it "does not include embedded documents multiple levels deep" do
+        expect(person._children).not_to include(location)
+      end
+    end
+  end
+
+  describe "#_descendants" do
+
+    let(:person) do
+      Person.new(title: "King")
+    end
+
+    context "with one level of embedding" do
+
+      let(:name) do
+        Name.new(first_name: "Titus")
+      end
+
+      let(:address) do
+        Address.new(street: "Queen St")
+      end
+
+      before do
+        person.name = name
+        person.addresses << address
+      end
+
+      it "includes embeds_one documents" do
+        expect(person._descendants).to include(name)
+      end
+
+      it "includes embeds_many documents" do
+        expect(person._descendants).to include(address)
+      end
+    end
+
+    context "with multiple levels of embedding" do
+
+      let(:name) do
+        Name.new(first_name: "Titus")
+      end
+
+      let(:address) do
+        Address.new(street: "Queen St")
+      end
+
+      let(:location) do
+        Location.new(name: "Work")
+      end
+
+      before do
+        person.name = name
+        address.locations << location
+        person.addresses << address
+      end
+
+      it "includes embeds_one documents" do
+        expect(person._descendants).to include(name)
+      end
+
+      it "includes embeds_many documents" do
+        expect(person._descendants).to include(address)
+      end
+
       it "includes embedded documents multiple levels deep" do
-        expect(person._children).to include(location)
+        expect(person._descendants).to include(location)
       end
     end
   end