activerecord-bitemporal icon indicating copy to clipboard operation
activerecord-bitemporal copied to clipboard

Prevent touch from saving unsaved changes

Open wata727 opened this issue 2 years ago • 0 comments

See also https://github.com/kufu/activerecord-bitemporal/pull/140

#140 fixes the issue where touch generates irrelevant changes by fixing _update_row. However, I noticed that the current touch implementation has another problem where unsaved changes are saved.

# non-bitemporalized model
tenant = Tenant.create!(name: "test")
tenant.name = "changed"
tenant.touch
tenant.reload.name # => "test"

# bitemporalized model
employee = Employee.create!(name: "Tom")
employee.name = "Jane"
employee.touch
employee.reload.name # => "Jane"

The problem lies in the difference in the implementation of _update_row: https://github.com/kufu/activerecord-bitemporal/blob/v4.3.0/lib/activerecord-bitemporal/bitemporal.rb#L315 https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/persistence.rb#L1210

The original implementation only saves the attribute_names, whereas ActiveRecord::Bitemporal::Persistence saves all unsaved changes.

The touch temporarily evacuate unsaved changes in ActiveRecord::AttributeMethods::Dirty#_touch_row. The order in which you call them is important here. In the original, this is done in the following order:

  1. Call ActiveRecord::Persistence#_touch_row
  2. Evacuate unrelated changes to the changes variable
  3. Apply changes
  4. Restore changes from the variable

https://github.com/rails/rails/blob/v7.1.0/activerecord/lib/active_record/attribute_methods/dirty.rb#L203-L222

This works fine when only attribute_names are saved, as mentioned above, but it does not work with bitemporalized models.

This PR overrides _touch_row and changes the order as follows:

  1. Evacuate unrelated changes to the changes variable
  2. Call ActiveRecord::Bitemporal::Persistence#_update_row
  3. Apply changes
  4. Restore changes from the variable

This solves the problem of unintended changes being saved and changes caused by bi-temporal operations remaining.

wata727 avatar Oct 10 '23 05:10 wata727