Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Return redirect result #99

Merged
merged 3 commits into from

3 participants

@fhars

The original implementation raised a BadResponseError even on perfectly good responses like 304 Not Modified. With this change, follow_redirect itself does only throw an exception if the retry chain is too long and it is up to the calling function to decide what to do with the result (like the changed get_content and post_content methods. I think this is a saner design, where the semantics of the pair do_request and follow_redirect mimics that of the pair of unix functions lstat and and stat.

@buildhive

Hiroshi Nakamura » httpclient #7 FAILURE
Looks like there's a problem with this pull request
(what's this?)

@buildhive

Hiroshi Nakamura » httpclient #9 SUCCESS
This pull request looks good
(what's this?)

@nahi nahi merged commit 727ad77 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 31, 2012
  1. @fhars
Commits on Jun 4, 2012
  1. @fhars
  2. @fhars

    use old style hash syntax

    fhars authored
This page is out of date. Refresh to see the latest.
Showing with 34 additions and 20 deletions.
  1. +12 −6 lib/httpclient.rb
  2. +22 −14 test/test_httpclient.rb
View
18 lib/httpclient.rb
@@ -577,7 +577,7 @@ def redirect_uri_callback=(redirect_uri_callback)
# follow HTTP redirect by yourself if you need.
def get_content(uri, *args, &block)
query, header = keyword_argument(args, :query, :header)
- follow_redirect(:get, uri, query, nil, header || {}, &block).content
+ success_content(follow_redirect(:get, uri, query, nil, header || {}, &block))
end
# Posts a content.
@@ -612,7 +612,7 @@ def get_content(uri, *args, &block)
# use post method.
def post_content(uri, *args, &block)
body, header = keyword_argument(args, :body, :header)
- follow_redirect(:post, uri, nil, body, header || {}, &block).content
+ success_content(follow_redirect(:post, uri, nil, body, header || {}, &block))
end
# A method for redirect uri callback. How to use:
@@ -939,18 +939,24 @@ def follow_redirect(method, uri, query, body, header, &block)
while retry_number < @follow_redirect_count
body.pos = pos if pos
res = do_request(method, uri, query, body, header, &filtered_block)
- if HTTP::Status.successful?(res.status)
- return res
- elsif HTTP::Status.redirect?(res.status)
+ if HTTP::Status.redirect?(res.status)
uri = urify(@redirect_uri_callback.call(uri, res))
retry_number += 1
else
- raise BadResponseError.new("unexpected response: #{res.header.inspect}", res)
+ return res
end
end
raise BadResponseError.new("retry count exceeded", res)
end
+ def success_content(res)
+ if HTTP::Status.successful?(res.status)
+ return res.content
+ else
+ raise BadResponseError.new("unexpected response: #{res.header.inspect}", res)
+ end
+ end
+
def protect_keep_alive_disconnected
begin
yield
View
36 test/test_httpclient.rb
@@ -134,6 +134,14 @@ def test_host_given
assert_equal("Host: foo", lines[4]) # use given param
end
+ def test_redirect_returns_not_modified
+ assert_nothing_raised do
+ timeout(2) do
+ @client.get(serverurl + 'status', {:status => 306}, {:follow_redirect => true})
+ end
+ end
+ end
+
def test_protocol_version_http11
assert_equal(nil, @client.protocol_version)
str = ""
@@ -167,7 +175,7 @@ def test_proxy
setup_proxyserver
escape_noproxy do
assert_raises(URI::InvalidURIError) do
- @client.proxy = "http://"
+ @client.proxy = "http://"
end
@client.proxy = ""
assert_nil(@client.proxy)
@@ -1002,7 +1010,7 @@ def test_reset_all
def test_cookies
cookiefile = File.join(File.dirname(File.expand_path(__FILE__)), 'test_cookies_file')
File.open(cookiefile, "wb") do |f|
- f << "http://rubyforge.org/account/login.php session_ser LjEwMy45Ni40Ni0q%2A-fa0537de8cc31 2000000000 .rubyforge.org / 13\n"
+ f << "http://rubyforge.org/account/login.php\tsession_ser\tLjEwMy45Ni40Ni0q%2A-fa0537de8cc31\t2000000000\t.rubyforge.org\t/\t13\n"
end
@client.set_cookie_store(cookiefile)
cookie = @client.cookie_manager.cookies.first
@@ -1014,7 +1022,7 @@ def test_cookies
@client.get_content('http://rubyforge.org/account/login.php')
@client.save_cookie_store
str = File.read(cookiefile)
- assert_match(%r(http://rubyforge.org/account/login.php foo bar 1924873200 rubyforge.org /account 1), str)
+ assert_match(%r(http://rubyforge.org/account/login.php\tfoo\tbar\t1924873200\trubyforge.org\t/account\t1), str)
File.unlink(cookiefile)
end
@@ -1460,8 +1468,8 @@ def setup_server
@serverport = @server.config[:Port]
[:hello, :sleep, :servlet_redirect, :redirect1, :redirect2, :redirect3, :redirect_self, :relative_redirect, :chunked, :largebody, :status, :compressed, :charset].each do |sym|
@server.mount(
- "/#{sym}",
- WEBrick::HTTPServlet::ProcHandler.new(method("do_#{sym}").to_proc)
+ "/#{sym}",
+ WEBrick::HTTPServlet::ProcHandler.new(method("do_#{sym}").to_proc)
)
end
@server.mount('/servlet', TestServlet.new(@server))
@@ -1489,11 +1497,11 @@ def do_sleep(req, res)
end
def do_servlet_redirect(req, res)
- res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "servlet")
+ res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "servlet")
end
def do_redirect1(req, res)
- res.set_redirect(WEBrick::HTTPStatus::MovedPermanently, serverurl + "hello")
+ res.set_redirect(WEBrick::HTTPStatus::MovedPermanently, serverurl + "hello")
end
def do_redirect2(req, res)
@@ -1501,15 +1509,15 @@ def do_redirect2(req, res)
end
def do_redirect3(req, res)
- res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "hello")
+ res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "hello")
end
def do_redirect_self(req, res)
- res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "redirect_self")
+ res.set_redirect(WEBrick::HTTPStatus::Found, serverurl + "redirect_self")
end
def do_relative_redirect(req, res)
- res.set_redirect(WEBrick::HTTPStatus::Found, "hello")
+ res.set_redirect(WEBrick::HTTPStatus::Found, "hello")
end
def do_chunked(req, res)
@@ -1555,7 +1563,7 @@ def get_instance(*arg)
end
def do_HEAD(req, res)
- res["x-head"] = 'head' # use this for test purpose only.
+ res["x-head"] = 'head' # use this for test purpose only.
res["x-query"] = query_response(req)
end
@@ -1615,9 +1623,9 @@ def body_response(req)
def query_escape(query)
escaped = []
query.sort_by { |k, v| k }.collect do |k, v|
- v.to_ary.each do |ve|
- escaped << CGI.escape(k) + '=' + CGI.escape(ve)
- end
+ v.to_ary.each do |ve|
+ escaped << CGI.escape(k) + '=' + CGI.escape(ve)
+ end
end
escaped.join('&')
end
Something went wrong with that request. Please try again.