From 819506435f3a29c32f60e9c67a224d0f728e7182 Mon Sep 17 00:00:00 2001 From: shields Date: Sat, 2 Apr 2022 20:38:07 +0900 Subject: [PATCH] Demonstration of issues with #touch method. Please run spec/mongoid/touchable_spec.rb 1. On referenced relations with touch: false, calling #touch on document A will touch document B. 2. On embedded relations with touch: false, calling #touch on child document touches its parent (the root cause is different than #2) 3. On embedded relations with touch: true, there is strange behavior where in some cases calling #save! and #destroy on the child does not touch its parent. This works correctly on referenced relations. We should also add more specs for: - embeds_one (calling #touch on parent) - embeds_one -> embedded_in (calling #touch on child) - embeds_many (calling #touch on parent) - referenced has_one - referenced has_many - referenced habtm --- spec/mongoid/touchable_spec.rb | 423 ++++++++++++++++++++------ spec/mongoid/touchable_spec_models.rb | 19 ++ spec/support/models/updatable.rb | 7 - 3 files changed, 353 insertions(+), 96 deletions(-) delete mode 100644 spec/support/models/updatable.rb diff --git a/spec/mongoid/touchable_spec.rb b/spec/mongoid/touchable_spec.rb index fd459e8e5a..22e0c10450 100644 --- a/spec/mongoid/touchable_spec.rb +++ b/spec/mongoid/touchable_spec.rb @@ -7,28 +7,113 @@ describe "#touch" do + context "when the document has no timestamps" do + let(:model) do + TouchableSpec::NoTimestamps.create! + end + + it "responds to #touch" do + expect(model).to respond_to(:touch) + end + + it "does not raise an error when called without a field" do + model.touch + end + + it "can touch an additional field" do + expect(model.last_used_at).to be_nil + + model.touch(:last_used_at) + last_used_at = model.last_used_at + expect(last_used_at).to be_within(1).of(Time.now) + expect(model.reload.last_used_at).to be_within(0.001).of(last_used_at) + last_used_at = model.last_used_at + + model.touch + expect(model.last_used_at).to eq last_used_at + expect(model.reload.last_used_at).to eq last_used_at + + model.touch(:last_used_at) + expect(model.last_used_at).to be > last_used_at + expect(model.reload.last_used_at).to be > last_used_at + end + + it "can touch an additional field using alias" do + expect(model.last_used_at).to be_nil + + model.touch(:aliased_field) + last_used_at = model.last_used_at + expect(last_used_at).to be_within(1).of(Time.now) + expect(model.reload.last_used_at).to be_within(0.001).of(last_used_at) + end + end + context "when the document has no associations" do - let(:updatable) do - Updatable.create! + let(:model) do + TouchableSpec::NoAssociations.create! end it "responds to #touch" do - expect(updatable).to respond_to(:touch) + expect(model).to respond_to(:touch) end it "updates the timestamp when called" do - expect(updatable.updated_at).to be_nil + model + time_before_action = Time.now + model.touch + updated_at = model.updated_at + expect(updated_at).to be > time_before_action + expect(model.reload.updated_at).to be_within(0.001).of(updated_at) + + model.touch + expect(model.updated_at).to be > updated_at + expect(model.reload.updated_at).to be > updated_at + end - updatable.touch - updated_at = updatable.updated_at - expect(updated_at).not_to be_nil + it "can touch an additional field" do + model + time_before_action = Time.now + model.touch(:last_used_at) + + updated_at = model.updated_at + expect(updated_at).to be > time_before_action + expect(model.last_used_at).to eq updated_at + model.reload + expect(model.updated_at).to be_within(0.001).of(updated_at) + expect(model.last_used_at).to eq model.updated_at + updated_at = model.updated_at + + model.touch + expect(model.updated_at).to be > updated_at + expect(model.last_used_at).to eq updated_at + model.reload + expect(model.updated_at).to be > updated_at + expect(model.last_used_at).to eq updated_at + + updated_at = model.updated_at + model.touch(:last_used_at) + expect(model.updated_at).to be > updated_at + expect(model.last_used_at).to eq model.updated_at + model.reload + expect(model.updated_at).to be > updated_at + expect(model.last_used_at).to eq model.updated_at + end - updatable.touch - expect(updatable.updated_at).to be > updated_at + it "can touch an additional field" do + model + time_before_action = Time.now + model.touch(:aliased_field) + + updated_at = model.updated_at + expect(updated_at).to be > time_before_action + expect(model.last_used_at).to eq updated_at + model.reload + expect(model.updated_at).to be_within(0.001).of(updated_at) + expect(model.last_used_at).to eq model.updated_at end end - context 'when the document has a parent association' do + context 'associations' do let(:building) do parent_cls.create! @@ -42,121 +127,281 @@ building.floors.create! end - let!(:start_time) { Timecop.freeze(Time.at(Time.now.to_i)) } + context 'when embedded' do + let(:parent_cls) { TouchableSpec::Embedded::Building } - let(:update_time) do - Timecop.freeze(Time.at(Time.now.to_i) + 2) - end + context 'when :touch option is true' do - after do - Timecop.return - end + it '#touch persists synchronized updated_at on both parent and child' do + floor + time_before_action = Time.now + floor.touch - shared_examples 'updates the child' do - it "updates the updated_at timestamp" do - entrance - update_time - entrance.touch + floor_updated = floor.updated_at + expect(floor_updated).to be > time_before_action + expect(building.updated_at).to be > time_before_action - entrance.updated_at.should == update_time - end + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(building.updated_at).to eq floor.updated_at + end - it "persists the changes" do - entrance - update_time - entrance.touch + it '#touch with additional field persists synchronized values on both parent and child' do + floor + time_before_action = Time.now + floor.touch(:last_used_at) - entrance.reload.updated_at.should == update_time - end - end + floor_updated = floor.updated_at + expect(floor_updated).to be > time_before_action + expect(floor.last_used_at).to eq floor_updated + expect(building.updated_at).to be > time_before_action - shared_examples 'updates the parent when :touch is true' do + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(floor.last_used_at).to eq floor.updated_at + expect(building.updated_at).to eq floor.updated_at + end - it 'updates updated_at on parent' do - floor - update_time - floor.touch + it '#save! persists non-synchronized updated_at on both parent and child' do + # TODO: Nice-to-have: #save! on embedded models should have synchronized timestamps between parent and child. + floor + floor.last_used_at = Time.now + time_before_action = Time.now + floor.save! - building.updated_at.should == update_time - end + # TODO: BROKEN! For some reason floor.building is nil and this causes the touch callbacks to not run on it. + puts "This should exist: #{floor.building.inspect}" + puts "It should be the same as this: #{floor._parent.inspect}" - it 'persists updated updated_at on parent' do - floor - update_time - floor.touch + floor_updated = floor.updated_at + building_updated = building.updated_at + expect(floor_updated).to be > time_before_action + expect(building_updated).to be > time_before_action - building.reload.updated_at.should == update_time - end - end + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(building.updated_at).to be_within(0.001).of(building_updated) + end - shared_examples 'updates the parent when :touch is not set' do - it 'does not update updated_at on parent' do - entrance - update_time - entrance.touch + it '#destroy persists updated_at on parent' do + floor + time_before_action = Time.now + floor.destroy - building.updated_at.should == update_time - end + # TODO: BROKEN! For some reason floor.building is nil and this causes the touch callbacks to not run on it. + puts "This should exist: #{floor.building.inspect}" + puts "It should be the same as this: #{floor._parent.inspect}" - it 'does not persist updated updated_at on parent' do - entrance - update_time - entrance.touch + building_updated = building.updated_at + expect(building_updated).to be > time_before_action - building.reload.updated_at.should == update_time + building.reload + expect(building.updated_at).to be_within(0.001).of(building_updated) + end 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 + context 'when :touch option is not set' do - building.updated_at.should == start_time - end + it '#touch persists updated_at on child but not parent' do + # TODO: BROKEN! This needs a guard method to prevent case when touch: false + # lib/mongoid/touchable.rb line 34 `if parent` needs a guard so that it does not proceed if touch: false. - it 'does not persist updated updated_at on parent' do - entrance - update_time - entrance.touch + entrance + time_before_action = Time.now + entrance.touch - building.reload.updated_at.should == start_time - end - end + entrance_updated = entrance.updated_at + building_updated = entrance.updated_at + expect(entrance_updated).to be > time_before_action + expect(building_updated).to be < time_before_action - context "when the document is embedded" do - let(:parent_cls) { TouchableSpec::Embedded::Building } + expect(entrance.reload.updated_at).to be_within(0.001).of(entrance_updated) + expect(building.reload.updated_at).to be_within(0.001).of(building_updated) + end - include_examples 'updates the child' - include_examples 'updates the parent when :touch is true' - include_examples 'updates the parent when :touch is not set' + it '#touch with additional field persists synchonized values on child but not parent' do + # TODO: BROKEN! This needs a guard method to prevent case when touch: false + # lib/mongoid/touchable.rb line 34 `if parent` needs a guard so that it does not proceed if touch: false. - context 'when also updating an additional field' do - it 'persists the update to the additional field' do entrance - update_time + time_before_action = Time.now entrance.touch(:last_used_at) + entrance_updated = entrance.updated_at + building_updated = entrance.updated_at + expect(entrance_updated).to be > time_before_action + expect(entrance.last_used_at).to eq entrance_updated + expect(building_updated).to be < time_before_action + + entrance.reload + building.reload + expect(entrance.updated_at).to be_within(0.001).of(entrance_updated) + expect(entrance.last_used_at).to be_within(0.001).of(entrance_updated) + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + + it '#save! persists updated_at on child but not parent' do + entrance + entrance.last_used_at = Time.now + time_before_action = Time.now + entrance.save! + + entrance_updated = entrance.updated_at + building_updated = building.updated_at + expect(entrance_updated).to be > time_before_action + expect(building_updated).to be < time_before_action + entrance.reload building.reload + expect(entrance.updated_at).to be_within(0.001).of(entrance_updated) + end - # This is the assertion we want. - entrance.last_used_at.should == update_time + it '#destroy does not set updated_at on parent' do + entrance + time_before_action = Time.now + entrance.destroy - # Check other timestamps for good measure. - entrance.updated_at.should == update_time - building.updated_at.should == update_time + expect(entrance.updated_at).to be < time_before_action + expect(building.updated_at).to be < time_before_action end end end - context "when the document is referenced" do + context 'when referenced' do let(:parent_cls) { TouchableSpec::Referenced::Building } - 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' + context 'when :touch option is true' do + + it '#touch persists non-synchronized updated_at on both parent and child' do + floor + time_before_action = Time.now + floor.touch + + floor_updated = floor.updated_at + building_updated = building.updated_at + expect(floor_updated).to be > time_before_action + expect(building_updated).to be > time_before_action + + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + + it '#touch with additional field persists non-synchronized values on both parent and child' do + floor + time_before_action = Time.now + floor.touch(:last_used_at) + + floor_updated = floor.updated_at + building_updated = building.updated_at + expect(floor_updated).to be > time_before_action + expect(floor.last_used_at).to eq floor_updated + expect(building_updated).to be > time_before_action + + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(floor.last_used_at).to eq floor.updated_at + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + + it '#save! persists non-synchronized updated_at on both parent and child' do + floor + floor.last_used_at = Time.now + time_before_action = Time.now + floor.save! + + floor_updated = floor.updated_at + building_updated = building.updated_at + expect(floor_updated).to be > time_before_action + expect(building_updated).to be > time_before_action + + floor.reload + building.reload + expect(floor.updated_at).to be_within(0.001).of(floor_updated) + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + + it '#destroy persists updated_at on parent' do + floor + time_before_action = Time.now + floor.destroy + + building_updated = building.updated_at + expect(building_updated).to be > time_before_action + + building.reload + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + end + + context 'when :touch option is not set' do + + it '#touch sets and persists updated_at on child but not parent' do + # TODO: BROKEN! :touch callbacks need a guard so the don't run unless association has touch: true + + entrance + time_before_action = Time.now + entrance.touch + + entrance_updated = entrance.updated_at + building_updated = entrance.updated_at + expect(entrance_updated).to be > time_before_action + expect(building_updated).to be < time_before_action + + expect(entrance.reload.updated_at).to be_within(0.001).of(entrance_updated) + expect(building.reload.updated_at).to be_within(0.001).of(building_updated) + end + + it '#touch with additional field persists synchronized values on child but not parent' do + # TODO: BROKEN! :touch callbacks need a guard so the don't run unless association has touch: true + + entrance + time_before_action = Time.now + entrance.touch(:last_used_at) + + entrance_updated = entrance.updated_at + building_updated = entrance.updated_at + expect(entrance_updated).to be > time_before_action + expect(entrance.last_used_at).to eq entrance_updated + expect(building_updated).to be < time_before_action + + entrance.reload + building.reload + expect(entrance.updated_at).to be_within(0.001).of(entrance_updated) + expect(entrance.last_used_at).to be_within(0.001).of(entrance_updated) + expect(building.updated_at).to be_within(0.001).of(building_updated) + end + + it '#save! persists updated_at on child but not parent' do + entrance + entrance.last_used_at = Time.now + time_before_action = Time.now + entrance.save! + + entrance_updated = entrance.updated_at + building_updated = building.updated_at + expect(entrance_updated).to be > time_before_action + expect(building_updated).to be < time_before_action + + entrance.reload + building.reload + expect(entrance.updated_at).to be_within(0.001).of(entrance_updated) + end + + it '#destroy does not set updated_at on parent' do + entrance + time_before_action = Time.now + entrance.destroy + + expect(entrance.updated_at).to be < time_before_action + expect(building.updated_at).to be < time_before_action + end + end end end diff --git a/spec/mongoid/touchable_spec_models.rb b/spec/mongoid/touchable_spec_models.rb index 7e86d38f12..5406e60c62 100644 --- a/spec/mongoid/touchable_spec_models.rb +++ b/spec/mongoid/touchable_spec_models.rb @@ -1,6 +1,19 @@ # frozen_string_literal: true module TouchableSpec + class NoTimestamps + include Mongoid::Document + + field :last_used_at, as: :aliased_field, type: Time + end + + class NoAssociations + include Mongoid::Document + include Mongoid::Timestamps + + field :last_used_at, as: :aliased_field, type: Time + end + module Embedded class Building include Mongoid::Document @@ -24,6 +37,8 @@ class Floor include Mongoid::Timestamps embedded_in :building, touch: true + + field :last_used_at, type: Time end end @@ -41,6 +56,8 @@ class Entrance include Mongoid::Timestamps belongs_to :building + + field :last_used_at, type: Time end class Floor @@ -48,6 +65,8 @@ class Floor include Mongoid::Timestamps belongs_to :building, touch: true + + field :last_used_at, type: Time end end end diff --git a/spec/support/models/updatable.rb b/spec/support/models/updatable.rb deleted file mode 100644 index 041926af5e..0000000000 --- a/spec/support/models/updatable.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -class Updatable - include Mongoid::Document - - field :updated_at, type: BSON::Timestamp -end