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

Dont call as_json on Delayed::Backend::Sequel::Job #473

Closed

Conversation

davidbegin
Copy link
Contributor

instead call #to_hash, and turn all the keys to strings, to
keep the rest of the implementation untouched

@davidbegin
Copy link
Contributor Author

We are seeing an error in Rollbar, when using Sequel and Delayed Job.
'undefined method `as_json'' for #Delayed::Backend::Sequel::Job:0x007fe7f57d31f8

In this file: https://github.com/rollbar/rollbar-gem/blob/master/lib/rollbar/plugins/delayed_job/job_data.rb#L9

we are calling on @as_json on this class in Delayed Job Sequel
https://github.com/TalentBox/delayed_job_sequel/blob/56e7d69e1710b75ae7044a884961e1ebfd6a4661/lib/delayed/backend/sequel.rb

I updated Rollbar to just use #to_hash, if #as_json is not defined.

  instead call #to_hash, and turn all the keys to strings, to
  keep the rest of the implementation untouched
@davidbegin davidbegin force-pushed the delayed-job-sequel-as-json-error branch from df4d175 to 81bf65c Compare May 24, 2016 20:18
@jondeandres
Copy link
Contributor

Hey @davidbegin, thanks for this. Let me test it and we'll merge it.

@@ -6,7 +6,12 @@ def initialize(job)
end

def to_hash
job_data = job.as_json
job_data = if job.respond_to?(:as_json)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should extract this code into its own method. And also, we should ensure that job responds to #to_hash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this, I will work on those changes right now.

@davidbegin
Copy link
Contributor Author

As I started to add specs to cover if the job object didn't respond to #as_json or #to_hash I noticed various problems.

Here are my debugging notes:


First I noticed that the spec file for JobData is not being run, because it doesn't end with _spec.

spec/rollbar/plugins/delayed_job/job_data.rb

Once I update to job_data_spec, and run the specs, I get:

cannot load such file -- rollbar/delayed_job (LoadError)

I am not sure what is trying to be loaded, as there is no rollbar/delayed_job file.
So for my next debugging step, I just commented out this require to see what would happen.

this triggers:

spec/rollbar/plugins/delayed_job/job_data_spec.rb:12:in `<top (required)>': uninitialized constant Rollbar::Delayed (NameError)'`

then I noticed that the spec is testing the class

describe Rollbar::Delayed::JobData do

but if we open up:

lib/rollbar/plugins/delayed_job/job_data.rb

We see it's only declared as

class JobData

and not in the Rollbar::Delayed namespace.

When I search to see where JobData is used, I see its only used in

lib/rollbar/plugins/delayed_job/plugin.rb

And it is indeed called within the Rollbar::Delayed namespace, so I assume this was a mistake.

So next I place JobData in this namespace

When I run the specs I still get the error:

spec/rollbar/plugins/delayed_job/job_data_spec.rb:12:in `<top (required)>': uninitialized constant Rollbar::Delayed (NameError)'`

So I require the JobData class explicitly:

require 'rollbar/plugins/delayed_job/job_data'

And the spec finally runs!
...except it fails with:

expected `{"attempts"=>1, "priority"=>0, "handler"=>{}}.eql?({"priority"=>0, "attempts"=>1, "handler"=>{"foo"=>"bar"}})` to return true, got false

I had a feeling that it was raising an error, and being caught in the handler_object method,
so when I outputed the error I see:

uninitialized constant Delayed::Backend::Base::DeserializationError

I assumed this was because I commented out the require "rollbar/delayed_job"

So I instead added require "delayed_job"

Now the spec knows about Delayed::Backend::Base::DeserializationError, but a new error is raised:

undefined method `object' for {"foo"=>"bar"}:Hash`

Which is from the first line in the handler_object method:

object = job.payload_object.object

job.payload_object returns a hash, which doesn't respond to object
job in this case is a Delayed::Backend::Test::Job

So far this is where I have got, but since I going down a rabbit-hole, I thought it might be good to share my findings and get some help.

Thanks!

@jondeandres
Copy link
Contributor

Hey @davidbegin,

thank you very much for the notes. You've found a missing piece we forgot to move when we added the plugins architecture. So, JobData should be in the correct namespace and the require in the job_data spec should be fixed.

We'll do this change ourself and check that the specs are passing in master. After that, I think it's a good starting for your PR.

@michaelirey
Copy link

@jondeandres Has there been any movement on this PR? Is there anything I could do to help?

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants