influitive / apartment-sidekiq

Sidekiq support for the Apartment Gem
MIT License
126 stars 54 forks source link

Job running on the wrong tanent #27

Open Frank004 opened 6 years ago

Frank004 commented 6 years ago

I'm using

gem 'apartment', '~> 2.0'
gem 'apartment-sidekiq', github: 'tmster/apartment-sidekiq'
gem 'sidekiq', '~> 5.0.0'

ruby '2.4.2'
gem 'rails', '5.0.6'
gem 'activerecord-import', '~> 0.20.0'

I got a job that creates a batch of records on the sidekiq dashboard I get this information.

Job BuildFooComponents

Extras {"apartment"=>"other_tanent"}

Error Class

ActiveRecord::RecordNotUnique

Error Message

PG::UniqueViolation: ERROR: duplicate key value violates unique constraint "foo_pkey" DETAIL: Key (id)=(3880) already exists. : INSERT INTO "foo"(column) VALUES (nextval('demo.foo_id_seq')

if you can see the tanent VALUES (nextval('demo.foo_id_seq') is not the same from the extras "apartment"=>"other_tanent"

the job

class BuildFooComponents
  include Sidekiq::Worker
  def perform(components, section_id, project_id)
    group_components=[]
    ...
   FooComponent.import group_components
  end

end
jdrago999 commented 6 years ago

Yes I have run into this problem myself.

I haven't yet looked into solving the problem, but I have sidekiq-web running on a non-multitenant Rails app, and when I retry jobs from the sidekiq-web dashboard there, they get run on the wrong tenant within my multitenant Rails apps.

This happens even though the job itself contains the apartment argument.

jdrago999 commented 6 years ago

For my Future Self, for anyone else from $MY_TEAMS coming here, and for everyone else:

If you're using Rails 5, do not use apartment-sidekiq until it is fixed for Rails 5.

See this blog: http://fuzzyblog.io/blog/rails/2017/08/21/rails-apartment-tenancy-and-sidekiq.html

Your welcome.

Frank004 commented 6 years ago

@jdrago999 I need to make time to figure this out. :/

Frank004 commented 6 years ago

@jdrago999 bookmarking it thanx

alejandrodevs commented 5 years ago

I checked this issue and I was not able to replicate it. Does someone know if this is still happening in newer versions of rails?

alejandrodevs commented 5 years ago

Nevermind, it is still happening.

alejandrodevs commented 5 years ago

I was able to solve this without this gem. This is the approach:

This is for active jobs (app/jobs/application_job.rb):

class ApplicationJob < ActiveJob::Base
  before_enqueue do |job|
    tenant = Apartment::Tenant.current
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    job.arguments.last.merge!(tenant: tenant)
  end

  before_perform do |job|
    tenant = job.arguments.last[:tenant]
    Apartment::Tenant.switch!(tenant)
  end
end

This is for action mailer config/initializers/mail_delivery_job.rb:

module ActionMailer
  class MailDeliveryJob < ActiveJob::Base
    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      args = job.arguments.last[:args]
      args.push({}) unless args.last.is_a?(Hash)
      args.last.merge!(tenant: tenant)
      job.arguments.last[:args] = args
    end

    before_perform do |job|
      args = job.arguments.last[:args]
      tenant = args.last[:tenant]
      Apartment::Tenant.switch!(tenant)
    end
  end
end

With this you can continue using your jobs and mailers without passing extra params and you get the correct tenant in your jobs.

I hope it is useful for someone.

Frank004 commented 5 years ago

I was able to solve this without this gem. This is the approach:

This is for active jobs (app/jobs/application_job.rb):

class ApplicationJob < ActiveJob::Base
  before_enqueue do |job|
    tenant = Apartment::Tenant.current
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    job.arguments.last.merge!(tenant: tenant)
  end

  before_perform do |job|
    tenant = job.arguments.last[:tenant]
    Apartment::Tenant.switch!(tenant)
  end
end

This is for action mailer config/initializers/mail_delivery_job.rb:

module ActionMailer
  class MailDeliveryJob < ActiveJob::Base
    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      args = job.arguments.last[:args]
      args.push({}) unless args.last.is_a?(Hash)
      args.last.merge!(tenant: tenant)
      job.arguments.last[:args] = args
    end

    before_perform do |job|
      args = job.arguments.last[:args]
      tenant = args.last[:tenant]
      Apartment::Tenant.switch!(tenant)
    end
  end
end

With this you can continue using your jobs and mailers without passing extra params and you get the correct tenant in your jobs.

I hope it is useful for someone.

Thank you, I will try this and let you know. this will be a game-changer for me.

Frank004 commented 5 years ago

@alejandrodevs Did you added anything else for normal jobs?? like processing background jobs (images, forms, and data process)?

alejandrodevs commented 5 years ago

No, I didn't add anything else. Just your job classes must inherit from ApplicationJob and that's it. Are you having an issue?

Frank004 commented 5 years ago

No at the moment, just checking the configurations before starting the test. This is a problem I been trying to fix for some time now. Gracias.

Frank004 commented 5 years ago

No, I didn't add anything else. Just your job classes must inherit from ApplicationJob and that's it. Are you having an issue? Oh I can't use include Sidekiq::Worker when I inherit from ApplicationJob. mmmMM..

alejandrodevs commented 5 years ago

My solution works using active job with sidekiq. I don't know how to do it directly with sidekiq workers.

Frank004 commented 5 years ago

My solution works using active job with sidekiq. I don't know how to do it directly with sidekiq workers.

Using this solution you can't use perform_now or you will get a tenant | nil as this code never gets run

    before_enqueue do |job|
      tenant = Apartment::Tenant.current
      job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
      job.arguments.last.merge!(tenant: tenant)
   end
alejandrodevs commented 5 years ago

You can make it works just by adding checking for the current tenant in the before_perform callback:

before_perform do |job|
  args = job.arguments.last[:args]
  tenant = args.last[:tenant] || Apartment::Tenant.current
  Apartment::Tenant.switch!(tenant)
end
Frank004 commented 5 years ago

@alejandrodevs This what I end up using.

  before_perform do |job|
    job.arguments.push({}) unless job.arguments.last.is_a?(Hash)
    if job.arguments.last.has_key?(:tenant)
      tenant = job.arguments.last[:tenant]
    else
      tenant = Apartment::Tenant.current
    end
    Apartment::Tenant.switch!(tenant)
  end
h0jeZvgoxFepBQ2C commented 4 years ago

So this is still not fixed? Is it fixable at all?

tlkiong commented 3 years ago

I think I found the issue. It should be most probably because this did not get run or it didn't run in the correct order

You can try adding the middleware directly yourself in your sidekiq initializer. Eg:

in config/initializers/sidekiq.rb:

Sidekiq.configure_client do |config|
  config.client_middleware do |chain|
    chain.add ::Apartment::Sidekiq::Middleware::Client
  end
end

Sidekiq.configure_server do |config|
  config.client_middleware do |chain|
    chain.add ::Apartment::Sidekiq::Middleware::Client
  end
  config.server_middleware do |chain|
    if defined?(::Sidekiq::Batch::Server)
      chain.insert_before ::Sidekiq::Batch::Server, ::Apartment::Sidekiq::Middleware::Server
    else
      chain.add ::Apartment::Sidekiq::Middleware::Server
    end
  end
end