crystal-lang / crystal

The Crystal Programming Language
https://crystal-lang.org
Apache License 2.0
19.46k stars 1.62k forks source link

HTTP::Client: follow redirects #2721

Open asterite opened 8 years ago

asterite commented 8 years ago

This is something that's missing from HTTP::Client right now and would be nice to have. Questions are:

  1. Should it be turned on by default?
  2. How to implement it?
  3. How to make it configurable? (maybe allowing more flexibility than just a limit number)
jhass commented 8 years ago

We could follow the pattern we have in the server and popular ruby gems (for example faraday) and add a middleware stack.

The question whether to follow them by default is difficult, I have a slight tendency to no with an easy way to enable them. Another approach could be to only follow in-domain redirects.

asterite commented 8 years ago

An optional middleware stack is probably good. I saw crystal-cossack the other day and although I like the idea, I wouldn't like thousands of HTTP clients for Crystal if we can have a good and complete one in the standard library.

@greyblake Feel free to comment here based on your experience so far with your library :-)

greyblake commented 8 years ago

Hey)

What I am aiming to with Cossack is something similar to Faraday and Hurley, including:

What else?:)

In my understanding the standard library shouldn't be too complex, but must allow to do everything whatever the protocol allows.

Maybe, it also makes sense to have two HTTP entries in the standard library:

So user can pick what he wants (in most cases it will be the high level client). If the high level can not do something very special, user can allways fallback on the "law level thing".

Regarding Cossack: I just started working on it. Once it's finished (I hope it will happen), and I'll get more feedback from users, and if everybody likes it, maybe if you don't mind, it can be merged (probably with different name and some updates) into the standard library as "high level client".

Those are my thoughts :)

asterite commented 8 years ago

@greyblake Thanks for the thorough explanation!

Some comments on your points:

I believe there can be one HTTP::Client that is configurable before executing requests. The simple class methods get and others would be the fast and simple way to execute requests, and the "low-level" API would actually involve configuring an instance. I think this is a simpler design, and it also removes a decision the user has to make.

Anyway, just my opinion :-)

greyblake commented 8 years ago

Thanks, I'll take a look at Go's transport.

So the missing things are:

Also what I'd like to do as a user, is to pass some params to get, post methods. If it's a GET requests, the params should appear in a query part of URI.

greyblake commented 8 years ago

If we could do all those things in the standard library, definitely the community will benefit)

ysbaddaden commented 8 years ago

Transports: I don't think we need them, just allowing to pass an IO object may be enough?

Inspiration: I'm personally more fond of the http gem —deapite missing a logger— than faraday for the design.

ysbaddaden commented 8 years ago

Low level API: required to support HTTP2. One can't write its own implementation without spending many weeks on it (I'm almost there).

BTW: the multiplexed nature of HTTP2 (along with priorities and flow control) is worth considering for the design of a HTTP client. Long lived connections and parallel requests processing will become more common. Chrome, for example, keeps the HTTP2 connection open after the page and assets are fully loaded, ready to send another request as quickly as possible.

straight-shoota commented 7 years ago

@ysbaddaden I guess a HTTP2 client/server would require a event based API?

bararchy commented 7 years ago

Hi All,

So the original discussion was about adding Redirect follow for the HTTP client, this feature is really needed by me for a production solution, Right now I use @greyblake cossack client and even though It's really good, I would much rather using the STDlib instead of relaying on a full different shard just for this one feature, is there anyway that this feature can be added ?

j8r commented 6 years ago

I've implemented follow redirects in a POC branch.

The implementation is quite minimal (~60 additions, ~20 deletions) and could be better, but this may need a refactoring. I've first tried to modify the current HTTP::Client object itself, with redirects modifying ivars @host, @port and the Host header. This is more efficient, but I haven't found a clean way to redirect to http => https without duplicating some checks and logics for tls/socket.

I think also yielding each HTTP::Client object for each redirection would be nice to have, for example to keep track of each URLs followed, and also to use the last HTTP::Client returned to avoid having to follow the redirects later. I haven't managed to do that yet.

Some sample code:

HTTP::Client.follow("http://www.github.com/").get

# Follow redirects up to 8 times
HTTP::Client.follow("http://www.github.com/", 8).get

# In fact #follow is a thin practical wrapper atop of the .new methods, that then call the constructor.
# To have the full features, like block yielding, call them directly
client = HTTP::Client.new("www.github.com", 80, false, redirects: 4)
# Actually I want 6
client.redirects = 6
client.get "/"
bararchy commented 6 years ago

@j8r how would a block yield look for redirects? Would I be able to yield each redirect answer back up?

j8r commented 6 years ago

@bararchy unfortunatly not for now. Like it was before, methods (.get, .post...) yields the response to the block.

I think this will require a redesign of the HTTP::Client, to remove duplication and add a redirection middleware.

I could propose something on this flavor the in POC branch, this will require some time, and discuss about the implementation.

straight-shoota commented 6 years ago

I think this feature can only efficiently be implemented with a transport interface as proposed in #6011.

thelinuxlich commented 4 years ago

+1 on this, it would be very useful!

crysbot commented 2 months ago

This issue has been mentioned on Crystal Forum. There might be relevant details there:

https://forum.crystal-lang.org/t/is-this-the-only-way-to-terminate/7135/5