Fix mailgun adapter error reporting #521
Conversation
Mailgun adapter was raising errors with encoded body. This caused the api_error handler to report one large string as the param. This was hard to debug and know which email was throwing the error because printable_limit config of `inspect\2` was truncating the string to 4092 characters. Dude to the way maps are sorted alphabetically we only got to see from= and html= params which didn't contain enough information to know which email it is. This change converts the body of mailgun request back to a map so we can see all params for the failed request.
When sending multipart form the body is a tuple with array of tuples for each param. We don't want to decode_query to blow up when the body is not a string so we are pattern matching to stringified body when decoding and passing the tuples straigh through
|
Thanks for this @antondomratchev! Do you think there's a way to add/update tests for this to make sure the functionality is working as expected? |
Test adds an assertion for cases when MailgunAdapter.deliver/2 raises due to mailgun API response and that it raises with a properly formatted error message containing a Map or Tuple (in the case of multipart body) instead of URL encoded string.
|
@maymillerricci Sorry it took so long to get this shored up. I hope the test coverage is satisfactory. |
|
bump |
There was a problem hiding this comment.
Hi @antondomratchev, thanks for opening this PR!
I'm taking over Bamboo maintenance, so I'm slowly catching up with issue/PR triage.
The PR looks great. I wasn't sure why the mix.lock file changed. Is that something you could revert? This is good to go aside from that.
mix.lock
Outdated
| "ranch": {:hex, :ranch, "1.3.2", "e4965a144dc9fbe70e5c077c65e73c57165416a901bd02ea899cfd95aa890986", [:rebar3], [], "hexpm"}, | ||
| "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.1", "28a4d65b7f59893bc2c7de786dec1e1555bd742d336043fe644ae956c3497fbe", [:make, :rebar], [], "hexpm"}, | ||
| "unicode_util_compat": {:hex, :unicode_util_compat, "0.3.1", "a1f612a7b512638634a603c8f401892afbf99b8ce93a45041f8aaca99cadb85e", [:rebar3], [], "hexpm"}, | ||
| "certifi": {:hex, :certifi, "2.3.1", "d0f424232390bf47d82da8478022301c561cf6445b5b5fb6a84d49a9e76d2639", [:rebar3], [{:parse_trans, "3.2.0", [hex: :parse_trans, repo: "hexpm", optional: false]}], "hexpm", "e12d667d042c11d130594bae2b0097e63836fe8b1e6d6b2cc48f8bb7a2cf7d68"}, |
There was a problem hiding this comment.
Did any dependencies change with this PR? I'm curious why we're seeing this.
There was a problem hiding this comment.
I was just doing some work on Bamboo, and I see these are coming from a simple mix deps.get. It seems we either missed committing the mix.lock file at some point or something else is going on. Either way, I don't think this is a blocker since we have to commit this file anyway.
| defp decode_body({:multipart, _} = multipart_body), do: multipart_body | ||
|
|
||
| defp decode_body(body) when is_binary(body) do | ||
| URI.decode_query(body) |
There was a problem hiding this comment.
I was giving this PR another look, and I realized this is something of a counterpart to the existing encode_body/1 function defined in line 270, right?
What do you think about using Plug.Conn.Query.decode since we use the Plug.Conn.Query.encode function to encode the data?
There was a problem hiding this comment.
Hello German! Firstly, thank you for following up on this PR, I must admit it fell a little bit off my radar, but I am happy to get the feedback. My initial inclination here was to rely solely on the elixir core modules, but given that the encode_body is already relying on Plug it does makes sense to continue the pattern here. I will be happy to make that happen. I am assuming multipart bodies are not affected here.
P.S. Hoping to hear you on the bike shed podcast again soon!
For consistency between encode_body and decode_body use Plug instead of URI directly. This is makes more sense when looking at both encode and decode in the context of the module.
|
@germsvel Feel free to merge when ready. I don't seem to have a merge button available to me |
|
Thanks for working on this @antondomratchev. 🎉 |
Mailgun adapter was raising errors with encoded body. This caused the
api_errorhandler to report one large string as the param. This was hardto debug and know which email was throwing the error because
printable_limitconfig ofinspect\2was truncating the string to 4096characters See here. Due to the alphabetical sorting of map keys we only got
to see
from=andhtml=params which didn't contain enough information toknow which email is failing. This change converts the body of mailgun request
back to a map so we can see all params for the failed request.
The limit of 150 keys is still being applied in case body has a lot of keys in it.
Curiously mailgun adapter is the only one that was raising the api_error with a string while other adapters used json decoded maps.