Open atcruice opened 6 months ago
I think #deserialize
should be designed similar to how #call
of ActsAsTenant::Sidekiq
looks like.
This means, it should use ActsAsTenant.with_tenant
def deserialize(job_data)
tenant_global_id = job_data.delete("current_tenant")
ActsAsTenant.current_tenant = tenant_global_id ? GlobalID::Locator.locate(tenant_global_id) : nil
super
end
Should become
def deserialize(job_data)
tenant_global_id = job_data.delete("current_tenant")
tenant = tenant_global_id ? GlobalID::Locator.locate(tenant_global_id) : nil
ActsAsTenant.with_tenant tenant do
super
end
end
@tmaier I've attempted your suggestion to resolve my issues losing current_tenant
in a Rake task after switching from Sucker Punch to GoodJob, but it doesn't seem to help.
Thinking through the change, I don't think with_tenant
for only the scope of the deserialize
method will have the intended effect, as I believe the idea is to set current_tenant
for the full duration of job execution.
I think the underlying issue with the ActiveJobExtensions implementation is still a problem and would benefit from the improvements suggested by @atcruice. I'm planning to spend a little more time thinking through this and #335 and then hopefully submit a PR. 💭
We recently encountered some difficulty trying to update to v1. The cause is an undesirable interaction between
ActsAsTenant::Sidekiq::Server
and the newActsAsTenant::ActiveJobExtensions
(introduced in #319) - due to their different tenancy control approaches:Context
We use a rolling deploy strategy, so have both old and new code in active use for a period during release. The exceptions we encountered were due to:
ActsAsTenant::ActiveJobExtensions#serialize
(missing the"current_tenant"
key)ActsAsTenant::Sidekiq::Client#call
saving job tenancy in"acts_as_tenant"
hashActsAsTenant::Sidekiq::Server#call
setting job tenancy from"acts_as_tenant"
hashActsAsTenant::ActiveJobExtensions#deserialize
then nullifyingActsAsTenant.current_tenant
because the"current_tenant"
key was absent (clobbering the existing tenancy context)Proposed Solution
I'd like to work on refactoring
ActsAsTenant::ActiveJobExtensions#deserialize
such that:ActsAsTenant.current_tenant
is already setDo these sound like reasonable improvements?