-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add JobIteration::DestroyAssociationJob #140
Open
bdewater
wants to merge
1
commit into
main
Choose a base branch
from
destroy_association_job
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
# frozen_string_literal: true | ||
|
||
require "active_job" | ||
|
||
module JobIteration | ||
# Port of https://github.com/rails/rails/blob/main/activerecord/lib/active_record/destroy_association_async_job.rb | ||
# (MIT license) but instead of +ActiveRecord::Batches+ this job uses the +Iteration+ API to destroy associated | ||
# objects. | ||
# | ||
# @see https://guides.rubyonrails.org/association_basics.html Using the 'dependent: :destroy_async' option | ||
# @see https://guides.rubyonrails.org/configuring.html#configuring-active-record Configuring Active Record | ||
# 'destroy_association_async_job', 'destroy_association_async_batch_size' and 'queues.destroy' options | ||
class DestroyAssociationJob < ::ApplicationJob | ||
include(JobIteration::Iteration) | ||
|
||
queue_as do | ||
# Compatibility with Rails 7 and 6.1 | ||
queues = defined?(ActiveRecord.queues) ? ActiveRecord.queues : ActiveRecord::Base.queues | ||
queues[:destroy] | ||
end | ||
|
||
discard_on(ActiveJob::DeserializationError) | ||
|
||
def build_enumerator(params, cursor:) | ||
association_model = params[:association_class].constantize | ||
owner_class = params[:owner_model_name].constantize | ||
owner = owner_class.find_by(owner_class.primary_key.to_sym => params[:owner_id]) | ||
|
||
unless owner_destroyed?(owner, params[:ensuring_owner_was_method]) | ||
raise ActiveRecord::DestroyAssociationAsyncError, "owner record not destroyed" | ||
end | ||
|
||
enumerator_builder.active_record_on_records( | ||
association_model.where(params[:association_primary_key_column] => params[:association_ids]), | ||
cursor: cursor, | ||
) | ||
end | ||
|
||
def each_iteration(record, _params) | ||
record.destroy | ||
end | ||
|
||
private | ||
|
||
def owner_destroyed?(owner, ensuring_owner_was_method) | ||
!owner || (ensuring_owner_was_method && owner.public_send(ensuring_owner_was_method)) | ||
end | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
# frozen_string_literal: true | ||
|
||
require "test_helper" | ||
|
||
module JobIteration | ||
class DestroyAssociationJobTest < IterationUnitTest | ||
setup do | ||
skip unless defined?(ActiveRecord.queues) || defined?(ActiveRecord::Base.queues) | ||
|
||
@product = Product.first | ||
["pink", "red"].each do |color| | ||
@product.variants.create!(color: color) | ||
end | ||
end | ||
|
||
test "destroys the associated records" do | ||
@product.destroy! | ||
|
||
assert_difference(->() { Variant.count }, -2) do | ||
work_job | ||
end | ||
end | ||
|
||
test "checks if owner was destroyed using custom method" do | ||
@product = SoftDeletedProduct.first | ||
@product.destroy! | ||
|
||
assert_difference(->() { Variant.count }, -2) do | ||
work_job | ||
end | ||
end | ||
|
||
test "throw an error if the record is not actually destroyed" do | ||
@product.destroy! | ||
Product.create!(id: @product.id, name: @product.name) | ||
|
||
assert_raises(ActiveRecord::DestroyAssociationAsyncError) do | ||
work_job | ||
end | ||
end | ||
|
||
private | ||
|
||
def work_job | ||
job = ActiveJob::Base.queue_adapter.enqueued_jobs.pop | ||
assert_equal(job["job_class"], "JobIteration::DestroyAssociationJob") | ||
ActiveJob::Base.execute(job) | ||
end | ||
end | ||
end |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
destroy!
? I believe that's not what's in the upstream Rails job, but otherwise some records might silently not be destroyed, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would get the job stuck if a single record cannot be deleted. It might be better to log the failures?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging for sure, perhaps even a AS::Notification topic to subscribe to so it can be hooked up to services like Bugsnag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
destroy!
would halt the deletion of the remaining objects, raise an error (which would be reported to e.g. Bugsnag), and then retry. If we then fix the deletion validation bug, the retries would automatically proceed and no custom retry or maintenance job would be required. If we fix the deletion validation bug too late, after retries are exhausted, then a maintenance task needs to be run.If we log or AS::Notification, then all services need to configure that to report to their Bugsnag equivalent, then the bug needs to be fixed, then some maintenance task needs to be run to delete what was missed.
Either way, we leave data in the DB that should be deleted. My preference is for
destroy!
, leaving less manual toil, but I don't mind too much.destroy
is what the Rails job does, so perhaps that's what should be done here. Perhaps it'd be even nicer to use the bang version on associations if the owner destroy was called with a bang.