Fix accessing provider_job_id inside active jobs for sidekiq adapter

[kelhusseiny]

Accessing provider_job_id attribute inside a job using sidekiq adapter always returns nil. I looked at SidekiqAdapter class inside ActiveJob, and it looks like that enqueue method here already fire the job before assigning its id to job.provider_job_id.

 def enqueue(job) #:nodoc:
        #Sidekiq::Client does not support symbols as keys
        job.provider_job_id = Sidekiq::Client.push(
          'class'   => JobWrapper,
          'wrapped' => job.class.to_s,
          'queue'   => job.queue_name,
          'args'    => [ job.serialize ])
      end

I need to access it so I can use Sidekiq::Status tracking-progress methods. The basic idea behind these methods is to store the progress using the jid attribute within Sidekiq::Worker. So inside ActiveJob while we don't have access to that jidattribute, we can easily work around that by using a before_perform callback like that

class ActiveJob::Base
  attr_accessor :jid

  before_perform do |job|
    job.jid = job.provider_job_id
  end
end

but the problem now, is that provider_job_id always returns nil.

so I fixed this by merging the jid to the job_data inside JobWrapper#perform method, and then assigning it inside deserialize method.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

[mperham]

Looks fine to me. Not sure if you need the same support for the other adapters + tests.

[maclover7]

Build failures are unrelated.

[rafaelfranca]

We need tests for sure.

[rafaelfranca]

If this is being done here is it still needed to set the provider_job_id in the enqueue method?

[kelhusseiny]

Yeah we still need to set it in the enqueue method so we can access it outside the job (not inside). ie:

job = SomeJob.perform_later
jid = job.provider_job_id

[kelhusseiny]

@rafaelfranca ok, so could you give me any idea how can I test this? I believe that all we need to do here is to make sure that job_provider_id is accessible inside the job and doesn't return nil. suggestions?

[rafaelfranca]

Something like this test but exposing the value that was inside the job? https://github.com/rails/rails/blob/master/activejob/test/integration/queuing_test.rb#L62

[kelhusseiny]

@rafaelfranca ok man, done! I've added a test and CI is green now

[rafaelfranca]

Backported in cbe752f

[volkanunsal]

I know this is an old issue, but I noticed that provider_job_id value is flaky in my tests.