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

fix httpc bodyless methods #550

Merged

Conversation

olafura
Copy link
Contributor

@olafura olafura commented Nov 29, 2022

Currently we are creating a request with body and content_type in many cases for methods that can't support it. Httpc doesn't know what to do an outputs a function match error.

Currently we are creating a request with body and content_type in
many cases for methods that can't support it. Httpc doesn't know
what to do an outputs a function match error.
@yordis
Copy link
Member

yordis commented Nov 29, 2022

I remember some discussions about it, and I remember something around the point of "HTTP" spec technically allowing a body.

@teamon I am gonna need your help in terms of what we should do here, I feel we should merge it, but still, different clients do different things so a bit tricky.

@olafura
Copy link
Contributor Author

olafura commented Dec 1, 2022

@yordis didn't know before this that GET could contain a body and I used to use telnet to send request for fun.

The reason why I implemented this is because httpc can't contain a body for anything but post, put, patch, and delete.
https://github.com/erlang/otp/blob/40922798411c2d23ee8a99456f96d6637c62b762/lib/inets/src/http_client/httpc.erl#L161

Reading more into the links provided where they mention that servers can reject the request sometimes with the body. It's an edge case but if we really do want to support it we will have to patch httpc to be able to support it.

Reading through the source code of hackney they do seem to support it for some times of body requests but not others.

@yordis
Copy link
Member

yordis commented Dec 1, 2022

I know you are 100% right on this one, httpc would need to fix their stuff.

So I guess people must be aware of such technicality, otherwise 🤷🏻

@yordis
Copy link
Member

yordis commented Dec 2, 2022

CI keeps failing 😢

@olafura
Copy link
Contributor Author

olafura commented Dec 2, 2022

Yeah don't know why the tests are trying to access priv directly and not through :code.priv_dir

@olafura
Copy link
Contributor Author

olafura commented Dec 2, 2022

I sound really judgemental 😁, I came to Elixir through Erlang ( was still getting tripped up by it's syntax ). So I've been exposed to a lot of the old best practices.

@yordis
Copy link
Member

yordis commented Dec 3, 2022

😭

@olafura
Copy link
Contributor Author

olafura commented Dec 3, 2022

There is at least one flaky test

@yordis
Copy link
Member

yordis commented Dec 3, 2022

I remember something about a flaky test for sure!

@yordis yordis merged commit 2161a47 into elixir-tesla:master Dec 3, 2022
@olafura olafura deleted the olafura/fix-https-bodyless-methods branch January 10, 2023 15:48
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 this pull request may close these issues.

2 participants