Re-introduce support for headers

[akalra]

Hey Brad --

Thanks for the gem. I noticed you pulled out headers support because of a bug surfaced in #3.

I needed headers for a project I'm working on (we rely on headers to detect spam and vacation auto-replies), so I went ahead and fixed the underlying issue here: https://github.com/akalra/griddler-mailgun/commit/56bf97232c147c44c7f61e73c549e77a1890a802

It's an interim fix at best, and though it works just fine for our emails on Mailgun, I didn't really put it through the wringer or anything yet. Let me know if you'd like me to submit a pull request.

IMHO, the right fix is to get Griddler to optionally accept fully-parsed headers instead of raw headers (since we already get those from Mailgun, and faking 'raw headers' seems silly). The challenge with that approach is two-fold. Firstly, Griddler doesn't do any parsing (it leans on ActionMailer for that) and the parsed version it returns is a Mail::Header not a plain HashWithIndifferentAccess. Also, Griddler returns both raw and parsed headers to the user -- if it wanted to continue doing that (even if we only gave it fully-parsed headers) it would then be doing the silly faking of 'raw headers'.

So it's possible that this is the least-evil of all fixes, but probably worth a conversation with Griddler?

Thanks!

Ankur

[bradpauly]

It might be the best way to go for now. I've been thinking about ways to get some mailgun specific things into the mail processor. There has been some discussion on the main griddler gem under this pull request https://github.com/thoughtbot/griddler/pull/132

@calebthompson have any thoughts on this? Would prefer that we send recomposed "raw" headers?

[calebhearth]

My vision is to have a field on Griddler::Email, such as #extras that returns a hash, which you've passed to me, including whatever you'd like to do extra processing work on or need access to.

If you'd like to take a whack at adding that, I'd appreciate it. #extras is just an off-the-top-of-my-head thing, so feel free to take liberties there.

[akalra]

I think that that is a related, but slightly different issue from the one we're discussing here.

I absolutely agree that provider-specific useful data makes sense to have in some supplementary location that Griddler-core ignores.

There's a separate issue about the interface between Griddler-core and the adapters. Right now, Griddler-core expects the adapters to do the least amount of processing possible on the incoming data and send in as close to raw data as possible. This is a super-friendly approach that makes it easy to keep things DRY.

The downside to this is that some providers (like Mailgun) don't even give the adapter the raw data. So as much as we'd prefer to have Griddler-core parse out the big blob of header text into individual keys and values, we don't have a big blob of text to give you. We get a fully parsed JSON dictionary from Mailgun, and the best we can do is fake it by serializing that into a big blob of text that you then re-parse out. That seems a little silly.

There's probably a variety of different things we could do here, not sure which makes the most sense overall.

As a strawman proposal, we could consider migrating to a world where the formal Griddler-core to Adapter interface expects the most processed form of each element, but has a variety of helpers and syntactic sugar to accept less processed versions. So adapters that need the processing aren't repeating the logic internally, but adapters that don't aren't doing silly things like undo-ing processing.

Thoughts?

[calebhearth]

We get a fully parsed JSON dictionary from Mailgun, and the best we can do is fake it by serializing that into a big blob of text that you then re-parse out.

Ironically, a better option is causing issues.

consider migrating to a world where the formal Griddler-core to Adapter interface expects the most processed form of each element

I'm against this for a couple of reasons:

• We've already done the major extraction, meaning that this would introduce a bunch of work for four or five different people across as many repos.
• It doesn't make sense from a numbers perspective, since only one of currently four adapters needs this change.

I'm still not sure I follow exactly what the problem is - what information that you need isn't available? You mentioned spam and auto replies, both of which still make sense to me as an "extras" thing.

[akalra]

The immediate issue is just that for Mailgun, we have no way to actually provide you raw_headers. We fake it now, but the faking seems brittle -- internally, griddler-mailgun doesn't know anything about the grammar of email headers. We just concatenate a bunch of strings that 'look right' for the cases in front of us.

And as you point out, it seems silly that we do a bunch of brittle work that Griddler-core immediately undoes :P

[calebhearth]

To me, "raw" always meant "whatever we're getting from the service", not "email headers". That's one (of many) thing I'd almost rather drop completely, as it's mostly a pain from a compatibility standpoint.

Seems like this could be solved with factories. @bradpauly what about something like this:

class Griddler::EmailFactory
  def initialize(params_from_service)
    # the contents of Griddler#Email.initialize
  end

  def email
    # a Griddler::Email
  end
end

And we delete basically all of Griddler::Email, making it a dumb object.

That would give you the opportunity to override that factory with something you could use more easily.

And in Griddler it would be

  def create
    normalized_params.each do |p|
      process_email build_email(p)
    end

    head :ok
  end

  def build_email(p)
    configured_factory.new(p).email
  end

[bradpauly]

@calebthompson this sounds pretty good to me. So any adapter that wanted to add more the the email would do so in the in the email method?

At one point I was thinking each adapter could return its own kind of email, so instead of a Griddler::Email, it would be a Mailgun::Email.. something along those lines. I haven't had much time to think it through.

[bradpauly]

I've merged a PR from @wingice (https://github.com/bradpauly/griddler-mailgun/pull/5) that will at least give access to headers until another solution can be implemented.

[bradpauly]

@calebthompson is the idea of #extras still something you're interested in adding to griddler?

[bradpauly]

@akalra Griddler allows passing parsed headers: https://github.com/thoughtbot/griddler/pull/185 I'm thinking about doing that instead of the way its handled now.

[calebhearth]

Pinging @gabebw. I've stepped back from Griddler maintenance for the most part.