make delayed_job exceptions work (issue 291)

[ri4a]

No description provided.

[jondeandres]

@ri4a thanks for this, I guess this fix #94 (comment) ?

I'll test this and merge if everything is ok. I think we'd do better using Rollbar::JSON.dump interface, but the fix looks reasonable.

Thanks!

[jondeandres]

@ri4a sorry, this PR doesn't fix #94.

I'm looking at this, thanks again for the PR.

[jondeandres]

@ri4a

I've looked at your comments in #291 (comment).

We removed this part of code:

is_symbol = object.is_a?(Symbol)

return object unless object == object.to_s || is_symbol

But we are doing this in lib/rollbar/encoding.rb:

def self.encode(object)
    can_be_encoded = object.is_a?(String) || object.is_a?(Symbol)

    return object unless can_be_encoded

    encoding_class.new(object).encode
end

I'd say it's basically the same, maybe I'm missing something. The goal there is only encode String and Symbol objects.

Having return object unless object == object.to_s is just a way to check it is a String object.

Are you sure the problem is here? It'd be nice having a test that exploits the current bug.

Thanks.

[ri4a]

@jondeandres

Yes, you are right. I looked at Rollbar::Encoding::Encoder#encode instead of Rollbar::Encoding::encode :unamused:

The real cause is in /lib/rollbar/truncation/mixin.rb, which changed from

def dump(payload)
  MultiJson.dump(payload)
end

to

def dump(payload)
  Rollbar::JSON.dump(payload)
end

Apparently the serialization of Ruby objects is different (seems that now only to_s is called).

To create a test that exploits the bug, it depends on what you consider to be the actual bug here. The fact that a method called "enforce_valid_utf8" is called with unserialized Ruby objects seems strange, since the serialization afterwards could cause new encoding issues. Should Rollbar::Encoding::encode really accept (i.e. ignore) Ruby objects and trust they will be serialized correctly later by Truncation#dump? Or should it be the responsibility of the data injectors to first serialize? Or maybe "enforce_valid_utf8" should just be called later.

This PR fixes #291 by serializing the Job object where it is injected. It could also be fixed by amending Rollbar::JSON. That may or may not also fix #94, haven't looked at that.

[jondeandres]

@ri4a that has more sense. the enforce_valid_ut8 method is not the problem here. I agree with you that maybe we should enforce utf8 after dump the payload. The thing here is that we had some problems with some srings when an async handler pushed to its queue, so we enforce utf8 before.

Apart of this, and focused in the bug, we should probably pass request: job.payload_object.as_json from the beginning and not a delayed job Job instance.

I've opened a PR for this, #293.

I think we can close this one, but anyway, thanks for open it!