evendis / mandrill-rails

Webhook processing and event decoration to make using Mandrill with Rails just that much easier
MIT License
288 stars 36 forks source link

Use base64 attribute for attachments #25

Closed lime closed 9 years ago

lime commented 9 years ago

I noticed that the Mandrill documentation lists a base64 boolean attribute which can be used to determine whether the raw attachment content is encoded or not. I confirmed that this is indeed the case.

This PR exposes a base64 getter on the Attachment class, and also uses it in decoded_content in place of matching the content type against /^text/.

tardate commented 9 years ago

nice improvement, thanks!

lime commented 9 years ago

Just a heads up: we received at least one incoming webhook from Mandrill last week where the base64 attribute was missing.

I'm currently contacting Mandrill support regarding this. Hopefully it was just a one-time fluke, I'd very much want to be able to rely on the stated API rather than making guesses based on the content type. I'll let you know.

tardate commented 9 years ago

@lime hey, thanks for following this up. Hopefully Mandrill support have a good answer - it would not be great to have to factor in a fallback!

Let us know what you find out ok?

lime commented 9 years ago

This is what Mandrill support responded:

[...] Our Inbound webhook format has been updated to no longer pass the base64 parameter for the images as the content portion of the images array and the attachments array will always be a base64-encoded string, similar to our messages/send API call.

I can see that online documentation still contains the base64 parameter for the inbound webhook format. I've alerted our development team so that they can update the documentation to reflect that update.

We do apologize for the trouble. [...]

So if I understand that correctly, we need to change the behaviour to assume all attachments and images are base64. I responded to Mandrill asking for confirmation that, indeed, even text/plain and similar will be base64 encoded from now on. Will let you know ASAP.

tardate commented 9 years ago

@lime Thanks! Yep, text/plain was my first question too;-) Appreciate you effort, and I'll be interested to hear the update ... then we should know which way to jump.

lime commented 9 years ago

Appreciate you effort

Getting your production code broken by a sudden API change is a surprisingly effective motivator. ;D

lime commented 9 years ago

Okay, word from Mandrill again:

You are correct, the content of all attachments and images are transmitted with base64 encoding.

However, when I tried it out myself on our servers, this is what the payload contained:

"attachments": {
  "sample.txt": {
    "base64": false,
    "content": "This is \na sample\ntext file\n\n",
    "name": "sample.txt",
    "type": "text/plain"
  }
}

Maybe they reverted the change? I guess we'll again wait for even more clarification from Mandrill...

lime commented 9 years ago

Meanwhile, an emergency fix for the breakage would be to treat base64 as a tri-state boolean. :sparkles:

self['base64'].nil? || self['base64']

So true means true, false means false and nil means true. Sigh.

If anyone really needs this now, I've pushed a branch with the above code. You can use it by changing your Gemfile to:

gem 'mandrill-rails', github: 'lime/mandrill-rails', branch: 'base64-quickfix'

However, I'm really hoping we'll get some clarity in this soon, so we can create a permanent fix (with working tests).

lime commented 9 years ago

Here's the latest and greatest:

The images and attachments arrays do behave differently for inbound messages. Images must be base64 encoded, so the images array content will always be base64 encoded and will not include the base64 attribute, while the Inbound attachments array will behave as described [...] with the base64 attribute included.

Sorry for any confusion.

Okay.

I guess the good news is that attachments behaves like it used to. And the images accessor was only added in #26, so only those users who have very recently upgraded might even be using it at all.

What would be the best way to resolve this then? The quick fix above works, but is ugly and unclear. We could create a new Mandrill::WebHook::Image class, that either inherits from Attachment or implements the almost-same-but-not-quite behaviour.

Thoughts, @tardate?

tardate commented 9 years ago

@lime thanks for the update, sorry took me a few days to follow-up.

Seems like Mandrill::WebHook::Image < Mandrill::WebHook::Attachment that overrides base64 is a good solution. Can you send PR for that? Or if no time right now let me know and I can easily add..

lime commented 9 years ago

Sure, I plan to get it done tonight. :)