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

Add the location to all responses returned #96

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

Conversation

QuinnWilton
Copy link
Contributor

This closes #90

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

  1. 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)

  2. 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()

@edgurgel
Copy link
Owner

Thanks for the PR! I will check soon! 👍

@edgurgel
Copy link
Owner

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?

@edgurgel
Copy link
Owner

I think that having the client will be really useful to give proper tools for request and response streaming (#97)

@QuinnWilton
Copy link
Contributor Author

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.

@edgurgel
Copy link
Owner

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?

@QuinnWilton
Copy link
Contributor Author

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!

@benoitc
Copy link

benoitc commented Jan 12, 2016

which change to client_ref?

@benoitc
Copy link

benoitc commented Jan 12, 2016

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.

@QuinnWilton
Copy link
Contributor Author

@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.

@benoitc
Copy link

benoitc commented Jan 12, 2016

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

@QuinnWilton
Copy link
Contributor Author

@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 :)

@zolrath
Copy link

zolrath commented May 1, 2016

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

@sntran
Copy link

sntran commented Aug 11, 2016

I also would like to know if there is any roadblock from getting this to merge.

@zolrath
Copy link

zolrath commented Dec 21, 2016

Would still love to see this make the 1.0.0 release cycle!

@rupertqin
Copy link

It seams the location prop was removed, how to get the last-location?

@Betree
Copy link

Betree commented Apr 26, 2017

Any news on that ? I would love to see this feature too

@RaphSfeir
Copy link

Yeah, still no news on the subject? I would really need to have the location of the response as well.

@bitboxer
Copy link

@edgurgel is there anything I can help with to make this be included into HTTPoison?

@edgurgel
Copy link
Owner

@bitboxer yes! Would you mind setting up a new PR with the necessary changes? I'm keen to get this merged 👍

@bitboxer
Copy link

@edgurgel okay, will try do this tonight.

@benoitc
Copy link

benoitc commented Oct 21, 2019

well why not using hackney:get_location? i'm confused.

@bitboxer
Copy link

bitboxer commented Oct 21, 2019

@benoitc now you are confusing me 😉 . What do you mean by that?

{:ok, status_code, headers, client} ->
location = :hackney.location(client) # FIXME The current version of Hackney (0.8) requires that we read the location before the body.
Copy link
Owner

Choose a reason for hiding this comment

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

@benoitc, do you mean this?

Copy link

Choose a reason for hiding this comment

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

yeah probably :) i need to think on a better way to keep that state..

Choose a reason for hiding this comment

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

So does that mean I should not rework this PR to make it work against the current httpoison codebase and wait?

Choose a reason for hiding this comment

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

@edgurgel @benoitc so what do I do here? Should I rebase or wait?

Copy link
Owner

Choose a reason for hiding this comment

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

I think we just keep moving as this has been open for a while. We can always rework it if hackney changes in the future. Thanks for pushing this :D

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.

When following redirects, I'd like to be able to access the final location
9 participants