Skip to content

Fix deleted time order between associations#154

Merged
wata727 merged 1 commit intokufu:masterfrom
wata727:fix_ordering_of_deleted_times
Jan 31, 2024
Merged

Fix deleted time order between associations#154
wata727 merged 1 commit intokufu:masterfrom
wata727:fix_ordering_of_deleted_times

Conversation

@wata727
Copy link
Copy Markdown
Contributor

@wata727 wata727 commented Jan 30, 2024

This PR fixes a strange problem with deletion times when deleting a model with a Bi-Temporal model association with dependent: :destroy.

Normally, when an association is deleted by dependent: :destroy, it is deleted before its parent by before_destroy callback.
https://github.com/rails/rails/blob/v7.1.3/activerecord/lib/active_record/associations/builder/association.rb#L141

However, strangely, the bitemporal gem creates a history where the parent is deleted first:

employee = Employee.create(name: "John", address: Address.new(city: "Tokyo"))
emplpyee.destroy!

employee.valid_at(employee.address.valid_to) { |e| e.reload } # => ActiveRecord::RecordNotFound

Why does this happen? The cause is the timing of calculating the operation time. ActiveRecord::Bitemporal::Persistence#destroy is assigned the time when it is called as operated_at:

def destroy(force_delete: false, operated_at: Time.current)

On the other hand, the callback fires inside the destroy method:

This causes the actual deletion to occur from child to parent, but the operation time is calculated from parent to child.

This PR fixes the problem by calculating the operation time inside the destroy callback.

end

it "create state-destroy record before _run_destroy_callbacks" do
it "create state-destroy record around _run_destroy_callbacks" do
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This description is already does not represent the actual scenario, so I have revised it.
Perhaps the description no longer matches what has already been tested in #19.

@wata727 wata727 marked this pull request as ready for review January 30, 2024 09:33
@auto-assign auto-assign bot requested review from lighty and yono January 30, 2024 09:33
@wata727 wata727 requested review from Dooor and krororo and removed request for lighty and yono January 30, 2024 09:33
Copy link
Copy Markdown
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.

🚀

Copy link
Copy Markdown
Contributor

@Dooor Dooor left a comment

Choose a reason for hiding this comment

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

🙆

@wata727 wata727 merged commit a958877 into kufu:master Jan 31, 2024
@wata727 wata727 deleted the fix_ordering_of_deleted_times branch January 31, 2024 08:04
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.

3 participants