MONGOID-5274 / MONGOID-5142 - Rework #touch with embedded documents (#5045)

* MONGOID-5142 - Optimize #touch method:
- Perform all touch operations on the document in a single write operation
- Synchronize the timestamp of all touches on the document (parent+child will have same timestamp)
- Remove touches from the atomic set operations, so that they are no longer there when saving the next time.
- Cleanup/refactor touch method to be more readable

* typo

* WIP on fixes

* all but one spec passing...

* Fix final spec

* Add missing code doc

* Fix failing spec

* Update options.rb

* Use Set

* Fix last commit

* Remove unnecessary relation touch conditional

* Use single underscores

* Add additional multi-level specs

* Update lib/mongoid/touchable.rb

Co-authored-by: Neil Shweky <[email protected]>

* MONGOID-5136 explicitly specify touch in models, and fix test bug

* Add release note

* Update docs/release-notes/mongoid-9.0.txt

* MONGOID-5136 fix and format specs

Co-authored-by: shields <[email protected]>
Co-authored-by: Neil Shweky <[email protected]>

Author: johnnyshields

docs/release-notes/mongoid-9.0.txt

@@ -75,6 +75,40 @@ persistence operations was already correctly using the
 ``Mongoid.use_activesupport_time_zone`` setting.
 
 
