From cdf18f4310989e3668858bc43048d61bf138fd92 Mon Sep 17 00:00:00 2001
From: "Stephen M. Coakley" <me@stephencoakley.com>
Date: Sat, 20 Jul 2019 13:38:03 -0500
Subject: [PATCH] Tweak how HTTP methods are configured

---
 src/client.rs      | 47 ++++++++++++++++++++++++++++++----------------
 tests/redirects.rs | 20 ++++++++++++++++++++
 tests/timeouts.rs  | 12 +++++++-----
 3 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/src/client.rs b/src/client.rs
index 4c411d28..b6ff5881 100644
--- a/src/client.rs
+++ b/src/client.rs
@@ -575,7 +575,7 @@ impl HttpClient {
     ) -> Result<(curl::easy::Easy2<RequestHandler>, RequestHandlerFuture), Error> {
         // Prepare the request plumbing.
         let (parts, body) = request.into_parts();
-        let body_is_empty = body.is_empty();
+        let has_body = !body.is_empty();
         let body_size = body.len();
         let (handler, future) = RequestHandler::new(body);
 
@@ -671,11 +671,32 @@ impl HttpClient {
         // Enable automatic response decompression.
         easy.accept_encoding("")?;
 
-        // Set the request data according to the request given.
-        easy.custom_request(parts.method.as_str())?;
-        // Curl handles HEAD requests differently.
-        if parts.method == http::Method::HEAD {
-            easy.nobody(true)?;
+        // Set the HTTP method to use. Curl ties in behavior with the request
+        // method, so we need to configure this carefully.
+        match (parts.method, has_body) {
+            // Normal GET request.
+            (http::Method::GET, false) => {
+                easy.get(true)?;
+            }
+            // HEAD requests do not wait for a response payload.
+            (http::Method::HEAD, has_body) => {
+                easy.custom_request("HEAD")?;
+                easy.nobody(true)?;
+                easy.upload(has_body)?;
+            }
+            // POST requests have special redirect behavior.
+            (http::Method::POST, _) => {
+                easy.post(true)?;
+            }
+            // Normal PUT request.
+            (http::Method::PUT, _) => {
+                easy.upload(true)?;
+            }
+            // Default case is to either treat request like a GET or PUT.
+            (method, has_body) => {
+                easy.custom_request(method.as_str())?;
+                easy.upload(has_body)?;
+            }
         }
 
         easy.url(&parts.uri.to_string())?;
@@ -687,16 +708,10 @@ impl HttpClient {
         }
         easy.http_headers(headers)?;
 
-        // If the request body is non-empty, tell curl that we are going to
-        // upload something.
-        if !body_is_empty {
-            easy.upload(true)?;
-
-            if let Some(len) = body_size {
-                // If we know the size of the request body up front, tell curl
-                // about it.
-                easy.in_filesize(len as u64)?;
-            }
+        // If we know the size of the request body up front, tell curl
+        // about it.
+        if let Some(len) = body_size {
+            easy.in_filesize(len as u64)?;
         }
 
         Ok((easy, future))
diff --git a/tests/redirects.rs b/tests/redirects.rs
index 2289d27d..8fec7e3c 100644
--- a/tests/redirects.rs
+++ b/tests/redirects.rs
@@ -74,6 +74,26 @@ speculate::speculate! {
         m2.assert();
     }
 
+    test "303 redirect changes POST to GET" {
+        let m1 = mock("POST", "/")
+            .with_status(303)
+            .with_header("Location", "/2")
+            .create();
+
+        let m2 = mock("GET", "/2").create();
+
+        let response = Request::post(server_url())
+            .redirect_policy(RedirectPolicy::Follow)
+            .body(())
+            .unwrap()
+            .send()
+            .unwrap();
+
+        assert_eq!(response.status(), 200);
+        m1.assert();
+        m2.assert();
+    }
+
     test "redirect limit is respected" {
         let m1 = mock("GET", "/")
             .with_status(301)
diff --git a/tests/timeouts.rs b/tests/timeouts.rs
index d947e0d3..a945b5c2 100644
--- a/tests/timeouts.rs
+++ b/tests/timeouts.rs
@@ -13,7 +13,7 @@ speculate::speculate! {
         // Spawn a slow server.
         let m = mock("POST", "/")
             .with_body_from_fn(|_| {
-                sleep(Duration::from_millis(500));
+                sleep(Duration::from_secs(1));
                 Ok(())
             })
             .create();
@@ -26,10 +26,12 @@ speculate::speculate! {
             .send();
 
         // Client should time-out.
-        assert!(match result {
-            Err(chttp::Error::Timeout) => true,
-            _ => false,
-        });
+        match result {
+            Err(chttp::Error::Timeout) => {}
+            e => {
+                panic!("expected timout error, got {:?}", e);
+            }
+        }
 
         m.assert();
     }