MONGOID-5016 default embedded touch to true (#5554)

* first steps toward a `touch: true` default for embedded_in relations

* add touch suppression so that `touch: false` can override `touch: true`

* typo

* refactor the deeply nested callbacks to another method

* update release notes

Author: jamis

docs/release-notes/mongoid-9.0.txt

@@ -191,6 +191,24 @@ persistence operation per parent document, even when using multiple
 levels of nested embedded documents.
 
 
+``embedded_in`` associations now default to ``touch: true``
+-----------------------------------------------------------
+
+Updating an embedded subdocument will now automatically touch the parent,
+unless you explicitly set ``touch: false`` on the relation:
+
+.. code-block:: ruby
+
+  class Address
+    include Mongoid::Document
+    include Mongoid::Timestamps
+
+    embedded_in :mall, touch: false
+  end
+
+For all other associations, the default remains ``touch: false``.
+
+
 Flipped default for ``:replace`` option in ``#upsert``
 ------------------------------------------------------
 

lib/mongoid/association/accessors.rb

@@ -341,12 +341,11 @@ def self.define_setter!(association)
           klass.re_define_method("#{name}=") do |object|
             without_autobuild do
               if value = get_relation(name, association, object)
-                if value.respond_to?(:substitute)
-                  set_relation(name, value.substitute(object.substitutable))
-                else
-                  value = __build__(name, value, association)
-                  set_relation(name, value.substitute(object.substitutable))
+                if !value.respond_to?(:substitute)
+                  value = __build__(name, value, association) 
                 end
+
+                set_relation(name, value.substitute(object.substitutable))
               else
                 __build__(name, object.substitutable, association)
               end

lib/mongoid/association/embedded/embedded_in.rb

@@ -34,6 +34,7 @@ class EmbeddedIn
         #
         # @return [ self ]
         def setup!
+          setup_defaults!
           setup_instance_methods!
           @owner_class.embedded = true
           self
@@ -92,6 +93,11 @@ def nested_builder(attributes, options)
 
         private
 
+        # Set up default values for any options used by this association.
+        def setup_defaults!
+          @options[:touch] = true unless @options.key?(:touch)
+        end
+
         def setup_instance_methods!
           define_getter!
           define_setter!

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

@@ -83,6 +83,15 @@ def substitute(replacement)
               unbind_one
               unless replacement
                 update_attributes_hash(replacement)
+
+                # when `touch: true` is the default (see MONGOID-5016), creating
+                # an embedded document will touch the parent, and will cause the
+                # _descendants list to be initialized and memoized. If the object
+                # is then deleted, we need to make sure and un-memoize that list,
+                # otherwise when the update happens, the memoized _descendants list
+                # gets used and the "deleted" subdocument gets added again.
+                _reset_memoized_descendants!
+
                 return nil
               end
               replacement = Factory.build(klass, replacement) if replacement.is_a?(::Hash)

lib/mongoid/attributes/processing.rb

@@ -116,6 +116,7 @@ def process_nested
       def process_pending
         process_nested and process_relations
         pending_nested.clear and pending_relations.clear
+        _reset_memoized_descendants!
       end
 
       # Process all the pending associations that needed to wait until ids were set

lib/mongoid/persistable/updatable.rb

@@ -102,21 +102,12 @@ def prepare_update(options = {})
           invalid?(options[:context] || :update)
         process_flagged_destroys
         update_children = cascadable_children(:update)
-        process_touch_option(options, update_children)
-        run_callbacks(:commit, with_children: true, skip_if: -> { in_transaction? }) do
-          run_callbacks(:save, with_children: false) do
-            run_callbacks(:update, with_children: false) do
-              run_callbacks(:persist_parent, with_children: false) do
-                _mongoid_run_child_callbacks(:save) do
-                  _mongoid_run_child_callbacks(:update, children: update_children) do
-                    result = yield(self)
-                    self.previously_new_record = false
-                    post_process_persist(result, options)
-                    true
-                  end
-                end
-              end
-            end
+        process_touch_option(options, update_children) do
+          run_all_callbacks_for_update(update_children) do
+            result = yield(self)
+            self.previously_new_record = false
+            post_process_persist(result, options)
+            true
           end
         end
       end
@@ -182,9 +173,12 @@ def update_document(options = {})
       # @option options [ true | false ] :touch Whether or not the updated_at
       #   attribute will be updated with the current time.
       def process_touch_option(options, children)
-        unless options.fetch(:touch, true)
+        if options.fetch(:touch, true)
+          yield
+        else
           timeless
           children.each(&:timeless)
+          suppress_touch_callbacks { yield }
         end
       end
 
@@ -212,6 +206,28 @@ def enforce_immutability_of_id_field!
           end
         end
       end
+
+      # Consolidates all the callback invocations into a single place, to
+      # avoid cluttering the logic in #prepare_update.
+      #
+      # @param [ Array<Document> ] update_children The children that the
+      #   :update callbacks will be executed on.
+      def run_all_callbacks_for_update(update_children)
+        run_callbacks(:commit, with_children: true, skip_if: -> { in_transaction? }) do
+          run_callbacks(:save, with_children: false) do
+            run_callbacks(:update, with_children: false) do
+              run_callbacks(:persist_parent, with_children: false) do
+                _mongoid_run_child_callbacks(:save) do
+                  _mongoid_run_child_callbacks(:update, children: update_children) do
+                    yield
+                  end # _mongoid_run_child_callbacks :update
+                end # _mongoid_run_child_callbacks :save
+              end # :persist_parent
+            end # :update
+          end # :save
+        end # :commit
+      end
+
     end
   end
 end

lib/mongoid/touchable.rb

@@ -2,9 +2,29 @@
 
 module Mongoid
   module Touchable
-
     module InstanceMethods
 
+      # Suppresses the invocation of touch callbacks, for the class that
+      # includes this module, for the duration of the block.
+      #
+      # @example Suppress touch callbacks on Person documents:
+      #   person.suppress_touch_callbacks { ... }
+      #
+      # @api private
+      def suppress_touch_callbacks
+        Touchable.suppress_touch_callbacks(self.class.name) { yield }
+      end
+
+      # Queries whether touch callbacks are being suppressed for the class
+      # that includes this module.
+      #
+      # @return [ true | false ] Whether touch callbacks are suppressed.
+      #
+      # @api private
+      def touch_callbacks_suppressed?
+        Touchable.touch_callbacks_suppressed?(self.class.name)
+      end
+
       # Touch the document, in effect updating its updated_at timestamp and
       # optionally the provided field to the current time. If any belongs_to
       # associations exist with a touch option, they will be updated as well.
@@ -45,7 +65,10 @@ def touch(field = nil)
       #
       # @api private
       def _gather_touch_updates(now, field = nil)
+        return if touch_callbacks_suppressed?
+
         field = database_field_name(field)
+
         write_attribute(:updated_at, now) if respond_to?("updated_at=")
         write_attribute(field, now) if field
 
@@ -71,6 +94,7 @@ def _clear_touch_updates(field = nil)
       #
       # @api private
       def _run_touch_callbacks_from_root
+        return if touch_callbacks_suppressed?
         _parent._run_touch_callbacks_from_root if _touchable_parent?
         run_callbacks(:touch)
       end
@@ -131,8 +155,41 @@ def define_touchable!(association)
       end
     end
 
+    # Suppresses touch callbacks for the named class, for the duration of
+    # the associated block.
+    #
+    # @api private
+    def suppress_touch_callbacks(name)
+      save, touch_callback_statuses[name] = touch_callback_statuses[name], true
+      yield
+    ensure
+      touch_callback_statuses[name] = save
+    end
+
+    # Queries whether touch callbacks are being suppressed for the named
+    # class.
+    #
+    # @return [ true | false ] Whether touch callbacks are suppressed.
+    #
+    # @api private
+    def touch_callbacks_suppressed?(name)
+      touch_callback_statuses[name]
+    end
+
     private
 
+    # The key to use to store the active touch callback suppression statuses
+    SUPPRESS_TOUCH_CALLBACKS_KEY = "[mongoid]:suppress-touch-callbacks"
+
+    # Returns a hash to be used to store and query the various touch callback
+    # suppression statuses for different classes.
+    #
+    # @return [ Hash ] The hash that contains touch callback suppression
+    #   statuses
+    def touch_callback_statuses
+      Thread.current[SUPPRESS_TOUCH_CALLBACKS_KEY] ||= {}
+    end
+
     # Define the method that will get called for touching belongs_to
     # associations.
     #
@@ -153,12 +210,11 @@ def define_relation_touch_method(name, association)
                            [ association.relation_class ]
                          end
 
-      relation_classes.each { |c| c.send(:include, InstanceMethods) }
       method_name = "touch_#{name}_after_create_or_destroy"
       association.inverse_class.class_eval do
         define_method(method_name) do
           without_autobuild do
-            if relation = __send__(name)
+            if !touch_callbacks_suppressed? && relation = __send__(name)
               # 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)

spec/integration/associations/embedded_spec.rb

@@ -178,8 +178,6 @@
         let(:parent) { EomParent.create! }
 
         it 'updates' do
-          pending 'https://jira.mongodb.org/browse/MONGOID-3252'
-
           first_updated_at = parent.updated_at
 
           parent.child = EomChild.new
@@ -193,8 +191,6 @@
         let(:parent) { EmmCongress.create! }
 
         it 'updates' do
-          pending 'https://jira.mongodb.org/browse/MONGOID-3252'
-
           first_updated_at = parent.updated_at
 
           parent.legislators << EmmLegislator.new
@@ -238,8 +234,6 @@
         let(:parent) { EomParent.create!(child: EomChild.new) }
 
         it 'updates' do
-          pending 'https://jira.mongodb.org/browse/MONGOID-3252'
-
           first_updated_at = parent.updated_at
 
           parent.child = nil

spec/mongoid/association/embedded/embedded_in_spec.rb

@@ -156,20 +156,27 @@ class EmbeddedObject; include Mongoid::Document; end
   describe '#touchable?' do
 
     context 'when :touch is in the options' do
+      shared_examples_for ':touch is in the options' do
+        it 'returns the value in the options' do
+          expect(association.send(:touchable?)).to be(options[:touch])
+        end
+      end
 
-      let(:options) do
-        { touch: true}
+      context 'when the option is true' do
+        let(:options) { { touch: true } }
+        it_behaves_like ':touch is in the options'
       end
 
-      it 'returns true' do
-        expect(association.send(:touchable?)).to be(true)
+      context 'when the option is false' do
+        let(:options) { { touch: false } }
+        it_behaves_like ':touch is in the options'
       end
     end
 
     context 'when :touch is not in the options' do
 
-      it 'return false' do
-        expect(association.send(:touchable?)).to be(false)
+      it 'return the default value' do
+        expect(association.send(:touchable?)).to be(true)
       end
     end
   end
@@ -582,7 +589,11 @@ class EmbeddedObject; include Mongoid::Document; end
     context 'when the :class_name option is specified' do
 
       let(:options) do
-        { class_name: 'OtherContainer' }
+        # `touch: true` is the default (see MONGOID-5016), which means by
+        # default, callbacks are added to the referenced class. In this case,
+        # the class does not exist, so we must explicitly set `touch: false`
+        # to prevent Mongoid from attempting to load the bogus class.
+        { touch: false, class_name: 'OtherContainer' }
       end
 
       it 'returns the class name option' do

spec/mongoid/association/embedded/embeds_many_models.rb

@@ -172,7 +172,11 @@ class EmmHatch
   include Mongoid::Document
 
   # No :class_name option on this association intentionally.
-  embedded_in :tank
+  # Also, re: MONGOID-5016, `touch: true` is the default, which means Mongoid
+  # will try to load the associated class in order to add relevant callbacks.
+  # We must set `touch: false` here to avoid Mongoid trying to load a
+  # non-existent class.
+  embedded_in :tank, touch: false
 end
 
 class EmmPost
@@ -235,4 +239,3 @@ class EmmChild
   field :order, type: Integer
   field :t
 end
-