+```#touch`` method on embedded documents correctly handles ``touch: false`` option
+----------------------------------------------------------------------------------
+
+When the ``touch: false`` option is set on an ``embedded_in`` relation,
+calling the ``#touch`` method on an embedded child document will not
+invoke ``#touch`` on its parent document.
+
+.. code-block:: ruby
+
+  class Address
+    include Mongoid::Document
+    include Mongoid::Timestamps
+
+    embedded_in :mall, touch: false
+  end
+
+  class Mall
+    include Mongoid::Document
+    include Mongoid::Timestamps
+
+    embeds_many :addresses
+  end
+
+  mall = Mall.create!
+  address = mall.addresses.create!
+
+  address.touch
+  #=> updates address.updated_at but not mall.updated_at
+
+In addition, the ``#touch`` method has been optimized to perform one
+persistence operation per parent document, even when using multiple
+levels of nested embedded documents.
+
+
 Flipped default for ``:replace`` option in ``#upsert``
 ------------------------------------------------------
 

lib/mongoid/association/options.rb

@@ -112,8 +112,13 @@ def touch_field
         @touch_field ||= options[:touch] if (options[:touch].is_a?(String) || options[:touch].is_a?(Symbol))
       end
 
-      private
-
+      # Whether the association object should be automatically touched
+      # when its inverse object is updated.
+      #
+      # @return [ true | false ] returns true if this association is
+      #   automatically touched, false otherwise. The default is false.
+      #
+      # @api private
       def touchable?
         !!@options[:touch]
       end

lib/mongoid/atomic.rb

@@ -328,51 +328,5 @@ def generate_atomic_updates(mods, doc)
       mods.add_to_set(doc.atomic_array_add_to_sets)
       mods.pull_all(doc.atomic_array_pulls)
     end
-
-    # Get the atomic updates for a touch operation. Should only include the
-    # updated_at field and the optional extra field.
-    #
-    # @api private
-    #
-    # @example Get the touch atomic updates.
-    #   document.touch_atomic_updates
-    #
-    # @param [ Symbol ] field The optional field.
-    #
-    # @return [ Hash ] The atomic updates.
-    def touch_atomic_updates(field = nil)
-      updates = atomic_updates
-      return {} unless atomic_updates.key?("$set")
-      touches = {}
-      wanted_keys = %w(updated_at u_at)
-      # TODO this permits field to be passed as an empty string in which case
-      # it is ignored, get rid of this behavior.
-      if field.present?
-        wanted_keys << field.to_s
-      end
-      updates["$set"].each_pair do |key, value|
-        if wanted_keys.include?(key.split('.').last)
-          touches.update(key => value)
-        end
-      end
-      { "$set" => touches }
-    end
-
-    # Returns the $set atomic updates affecting the specified field.
-    #
-    # @param [ String ] field The field name.
-    #
-    # @api private
-    def set_field_atomic_updates(field)
-      updates = atomic_updates
-      return {} unless atomic_updates.key?("$set")
-      sets = {}
-      updates["$set"].each_pair do |key, value|
-        if key.split('.').last == field
-          sets.update(key => value)
-        end
-      end
-      { "$set" => sets }
-    end
   end
 end

lib/mongoid/touchable.rb

@@ -22,46 +22,73 @@ module InstanceMethods
       # @return [ true/false ] false if document is new_record otherwise true.
       def touch(field = nil)
         return false if _root.new_record?
-        current = Time.configured.now
+
+        touches = _gather_touch_updates(Time.configured.now, field)
+        _root.send(:persist_atomic_operations, '$set' => touches) if touches.present?
+
+        _run_touch_callbacks_from_root
+        true
+      end
+
+      # Recursively sets touchable fields on the current document and each of its
+      # parents, including the root node. Returns the combined atomic $set
+      # operations to be performed on the root document.
+      #
+      # @param [ Time ] now The timestamp used for synchronizing the touched time.
+      # @param [ Symbol ] field The name of an additional field to update.
+      #
+      # @return [ Hash<String, Time> ] The touch operations to perform as an atomic $set.
+      #
+      # @api private
+      def _gather_touch_updates(now, field = nil)
         field = database_field_name(field)
-        write_attribute(:updated_at, current) if respond_to?("updated_at=")
-        write_attribute(field, current) if field
-
-        # If the document being touched is embedded, touch its parents
-        # all the way through the composition hierarchy to the root object,
-        # because when an embedded document is changed the write is actually
-        # performed by the composition root. See MONGOID-3468.
-        if _parent
-          # This will persist updated_at on this document as well as parents.
-          # TODO support passing the field name to the parent's touch method;
-          # I believe it should be read out of
-          # _association.inverse_association.options but inverse_association
-          # seems to not always/ever be set here. See MONGOID-5014.
-          _parent.touch
-
-          if field
-            # If we are told to also touch a field, perform a separate write
-            # for that field. See MONGOID-5136.
-            # In theory we should combine the writes, which would require
-            # passing the fields to be updated to the parents - MONGOID-5142.
-            sets = set_field_atomic_updates(field)
-            selector = atomic_selector
-            _root.collection.find(selector).update_one(positionally(selector, sets), session: _session)
-          end
-        else
-          # If the current document is not embedded, it is composition root
-          # and we need to persist the write here.
-          touches = touch_atomic_updates(field)
-          unless touches["$set"].blank?
-            selector = atomic_selector
-            _root.collection.find(selector).update_one(positionally(selector, touches), session: _session)
-          end
-        end
+        write_attribute(:updated_at, now) if respond_to?("updated_at=")
+        write_attribute(field, now) if field
 
-        # Callbacks are invoked on the composition root first and on the
-        # leaf-most embedded document last.
+        touches = _extract_touches_from_atomic_sets(field) || {}
+        touches.merge!(_parent._gather_touch_updates(now) || {}) if _touchable_parent?
+        touches
+      end
+
+      # Recursively runs :touch callbacks for the document and its parents,
+      # beginning with the root document and cascading through each successive
+      # child document.
+      #
+      # @api private
+      def _run_touch_callbacks_from_root
+        _parent._run_touch_callbacks_from_root if _touchable_parent?
         run_callbacks(:touch)
-        true
+      end
+
+      # Indicates whether the parent exists and is touchable.
+      #
+      # @api private
+      def _touchable_parent?
+        _parent && _association&.inverse_association&.touchable?
+      end
+
+      private
+
+      # Extract and remove the atomic updates for the touch operation(s)
+      # from the currently enqueued atomic $set operations.
+      #
+      # @param [ Symbol ] field The optional field.
+      #
+      # @return [ Hash ] The field-value pairs to update atomically.
+      #
+      # @api private
+      def _extract_touches_from_atomic_sets(field = nil)
+        updates = atomic_updates['$set']
+        return {} unless updates
+
+        touchable_keys = Set['updated_at', 'u_at']
+        touchable_keys << field.to_s if field.present?
+
+        updates.keys.each_with_object({}) do |key, touches|
+          if touchable_keys.include?(key.split('.').last)
+            touches[key] = updates.delete(key)
+          end
+        end
       end
     end
 
@@ -82,7 +109,10 @@ def define_touchable!(association)
       association.inverse_class.tap do |klass|
         klass.after_save method_name
         klass.after_destroy method_name
-        klass.after_touch method_name
+
+        # Embedded docs handle touch updates recursively within
+        # the #touch method itself
+        klass.after_touch method_name unless association.embedded?
       end
     end
 
@@ -114,13 +144,9 @@ def define_relation_touch_method(name, association)
         define_method(method_name) do
           without_autobuild do
             if relation = __send__(name)
-              if association.touch_field
-                # Note that this looks up touch_field at runtime, rather than
-                # at method definition time.
-                relation.touch association.touch_field
-              else
-                relation.touch
-              end
+              # This looks up touch_field at runtime, rather than at method definition time.
+              # If touch_field is nil, it will only touch the default field (updated_at).
+              relation.touch(association.touch_field)
             end
           end
         end

spec/integration/callbacks_models.rb

@@ -23,7 +23,7 @@ class Star
   include Mongoid::Document
   include Mongoid::Timestamps
 
-  embedded_in :galaxy
+  embedded_in :galaxy, touch: true
 
   field :age, type: Integer
   field :was_touched_after_parent, type: Mongoid::Boolean, default: false
@@ -47,7 +47,7 @@ class Planet
   include Mongoid::Document
   include Mongoid::Timestamps
 
-  embedded_in :star
+  embedded_in :star, touch: true
 
   field :age, type: Integer
   field :was_touched_after_parent, type: Mongoid::Boolean, default: false