Fix Sidekiq delivery method for non-UTF8 email

[DouweM]

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/2698.

[DouweM]

@tpitale Could you do another release? We want to get this in a GitLab patch release.

[tpitale]

Woah. You just created a PR, and merged it yourself without any look from me? And you added a new gem dependency to do it?

[DouweM]

@tpitale :/ Sorry about that, I didn't really think about it. I thought it'd be okay as it's a bug we've encountered with Sidekiq/JSON/text encoding in GitLab a few times already, where we use the same fix.

I thought part of the reason why I was added as a collaborator was to be able to quickly merge minor (in my eyes) stuff like this, but I realize we never discussed that.

I'm happy to revert it and discuss it properly.

[tpitale]

It's okay, I'm going to release this gem. But, I'd like to discuss ways we can get rid of the dependency. I think there are some simple things we can do in ruby to check for compatible encoding changes and try to encode them. If they can't be switched between encodings … we can decide what to do then.

We also probably have this issue with essentially every other delivery method.

[tpitale]

Released 0.5.2.

[tpitale]

So, one issue I see is that charlock_holmes is a C extension. Not that we explicitly support jruby, but I like to imagine we do 😄

[DouweM]

I think there are some simple things we can do in ruby to check for compatible encoding changes and try to encode them.

I'm not so sure, what we need to do is detect the encoding based on the content, and that's exactly what CharlockHolmes was written for. Next, we need to convert it, which Ruby can't do by default either.

We also probably have this issue with essentially every other delivery method.

We have the issue with Sidekiq, because we're encoding the message body as JSON, which requires UTF-8. If the message body isn't UTF-8 but ASCII-8BIT, JSON will error out. :( Other delivery methods don't do JSON encoding, so probably don't have this issue, right?

So, one issue I see is that charlock_holmes is a C extension. Not that we explicitly support jruby, but I like to imagine we do 😄

Fair enough. :)

[tpitale]

Old but relevant: http://graysoftinc.com/character-encodings/ruby-19s-string

[tpitale]

Also, the new Que delivery method does dump json: https://github.com/tpitale/mail_room/blob/master/lib/mail_room/delivery/que.rb#L57

[DouweM]

@tpitale Right you are :/ I jumped the gun on merging this, my bad.

[tpitale]

Can you open an issue, with some of the email encoding that was causing issues for gitlab users? I'd like to take a stab at running it through mail_room and fixing the issue with just ruby. If it turns out it can't be done, we'll expand our use of charlock (or something else maybe).

[tpitale]

@DouweM in the future, I'd love to at least have a chance to 👍 any PRs before merging. I'll try to be responsive. I do appreciate all your help with this, and I'm excited that it's helping gitlab.

[DouweM]

A reproduction of the specific case GitLab users were having trouble with is this:

message = "é"
message.force_encoding("ASCII-8BIT")

Note that in this case, the message was actually UTF-8-encoded, but for some reason Ruby thought it was ASCII-8BIT, and when trying to do JSON.generate, it tried to convert this string that was actually UTF-8 encoded from ASCII-8BIT to UTF-8, yielding the "\xC3" from ASCII-8BIT to UTF-8 (Encoding::UndefinedConversionError) error you see in https://gitlab.com/gitlab-org/gitlab-ce/issues/2698.

Reproduce this by doing:

JSON.generate(message) # => error

In this case, simply doing message.force_encoding('UTF-8') would work, but not in this case, where the original message was ISO-8859-1 rather than UTF-8:

message = "é".encode('ISO-8859-1')
message.force_encoding('ASCII-8BIT')
message.force_encoding("UTF-8")
JSON.generate(message) # => error

[tpitale]

Thanks, I'm going to make this an issue.

[DouweM]

@tpitale I completely understand, and I will do better at that in the future. I thought this MR was minor, but as we see now, it definitely wasn't.

[tpitale]

No worries. Every OSS project is different, too. I appreciate you being considerate and working with me on this.

[tpitale]

I've opened issue #44 and I'll try to see if I can fix this in ruby for Que.

[DouweM]

Thanks @tpitale