Preserve non-POST methods on 301/302 requests.

net/url_request/url_request_unittest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "build/build_config.h"
 
 #if defined(OS_WIN)
 #include <shlobj.h>
 #include <windows.h>
 #endif
@@ skipping 265 matching lines @@
 
 class URLRequestTestHTTP : public URLRequestTest {
  public:
   URLRequestTestHTTP()
       : test_server_(TestServer::TYPE_HTTP,
                      FilePath(FILE_PATH_LITERAL(
                                   "net/data/url_request_unittest"))) {
   }
 
  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;
     char *uploadBytes = new char[kMsgSize+1];
     char *ptr = uploadBytes;
     char marker = 'a';
     for (int idx = 0; idx < kMsgSize/10; idx++) {
       memcpy(ptr, "----------", 10);
       ptr += 10;
       if (idx % 100 == 0) {
@@ skipping 2140 matching lines @@
   EXPECT_FALSE(ContainsString(data, "Content-Length:"));
   EXPECT_FALSE(ContainsString(data, "Content-Type:"));
   EXPECT_FALSE(ContainsString(data, "Origin:"));
 
   // These extra request headers should not have been stripped.
   EXPECT_TRUE(ContainsString(data, "Accept:"));
   EXPECT_TRUE(ContainsString(data, "Accept-Language:"));
   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());
 
   const char kData[] = "hello world";
 
   TestDelegate d;
   TestURLRequest req(test_server_.GetURL("empty.html"), &d);
   req.set_context(default_context_);
@@ skipping 1098 matching lines @@
   req.SetExtraRequestHeaders(headers);
   req.Start();
   MessageLoop::current()->Run();
   // If the net tests are being run with ChromeFrame then we need to allow for
   // the 'chromeframe' suffix which is added to the user agent before the
   // closing parentheses.
   EXPECT_TRUE(StartsWithASCII(d.data_received(), "Lynx (textmode", true));
 }
 
 }  // namespace net