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

Add JobIteration::DestroyAssociationJob #140

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
### Master (unreleased)

- [241](https://github.com/Shopify/job-iteration/pull/241) - Require Ruby 2.7+, dropping 2.6 support
- [241](https://github.com/Shopify/job-iteration/pull/241) - Require Rails 6.0+, dropping 5.2 support
- [140](https://github.com/Shopify/job-iteration/pull/140) - Add `JobIteration::DestroyAssociationJob` to be used by Active Record associations with the `dependent: :destroy_async` option

## v1.3.6 (Mar 9, 2022)

Expand Down
49 changes: 49 additions & 0 deletions lib/job-iteration/destroy_association_job.rb
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
Copy link

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link

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.

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
43 changes: 42 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,41 @@ def enqueue_at(job, _delay)

ActiveJob::Base.queue_adapter = :iteration_test

class Product < ActiveRecord::Base
if defined?(ActiveRecord.queues) || defined?(ActiveRecord::Base.queues)
class ApplicationJob < ::ActiveJob::Base
end

require "active_record/destroy_association_async_job"
require "job-iteration/destroy_association_job"

ActiveRecord::Base.destroy_association_async_job = JobIteration::DestroyAssociationJob

class Product < ActiveRecord::Base
has_many :variants, dependent: :destroy_async
end

class SoftDeletedProduct < ActiveRecord::Base
self.table_name = "products"
has_many :variants, foreign_key: "product_id", dependent: :destroy_async, ensuring_owner_was: :deleted?

def deleted?
deleted
end

def destroy
update!(deleted: true)
run_callbacks(:destroy)
run_callbacks(:commit)
end
end
else
class Product < ActiveRecord::Base
has_many :variants, dependent: :destroy
end
end

class Variant < ActiveRecord::Base
belongs_to :product
end

host = ENV["USING_DEV"] == "1" ? "job-iteration.railgun" : "localhost"
Expand Down Expand Up @@ -71,6 +105,13 @@ class Product < ActiveRecord::Base

ActiveRecord::Base.connection.create_table(Product.table_name, force: true) do |t|
t.string(:name)
t.string(:deleted, default: false)
t.timestamps
end

ActiveRecord::Base.connection.create_table(Variant.table_name, force: true) do |t|
t.references(:product)
t.string(:color)
t.timestamps
end

Expand Down
50 changes: 50 additions & 0 deletions test/unit/destroy_association_job_test.rb
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