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

Use base64 attribute for attachments #25

Merged
merged 3 commits into from
Apr 22, 2015
Merged

Use base64 attribute for attachments #25

merged 3 commits into from
Apr 22, 2015

Conversation

lime
Copy link
Contributor

@lime lime commented Apr 16, 2015

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 added a commit that referenced this pull request Apr 22, 2015
Use base64 attribute for attachments
@tardate tardate merged commit c29951a into evendis:master Apr 22, 2015
@tardate
Copy link
Member

tardate commented Apr 22, 2015

nice improvement, thanks!

@lime
Copy link
Contributor Author

lime commented May 5, 2015

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
Copy link
Member

tardate commented May 5, 2015

@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
Copy link
Contributor Author

lime commented May 6, 2015

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
Copy link
Member

tardate commented May 6, 2015

@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
Copy link
Contributor Author

lime commented May 6, 2015

Appreciate you effort

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

@lime
Copy link
Contributor Author

lime commented May 6, 2015

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
Copy link
Contributor Author

lime commented May 6, 2015

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

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
Copy link
Contributor Author

lime commented May 6, 2015

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
Copy link
Member

tardate commented May 13, 2015

@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
Copy link
Contributor Author

lime commented May 13, 2015

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

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.

2 participants