MONGOID-5782 Add block-based Timeless API to fix cascaded timestamp leak

.github/workflows/test.yml

@@ -36,7 +36,7 @@ jobs:
 
     - id: start-mongodb
       name: start mongodb
-      uses: mongodb-labs/drivers-evergreen-tools@b26580ac7b03a7eb282053871da0a050c7075288
+      uses: mongodb-labs/drivers-evergreen-tools@18aed4f176dab1ed505c9a2a53466ddf3f4796ec
       with:
         version: "${{matrix.mongodb}}"
         topology: "${{matrix.topology}}"

lib/mongoid/persistable/updatable.rb

@@ -103,7 +103,7 @@ def prepare_update(options = {})
 
         process_flagged_destroys
         update_children = cascadable_children(:update)
-        process_touch_option(options, update_children) do
+        process_touch_option(options) do
           run_all_callbacks_for_update(update_children) do
             result = yield(self)
             self.previously_new_record = false
@@ -160,24 +160,22 @@ def update_document(options = {})
         end
       end
 
-      # If there is a touch option and it is false, this method will call the
-      # timeless method so that the updated_at attribute is not updated. It
-      # will call the timeless method on all of the cascadable children as
-      # well. Note that timeless is cleared in the before_update callback.
+      # If there is a touch option and it is false, this method suppresses
+      # timestamping for the duration of the update using a block-based
+      # timeless scope, which covers this document and every cascaded child
+      # (at any nesting depth), and also suppresses touch callbacks.
       #
       # @param [ Hash ] options The options.
-      # @param [ Array<Document> ] children The children that the :update
-      #   callbacks will be executed on.
       #
       # @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, &block)
+      def process_touch_option(options, &block)
         if options.fetch(:touch, true)
           yield
         else
-          timeless
-          children.each(&:timeless)
-          suppress_touch_callbacks(&block)
+          Mongoid::Timestamps::Timeless.with_timeless do
+            suppress_touch_callbacks(&block)
+          end
         end
       end
 

lib/mongoid/timestamps/timeless.rb

@@ -7,6 +7,10 @@ module Timestamps
     module Timeless
       extend ActiveSupport::Concern
 
+      # Deprecator for the block-less form of the timeless API. Its removal
+      # horizon is computed automatically as (current major + 1).0.
+      DEPRECATION = Mongoid::Deprecation.new
+
       # Clears out the timeless option.
       #
       # @example Clear the timeless option.
@@ -22,13 +26,20 @@ def clear_timeless_option
         true
       end
 
-      # Begin an execution that should skip timestamping.
+      # Skip timestamping for the duration of the given block, or (in the
+      # deprecated, block-less form) for the next persistence operation.
+      #
+      # @example Save a document but don't timestamp (block form).
+      #   person.timeless { person.save }
       #
-      # @example Save a document but don't timestamp.
+      # @example Save a document but don't timestamp (deprecated chained form).
       #   person.timeless.save
       #
-      # @return [ Document ] The document this was called on.
-      def timeless
+      # @return [ Object | Document ] The return value of the block, or (in the
+      #   block-less form) the document this was called on.
+      def timeless(&block)
+        return Timeless.with_timeless(&block) if block
+
         self.class.timeless
         self
       end
@@ -47,6 +58,9 @@ class << self
         # The key to use to store the timeless table
         TIMELESS_TABLE_KEY = '[mongoid]:timeless'
 
+        # The key to use to store the block-based timeless flag.
+        TIMELESS_FLAG_KEY = '[mongoid]:timeless-flag'
+
         # Returns the in-memory thread cache of classes
         # for which to skip timestamping.
         #
@@ -58,16 +72,70 @@ def timeless_table
         end
 
         def_delegators :timeless_table, :[]=, :[]
+
+        # Skip timestamping for the duration of the given block, on the
+        # current thread or fiber. This applies to every document persisted
+        # while the block is executing, regardless of class, including
+        # cascaded embedded children at any nesting depth.
+        #
+        # @example Skip timestamping for a block.
+        #   Mongoid::Timestamps::Timeless.with_timeless do
+        #     person.save
+        #   end
+        #
+        # @return [ Object ] The return value of the block.
+        def with_timeless
+          # Only the outermost block owns the flag: if we are already inside a
+          # timeless scope, we leave the suppression in place when this block
+          # ends. This avoids tracking a nesting depth that could drift out of
+          # sync.
+          already_timeless = suppressing_timestamps?
+          set_suppressing_timestamps(true) unless already_timeless
+          yield
+        ensure
+          set_suppressing_timestamps(false) unless already_timeless
+        end
+
+        # Whether a block-based timeless scope is currently active on this
+        # thread/fiber.
+        #
+        # @return [ true | false ] Whether timestamps are being suppressed.
+        #
+        # @api private
+        def suppressing_timestamps?
+          !!Threaded.get(TIMELESS_FLAG_KEY) { false }
+        end
+
+        # Set whether a block-based timeless scope is active on this
+        # thread/fiber.
+        #
+        # @param [ true | false ] value Whether to suppress timestamps.
+        #
+        # @api private
+        def set_suppressing_timestamps(value)
+          Threaded.set(TIMELESS_FLAG_KEY, value)
+        end
       end
 
       module ClassMethods
-        # Begin an execution that should skip timestamping.
+        # Skip timestamping for the duration of the given block, or (in the
+        # deprecated, block-less form) for the next persistence operation.
         #
-        # @example Create a document but don't timestamp.
+        # @example Create a document but don't timestamp (block form).
+        #   Person.timeless { Person.create(title: "Sir") }
+        #
+        # @example Create a document but don't timestamp (deprecated form).
         #   Person.timeless.create(:title => "Sir")
         #
-        # @return [ Class ] The class this was called on.
-        def timeless
+        # @return [ Object | Class ] The return value of the block, or (in the
+        #   block-less form) the class this was called on.
+        def timeless(&block)
+          return Timeless.with_timeless(&block) if block
+
+          DEPRECATION.warn(
+            'Calling #timeless without a block is deprecated; pass a block ' \
+            'instead, e.g. `record.timeless { record.save }`.'
+          )
           counter = 0
           counter += 1 if self < Mongoid::Timestamps::Created
           counter += 1 if self < Mongoid::Timestamps::Updated
@@ -109,12 +177,14 @@ def set_timeless_counter(counter)
           Timeless[name] = (counter == 0) ? nil : counter
         end
 
-        # Returns whether the current class should skip timestamping.
+        # Returns whether the current class should skip timestamping. This is
+        # true when either a block-based timeless scope is active on the
+        # current thread/fiber, or the deprecated per-class counter is set.
         #
         # @return [ true | false ] Whether the current class should
         #   skip timestamping.
         def timeless?
-          !!Timeless[name]
+          Timeless.suppressing_timestamps? || !!Timeless[name]
         end
       end
     end

spec/mongoid/timestamps/timeless_spec.rb

@@ -135,4 +135,194 @@ class Egg
       end
     end
   end
