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

Follow HTTP redirects #67

Closed
ghost opened this issue Nov 24, 2012 · 20 comments
Closed

Follow HTTP redirects #67

ghost opened this issue Nov 24, 2012 · 20 comments

Comments

@ghost
Copy link

ghost commented Nov 24, 2012

The HTTPI library should automaticaly follow HTTP redirects (3xx status codes), at least 301 Moved Permanently, 302 Found, 307 Temporary Redirect, and 308 Permanent Redirect.

@rogerleite
Copy link
Member

I don't think this is HTTPI's responsibility. To help with it, you can use block to access "native" client. In my tests, only with Curb client i could set follow location option. Httpclient does not have a "follow setter".

# using version 1.1.1
request = HTTPI::Request.new("http://google.com")
response = HTTPI.get(request, :curb)
puts response.code  # => 301

response = HTTPI.get(request, :curb) do |client|
  client.follow_location = true
end
puts response.code  # => 200
# using version 2.0.0.rc1
request = HTTPI::Request.new("http://google.com")
response = HTTPI.get(request, :curb)
puts response.code  # => 301

response = HTTPI.request(:get, request, :curb) do |client|
  client.follow_location = true
end
puts response.code  # => 200

I started a client that wraps HTTPI and have many features, one of them is follow redirect. You can try http_monkey or another client of course.

Hope that helps,

Roger Leite

@rubiii
Copy link
Contributor

rubiii commented Nov 30, 2012

i'm not sure about whether this should be supported or not. i'd like to be httpi as complete as possible, but since it's possible to manually follow redirects (or directly configure the client), this is not on top of my todo list right now.

if anyone has an opinion about this, let me know.

@rogerleite
Copy link
Member

Study on HTTPI adapters about redirect feature:

  • curb supports by setting attribute on native client.
  • net/http does not support.
  • httpclient has the feature, but, we can't pass the option to native client, because is on the "request" moment.
  • em-http-request has the same problem like httpclient.

My opinion:
I don't think that HTTPI have to be this feature.
HTTPI have to support "send options" to native clients, if we want to do something about this.

@rubiii
Copy link
Contributor

rubiii commented Dec 10, 2012

savonrb/wasabi#18

@rubiii
Copy link
Contributor

rubiii commented Dec 10, 2012

thanks for doing the research @rogerleite. i appreciate it. since this is definitely a problem for savon,
i need to look into this some more.

@rogerleite
Copy link
Member

@rubiii make wasabi responsible to follow redirect is an option. At Resolver you can check for responses [301, 302, 303, 307], change request.url and call load_from_remote again. I don't know if this have some side effect, but is an idea.

@rubiii
Copy link
Contributor

rubiii commented Dec 10, 2012

i actually started doing exactly that a few hours ago, before i came back to respond to this ticket. i think the http response definitely needs a #redirect? method along with #success? and i'm also not sure if redirects should be considered an error.

but implementing redirects in a wsdl parser library felt weird. don't know.

@rogerleite
Copy link
Member

You're right.

Doesn't looks difficult to implement after adapter request. Put an #follow_redirect attribute on Request, with #redirect? on Response and it's easy to implement. My only fear is to bloat HTTPI.

@rubiii
Copy link
Contributor

rubiii commented Dec 10, 2012

it definitely comes with a cost to add any feature, but since httpi was build to support savon and it's actually causing problems, it just has to be fixed. thanks. i'll get back to this tomorrow.

@rogerleite
Copy link
Member

👍

@owain68
Copy link

owain68 commented Apr 12, 2013

Has anyone come up with a solution to this?

@rubiii
Copy link
Contributor

rubiii commented Apr 15, 2013

don't think so. feel free to jump in.

@tjarratt
Copy link
Contributor

@rogerleite I'm following up on this issue after seeing some activity on savonrb/wasabi#18 -- do you have any strong feelings on this?

I see some discussion of implementing this behavior in Wasabi versus implementing it in HTTPI. @rubiii's thoughts re: implementing it in the WSDL parser definitely makes me think twice, although I understand how complicated it is to implement it in HTTPI (because of the adapters... oh no the adapters).

Let's make a decision so we can close one of these two issues and try to make some forward progress. You've thought about this a lot, are you comfortable saying this is outside the scope of HTTPI and it should happen in Wasabi ?

@rogerleite
Copy link
Member

Hi @tjarratt. Thinking more about this, i agree with @rubiii and we should implement this on HTTPI.
Here are some ideas to implement this on HTTPI:

  • We'll need a #follow_redirect attribute on Request object.
  • Capture response on request and follow redirect if response #redirect?.

I think this is the easiest way. What do you think @tjarratt?

@tjarratt
Copy link
Contributor

👍

That sounds like the easiest way forward without bloating the interface too much.

@mikeantonelli
Copy link

Is anyone actively working on this?

@rogerleite
Copy link
Member

@mikeantonelli maybe @tjarratt started something.

@mikeantonelli
Copy link

@rogerleite, I made a first pass at it: mikeantonelli/httpi:02c223d, and the integration: mikeantonelli/savon:3295603. Thoughts?

@tjarratt
Copy link
Contributor

Looks good! Wouldn't hurt to add a simple integration test in Savon. It's been my experience that these integration points are the most likely to be broken during aggressive refactoring and merging pull requests.

@mikeantonelli
Copy link

@tjarratt, I opened #117 here and savonrb/savon#589, which would require minor update once #117 is merged and released.

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

No branches or pull requests

5 participants