Rescue Mail::AddressList parse errors

[cjolly]

If a malformed email address makes it's way to normalize_params it currently explodes resulting in a 500 which Sendgrid retries for 72 hours. If a few of these come in per day the resulting errors snowball pretty quickly. The email will never properly parse, of course, so retrying is futile.

We've been using griddler-sendgrid for quite a few years now and for whatever reason we've been getting inundated with these the last week or so, had not seen it before! Maybe something changed at Sendgrid? In either case we think this is probably the right behavior, charsets method does the same.

[sdhull]

Interesting... I'm no longer at the company where I was using this gem, so I haven't run into this problem.

1. Have you contacted Sendgrid support to ask about the uptick in malformed to addresses?
2. (Assuming the spec you wrote is representative of the type of malformation) would it be too crazy to try to parse out the raw email addresses with a regex?
3. Is rescuing that exception in your application code insufficient for some reason? The to address is often the most important part of the message. I know that when we were using this gem, without a to address we couldn't do anything with the message.

Also assuming that spec is representative, it looks suspiciously like that last character got lopped off somewhere along the way.

[cjolly]

Hi @sdhull, thanks for taking a look.

1. Not specifically. I suspect either a mail client let a bad address get into the headers and Sendgrid let it pass thru or perhaps Sendgrid had a temporary bug that was letting them pass thru. In either case I think it validates the need to handle bad addresses somehow getting passed along.

2. No, definitely not crazy as some of the other griddler-* processor libraries do not rely on Mail::AddressList (it also seems the base griddler library has a parsing handler that could be used?) I wanted to keep my PR focused on this issue and not mess with too much.

3. Since this is a rails engine and the exception is occurring before it gets to the EmailProcessor class we control, you'd have to open up the engine's controller class and rescue it there, which seems heavy handed for something that's occurring in the library.

This missing closing bracket in the specs is intentional and representational of the data being sent to us from Sendgrid. I did update the spec to have the malformed address to be in the cc field, though.

Overall, definitely not the end of the world, as I said above, we've been using this gem for years without issue, but if a few emails slip through and get into this failure loop it can get really noisy for a few days while Sendgrid keeps retrying them.

[sdhull]

Ohhh right I forgot this was a Rails Engine. Hmmm 🤔

Checked out the Griddler::EmailParser you mention...

def self.extract_email_address(full_address)
  full_address.split('<').last.delete('>').strip
end

I think that would work?

I'm a bit concerned about silently swallowing errors because if you imagine an important email that should have been processed and it just disappears, not in your error reporting, not in your list of processed messages, how do you even debug that? Maybe add an optional error handler?

[cjolly]

@sdhull yeah, definitely don't want things disappearing without notices. However, the current situation prevents the incoming message from being processed at all. With this solution it'll at least process and show up in the EmailProcessor class you write, where you can decide to handle the empty recipient fields or raise your own error. The full headers will still be available in raw_headers.

Perhaps a parse_error? attribute on the message would help, but I think that'd have to start griddler implementing and documenting it first, then this adapter could add support.

[sdhull]

Ohhh yeah good point. Sorry I totally forgot how this library works 😂 someone else should probably be in charge of merging PRs at this point 😞

OK well your explanation makes perfect sense & is a totally reasonable way to deal with these cases. I think we should merge this. No one else has chimed in and it's been two weeks so I'm gonna merge it. If someone else wants to change it they can submit a PR to do so

[AnthonyClark]

This just came up for us this weekend too, thanks for getting a fix in already @cjolly! Any chance of a new release with this change @sdhull?

[sdhull]

@AnthonyClark I checked my access on Rubygems and unfortunately I don't have rights to publish a new version.

Looking through commit history, it looks like maybe @wingrunr21 can publish?

[wingrunr21]

Yep, I’ll cut a release today

[wingrunr21]

Released as v1.2.0

[AnthonyClark]

Thank you @wingrunr21 ! 😃

[wingrunr21]

Yep! BTW I don’t use this gem. I just inherited maintenance and such on it. If you or someone else is interested then happy to talk!

[cjolly]

@wingrunr21 and @sdhull thanks for the version bump and continued maintenance. This gem has always Just Worked:tm: for us so it's been great.

It looks like similar functionality is going to be part of Rails 6 as ActionMailbox out of the box, so I assume usage of this gem is going to start declining. I haven't tried it out yet, but if it's basically a drop-in replacement I'd assume we and others will gravitate toward the "rails way". It'll be interesting to see what the griddler project as a whole decides to do going forward.