+
+  describe '#timeless with a block' do
+    before(:all) do
+      class TimelessOther
+        include Mongoid::Document
+        include Mongoid::Timestamps
+      end
+    end
+
+    after(:all) do
+      Object.send(:remove_const, :TimelessOther)
+    end
+
+    context 'when called on an instance' do
+      let(:document) { Dokument.new }
+
+      it 'executes the block and persists the document' do
+        document.timeless { document.save! }
+        expect(document).to be_persisted
+      end
+
+      it 'does not set the created timestamp' do
+        document.timeless { document.save! }
+        expect(document.created_at).to be_nil
+      end
+
+      it 'does not set the updated timestamp' do
+        document.timeless { document.save! }
+        expect(document.updated_at).to be_nil
+      end
+
+      it 'returns the value of the block' do
+        expect(document.timeless { 42 }).to eq(42)
+      end
+
+      it 'resumes timestamping after the block' do
+        document.timeless { document.save! }
+        document.update_attribute(:title, 'Sir')
+        expect(document.updated_at).not_to be_nil
+      end
+
+      it 'is not timeless outside the block' do
+        document.timeless { document.save! }
+        expect(document).not_to be_timeless
+      end
+
+      it 'restores state even when the block raises' do
+        expect do
+          document.timeless { raise 'boom' }
+        end.to raise_error('boom')
+        expect(document).not_to be_timeless
+      end
+    end
+
+    context 'when called on the class' do
+      it 'does not set timestamps for documents created in the block' do
+        document = Dokument.timeless { Dokument.create! }
+        expect(document.created_at).to be_nil
+        expect(document.updated_at).to be_nil
+      end
+    end
+
+    context 'when nested' do
+      let(:document) { Dokument.new }
+
+      it 'remains timeless until the outermost block exits' do
+        Dokument.timeless do
+          Dokument.timeless { document.save! }
+          # inner block has exited, but we are still inside the outer block
+          expect(document).to be_timeless
+        end
+        expect(document).not_to be_timeless
+        expect(document.created_at).to be_nil
+      end
+    end
+
+    context 'when other documents are persisted in the block' do
+      it 'suppresses timestamps globally on the thread for the block duration' do
+        other = nil
+        Dokument.timeless { other = TimelessOther.create! }
+        expect(other.created_at).to be_nil
+      end
+    end
+  end
+
+  # Regression for MONGOID-5782: saving a parent timeless must not bump the
+  # updated_at of embedded children, at any nesting depth, when the
+  # associations cascade callbacks.
+  describe 'MONGOID-5782 cascaded embedded timestamps' do
+    before(:all) do
+      class TimelessBaz
+        include Mongoid::Document
+        include Mongoid::Timestamps
+
+        embedded_in :timeless_bar
+        field :val, type: String
+      end
+
+      class TimelessBar
+        include Mongoid::Document
+        include Mongoid::Timestamps
+
+        embedded_in :timeless_foo
+        embeds_many :timeless_bazs, cascade_callbacks: true
+        field :val, type: String
+      end
+
+      class TimelessFoo
+        include Mongoid::Document
+        include Mongoid::Timestamps
+
+        embeds_many :timeless_bars, cascade_callbacks: true
+        field :val, type: String
+      end
+    end
+
+    after(:all) do
+      Object.send(:remove_const, :TimelessBaz)
+      Object.send(:remove_const, :TimelessBar)
+      Object.send(:remove_const, :TimelessFoo)
+    end
+
+    let!(:start_time) { Timecop.freeze(Time.at(Time.now.to_i)) }
+
+    let!(:foo) do
+      TimelessFoo.create!(timeless_bars: [ { val: 'a', timeless_bazs: [ { val: 'x' } ] } ])
+    end
+
+    let(:bar) { foo.timeless_bars.first }
+    let(:baz) { bar.timeless_bazs.first }
+
+    after do
+      Timecop.return
+    end
+
+    it 'does not bump the embedded child updated_at' do
+      original = bar.updated_at
+      bar.val = 'b'
+      Timecop.freeze(Time.at(Time.now.to_i) + 2)
+      foo.timeless { foo.save! }
+      # the change was actually persisted (the block ran)...
+      expect(foo.reload.timeless_bars.first.val).to eq('b')
+      # ...but the timestamp was suppressed.
+      expect(bar.updated_at).to eq(original)
+    end
+
+    it 'does not bump the nested embedded child updated_at' do
+      original = baz.updated_at
+      baz.val = 'y'
+      Timecop.freeze(Time.at(Time.now.to_i) + 2)
+      foo.timeless { foo.save! }
+      expect(foo.reload.timeless_bars.first.timeless_bazs.first.val).to eq('y')
+      expect(baz.updated_at).to eq(original)
+    end
+
+    it 'still preserves the parent updated_at' do
+      original = foo.updated_at
+      foo.val = 'c'
+      Timecop.freeze(Time.at(Time.now.to_i) + 2)
+      foo.timeless { foo.save! }
+      expect(TimelessFoo.find(foo.id).val).to eq('c')
+      expect(foo.updated_at).to eq(original)
+    end
+  end
+
+  describe 'deprecation of the block-less form' do
+    let(:document) { Dokument.new }
+
+    it 'warns when called on an instance without a block' do
+      expect(Mongoid.logger).to receive(:warn).with(/timeless/).and_call_original
+      document.timeless.save!
+    end
+
+    it 'warns when called on the class without a block' do
+      expect(Mongoid.logger).to receive(:warn).with(/timeless/).and_call_original
+      Dokument.timeless.create!
+    end
+
+    it 'does not warn when called with a block' do
+      expect(Mongoid.logger).not_to receive(:warn)
+      document.timeless { document.save! }
+    end
+
+    it 'does not warn from the internal touch: false path' do
+      document.save!
+      document.title = 'changed'
+      expect(Mongoid.logger).not_to receive(:warn).with(/timeless/)
+      document.save!(touch: false)
+    end
+  end
 end