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>

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)
 		}
 	}
 }

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)
 	}
 }