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

Author: johnnyshields

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,69 @@ module InstanceMethods
       # @return [ true/false ] false if record 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
+
+        touches = __extract_touches_from_atomic_sets(field) || {}
+
+        # TODO: this needs to a guard `... if _parent && _association_to_parent.options[:touch]`
+        # However, the `_association_to_parent` method doesn't exist!
+        touches.merge!(_parent.__gather_touch_updates(now) || {}) if _parent
+        touches
+      end
 
-        # Callbacks are invoked on the composition root first and on the
-        # leaf-most embedded document last.
+      # 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 _parent
         run_callbacks(:touch)
-        true
+      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 = %w(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
 
@@ -80,9 +103,14 @@ def define_touchable!(association)
       name = association.name
       method_name = define_relation_touch_method(name, association)
       association.inverse_class.tap do |klass|
-        klass.after_save method_name
-        klass.after_destroy method_name
-        klass.after_touch method_name
+        # TODO: for EMBEDDED docs, to ensure synchronized timestamps,
+        # we should call .touch within the save/destroy
+        # action rather than as a callback 
+        klass.after_save(method_name)
+        klass.after_destroy(method_name)
+
+        # Embedded docs recursively handle touch updates within the #touch method itself
+        klass.after_touch(method_name) unless association.embedded?
       end
     end
 

spec/mongoid/touchable_spec.rb

@@ -142,11 +142,11 @@
             building.reload
 
             # This is the assertion we want.
-            entrance.last_used_at.should == update_time
+            expect(entrance.last_used_at).to eq update_time
 
             # Check other timestamps for good measure.
-            entrance.updated_at.should == update_time
-            building.updated_at.should == update_time
+            expect(entrance.updated_at).to eq update_time
+            expect(building.updated_at).to eq update_time
           end
         end
       end