Skip to content

Commit

Permalink
Prevent touch from saving unsaved changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@wata727
wata727 committed Oct 11, 2023
1 parent dc38488 commit f77f70c
Show file tree
Hide file tree
Showing 2 changed files with 130 additions and 3 deletions.
53 changes: 53 additions & 0 deletions lib/activerecord-bitemporal/bitemporal.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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?)

Expand Down
80 changes: 77 additions & 3 deletions spec/activerecord-bitemporal/bitemporal_spec.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
)
)
Expand All @@ -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
Expand Down Expand Up @@ -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") }
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit f77f70c

Please sign in to comment.