elixir-tesla / tesla

The flexible HTTP client library for Elixir, with support for middleware and multiple adapters.
MIT License
2.01k stars 348 forks source link

Versions 1.9 and later changed the return value for a Mint.TransportError in Tesla.Adapter.Finch.call/2 #696

Open cliftonmcintosh opened 4 months ago

cliftonmcintosh commented 4 months ago

Thank you to everyone who contributes to Tesla. Our organization really, really appreciates having it.

I wanted to bring your attention to a change that has led to a change in the return value for the Finch adapter. This occurred in the 1.9.0 update, and I'm not sure if there is anything to be done about it now. In our organization, this change will cause us to do a major version bump in an internal library we use when we upgrade Tesla in that library.

When upgrading from 1.8 in one of our libraries, tests have started failing because the return value from a Mint.TransportError has changed.

Previously, when a connection was down, it returned "connection refused".

It now returns :econnrefused.

The behaviour for Tesla.Adapter.call/2 only guarantees a return of {:error, any()} for an error, so the new value fits the contract, but it is a change from what was previously returned so any project depending on the return value for the error will need to be updated.

I believe the changes in https://github.com/elixir-tesla/tesla/commit/d488bb272939cdafc922fb7f2f8c9d2a22682e38 led to this.

It previously did this:

        {:error, mint_error} ->
          {:error, Exception.message(mint_error)}

It now does this:

        {:error, %Mint.TransportError{reason: reason}} ->
          {:error, reason}

This has led to a different return value from the Tesla.Adapter.Finch.call/2 function.

The result of calling Exception.message/1 on the%Mint.TransportError{} struct is not the same as the reason from the struct, as seen from this example in iex.

iex(1)> mint_error = %Mint.TransportError{reason: :econnrefused}
%Mint.TransportError{reason: :econnrefused}
iex(2)> Exception.message(mint_error)
"connection refused"
yordis commented 4 weeks ago

@cliftonmcintosh, I'm sorry for causing you some headaches. You are right; the contract changed. If you know how to have proper tooling to ensure we avoid making this mistake again, please share it. It is stressful to code review and maintain the codebase sometimes. Nobody likes to be responsible for breaking people's code.

The tricky situation is answering, "Who depends on what behavior right now?" Should we roll back the problem by now? What if others depend on the new behavior?

I don't know what is the appropriate answer. I am unsure what to do with the issue.