From 51c4c1496260160b1f478ec714e56cb989651c2d Mon Sep 17 00:00:00 2001
From: Samuel Williams <samuel.williams@oriontransfer.co.nz>
Date: Wed, 14 Aug 2024 11:21:16 +1200
Subject: [PATCH] Extract redirect location handling and clarify behaviour.

---
 lib/async/http/relative_location.rb  | 47 +++++++++++++++++++++-------
 test/async/http/relative_location.rb |  4 +--
 2 files changed, 38 insertions(+), 13 deletions(-)

diff --git a/lib/async/http/relative_location.rb b/lib/async/http/relative_location.rb
index ad71194e..16f969f7 100644
--- a/lib/async/http/relative_location.rb
+++ b/lib/async/http/relative_location.rb
@@ -16,7 +16,9 @@ module HTTP
 		class TooManyRedirects < StandardError
 		end
 		
-		# A client wrapper which transparently handles both relative and absolute redirects to a given maximum number of hops.
+		# A client wrapper which transparently handles redirects to a given maximum number of hops.
+		#
+		# The default implementation will only follow relative locations (i.e. those without a scheme) and will switch to GET if the original request was not a GET.
 		#
 		# The best reference for these semantics is defined by the [Fetch specification](https://fetch.spec.whatwg.org/#http-redirect-fetch).
 		#
@@ -58,14 +60,38 @@ def redirect_with_get?(request, response)
 				end
 			end
 			
+			# Handle a redirect to a relative location.
+			#
+			# @parameter request [Protocol::HTTP::Request] The original request, which you can modify if you want to handle the redirect.
+			# @parameter location [String] The relative location to redirect to.
+			# @returns [Boolean] True if the redirect was handled, false if it was not.
+			def handle_redirect(request, location)
+				uri = URI.parse(location)
+				
+				if uri.absolute?
+					return false
+				end
+				
+				# Update the path of the request:
+				request.path = Reference[request.path] + location
+				
+				# Follow the redirect:
+				return true
+			end
+			
 			def call(request)
 				# We don't want to follow redirects for HEAD requests:
 				return super if request.head?
 				
 				if body = request.body
-					# We need to cache the body as it might be submitted multiple times if we get a response status of 307 or 308:
-					body = ::Protocol::HTTP::Body::Rewindable.new(body)
-					request.body = body
+					if body.respond_to?(:rewind)
+						# The request body was already rewindable, so use it as is:
+						body = request.body
+					else
+						# The request body was not rewindable, and we might need to resubmit it if we get a response status of 307 or 308, so make it rewindable:
+						body = ::Protocol::HTTP::Body::Rewindable.new(body)
+						request.body = body
+					end
 				end
 				
 				hops = 0
@@ -83,23 +109,22 @@ def call(request)
 						
 						response.finish
 						
-						uri = URI.parse(location)
-						
-						if uri.absolute?
+						unless handle_redirect(request, location)
 							return response
-						else
-							request.path = Reference[request.path] + location
 						end
 						
+						# Ensure the request (body) is finished and set to nil before we manipulate the request:
+						request.finish
+						
 						if request.method == GET or response.preserve_method?
 							# We (might) need to rewind the body so that it can be submitted again:
 							body&.rewind
+							request.body = body
 						else
 							# We are changing the method to GET:
 							request.method = GET
 							
-							# Clear the request body:
-							request.finish
+							# We will no longer be submitting the body:
 							body = nil
 							
 							# Remove any headers which are not allowed in a GET request:
diff --git a/test/async/http/relative_location.rb b/test/async/http/relative_location.rb
index 4f453dbb..f3bcc6ae 100644
--- a/test/async/http/relative_location.rb
+++ b/test/async/http/relative_location.rb
@@ -44,9 +44,9 @@
 				end
 				
 				it 'should fail with maximum redirects' do
-					expect{
+					expect do
 						response = relative_location.get('/home')
-					}.to raise_exception(Async::HTTP::TooManyRedirects, message: be =~ /maximum/)
+					end.to raise_exception(Async::HTTP::TooManyRedirects, message: be =~ /maximum/)
 				end
 			end
 		end