From f77f70cb81a033f21c6ab34f9a55bfc10ca75e3c Mon Sep 17 00:00:00 2001 From: wata727 Date: Tue, 10 Oct 2023 14:09:26 +0900 Subject: [PATCH] Prevent touch from saving unsaved changes --- lib/activerecord-bitemporal/bitemporal.rb | 53 ++++++++++++ .../bitemporal_spec.rb | 80 ++++++++++++++++++- 2 files changed, 130 insertions(+), 3 deletions(-) diff --git a/lib/activerecord-bitemporal/bitemporal.rb b/lib/activerecord-bitemporal/bitemporal.rb index f7d5898..8551688 100644 --- a/lib/activerecord-bitemporal/bitemporal.rb +++ b/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?) diff --git a/spec/activerecord-bitemporal/bitemporal_spec.rb b/spec/activerecord-bitemporal/bitemporal_spec.rb index ed9661c..c5c7b4a 100644 --- a/spec/activerecord-bitemporal/bitemporal_spec.rb +++ b/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