httpoison Add the location to all responses returned

Add the location to all responses returned

Open QuinnWilton opened this issue 8 years ago • 22 comments

This closes #90

There's two things to note in this pull request:

We're forced to read the location off of the client_ref() before we read the body. In the current version of Hackney, reading the body destroys that client_ref(). A new release of Hackney will be released within the next week or so that resolves this issue, at which point we can inline the call to :hackney.location(client)
In the cases where a client_ref() is not returned, I've chosen to return nil as the location. I would prefer always returning the location, but defaulting to request_url is not reliable. Both HEAD requests and requests that make use of the with_body parameter are still capable of following redirects, but will not return a client_ref(). Let me know if you'd prefer different behaviour here.

I'm happy to modify this pull request once we get the new version of Hackney. Once we have that, I'll also open a new PR to return the client_ref()

Nov 11 '15 01:11 QuinnWilton

Thanks for the PR! I will check soon! :+1:

Nov 11 '15 21:11 edgurgel

If we merge this we will include location to the struct and it will be harder to remove as we would need to deprecate etc etc. I'm not sure if it's better to wait for the next hackney version or keep it for now.

Any thoughts @ShaneWilton and @benoitc?

Nov 12 '15 07:11 edgurgel

I think that having the client will be really useful to give proper tools for request and response streaming (https://github.com/edgurgel/httpoison/issues/97)

Nov 12 '15 07:11 edgurgel

I'd agree with the deprecation point. I'm happy to wait for the next version of Hackney, then swap out the location for the client. Calling into Hackney directly isn't too bad.

Nov 12 '15 08:11 QuinnWilton

What if we expose HTTPoison.Response.location/1? Now it would simply return location from the struct but we advise to use the function instead of direct access? Then with next hackney we would keep the API and instead of going for the location we would use the client to fetch the location.

What do you think?

Nov 24 '15 09:11 edgurgel

Sorry about vanishing for a while, I got swamped with other work.

I just looked into this again, and as far as I can tell, the client_ref changes were never committed to Hackney. I'll implement the changes you requested and update the PR!

Jan 12 '16 21:01 QuinnWilton

which change to client_ref?

Jan 12 '16 22:01 benoitc

there is an update of hackney that will land sometimes this week which simplify the api. It will be easier to retrieve and reuse the location with it.

Jan 12 '16 22:01 benoitc

@benoitc Right now, reading the body off a client_ref will destroy that client_ref. This means that we need to first read the location, and return it as part of the struct.

Ideally, we'd like to return the client_ref in the struct instead, to expose all of Hackney's API.

Jan 12 '16 22:01 QuinnWilton

OK I see. It should be handled in the coming version then :) I will try to publish it before friday.

Jan 12 '16 22:01 benoitc

@benoitc That's great to hear, thanks!

@edgurgel I've updated the PR to wrap the location behind a method on Response. Feel free to hold off on this if you'd rather wait until we get the new Hackney though :)

Jan 12 '16 22:01 QuinnWilton

Has Hackney added the update this is waiting on? I'd love to see this feature merged!