peburrows / goth

Elixir package for Oauth authentication via Google Cloud APIs
http://hexdocs.pm/goth
MIT License
289 stars 111 forks source link

Simplify http client contract #125

Closed wojtekmach closed 2 years ago

wojtekmach commented 2 years ago

While the behaviour works well, I believe a much simpler contract will be a 1-arity function that accepts a keyword list with :method, :url, :headers, :body and passes remaining options through as adapter-specific. And it returns {:ok, map} | {:error, exception}.

We'd configure it as simply: http_client: &mod.fun/1. If you'd want to adjust options, you'd: http_client: {&mod.fun/1, options}.

By default, we'd ship with Hackney-based adapter. To pass additional options to it, we'd do:

http_client: {&Goth.__hackney__/1, options}

Here's roughly how the implementation would work (untested):

  defmacrop ensure_hackney do
    if Code.ensure_loaded?(:hackney) do
      :ok
    else
      quote do
        unless Code.ensure_loaded?(:hackney) do
          raise "missing hackney dependency"
        end
      end
    end
  end

  def __hackney__(options) do
    ensure_hackney()

    {method, options} = Keyword.pop!(options, :method)
    {url, options} = Keyword.pop!(options, :url)
    {headers, options} = Keyword.pop!(options, :headers)
    {body, options} = Keyword.pop!(options, :body)
    options = [:with_body] ++ options

    case :hackney.request(method, url, headers, body, options) do
      {:ok, status, headers, body} ->
        {:ok, %{status: status, headers: headers, body: body}}

      {:error, reason} ->
        {:error, RuntimeError.exception(inspect(reason))}
    end
  end
aleDsz commented 2 years ago

I was thinking about keep the current implementation but, using pattern matching, if the configuration is with {module, opts} we emit a warning about the implementation is being deprecated. WDYT?

  defp start_http_client({module, _} = config) when is_atom(module) do
    Logger.warn("""
    The `:httpc_client` configuration with `{module, options}` is being deprecated.
    We will be supporting this implementation until the legacy code been deleted.

    Consider updating your configuration:

        defmodule MyApp.Application do
          use Application

          def start(_, _) do
            children = [
              # ...
              {Goth,
                name: MyApp.Goth,
                source: {:service_account, credentials, opts},
                httpc_client: &MyApp.HTTP.request/1
              }
            ]

            Supervisor.init(children, strategy: :one_for_one)
          end
        end

    Check the documentation for more information.
    """)

    Goth.HTTPClient.init(config)
  end
wojtekmach commented 2 years ago

:http_client option never existed in a stable version, it was only for a RC, but as a thank you to people who tried RC, if doesn't complicate things to keep it around, let's do that. I would keep it only until 1.4 or so though. Deprecation can be much simpler too: setting http_client: {mod, opts} is deprecated in favour of http_client: &fun/1