Any HEAD requests such as `AWS.S3.head_object/4` fails
Closed this issue · 6 comments
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
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
@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?
And thank you for opening the issue :)
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.
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.
I'm going to tackle this one :)