Preserve non-POST methods on 301/302 requests.

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
===================================================================
--- net/url_request/url_request_unittest.cc	(revision 106917)
+++ net/url_request/url_request_unittest.cc	(working copy)
@@ -283,6 +283,44 @@
   }
 
  protected:
+  // Requests |redirect_url|, which must return a HTTP 3xx redirect.
+  // |request_method| is the method to use for the initial request.
+  // |redirect_method| is the method that is expected to be used for the second
+  // request, after redirection.
+  // If |include_data| is true, data is uploaded with the request.  The
+  // response body is expected to match it exactly, if and only if
+  // |request_method| == |redirect_method|.
+  void HTTPRedirectMethodTest(const GURL& redirect_url,
+                              const std::string& request_method,
+                              const std::string& redirect_method,
+                              bool include_data) {
+    LOG(WARNING) << "Request method: " << request_method;

[willchan no longer on Chromium, 2011/10/25 15:11:36]

Is this necessary?

[mmenke, 2011/10/25 15:36:06]

I don't mind removing it, but if any check fails other than the
"EXPECT_EQ(redirect_method, req.method());", we won't be able to tell if the
failure was with a POST or PUT from trybot output.

I could have just added 8 tests instead of doing two redirects per test, but
figured this way was simpler, since I don't have to worry about duplicating URL
strings, or adding them as constants to the top of the file.

[willchan no longer on Chromium, 2011/10/25 15:49:13]

How about only printing it out if there's a test failure? See
http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest.h....

[mmenke, 2011/10/25 16:09:42]

Done.

+    const char kData[] = "hello world";

[willchan no longer on Chromium, 2011/10/25 15:11:36]

static

[mmenke, 2011/10/25 15:36:06]

Done.  Not sure why this file doesn't just use const char* everywhere instead.

[willchan no longer on Chromium, 2011/10/25 15:49:13]

Would need to be const char* const if we wanted to make the pointer constant
which is necessary for certain compiler optimizations. And often times we don't
need the pointer. That said, it doesn't matter much for test code.

+    TestDelegate d;
+    TestURLRequest req(redirect_url, &d);
+    req.set_context(default_context_);
+    req.set_method(request_method);
+    if (include_data) {
+      req.set_upload(CreateSimpleUploadData(kData).get());
+      HttpRequestHeaders headers;
+      headers.SetHeader(HttpRequestHeaders::kContentLength,
+                        base::UintToString(arraysize(kData) - 1));
+      req.SetExtraRequestHeaders(headers);
+    }
+    req.Start();
+    MessageLoop::current()->Run();
+    EXPECT_EQ(redirect_method, req.method());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status());
+    EXPECT_EQ(OK, req.status().error());
+    if (include_data) {
+      if (request_method == redirect_method) {
+        EXPECT_EQ(kData, d.data_received());
+      } else {
+        EXPECT_NE(kData, d.data_received());
+      }
+    }
+  }
+
   void HTTPUploadDataOperationTest(const std::string& method) {
     const int kMsgSize = 20000;  // multiple of 10
     const int kIterations = 50;
@@ -2443,26 +2481,45 @@
   EXPECT_TRUE(ContainsString(data, "Accept-Charset:"));
 }
 
-TEST_F(URLRequestTestHTTP, Post307RedirectPost) {
+// The following tests check that we handle mutating the request method for
+// HTTP redirects as expected.  See http://crbug.com/56373.
+
+TEST_F(URLRequestTestHTTP, Redirect301Tests) {
   ASSERT_TRUE(test_server_.Start());
 
-  const char kData[] = "hello world";
+  const GURL url = test_server_.GetURL("files/redirect301-to-echo");
 
-  TestDelegate d;
-  TestURLRequest req(test_server_.GetURL("files/redirect307-to-echo"), &d);
-  req.set_context(default_context_);
-  req.set_method("POST");
-  req.set_upload(CreateSimpleUploadData(kData).get());
-  HttpRequestHeaders headers;
-  headers.SetHeader(HttpRequestHeaders::kContentLength,
-                    base::UintToString(arraysize(kData) - 1));
-  req.SetExtraRequestHeaders(headers);
-  req.Start();
-  MessageLoop::current()->Run();
-  EXPECT_EQ("POST", req.method());
-  EXPECT_EQ(kData, d.data_received());
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "PUT", true);
 }
 
+TEST_F(URLRequestTestHTTP, Redirect302Tests) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const GURL url = test_server_.GetURL("files/redirect302-to-echo");
+
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "PUT", true);
+}
+
+TEST_F(URLRequestTestHTTP, Redirect303Tests) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const GURL url = test_server_.GetURL("files/redirect303-to-echo");
+
+  HTTPRedirectMethodTest(url, "POST", "GET", true);
+  HTTPRedirectMethodTest(url, "PUT", "GET", true);
+}
+
+TEST_F(URLRequestTestHTTP, Redirect307Tests) {
+  ASSERT_TRUE(test_server_.Start());
+
+  const GURL url = test_server_.GetURL("files/redirect307-to-echo");
+
+  HTTPRedirectMethodTest(url, "POST", "POST", true);
+  HTTPRedirectMethodTest(url, "PUT", "PUT", true);
+}
+
 TEST_F(URLRequestTestHTTP, InterceptPost302RedirectGet) {
   ASSERT_TRUE(test_server_.Start());