elixir-tesla / tesla

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

Mint reduce_responses doesn't handle all possible responses #450

Open sezaru opened 3 years ago

sezaru commented 3 years ago

Mint's adapter reduce_responses/3 https://github.com/teamon/tesla/blob/cc18e12226ef5b61f19daf7cc85cb8f94ae428b1/lib/tesla/adapter/mint.ex#L327-L340 doesn't handle all possible returns HTTP.stream/2 can return.

The returns missing are: {:error, request_ref(), reason :: term()} {:pong, request_ref()} {:push_promise, request_ref(), promised_request_ref :: request_ref(), headers()}

The last two are just for HTTP2 streams.

More information here: https://hexdocs.pm/mint/1.2.1/Mint.Types.html#t:response/0

teamon commented 3 years ago

@sezaru Thanks for reporting this. Would you be able to provide a PR to fix this?

crova commented 3 years ago

Hello everyone. @teamon , in a project I'm working on we bumped on this issue, specifically the missing clause for {:push_promise, request_ref(), promised_request_ref :: request_ref(), headers()}.

To be honest, I'm somewhat lost on how to properly fix this but I would be happy to provide a PR handling those 3 cases if you're willing to enlighten me a bit.

For now, I managed to get our project working with the patch you can see on this branch. Your test suite still pass, but I'm not certain if this is how things were supposed to be done, that's why I didn't open a PR (and because I'm only handling one of the 3 missing cases right now).

Thanks for your effort on this library and let me know if there is anything else I can help with.

teamon commented 2 years ago

@crova Could you open a PR against current tesla master with the necessary changes?

crova commented 2 years ago

Sure @teamon , the same one I linked above?

teamon commented 2 years ago

Yes, with the patch that fixes the issue :)

crova commented 2 years ago

I'll open it by tomorrow. This week has been rough.