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

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

Open
cliftonmcintosh opened this issue Jul 21, 2024 · 1 comment

Comments

@cliftonmcintosh
Copy link

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 d488bb2 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
Copy link
Member

yordis commented Oct 25, 2024

@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.

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

No branches or pull requests

2 participants