Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent touch from saving unsaved changes #148

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wata727
Copy link
Contributor

@wata727 wata727 commented Oct 10, 2023

See also #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 wata727 force-pushed the prevent_incorrect_dirties_by_touching branch 4 times, most recently from ea3d0dc to 03753e0 Compare October 10, 2023 07:38
if @_skip_dirty_tracking ||= false
clear_attribute_changes(_touch_attr_names)
return affected_rows
end
Copy link
Contributor Author

@wata727 wata727 Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably this @_skip_dirty_tracking check is not appropriate. I'll review it later.
See also rails/rails#36271

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, perhaps we need to start with how to define implicit touching by TouchLater in bi-temporal semantics. There's a long way to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ActiveRecord::TouchLater seems completely broken in the bi-temporal gem...
Added a comment about the behavior I investigated.

@wata727 wata727 force-pushed the prevent_incorrect_dirties_by_touching branch from 03753e0 to f77f70c Compare October 11, 2023 07:54
@wata727 wata727 changed the title Prevent incorrect dirties by touching Prevent touch from saving unsaved changes Oct 11, 2023
@wata727 wata727 marked this pull request as ready for review October 11, 2023 09:07
@auto-assign auto-assign bot requested review from mkmn and wakasa51 October 11, 2023 09:07
@wata727 wata727 requested review from osyo-manga, krororo and Dooor and removed request for mkmn and wakasa51 October 11, 2023 09:07
# 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# FIXME: Implicit touching with `touch: true`` is **completely** broken in the bi-temporal gem.
# FIXME: Implicit touching with `touch: true` is **completely** broken in the bi-temporal gem.

Copy link
Collaborator

@krororo krororo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants