Prevent touch from saving unsaved changes

lib/activerecord-bitemporal/bitemporal.rb

@@ -312,6 +312,59 @@ def save!(**)
         end
       end
 
+      # @see https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/attribute_methods/dirty.rb#L200
+      # @see https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/persistence.rb#L1200
+      def _touch_row(attribute_names, time)
+        _touch_attr_names = Set.new(attribute_names)
+
+        time ||= current_time_from_proper_timezone
+
+        attribute_names.each do |attr_name|
+          _write_attribute(attr_name, time)
+        end
+
+        changes = {}
+        @attributes.keys.each do |attr_name|
+          next if _touch_attr_names.include?(attr_name)
+
+          if attribute_changed?(attr_name)
+            changes[attr_name] = _read_attribute(attr_name)
+            _write_attribute(attr_name, attribute_was(attr_name))
+            clear_attribute_change(attr_name)
+          end
+        end
+
+        # The ActiveRecord::Bitemporal::Persistence#_update_row is different from the original
+        # and also save changes other than the passed attribute_names.
+        # To avoid saving unintended changes, evacuate the unsaved changes to a variable
+        # before _update_row is called. ActiveRecord::AttributeMethods::Dirty#_touch_row does this **after** _update_row.
+        affected_rows = _update_row(attribute_names, "touch")
+
+        # @_skip_dirty_tracking was introduced to prevent unintentional changes of previous_changes
+        # due to deferred touches caused by `touch: true`. See https://github.com/rails/rails/pull/36271
+        # To reproduce similar behavior, return affected_rows without calling changes_applied.
+        # Note that unlike the original, changes must be evacuated and restored.
+        # As explained above, this is to avoid implementation differences in _update_row.
+        #
+        # FIXME: Implicit touching with `touch: true`` is **completely** broken in the bi-temporal gem.
+        #        ActiveRecord::TouchLater#touch_later sets an instance variable and performs the actual touching
+        #        with before_committed!, but the bi-temporal gem internally creates multiple Active Record instances
+        #        and calls touch_later and before_committed! for each. So, we cannot propagate instance variables properly.
+        #        As a result, there is no case where @_skip_dirty_tracking works effectively in Rails v7.0.
+        if @_skip_dirty_tracking ||= false
+          clear_attribute_changes(_touch_attr_names)
+          changes.each { |attr_name, value| _write_attribute(attr_name, value) }
+          return affected_rows
+        end
+
+        changes_applied
+        changes.each { |attr_name, value| _write_attribute(attr_name, value) }
+
+        affected_rows
+      ensure
+        @_skip_dirty_tracking = nil
+      end
+
       def _update_row(attribute_names, attempted_action = 'update')
         current_valid_record, before_instance, after_instance = bitemporal_build_update_records(valid_datetime: self.valid_datetime, force_update: self.force_update?)
 

spec/activerecord-bitemporal/bitemporal_spec.rb

