Refactor redirection handling.

- Remove MaxRedirectsReached, instead raise the normal
  ExceptionWithResponse subclasses.
- Handle max_redirects < 0.
- Remove unnecessary arguments to AbstractResponse#return! and
  #follow_redirection.
- Generally clean up the redirection code to be somewhat saner.

lib/restclient/abstract_response.rb

@@ -57,23 +57,19 @@ def cookie_jar
 
     # Return the default behavior corresponding to the response code:
     # the response itself for code in 200..206, redirection for 301, 302 and 307 in get and head cases, redirection for 303 and an exception in other cases
-    def return! request = nil, result = nil, & block
+    def return!(&block)
       if (200..207).include? code
         self
       elsif [301, 302, 307].include? code
         unless [:get, :head].include? args[:method]
-          raise Exceptions::EXCEPTIONS_MAP[code].new(self, code)
+          raise exception_with_response
         else
-          follow_redirection(request, result, & block)
+          follow_redirection(&block)
         end
       elsif code == 303
-        args[:method] = :get
-        args.delete :payload
-        follow_redirection(request, result, & block)
-      elsif Exceptions::EXCEPTIONS_MAP[code]
-        raise Exceptions::EXCEPTIONS_MAP[code].new(self, code)
+        follow_get_redirection(&block)
       else
-        raise RequestFailed.new(self, code)
+        raise exception_with_response
       end
     end
 
@@ -86,34 +82,20 @@ def description
       "#{code} #{STATUSES[code]} | #{(headers[:content_type] || '').gsub(/;.*$/, '')} #{size} bytes\n"
     end
 
-    # Follow a redirection
-    #
-    # @param request [RestClient::Request, nil]
-    # @param result [Net::HTTPResponse, nil]
-    #
-    def follow_redirection request = nil, result = nil, & block
-      new_args = @args.dup
+    # Follow a redirection response by making a new HTTP request to the
+    # redirection target.
+    def follow_redirection(&block)
+      _follow_redirection(@args.dup, &block)
+    end
 
-      url = headers[:location]
-      if url !~ /^http/
-        url = URI.parse(request.url).merge(url).to_s
-      end
-      new_args[:url] = url
-      if request
-        if request.max_redirects == 0
-          raise MaxRedirectsReached
-        end
-        new_args[:password] = request.password
-        new_args[:user] = request.user
-        new_args[:headers] = request.headers
-        new_args[:max_redirects] = request.max_redirects - 1
-
-        # TODO: figure out what to do with original :cookie, :cookies values
-        new_args[:headers]['Cookie'] = HTTP::Cookie.cookie_value(
-          cookie_jar.cookies(new_args.fetch(:url)))
-      end
+    # Follow a redirection response, but change the HTTP method to GET and drop
+    # the payload from the original request.
+    def follow_get_redirection(&block)
+      new_args = @args.dup
+      new_args[:method] = :get
+      new_args.delete(:payload)
 
-      Request.execute(new_args, &block)
+      _follow_redirection(new_args, &block)
     end
 
     # Convert headers hash into canonical form.
@@ -151,5 +133,47 @@ def self.beautify_headers(headers)
         out
       end
     end
+
+    private
+
+    # Follow a redirection
+    #
+    # @param new_args [Hash] Start with this hash of arguments for the
+    #   redirection request. The hash will be mutated, so be sure to dup any
+    #   existing hash that should not be modified.
+    #
+    def _follow_redirection(new_args, &block)
+
+      # parse location header and merge into existing URL
+      url = headers[:location]
+      if url !~ /^http/
+        url = URI.parse(request.url).merge(url).to_s
+      end
+      new_args[:url] = url
+
+      if request.max_redirects <= 0
+        raise exception_with_response
+      end
+      new_args[:password] = request.password
+      new_args[:user] = request.user
+      new_args[:headers] = request.headers
+      new_args[:max_redirects] = request.max_redirects - 1
+
+      # TODO: figure out what to do with original :cookie, :cookies values
+      new_args[:headers]['Cookie'] = HTTP::Cookie.cookie_value(
+        cookie_jar.cookies(new_args.fetch(:url)))
+
+      Request.execute(new_args, &block)
+    end
+
+    def exception_with_response
+      begin
+        klass = Exceptions::EXCEPTIONS_MAP.fetch(code)
+      rescue KeyError
+        raise RequestFailed.new(self, code)
+      end
+
+      raise klass.new(self, code)
+    end
   end
 end

lib/restclient/exceptions.rb

@@ -210,12 +210,6 @@ def initialize(url)
     end
   end
 
-  class MaxRedirectsReached < Exception
-    def message
-      'Maximum number of redirect reached'
-    end
-  end
-
   # The server broke the connection prior to the request completing.  Usually
   # this means it crashed, or sometimes that your network connection was
   # severed before it could complete.

lib/restclient/request.rb

@@ -556,13 +556,14 @@ def process_result res, & block
         # We don't decode raw requests
         response = RawResponse.new(@tf, res, args, self)
       else
