Redirection dose not work with path or query option. #230

Closed
wants to merge 1 commit into from

2 participants

@sugi

Redirection dose not work properly when HttpConnection#get
is called with :path or :query options.

This patch clear @path and @query from request option when
HttpClientOptions#set_uri is called again.

@sugi sugi Fix for redirection handling with path and query option.
Redirection dose not work properly when HttpConnection#get
is called with :path or :query options.

This patch clear @path and @query from request option when
HttpClientOptions#set_uri is called again.
b89df27
@igrigorik igrigorik commented on the diff Apr 19, 2013
lib/em-http/http_client_options.rb
@@ -29,6 +29,7 @@ def ssl?; @uri.scheme == "https" || @uri.port == 443; end
def no_body?; @method == "HEAD"; end
def set_uri(uri)
+ @path = @query = nil if @uri # clear path and query except first time.
@igrigorik
Owner

The intent is to allow both URI params and custom params, which then get merged. I have seen and written code that depends on this behavior.

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

Hmm...
Is it better that splits method and use new one on redirect (like; set_uri_force or set_full_uri_with_overrides_query_and_path...)?

If so, I'd like to write another patch.

@igrigorik
Owner

Actually, I just looked at it closer... It works just fine, there is nothing to fix. The issue is the test itself. :-)

https://github.com/sugi/em-http-request/blob/b89df271880ed5d88441e36ce8fec1db22f6abe9/spec/stallion.rb#L138 - only looks at the path, which is why your test fails. If you inspect the actual URL sent, it does contain the query string.

@sugi

I think redirection of em-http-request is broken when I use ":path" or ":query" argument.

For example;
My localhost http server will redirects "/" to "/apache2-default/". It can be confirmd by wget.

sugi@tempest:~% wget -S -O/dev/null http://localhost/
--2013-04-20 03:55:04--  http://localhost/
Resolving localhost (localhost)... 127.0.0.1
Connecting to localhost (localhost)|127.0.0.1|:80... connected.
HTTP request sent, awaiting response... 
  HTTP/1.1 302 Found
  Date: Fri, 19 Apr 2013 18:55:04 GMT
  Server: Apache/2.2.22 (Debian)
  Location: http://localhost/apache2-default/
  Content-Length: 292
  Keep-Alive: timeout=5, max=100
  Connection: Keep-Alive
  Content-Type: text/html; charset=iso-8859-1
  X-Pad: avoid browser bug
Location: http://localhost/apache2-default/ [following]
--2013-04-20 03:55:04--  http://localhost/apache2-default/
Reusing existing connection to localhost:80.
HTTP request sent, awaiting response... 
  HTTP/1.1 200 OK
  Date: Fri, 19 Apr 2013 18:55:04 GMT
  Server: Apache/2.2.22 (Debian)
  Last-Modified: Fri, 19 Apr 2013 18:54:26 GMT
  ETag: "357abb-62-4dabb3f8abf8b"
  Accept-Ranges: bytes
  Content-Length: 98
  Keep-Alive: timeout=5, max=99
  Connection: Keep-Alive
  Content-Type: text/html
Length: 98 [text/html]
Saving to: ‘/dev/null’

However, em-http-request dose NOT follow the redirection;

sugi@tempest:~% ruby1.9.1 -r em-http/version -r em-http-request -e '
p EM::HttpRequest::VERSION
EM.run {
  r = EM::HttpRequest.new("http://localhost").get :path => "/", :redirects => 100
  r.callback { p r.last_effective_url; p r.redirects; p r.response.size; EM.stop; }
}'
"1.0.3"
#<Addressable::URI:0x5305746 URI:http://localhost:80/>
100
292

In this case, last_effective_url must returns http://localhost/apache2-default/,
redirect count must be 1 and response byte size must be 98.
And I get 101 lines of "GET /" on my access.log.

@sugi

Event if redirected URL points to another server, it's also wrong when I use ":path" argument.

For example; http://bit.ly/3mWN3O is redirected to https://github.com

sugi@tempest:~% ruby1.9.1 -r em-http-request -e '
EM.run {                  
  req = EM::HttpRequest.new("http://bit.ly").get :path => "/3mWN3O", :redirects => 100
  req.callback { p req.last_effective_url; p req.redirects; EM.stop; }
}'
#<Addressable::URI:0x4da24d4 URI:https://github.com:443/3mWN3O>
1

This result is wrong. The path part "/3mWN3O" was carried after redirect.

It works properly if I set URL with new(), like that;

EM::HttpRequest.new("http://bit.ly/3mWN3O").get

But I need to use ":path" argument to enable keep_alive or pipeline.

@igrigorik igrigorik closed this in ff935e8 Apr 19, 2013
@igrigorik
Owner

Good catch, that should fix it.. That code warrants a refactor with respect to query processing (move the logic back into options class), but that's a different subject.

@sugi

Thanks! I confirmed the fix.

@sugi sugi deleted the sugi:fix-redirect-with-path-arg branch May 10, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment