Any HEAD requests such as `AWS.S3.head_object/4` fails

[danschultzer]

The AWS.HTTPClient logic doesn't handle HEAD HTTP calls (in this case using AWS.S3.head_object/4):

** (exit) an exception was raised:
    ** (CaseClauseError) no case clause matching: {:ok, 200, [REDACTED]}
        (aws 0.7.0) lib/aws/request.ex:98: AWS.Request.request_rest/9

This is due to this logic in AWS.HTTPClient:

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

      error ->
        error
    end

A better approach would be to explicitly listen for the error tuples to prevent unexpected return values:

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

      {:error, reason} ->
        {:error, reason}
    end

The specs for http client expects body to be a binary, but I think either the body should be optional, or it should be permit nil value:

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

      {:ok, status_code, response_headers} ->
        {:ok, %{status_code: status_code, headers: response_headers}}
        # {:ok, %{status_code: status_code, headers: response_headers, body: nil}}

      {:error, reason} ->
        {:error, reason}
    end

[danschultzer]

If using nil value then places like these two places have to be updated:

https://github.com/aws-beam/aws-elixir/blob/master/lib/aws/request.ex#L103-L113
https://github.com/aws-beam/aws-elixir/blob/master/lib/aws/request.ex#L44

FWIW, this is the current HTTP client impl I'm using to resolve this:

defmodule MyApp.AwsHttpClient do
  require Logger

  # TODO: Waiting for upstream fix https://github.com/aws-beam/aws-elixir/issues/59

  def request(method, url, body, headers, options) do
    ensure_hackney_running!()

    options = [:with_body | options]

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

      {:ok, status_code, response_headers} ->
        {:ok, %{status_code: status_code, headers: response_headers, body: ""}}

      {:error, reason} ->
        {:error, reason}
    end
  end

  defp ensure_hackney_running!() do
    unless Code.ensure_loaded?(:hackney) do
      Logger.error("""
      Could not find hackney dependency.
      Please add :hackney to your dependencies:
          {:hackney, "~> 1.16"}
      Or provide your own #{__MODULE__} implementation:
          %AWS.Client{http_client: {MyCustomHTTPClient, []}}
      """)

      raise "missing hackney dependency"
    end

    {:ok, _apps} = Application.ensure_all_started(:hackney)
  end
end

[philss]

@danschultzer good catch! I was using a custom HTTP client and I didn't see this problem.

The specs for http client expects body to be a binary, but I think either the body should be optional, or it should be permit nil value:

The HEAD requests should not return a body, so I think we should return nil as the body for those requests, and change the appropriate places.

Would you mind to send a PR?

[philss]

And thank you for opening the issue :)

[wojtekmach]

Consider returning empty string so instead of return value be binary() | nil it’s just binary. At least Finch does that but it might vary between http clients.

[philss]

Wojtek has a good point. Returning only binary() makes things easier, since clients don't need to worry about treating nil values. So I am 👍 on returning an empty string.

[philss]

I'm going to tackle this one :)