@@ -27,10 +27,13 @@
         it {
           is_expected.to have_attributes(
             bitemporal_id: subject.id,
+            # changes: be_empty, FIXME: Creating with bitemporal_id produces the unexpected "id" changes. See #144 or #147
             previous_changes: include(
               "id" => [nil, subject.swapped_id],
               "valid_from" => [nil, be_present],
               "valid_to" => [nil, "2019/10/01".in_time_zone],
+              "transaction_from" => [nil, be_present],
+              "transaction_to" => [nil, ActiveRecord::Bitemporal::DEFAULT_TRANSACTION_TO],
               "name" => [nil, "Tom"]
             )
           )
@@ -42,10 +45,13 @@
         it {
           is_expected.to have_attributes(
             bitemporal_id: subject.id,
+            changes: be_empty,
             previous_changes: include(
               "id" => [nil, subject.id],
               "valid_from" => [nil, be_present],
               "valid_to" => [nil, ActiveRecord::Bitemporal::DEFAULT_VALID_TO],
+              "transaction_from" => [nil, be_present],
+              "transaction_to" => [nil, ActiveRecord::Bitemporal::DEFAULT_TRANSACTION_TO],
               "name" => [nil, "Tom"]
             ),
             previously_force_updated?: false
@@ -654,7 +660,7 @@ def self.bitemporal_id_key
       let(:from) { time_current }
       let(:to) { from + 10.days }
       let(:finish) { Time.utc(9999, 12, 31).in_time_zone }
-      let!(:employee) { Employee.create!(name: "Jone", valid_from: from, valid_to: to) }
+      let!(:employee) { Timecop.freeze(from - 1.month) { Employee.create!(name: "Jone", valid_from: from, valid_to: to) } }
       let!(:swapped_id) { employee.swapped_id }
       let(:count) { -> { Employee.where(bitemporal_id: employee.id).ignore_valid_datetime.count } }
       let(:old_jone) { Employee.ignore_valid_datetime.within_deleted.find_by(bitemporal_id: employee.id, name: "Jone") }
@@ -684,6 +690,13 @@ def self.bitemporal_id_key
         let(:now) { from + 5.days }
         it { expect { subject }.to change(&count).by(1) }
         it { expect { subject }.to change(employee, :name).from("Jone").to("Tom") }
+        it { expect { subject }.to change(employee, :valid_from).from(from).to(now) }
+        it { expect { subject }.not_to change(employee, :valid_to) }
+        it { expect { subject }.to change(employee, :transaction_from).to(now) }
+        it { expect { subject }.not_to change(employee, :transaction_to) }
+        it { expect(employee.changes).to be_empty }
+        it { expect { subject }.not_to change(employee, :changes) }
+        it { expect { subject }.to change { employee.saved_changes.keys }.to(contain_exactly("name", "valid_from", "transaction_from", "updated_at")) }
         it { expect { subject }.to change(employee, :swapped_id).from(swapped_id).to(kind_of(Integer)) }
         it { expect { subject }.to change(employee, :swapped_id_previously_was).from(nil).to(swapped_id) }
         it_behaves_like "updated Jone" do
@@ -707,6 +720,13 @@ def self.bitemporal_id_key
         let(:now) { from - 5.days }
         it { expect { subject }.to change(&count).by(1) }
         it { expect { subject }.to change(employee, :name).from("Jone").to("Tom") }
+        it { expect { subject }.to change(employee, :valid_from).from(from).to(now) }
+        it { expect { subject }.to change(employee, :valid_to).from(to).to(from) }
+        it { expect { subject }.to change(employee, :transaction_from).to(now) }
+        it { expect { subject }.not_to change(employee, :transaction_to) }
+        it { expect(employee.changes).to be_empty }
+        it { expect { subject }.not_to change(employee, :changes) }
+        it { expect { subject }.to change { employee.saved_changes.keys }.to contain_exactly("name", "valid_from", "valid_to", "transaction_from", "updated_at") }
         it { expect { subject }.to change(employee, :swapped_id).from(swapped_id).to(kind_of(Integer)) }
         it { expect { subject }.to change(employee, :swapped_id_previously_was).from(nil).to(swapped_id) }
         it_behaves_like "updated Jone" do
@@ -1052,6 +1072,8 @@ class EmployeeWithUniquness < Employee
     it { expect { subject }.to change { Employee.ignore_valid_datetime.within_deleted.count }.by(1) }
     it { expect { subject }.to change(employee, :swapped_id).from(@swapped_id_before_destroy).to(kind_of(Integer)) }
     it { expect { subject }.to change(employee, :swapped_id_previously_was).from(kind_of(Integer)).to(@swapped_id_before_destroy) }
+    xit { expect { subject }.to change(employee, :changes).to(be_empty) } # FIXME: Destroying produces the unexpected "valid_to", "transaction_from", and "transaction_to" changes.
+    it { expect { subject }.not_to change(employee, :saved_changes) }
     it { expect(subject).to eq employee }
 
     it do
@@ -1223,14 +1245,66 @@ class EmployeeWithUniquness < Employee
   end
 
   describe "#touch" do
-    let!(:employee) { Employee.create(name: "Jane").tap { |it| it.update!(name: "Tom") } }
+    let!(:employee) { Timecop.freeze(created_time) { Employee.create(name: "Tom") } }
     let(:employee_count) { -> { Employee.ignore_valid_datetime.bitemporal_for(employee.id).count } }
-    subject { employee.touch(:archived_at) }
+    let(:created_time) { time_current - 10.seconds }
+    let(:touched_time) { time_current - 5.seconds }
+    subject { Timecop.freeze(touched_time) { employee.touch(:archived_at) } }
+
+    before { @swapped_id_before_touch = employee.swapped_id }
 
     it { expect(employee).to have_attributes(name: "Tom", id: employee.id) }
     it { expect(subject).to eq true }
     it { expect { subject }.to change(&employee_count).by(1) }
     it { expect { subject }.to change { employee.reload.archived_at }.from(nil) }
+    it { expect { subject }.to change(employee, :valid_from).from(created_time).to(touched_time) }
+    it { expect { subject }.not_to change(employee, :valid_to) }
+    it { expect { subject }.to change(employee, :transaction_from).from(created_time).to(touched_time) }
+    it { expect { subject }.not_to change(employee, :transaction_to) }
+    it { expect { subject }.to change(employee, :swapped_id).from(@swapped_id_before_touch).to(kind_of(Integer)) }
+    it { expect { subject }.to change(employee, :swapped_id_previously_was).from(nil).to(@swapped_id_before_touch) }
+    it { expect(employee.changes).to be_empty }
+    it { expect { subject }.not_to change(employee, :changes) }
+    it { expect { subject }.to change { employee.saved_changes.keys }.to(contain_exactly("archived_at", "valid_from", "transaction_from", "updated_at")) }
+
+    context "with unsaved changes" do
+      let(:changed_time) { time_current - 7.seconds } # Neither created_time nor touched_time
+
+      before do
+        employee.name = "Jane"
+        employee.valid_from = changed_time
+        employee.archived_at = changed_time
+        @updated_at_before_touch = employee.updated_at
+      end
+
+      it { expect(employee).to have_attributes(name: "Jane", id: employee.id) }
+      it { expect(subject).to eq true }
+      it { expect { subject }.to change(&employee_count).by(1) }
+      # Do not save temporary changes
+      it { expect { subject }.not_to change { employee.reload.name } }
+      it { expect { subject }.to change { employee.reload.valid_from }.from(created_time).to(touched_time) }
+      it { expect { subject }.to change { employee.reload.archived_at }.from(nil).to(touched_time) }
+      # Merge temporary changes with changes by bi-temporal operations
+      it { expect { subject }.not_to change(employee, :valid_from) }
+      it { expect { subject }.not_to change(employee, :valid_to) }
+      it { expect { subject }.to change(employee, :transaction_from).from(created_time).to(touched_time) }
+      it { expect { subject }.not_to change(employee, :transaction_to) }
+      it { expect(employee.changes).to eq({ "name" => ["Tom", "Jane"], "valid_from" => [created_time, changed_time], "archived_at" => [nil, changed_time] }) }
+      it { expect { subject }.to change { employee.changes }.to(eq({ "name" => ["Tom", "Jane"], "valid_from" => [touched_time, changed_time] })) } # Discard archived_at and changes the previous value of valid_from
+      it { expect { subject }.to change { employee.saved_changes }.to(eq({ "archived_at" => [nil, touched_time], "valid_from" => [created_time, touched_time], "transaction_from" => [created_time, touched_time], "updated_at" => [@updated_at_before_touch, touched_time] })) }
+
+      context "and #force_update" do
+        subject { Timecop.freeze(touched_time) { employee.force_update { |e| e.touch(:archived_at) } } }
+
+        it { expect(subject).to eq true }
+        it { expect { subject }.not_to change(&employee_count) }
+        # With update + force_update, valid_from changes are reflected as is (saved as changed_time), but touch + force_update ignores the changes
+        it { expect(employee.reload.valid_from).to eq(created_time) }
+        it { expect { subject }.not_to change { employee.reload.valid_from } }
+        it { expect(employee.valid_from).to eq(changed_time) }
+        it { expect { subject }.not_to change(employee, :valid_from) }
+      end
+    end
   end
 
   describe "validation" do