Allow passing extra arguments to Interceptor modules

[gugahoa]

Today to configure a interceptors, we add the module to a list in the config file:

# config/config.exs
config :my_app, MyApp.Mailer,
  adapter: Bamboo.MandrillAdapter,
  interceptors: [MyApp.DenyListInterceptor]

So if we want to change the behavior of a Interceptor with a config, we have to fetch the env inside the module and do what we need to do. For example:

defmodule MyApp.Mailer.Interceptor do
  @moduledoc """
  The Mailer Interceptor.

  It changes the email structure when it's executed
  in a Staging environment to avoid sending e-mails to
  real recipients.
  """
  @behaviour Bamboo.Interceptor

  import Bamboo.Email

  @staging_recipient "[email protected]"

  def call(email = %Bamboo.Email{}) do
    if staging?() do
      recipients =
        email
        |> all_recipients()
        |> normalize_recipients()

      subject = "[STAGING] - #{inspect(recipients)} - #{email.subject}"

      %{email | cc: [], bcc: [], subject: subject, to: [{nil, @staging_recipient}]}
    else
      email
    end
  end

  defp normalize_recipients(recipients) do
    Enum.map(recipients, fn
      {nil, email} ->
        email

      {name, email} ->
        "#{name} <#{email}>"
    end)
  end

  defp staging? do
    Application.get_env(:my_app, :environment) == "staging"
  end
end

What about passing these options via the config file, like this?

# config/config.exs
config :my_app, MyApp.Mailer,
  adapter: Bamboo.MandrillAdapter,
  interceptors: [{MyApp.Mailer.Interceptor, "staging"}]

We would have to then pass the option to the call function in the Interceptor, like it's done with Plugs. What do you think of this? If it's a desirable contribution, could you give some pointers on how to implement?

[aleDsz]

# config/config.exs
config :my_app, MyApp.Mailer,
  adapter: Bamboo.MandrillAdapter,
  interceptors: [{MyApp.Mailer.Interceptor, "staging"}]

Follwing this example, i think we could have the opts paramater to the configuration, so:

config :my_app, MyApp.Mailer,
  adapter: Bamboo.MandrillAdapter,
  interceptors: [{MyApp.Mailer.Interceptor, environment: "staging", foo: :bar}]

[germsvel]

Thanks @gugahoa for opening this issue.

I can see how passing an extra set of options could be desired. If we do this, I think @aleDsz's idea of passing a tuple with the interceptor module and then the options could work nicely.

So we could make it so that the elements of the interceptors config are either the module or a two tuple with module and options. E.g.

config :my_app, MyApp.Mailer,
  adapter: Bamboo.MandrillAdapter,
  interceptors: [MyApp.Mailer.OneInterceptor, {MyApp.Mailer.AnotherInterceptor, environment: "staging", foo: :bar}]

Then, when we're applying the interceptors in the mailer, we can pattern match based on whether it's a tuple or not:

  defp apply_interceptors(email, config) do    
    interceptors = config[:interceptors] || []
    
    Enum.reduce(interceptors, email, fn 
       {interceptor, opts}, email ->   apply(interceptor, :call, [email, opts])    end)  
       interceptor, email ->   apply(interceptor, :call, [email])    end)  
    end
  end

We'd then have to change the interceptor behaviour to allow for call/1 or call/2. And I think that would still be a backwards compatible change, so no need to bump a major version.

If that fits your use case, I'd be happy to review a pull request with these changes. Otherwise, I'll try to get to it as soon as I can!

[gugahoa]

@germsvel we can't define two callbacks with different arguments for the behavior, so the approach would have to be a optional callback—to not break existing code—but since call/1 itself is not optional, the API isn't quite ergonomic. I created a commit which follows this approach, to see how it gets https://github.com/gugahoa/bamboo/commit/e5d65f8d007bddb415e07597537079143f6b3a6d

As a user I believe this API is not good. Maybe we could approach it another way? With a new behavior. The new behavior could be the recommended way to define interceptors, with the "old" way being deprecated and removed in the next major version

[aleDsz]

Using your commit, we should still use call_opts/2 instead of call/2? Using the optional callback is a good way to not ship breaking changes

[germsvel]

@gugahoa @aleDsz what if we were to make both call: 1 and call: 2 optional? (though of course, users would have to implement at least one of them) I don't think it's a breaking change since we're only relaxing the constraints of call/1. People who have defined their own call/1 can continue using it as is, and those who want to add new ones with call/2 can do so.

I did a quick spike with a simple test to check that it works: https://github.com/thoughtbot/bamboo/compare/spike-interceptors.

What do you think?

[aleDsz]

Making both optional sounds good, but i don't think it would be a good interface. Because if someone is implementing the Bamboo.Interceptor behaviour, he should implement one of both options.

So, if we make it with __using__/1 macro, we could check if actual module have call/1 or call/2 implemented. What do you think?

[germsvel]

@aleDsz yeah, I think adding the __using__/1 macro to the Interceptor module would make sense so that we can make sure at least one of the call functions is implemented.