diff --git a/docs/release-notes/mongoid-9.0.txt b/docs/release-notes/mongoid-9.0.txt index 58a680d0d5..6e9ab7ca24 100644 --- a/docs/release-notes/mongoid-9.0.txt +++ b/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`` ------------------------------------------------------ diff --git a/lib/mongoid/association/options.rb b/lib/mongoid/association/options.rb index 6bdda5f804..884ef4f8b5 100644 --- a/lib/mongoid/association/options.rb +++ b/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 diff --git a/lib/mongoid/atomic.rb b/lib/mongoid/atomic.rb index 3b1751c15b..7eb55d9d7a 100644 --- a/lib/mongoid/atomic.rb +++ b/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 diff --git a/lib/mongoid/touchable.rb b/lib/mongoid/touchable.rb index 6fd8aeeb7e..a9242ccf36 100644 --- a/lib/mongoid/touchable.rb +++ b/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 ] 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 diff --git a/spec/integration/callbacks_models.rb b/spec/integration/callbacks_models.rb index 8e6f59d655..2d16141c7f 100644 --- a/spec/integration/callbacks_models.rb +++ b/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 diff --git a/spec/mongoid/touchable_spec.rb b/spec/mongoid/touchable_spec.rb index 8d28ec84bf..bbdd3788ba 100644 --- a/spec/mongoid/touchable_spec.rb +++ b/spec/mongoid/touchable_spec.rb @@ -58,7 +58,7 @@ update_time entrance.touch - entrance.updated_at.should == update_time + expect(entrance.updated_at).to eq(update_time) end it "persists the changes" do @@ -66,7 +66,7 @@ update_time entrance.touch - entrance.reload.updated_at.should == update_time + expect(entrance.reload.updated_at).to eq(update_time) end end @@ -77,7 +77,7 @@ update_time floor.touch - building.updated_at.should == update_time + expect(building.updated_at).to eq(update_time) end it 'persists updated updated_at on parent' do @@ -85,17 +85,17 @@ update_time floor.touch - building.reload.updated_at.should == update_time + expect(building.reload.updated_at).to eq(update_time) end end - shared_examples 'updates the parent when :touch is false' do + shared_examples 'does not update the parent when :touch is false' do it 'does not update updated_at on parent' do entrance update_time entrance.touch - building.updated_at.should == update_time + expect(building.updated_at).to eq(start_time) end it 'does not persist updated updated_at on parent' do @@ -103,26 +103,12 @@ update_time entrance.touch - building.reload.updated_at.should == update_time + expect(building.reload.updated_at).to eq(start_time) end end shared_examples 'does not update the parent when :touch is not set' do - it 'does not update updated_at on parent' do - entrance - update_time - entrance.touch - - building.updated_at.should == start_time - end - - it 'does not persist updated updated_at on parent' do - entrance - update_time - entrance.touch - - building.reload.updated_at.should == start_time - end + it_behaves_like 'does not update the parent when :touch is false' end context "when the document is embedded" do @@ -130,9 +116,9 @@ include_examples 'updates the child' include_examples 'updates the parent when :touch is true' - include_examples 'updates the parent when :touch is false' + include_examples 'does not update the parent when :touch is not set' - context 'when also updating an additional field' do + context 'when also updating an additional field when :touch is true' do it 'persists the update to the additional field' do entrance update_time @@ -142,11 +128,29 @@ 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. + expect(entrance.updated_at).to eq(update_time) + expect(building.updated_at).to eq(start_time) + end + end + + context 'when also updating an additional field when :touch is not set' do + it 'persists the update to the additional field' do + floor + update_time + floor.touch(:last_used_at) + + floor.reload + building.reload + + # This is the assertion we want. + expect(floor.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(floor.updated_at).to eq(update_time) + expect(building.updated_at).to eq(update_time) end end end @@ -156,7 +160,7 @@ include_examples 'updates the child' include_examples 'updates the parent when :touch is true' - include_examples 'does not update the parent when :touch is not set' + include_examples 'does not update the parent when :touch is false' end end @@ -619,8 +623,8 @@ end it "updates the parent's timestamp" do - building.updated_at.should == update_time - building.reload.updated_at.should == update_time + expect(building.updated_at).to eq(update_time) + expect(building.reload.updated_at).to eq(update_time) end end @@ -683,12 +687,8 @@ context "when the touch option is false" do shared_examples "does not update the parent" do - let!(:start_time) { Timecop.freeze(Time.at(Time.now.to_i)) } - - let(:update_time) do - Timecop.freeze(Time.at(Time.now.to_i) + 2) - end + let(:update_time) { Timecop.freeze(Time.at(Time.now.to_i) + 2) } after do Timecop.return @@ -705,17 +705,22 @@ before do entrance update_time - entrance.touch + entrance.level = 1 + entrance.send(meth) end it "updates the child's timestamp" do - entrance.updated_at.should == update_time - entrance.reload.updated_at.should == update_time + if entrance.destroyed? + expect(entrance.updated_at).to eq(start_time) + else + expect(entrance.updated_at).to eq(update_time) + expect(entrance.reload.updated_at).to eq(update_time) + end end it "does not update the parent's timestamp" do - building.updated_at.should == start_time - building.reload.updated_at.should == start_time + expect(building.updated_at).to eq(start_time) + expect(building.reload.updated_at).to eq(start_time) end end @@ -731,11 +736,353 @@ let(:meth) { meth } let(:parent_cls) { TouchableSpec::Embedded::Building } - before do - skip "MONGOID-5274" + include_examples "does not update the parent" + end + end + end + + context 'multi-level' do + + let!(:start_time) { Timecop.freeze(Time.at(Time.now.to_i)) } + + let(:update_time) { Timecop.freeze(Time.at(Time.now.to_i) + 2) } + + let(:child_name) do + child_cls.name.demodulize.underscore + end + + let(:grandchild_name) do + grandchild_cls.name.demodulize.underscore + end + + let(:parent) do + parent_cls.create! + end + + let(:child) do + parent.send(child_name.pluralize).create! + end + + let(:grandchild) do + grandchild = child.send(grandchild_name.pluralize).create! + grandchild.created_at = Time.now + 1.day # arbitrary change so save! works + grandchild + end + + shared_examples "updates the parent" do + it "updates the parent's timestamp" do + expect(parent.updated_at).to eq(update_time) + expect(parent.reload.updated_at).to eq(update_time) + end + end + + shared_examples "does not update the parent" do + it "does not update the parent's timestamp" do + expect(parent.updated_at).to eq(start_time) + expect(parent.reload.updated_at).to eq(start_time) + end + end + + shared_examples "updates the child" do + it "updates the child's timestamp" do + expect(child.updated_at).to eq(update_time) + expect(child.reload.updated_at).to eq(update_time) + end + end + + shared_examples "does not update the child" do + it "does not update the child's timestamp" do + expect(child.updated_at).to eq(start_time) + expect(child.reload.updated_at).to eq(start_time) + end + end + + shared_examples "updates the grandchild" do + it "updates the grandchild's timestamp" do + if grandchild.destroyed? + expect(grandchild.updated_at).to eq(start_time) + else + expect(grandchild.updated_at).to eq(update_time) + expect(grandchild.reload.updated_at).to eq(update_time) end + end + end - include_examples "does not update the parent" + shared_examples "does not update the grandchild" do + it "does not update the grandchild's timestamp" do + expect(grandchild.updated_at).to eq(start_time) + expect(grandchild.reload.updated_at).to eq(start_time) unless grandchild.destroyed? + end + end + + before do + grandchild + update_time + grandchild.send(meth) + end + + after do + Timecop.return + end + + context 'parent > embedded child > embedded grandchild' do + + let(:parent_cls) { TouchableSpec::Embedded::Building } + + context 'child touch: true' do + + let(:child_cls) do + TouchableSpec::Embedded::Floor + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Embedded::Sofa + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "updates the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Embedded::Chair + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end + end + + context 'child touch: false' do + + let(:child_cls) do + TouchableSpec::Embedded::Entrance + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Embedded::Camera + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Embedded::Keypad + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end + end + end + + context 'parent > referenced child > embedded grandchild' do + + let(:parent_cls) { TouchableSpec::Referenced::Building } + + context 'child touch: true' do + + let(:child_cls) do + TouchableSpec::Referenced::Floor + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Sofa + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "updates the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Chair + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end + end + + context 'child touch: false' do + + let(:child_cls) do + TouchableSpec::Referenced::Entrance + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Camera + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Keypad + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end + end + end + + context 'parent > referenced child > referenced grandchild' do + + let(:parent_cls) { TouchableSpec::Referenced::Building } + + context 'child touch: true' do + + let(:child_cls) do + TouchableSpec::Referenced::Floor + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Window + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "updates the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Plant + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end + end + + context 'child touch: false' do + + let(:child_cls) do + TouchableSpec::Referenced::Entrance + end + + context 'grandchild touch: true' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Window + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "updates the child" + it_behaves_like "updates the grandchild" + end + end + end + + context 'grandchild touch: false' do + + let(:grandchild_cls) do + TouchableSpec::Referenced::Plant + end + + [ :save!, :destroy, :touch ].each do |meth| + context "when calling #{meth} method" do + let(:meth) { meth } + + it_behaves_like "does not update the parent" + it_behaves_like "does not update the child" + it_behaves_like "updates the grandchild" + end + end + end end end end @@ -849,7 +1196,7 @@ end end - context "when saving embedded associations with cascadable callbacks" do + context "when saving embedded associations with cascading callbacks" do shared_examples "timeless is cleared" do it "clears the timeless option" do diff --git a/spec/mongoid/touchable_spec_models.rb b/spec/mongoid/touchable_spec_models.rb index f34d122c9f..f74a612127 100644 --- a/spec/mongoid/touchable_spec_models.rb +++ b/spec/mongoid/touchable_spec_models.rb @@ -16,9 +16,13 @@ class Entrance include Mongoid::Document include Mongoid::Timestamps + field :last_used_at, type: Time + field :level, type: Integer + embedded_in :building, touch: false, class_name: "TouchableSpec::Embedded::Building" - field :last_used_at, type: Time + embeds_many :keypads, class_name: "TouchableSpec::Embedded::Keypad" + embeds_many :cameras, class_name: "TouchableSpec::Embedded::Camera" end class Floor @@ -26,8 +30,40 @@ class Floor include Mongoid::Timestamps field :level, type: Integer + field :last_used_at, type: Time embedded_in :building, touch: true, class_name: "TouchableSpec::Embedded::Building" + + embeds_many :chairs, class_name: "TouchableSpec::Embedded::Chair" + embeds_many :sofas, class_name: "TouchableSpec::Embedded::Sofa" + end + + class Keypad + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :entrance, touch: false, class_name: "TouchableSpec::Embedded::Entrance" + end + + class Camera + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :entrance, touch: true, class_name: "TouchableSpec::Embedded::Entrance" + end + + class Chair + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :floor, touch: false, class_name: "TouchableSpec::Embedded::Floor" + end + + class Sofa + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :floor, touch: true, class_name: "TouchableSpec::Embedded::Floor" end end @@ -44,7 +80,15 @@ class Entrance include Mongoid::Document include Mongoid::Timestamps + field :level, type: Integer + belongs_to :building, touch: false, class_name: "TouchableSpec::Referenced::Building" + + embeds_many :keypads, class_name: "TouchableSpec::Referenced::Keypad" + embeds_many :cameras, class_name: "TouchableSpec::Referenced::Camera" + + has_many :plants, class_name: "TouchableSpec::Referenced::Plant" + has_many :windows, class_name: "TouchableSpec::Referenced::Window" end class Floor @@ -54,6 +98,56 @@ class Floor field :level, type: Integer belongs_to :building, touch: true, class_name: "TouchableSpec::Referenced::Building" + + embeds_many :chairs, class_name: "TouchableSpec::Referenced::Chair" + embeds_many :sofas, class_name: "TouchableSpec::Referenced::Sofa" + + has_many :plants, class_name: "TouchableSpec::Referenced::Plant" + has_many :windows, class_name: "TouchableSpec::Referenced::Window" + end + + class Plant + include Mongoid::Document + include Mongoid::Timestamps + + belongs_to :floor, touch: false, class_name: "TouchableSpec::Referenced::Floor" + belongs_to :entrance, touch: false, class_name: "TouchableSpec::Referenced::Entrance" + end + + class Window + include Mongoid::Document + include Mongoid::Timestamps + + belongs_to :floor, touch: true, class_name: "TouchableSpec::Referenced::Floor" + belongs_to :entrance, touch: true, class_name: "TouchableSpec::Referenced::Entrance" + end + + class Keypad + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :entrance, touch: false, class_name: "TouchableSpec::Referenced::Entrance" + end + + class Camera + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :entrance, touch: true, class_name: "TouchableSpec::Referenced::Entrance" + end + + class Chair + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :floor, touch: false, class_name: "TouchableSpec::Referenced::Floor" + end + + class Sofa + include Mongoid::Document + include Mongoid::Timestamps + + embedded_in :floor, touch: true, class_name: "TouchableSpec::Referenced::Floor" end end end