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

added UseLibHeaders option to allow disabling of lib headers #532

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yuchunc
Copy link

@yuchunc yuchunc commented Sep 13, 2018

#529
I am not familiar with erlang, and I can't, for the life of me, seem to figure out how to get erlang test suite to run. I fiddle with the code under my /deps, and this is working for me.

Please let me know if anything need to be updated.

Thank you!

@hazardfn
Copy link

hazardfn commented Sep 17, 2018

@yuchunc I can't speak for @benoitc and this is his project, I personally see a potential problem regarding multi-part-streams partially because I don't know what the code you are bypassing does, here are a list of things I see:

  • You have bypassed handle_body for multi part streams, which sounds like a massive side-effect for just disabling standard headers. It may not be important but I suspect it is without looking deeper.
  • I would have the proplist value be a boolean and be called disable_default_headers (true/false)
  • I would use proplist:get_value/3 and explicitly write a default of false
  • I would use a function guard instead of having maybe_add_host like below
  • A big one: lack of tests.
add_host(Headers, Netloc, DisableHeaders) when DisableHeaders ->
  Headers;
add_host(Headers0, Netloc, _DisableHeaders) ->
  {_, Headers1} = hackney_headers_new:store_new(<<"Host">>, Netloc, Headers0),
  Headers1.

@@ -81,6 +84,13 @@ perform(Client0, {Method0, Path, Headers0, Body0}) ->
handle_body(Headers2, ReqType0, Body0, Client0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the method you are bypassing when headers are disabled. Not sure what it does or what horrific things may happen to multipart streams if it isn't run. This you should definitely check 😈

@hazardfn
Copy link

@benoitc for some context, here is the original post and reasoning behind why this may be a requirement (despite my personal belief the person who wrote this third-party API should have their computer(s) revoked): https://elixirforum.com/t/options-for-httpoison-and-hackney-to-not-append-library-headers/16519/4

@benoitc
Copy link
Owner

benoitc commented Sep 30, 2018

@hazardfn it would have been interesting which headers are triggering an error als.

@yuchunc Patch looks good but I think I would prefer to have it named skip_default_headers (false by default). Also I think that in such case the request should probably be validated before being sent. to make sure it comply to http 1.1, this would fix possible errors when sending a body or else as spotted by @hazardfn . Can you make such changes?

@hazardfn
Copy link

@benoitc that's up to @yuchunc, I just offered to give it a general review as he requested some input due to him being more comfortable with Elixir than Erlang 👍

@yuchunc
Copy link
Author

yuchunc commented Oct 3, 2018

@benoitc, @hazardfn Thank you both for reviewing this PR.
To be perfectly honest, I am not familiar with http 1.1 compliant, but I can send sometime looking in to it. I'll find sometime to make the changes.

I also do agree with @hazardfn, this api is pretty stupid . ☹️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants