Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error responses from http client are not captured in generated type specs #121

Open
jessestimpson opened this issue Mar 4, 2025 · 3 comments · May be fixed by #122
Open

Error responses from http client are not captured in generated type specs #121

jessestimpson opened this issue Mar 4, 2025 · 3 comments · May be fixed by #122

Comments

@jessestimpson
Copy link

Dialyzer fails to validate the following code, which I believe to be correct usage of aws-elixir.

  def get_object(client, bucket, key) do
    case AWS.S3.get_object(client, bucket, key) do
      {:ok, _, %{body: body, status_code: 200}} ->
        body

     error={:error, :timeout} ->
        Logger.error("Timeout GET for: #{bucket}:#{key}.")
        error
    end
  end

The error is

 lib/myapp/s3.ex:xxx:pattern_match
The pattern can never match the type.

Pattern:
{:error, :timeout}

Type:

  {:error, {:unexpected_response, _} | %{binary() => _}}
  | {:ok, %{binary() => _}, %{:body => _, :status_code => _, _ => _}}

If you trace the supported error types for this example, you'll find:

  • :hackney:request/5 return spec includes {:error, term()}
  • AWS.HTTPClient.request/5 return spec includes {:error, term()}
  • AWS.Request.request_rest/9 and AWS.Client.request/6 do not define a type spec, but pass through {:error, _} from the HTTPClient
  • AWS.S3.get_object/23 return spec does not allow for {:error, term()}. However, the last function call is to AWS.Request.request_rest/9, so there is a discrepancy here on allowable returns

I confirmed that adding {:error, term()} to the return spec on AWS.S3.get_object/23 fixes this dialyzer error, and I'm prepared to submit a proper PR to the codegen for this.

However, this is obviously not ideal since it counteracts the intention of the more prescriptive error response type specs. In other words, in dialyzer's eyes, allowing term() means t:get_object_errors/0 is irrelevant.

Looking for guidance on a proper way to fix the type discrepancy. Thanks!

@onno-vos-dev
Copy link
Member

I'll have a look earliest on Friday at this. Need some time to dive into this in suggest a good way forward. Quick thought, I don't have a problem per sé with your suggestion here. I agree it'll make the spec partially redundant but it's something we've also done in aws-erlang (See: aws_s3.erl#L8446-L8449) so I'm not necessarily opposed to your suggestion just to get you to move forward 👍

@jessestimpson
Copy link
Author

Ah, since there is some precedent, I'll create my PR, so that if you decide on that approach, maybe it will save you some work 😄 . Thanks!

@jessestimpson jessestimpson linked a pull request Mar 5, 2025 that will close this issue
@jessestimpson
Copy link
Author

Hi @onno-vos-dev any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants