Merge pull request #21 from seanmonstar/aidanhs-aphs-307-308

Add handling of 307 and 308 redirects

src/body.rs

@@ -10,6 +10,17 @@ pub struct Body {
 
 impl Body {
     /// Instantiate a `Body` from a reader.
+    ///
+    /// # Note
+    ///
+    /// While allowing for many types to be used, these bodies do not have
+    /// a way to reset to the beginning and be reused. This means that when
+    /// encountering a 307 or 308 status code, instead of repeating the
+    /// request at the new location, the `Response` will be returned with
+    /// the redirect status code set.
+    ///
+    /// A `Body` constructed from a set of bytes, like `String` or `Vec<u8>`,
+    /// are stored differently and can be reused.
     pub fn new<R: Read + 'static>(reader: R) -> Body {
         Body {
             reader: Kind::Reader(Box::new(reader), None),
@@ -115,3 +126,10 @@ pub fn as_hyper_body<'a>(body: &'a mut Body) -> ::hyper::client::Body<'a> {
         }
     }
 }
+
+pub fn can_reset(body: &Body) -> bool {
+    match body.reader {
+        Kind::Bytes(_) => true,
+        Kind::Reader(..) => false,
+    }
+}

src/client.rs

@@ -198,57 +198,68 @@ impl<'a> RequestBuilder<'a> {
 
                 try!(req.send())
             };
-            body.take();
 
-            match res.status {
+            let should_redirect = match res.status {
                 StatusCode::MovedPermanently |
                 StatusCode::Found |
                 StatusCode::SeeOther => {
-
+                    body = None;
-                    //TODO: turn this into self.redirect_policy.check()
+                    match method {
-                    if redirect_count > 10 {
+                        Method::Get | Method::Head => {},
-                        return Err(::Error::TooManyRedirects);
+                        _ => {
+                            method = Method::Get;
+                        }
                     }
-                    redirect_count += 1;
+                    true
-
-                    method = match method {
-                        Method::Post | Method::Put => Method::Get,
-                        m => m
-                    };
-
-                    headers.set(Referer(url.to_string()));
-
-                    let loc = {
-                        let loc = res.headers.get::<Location>().map(|loc| url.join(loc));
-                        if let Some(loc) = loc {
-                            loc
-                        } else {
-                            return Ok(Response {
-                                inner: res
-                            });
-                        }
-                    };
-
-                    url = match loc {
-                        Ok(u) => u,
-                        Err(e) => {
-                            debug!("Location header had invalid URI: {:?}", e);
-                            return Ok(Response {
-                                inner: res
-                            })
-                        }
-                    };
-
-                    debug!("redirecting to '{}'", url);
-
-                    //TODO: removeSensitiveHeaders(&mut headers, &url);
-
                 },
-                _ => {
+                StatusCode::TemporaryRedirect |
-                    return Ok(Response {
+                StatusCode::PermanentRedirect => {
-                        inner: res
+                    if let Some(ref body) = body {
-                    });
+                        body::can_reset(body)
+                    } else {
+                        true
+                    }
+                },
+                _ => false,
+            };
+
+            if should_redirect {
+                //TODO: turn this into self.redirect_policy.check()
+                if redirect_count > 10 {
+                    return Err(::Error::TooManyRedirects);
                 }
+                redirect_count += 1;
+
+                headers.set(Referer(url.to_string()));
+
+                let loc = {
+                    let loc = res.headers.get::<Location>().map(|loc| url.join(loc));
+                    if let Some(loc) = loc {
+                        loc
+                    } else {
+                        return Ok(Response {
+                            inner: res
+                        });
+                    }
+                };
+
+                url = match loc {
+                    Ok(u) => u,
+                    Err(e) => {
+                        debug!("Location header had invalid URI: {:?}", e);
+                        return Ok(Response {
+                            inner: res
+                        })
+                    }
+                };
+
+                debug!("redirecting to {:?} '{}'", method, url);
+
+                //TODO: removeSensitiveHeaders(&mut headers, &url);
+            } else {
+                return Ok(Response {
+                    inner: res
+                });
             }
         }
     }

tests/client.rs

@@ -33,45 +33,130 @@ fn test_get() {
 }
 
 #[test]
-fn test_redirect_302_changes_post_to_get() {
+fn test_redirect_301_and_302_and_303_changes_post_to_get() {
-
-    let redirect = server! {
-        request: b"\
-            POST /302 HTTP/1.1\r\n\
-            Host: $HOST\r\n\
-            User-Agent: $USERAGENT\r\n\
-            Content-Length: 0\r\n\
-            \r\n\
-            ",
-        response: b"\
-            HTTP/1.1 302 Found\r\n\
-            Server: test-redirect\r\n\
-            Content-Length: 0\r\n\
-            Location: /dst\r\n\
-            Connection: close\r\n\
-            \r\n\
-            ",
-
-        request: b"\
-            GET /dst HTTP/1.1\r\n\
-            Host: $HOST\r\n\
-            User-Agent: $USERAGENT\r\n\
-            Referer: http://$HOST/302\r\n\
-            \r\n\
-            ",
-        response: b"\
-            HTTP/1.1 200 OK\r\n\
-            Server: test-dst\r\n\
-            Content-Length: 0\r\n\
-            \r\n\
-            "
-    };
-
     let client = reqwest::Client::new().unwrap();
-    let res = client.post(&format!("http://{}/302", redirect.addr()))
+    let codes = [301, 302, 303];
-        .send()
-        .unwrap();
-    assert_eq!(res.status(), &reqwest::StatusCode::Ok);
-    assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
 
+    for code in codes.iter() {
+        let redirect = server! {
+            request: format!("\
+                POST /{} HTTP/1.1\r\n\
+                Host: $HOST\r\n\
+                User-Agent: $USERAGENT\r\n\
+                Content-Length: 0\r\n\
+                \r\n\
+                ", code),
+            response: format!("\
+                HTTP/1.1 {} reason\r\n\
+                Server: test-redirect\r\n\
+                Content-Length: 0\r\n\
+                Location: /dst\r\n\
+                Connection: close\r\n\
+                \r\n\
+                ", code),
+
+            request: format!("\
+                GET /dst HTTP/1.1\r\n\
+                Host: $HOST\r\n\
+                User-Agent: $USERAGENT\r\n\
+                Referer: http://$HOST/{}\r\n\
+                \r\n\
+                ", code),
+            response: b"\
+                HTTP/1.1 200 OK\r\n\
+                Server: test-dst\r\n\
+                Content-Length: 0\r\n\
+                \r\n\
+                "
+        };
+
+        let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
+            .send()
+            .unwrap();
+        assert_eq!(res.status(), &reqwest::StatusCode::Ok);
+        assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
+    }
+}
+
+#[test]
+fn test_redirect_307_and_308_tries_to_post_again() {
+    let client = reqwest::Client::new().unwrap();
+    let codes = [307, 308];
+    for code in codes.iter() {
+        let redirect = server! {
+            request: format!("\
+                POST /{} HTTP/1.1\r\n\
+                Host: $HOST\r\n\
+                User-Agent: $USERAGENT\r\n\
+                Content-Length: 5\r\n\
+                \r\n\
+                Hello\
+                ", code),
+            response: format!("\
+                HTTP/1.1 {} reason\r\n\
+                Server: test-redirect\r\n\
+                Content-Length: 0\r\n\
+                Location: /dst\r\n\
+                Connection: close\r\n\
+                \r\n\
+                ", code),
+
+            request: format!("\
+                POST /dst HTTP/1.1\r\n\
+                Host: $HOST\r\n\
+                User-Agent: $USERAGENT\r\n\
+                Referer: http://$HOST/{}\r\n\
+                Content-Length: 5\r\n\
+                \r\n\
+                Hello\
+                ", code),
+            response: b"\
+                HTTP/1.1 200 OK\r\n\
+                Server: test-dst\r\n\
+                Content-Length: 0\r\n\
+                \r\n\
+                "
+        };
+
+        let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
+            .body("Hello")
+            .send()
+            .unwrap();
+        assert_eq!(res.status(), &reqwest::StatusCode::Ok);
+        assert_eq!(res.headers().get(), Some(&reqwest::header::Server("test-dst".to_string())));
+    }
+}
+
+#[test]
+fn test_redirect_307_does_not_try_if_reader_cannot_reset() {
+    let client = reqwest::Client::new().unwrap();
+    let codes = [307, 308];
+    for &code in codes.iter() {
+        let redirect = server! {
+            request: format!("\
+                POST /{} HTTP/1.1\r\n\
+                Host: $HOST\r\n\
+                User-Agent: $USERAGENT\r\n\
+                Transfer-Encoding: chunked\r\n\
+                \r\n\
+                5\r\n\
+                Hello\r\n\
+                0\r\n\r\n\
+                ", code),
+            response: format!("\
+                HTTP/1.1 {} reason\r\n\
+                Server: test-redirect\r\n\
+                Content-Length: 0\r\n\
+                Location: /dst\r\n\
+                Connection: close\r\n\
+                \r\n\
+                ", code)
+        };
+
+        let res = client.post(&format!("http://{}/{}", redirect.addr(), code))
+            .body(reqwest::Body::new(&b"Hello"[..]))
+            .send()
+            .unwrap();
+        assert_eq!(res.status(), &reqwest::StatusCode::from_u16(code));
+    }
 }