-        response = Response.create(Request.decode(res['content-encoding'], res.body), res, args, self)
+        decoded = Request.decode(res['content-encoding'], res.body)
+        response = Response.create(decoded, res, args, self)
       end
 
       if block_given?
         block.call(response, self, res, & block)
       else
-        response.return!(self, res, & block)
+        response.return!(&block)
       end
 
     end

spec/unit/response_spec.rb

@@ -73,17 +73,17 @@
     it "should return itself for normal codes" do
       (200..206).each do |code|
         net_http_res = response_double(:code => '200')
-        response = RestClient::Response.create('abc', net_http_res, {}, @request)
-        response.return! @request
+        resp = RestClient::Response.create('abc', net_http_res, {}, @request)
+        resp.return!
       end
     end
 
     it "should throw an exception for other codes" do
       RestClient::Exceptions::EXCEPTIONS_MAP.each_key do |code|
         unless (200..207).include? code
           net_http_res = response_double(:code => code.to_i)
-          response = RestClient::Response.create('abc', net_http_res, {}, @request)
-          lambda { response.return!}.should raise_error
+          resp = RestClient::Response.create('abc', net_http_res, {}, @request)
+          lambda { resp.return! }.should raise_error
         end
       end
     end
@@ -118,26 +118,38 @@
 
     it "doesn't follow a 301 when the request is a post" do
       net_http_res = response_double(:code => 301)
-      response = RestClient::Response.create('abc', net_http_res, {:method => :post}, @request)
-      lambda { response.return!(@request)}.should raise_error(RestClient::MovedPermanently)
+      response = RestClient::Response.create('abc', net_http_res,
+                                             {:method => :post}, @request)
+      lambda {
+        response.return!
+      }.should raise_error(RestClient::MovedPermanently)
     end
 
     it "doesn't follow a 302 when the request is a post" do
       net_http_res = response_double(:code => 302)
-      response = RestClient::Response.create('abc', net_http_res, {:method => :post}, @request)
-      lambda { response.return!(@request)}.should raise_error(RestClient::Found)
+      response = RestClient::Response.create('abc', net_http_res,
+                                             {:method => :post}, @request)
+      lambda {
+        response.return!
+      }.should raise_error(RestClient::Found)
     end
 
     it "doesn't follow a 307 when the request is a post" do
       net_http_res = response_double(:code => 307)
-      response = RestClient::Response.create('abc', net_http_res, {:method => :post}, @request)
-      lambda { response.return!(@request)}.should raise_error(RestClient::TemporaryRedirect)
+      response = RestClient::Response.create('abc', net_http_res,
+                                             {:method => :post}, @request)
+      lambda {
+        response.return!
+      }.should raise_error(RestClient::TemporaryRedirect)
     end
 
     it "doesn't follow a redirection when the request is a put" do
       net_http_res = response_double(:code => 301)
-      response = RestClient::Response.create('abc', net_http_res, {:method => :put}, @request)
-      lambda { response.return!(@request)}.should raise_error(RestClient::MovedPermanently)
+      response = RestClient::Response.create('abc', net_http_res,
+                                             {:method => :put}, @request)
+      lambda {
+        response.return!
+      }.should raise_error(RestClient::MovedPermanently)
     end
 
     it "follows a redirection when the request is a post and result is a 303" do
@@ -173,14 +185,18 @@
     it "follows no more than 10 redirections before raising error" do
       stub_request(:get, 'http://some/redirect-1').to_return(:body => '', :status => 301, :headers => {'Location' => 'http://some/redirect-2'})
       stub_request(:get, 'http://some/redirect-2').to_return(:body => '', :status => 301, :headers => {'Location' => 'http://some/redirect-2'})
-      lambda { RestClient::Request.execute(:url => 'http://some/redirect-1', :method => :get) }.should raise_error(RestClient::MaxRedirectsReached)
+      lambda {
+        RestClient::Request.execute(url: 'http://some/redirect-1', method: :get)
+      }.should raise_error(RestClient::MovedPermanently)
       WebMock.should have_requested(:get, 'http://some/redirect-2').times(10)
     end
 
     it "follows no more than max_redirects redirections, if specified" do
       stub_request(:get, 'http://some/redirect-1').to_return(:body => '', :status => 301, :headers => {'Location' => 'http://some/redirect-2'})
       stub_request(:get, 'http://some/redirect-2').to_return(:body => '', :status => 301, :headers => {'Location' => 'http://some/redirect-2'})
-      lambda { RestClient::Request.execute(:url => 'http://some/redirect-1', :method => :get, :max_redirects => 5) }.should raise_error(RestClient::MaxRedirectsReached)
+      lambda {
+        RestClient::Request.execute(url: 'http://some/redirect-1', method: :get, max_redirects: 5)
+      }.should raise_error(RestClient::MovedPermanently)
       WebMock.should have_requested(:get, 'http://some/redirect-2').times(5)
     end
   end