net/http: write status code in Redirect when Content-Type header set

This is a followup to CL 110296. That change added a new behavior
to Redirect, where the short HTML body is not written if the
Content-Type header is already set. It was implemented by doing
an early return. That unintentionally prevented the correct status
code from being written, so it would always default to 200.
Existing tests didn't catch this because they don't check status code.

This change fixes that issue by removing the early return and
moving the code to write a short HTML body behind an if statement.
It adds written status code checks to Redirect tests.

It also tries to improve the documentation wording and code style
in TestRedirect_contentTypeAndBody.

Updates #25166.

Change-Id: Idce004baa88e278d098661c03c9523426c5eb898
Reviewed-on: https://go-review.googlesource.com/111517
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 4be94a67094910d4396ff1a2732beb9a5df6c5d8..c14d87dcf9abf9bfb45722bd93fd4ba114bcf8db 100644 (file)
--- a/src/net/http/serve_test.go
+++ b/src/net/http/serve_test.go
@@ -2581,40 +2581,49 @@ func TestRedirect(t *testing.T) {
        for _, tt := range tests {
                rec := httptest.NewRecorder()
                Redirect(rec, req, tt.in, 302)
+               if got, want := rec.Code, 302; got != want {
+                       t.Errorf("Redirect(%q) generated status code %v; want %v", tt.in, got, want)
+               }
                if got := rec.Header().Get("Location"); got != tt.want {
                        t.Errorf("Redirect(%q) generated Location header %q; want %q", tt.in, got, tt.want)
                }
        }
 }
 
-// Test that Content-Type header is set for GET and HEAD requests.
-func TestRedirectContentTypeAndBody(t *testing.T) {
-       var unsetCT = []string{"sentinalValNoCT"}
+// Test that Redirect sets Content-Type header for GET and HEAD requests
+// and writes a short HTML body, unless the request already has a Content-Type header.
+func TestRedirect_contentTypeAndBody(t *testing.T) {
+       type ctHeader struct {
+               Values []string
+       }
 
        var tests = []struct {
-               initCT   []string
                method   string
+               ct       *ctHeader // Optional Content-Type header to set.
                wantCT   string
                wantBody string
        }{
-               {unsetCT, MethodGet, "text/html; charset=utf-8", "<a href=\"/foo\">Found</a>.\n\n"},
-               {unsetCT, MethodHead, "text/html; charset=utf-8", ""},
-               {unsetCT, MethodPost, "", ""},
-               {unsetCT, MethodDelete, "", ""},
-               {unsetCT, "foo", "", ""},
-               {[]string{"application/test"}, MethodGet, "application/test", ""},
-               {[]string{}, MethodGet, "", ""},
-               {nil, MethodGet, "", ""},
+               {MethodGet, nil, "text/html; charset=utf-8", "<a href=\"/foo\">Found</a>.\n\n"},
+               {MethodHead, nil, "text/html; charset=utf-8", ""},
+               {MethodPost, nil, "", ""},
+               {MethodDelete, nil, "", ""},
+               {"foo", nil, "", ""},
+               {MethodGet, &ctHeader{[]string{"application/test"}}, "application/test", ""},
+               {MethodGet, &ctHeader{[]string{}}, "", ""},
+               {MethodGet, &ctHeader{nil}, "", ""},
        }
        for _, tt := range tests {
                req := httptest.NewRequest(tt.method, "http://example.com/qux/", nil)
                rec := httptest.NewRecorder()
-               if len(tt.initCT) != 1 || &tt.initCT[0] != &unsetCT[0] {
-                       rec.Header()["Content-Type"] = tt.initCT
+               if tt.ct != nil {
+                       rec.Header()["Content-Type"] = tt.ct.Values
                }
                Redirect(rec, req, "/foo", 302)
+               if got, want := rec.Code, 302; got != want {
+                       t.Errorf("Redirect(%q, %#v) generated status code %v; want %v", tt.method, tt.ct, got, want)
+               }
                if got, want := rec.Header().Get("Content-Type"), tt.wantCT; got != want {
-                       t.Errorf("Redirect(%q) generated Content-Type header %q; want %q", tt.method, got, want)
+                       t.Errorf("Redirect(%q, %#v) generated Content-Type header %q; want %q", tt.method, tt.ct, got, want)
                }
                resp := rec.Result()
                body, err := ioutil.ReadAll(resp.Body)
@@ -2622,7 +2631,7 @@ func TestRedirectContentTypeAndBody(t *testing.T) {
                        t.Fatal(err)
                }
                if got, want := string(body), tt.wantBody; got != want {
-                       t.Errorf("Redirect(%q) generated Body %q; want %q", tt.method, got, want)
+                       t.Errorf("Redirect(%q, %#v) generated Body %q; want %q", tt.method, tt.ct, got, want)
                }
        }
 }

diff --git a/src/net/http/server.go b/src/net/http/server.go
index fca46d34809029cab7999cb49f83887b5b0afa9f..be28a252c8cbfb3f0510d6f4794bac97df247f94 100644 (file)
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -2003,9 +2003,11 @@ func StripPrefix(prefix string, h Handler) Handler {
 //
 // The provided code should be in the 3xx range and is usually
 // StatusMovedPermanently, StatusFound or StatusSeeOther.
-// If Content-Type has not been set Redirect sets the header to
-// "text/html; charset=utf-8" and writes a small HTML body.
-// Setting the Content-Type header to nil also prevents writing the body.
+//
+// If the Content-Type header has not been set, Redirect sets it
+// to "text/html; charset=utf-8" and writes a small HTML body.
+// Setting the Content-Type header to any value, including nil,
+// disables that behavior.
 func Redirect(w ResponseWriter, r *Request, url string, code int) {
        // parseURL is just url.Parse (url is shadowed for godoc).
        if u, err := parseURL(url); err == nil {
@@ -2043,22 +2045,22 @@ func Redirect(w ResponseWriter, r *Request, url string, code int) {
        }
 
        h := w.Header()
-       h.Set("Location", hexEscapeNonASCII(url))
 
-       if _, ok := h["Content-Type"]; ok {
-               return
-       }
-       if r.Method == "GET" || r.Method == "HEAD" {
+       // RFC 7231 notes that a short HTML body is usually included in
+       // the response because older user agents may not understand 301/307.
+       // Do it only if the request didn't already have a Content-Type header.
+       _, hadCT := h["Content-Type"]
+
+       h.Set("Location", hexEscapeNonASCII(url))
+       if !hadCT && (r.Method == "GET" || r.Method == "HEAD") {
                h.Set("Content-Type", "text/html; charset=utf-8")
        }
        w.WriteHeader(code)
 
-       // RFC 7231 notes that a short hypertext note is usually included in
-       // the response because older user agents may not understand 301/307.
-       // Shouldn't send the response for POST or HEAD; that leaves GET.
-       if r.Method == "GET" {
-               note := "<a href=\"" + htmlEscape(url) + "\">" + statusText[code] + "</a>.\n"
-               fmt.Fprintln(w, note)
+       // Shouldn't send the body for POST or HEAD; that leaves GET.
+       if !hadCT && r.Method == "GET" {
+               body := "<a href=\"" + htmlEscape(url) + "\">" + statusText[code] + "</a>.\n"
+               fmt.Fprintln(w, body)
        }
 }