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.

Sign up for GitHub

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

Jump to bottom

Mint reduce_responses doesn't handle all possible responses #450

Open
sezaru opened this issue Feb 21, 2021 · 6 comments · May be fixed by #510
Open

Mint reduce_responses doesn't handle all possible responses #450

sezaru opened this issue Feb 21, 2021 · 6 comments · May be fixed by #510
Labels
bug 🔥 mint Issues related to mint adapter

Comments

@sezaru
Copy link
Contributor

sezaru commented Feb 21, 2021

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 teamon added the mint Issues related to mint adapter label Mar 24, 2021
@teamon
Copy link
Member

teamon commented Mar 24, 2021

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

@crova
Copy link

crova commented Aug 21, 2021

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

teamon commented Dec 17, 2021

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

@teamon teamon mentioned this issue Dec 17, 2021
Closed
@crova
Copy link

crova commented Dec 21, 2021

Sure @teamon , the same one I linked above?

@teamon
Copy link
Member

teamon commented Dec 21, 2021

Yes, with the patch that fixes the issue :)

@crova
Copy link

crova commented Dec 21, 2021

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

@teamon teamon added bug 🔥 and removed blocked labels May 8, 2022
@teamon teamon added this to Roadmap May 8, 2022
@teamon teamon moved this to Todo in Roadmap May 8, 2022
@teamon teamon linked a pull request May 8, 2022 that will close this issue
Partially fix issues with #450, Mint Reduce Reponses. #510
Open
crova added a commit to crova/tesla that referenced this issue Jul 25, 2024
@crova
Merge branch 'elixir-tesla:master' into issue-elixir-tesla#450-mint-r…
9effca6 
…educe-response-doesnt-handle-all-possible-responses
crova added a commit to crova/tesla that referenced this issue Aug 12, 2024
@crova
Merge branch 'elixir-tesla:master' into issue-elixir-tesla#450-mint-r…
86483f9 
…educe-response-doesnt-handle-all-possible-responses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🔥 mint Issues related to mint adapter
Projects
Roadmap
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Partially fix issues with #450, Mint Reduce Reponses. crova/tesla
3 participants
@teamon @sezaru @crova