From 36036781d55c03e1120911e299ea6a48ed718524 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 12 Oct 2011 11:48:25 -0700 Subject: [PATCH] http: remove Request.RawURL Its purpose is not only undocumented, it's also unknown (to me and Russ, at least) and leads to complexity, bugs and confusion. R=golang-dev, adg, rsc CC=golang-dev https://golang.org/cl/5213043 --- src/pkg/http/cgi/child.go | 12 +++++----- src/pkg/http/cgi/child_test.go | 6 ----- src/pkg/http/cgi/host.go | 1 - src/pkg/http/readrequest_test.go | 3 --- src/pkg/http/request.go | 37 ++++++++++++++-------------- src/pkg/http/requestwrite_test.go | 40 +++++++++++++++++-------------- src/pkg/http/transport.go | 6 ++--- src/pkg/http/transport_test.go | 32 +++---------------------- src/pkg/websocket/hixie_test.go | 6 ++--- src/pkg/websocket/hybi_test.go | 12 +++++----- 10 files changed, 61 insertions(+), 94 deletions(-) diff --git a/src/pkg/http/cgi/child.go b/src/pkg/http/cgi/child.go index 8d0eca8d55b..bf14c04a843 100644 --- a/src/pkg/http/cgi/child.go +++ b/src/pkg/http/cgi/child.go @@ -93,20 +93,20 @@ func RequestFromMap(params map[string]string) (*http.Request, os.Error) { if r.Host != "" { // Hostname is provided, so we can reasonably construct a URL, // even if we have to assume 'http' for the scheme. - r.RawURL = "http://" + r.Host + params["REQUEST_URI"] - url, err := url.Parse(r.RawURL) + rawurl := "http://" + r.Host + params["REQUEST_URI"] + url, err := url.Parse(rawurl) if err != nil { - return nil, os.NewError("cgi: failed to parse host and REQUEST_URI into a URL: " + r.RawURL) + return nil, os.NewError("cgi: failed to parse host and REQUEST_URI into a URL: " + rawurl) } r.URL = url } // Fallback logic if we don't have a Host header or the URL // failed to parse if r.URL == nil { - r.RawURL = params["REQUEST_URI"] - url, err := url.Parse(r.RawURL) + uriStr := params["REQUEST_URI"] + url, err := url.Parse(uriStr) if err != nil { - return nil, os.NewError("cgi: failed to parse REQUEST_URI into a URL: " + r.RawURL) + return nil, os.NewError("cgi: failed to parse REQUEST_URI into a URL: " + uriStr) } r.URL = url } diff --git a/src/pkg/http/cgi/child_test.go b/src/pkg/http/cgi/child_test.go index eee043bc90d..ec53ab851ba 100644 --- a/src/pkg/http/cgi/child_test.go +++ b/src/pkg/http/cgi/child_test.go @@ -49,9 +49,6 @@ func TestRequest(t *testing.T) { if g, e := req.Header.Get("Foo-Bar"), "baz"; e != g { t.Errorf("expected Foo-Bar %q; got %q", e, g) } - if g, e := req.RawURL, "http://example.com/path?a=b"; e != g { - t.Errorf("expected RawURL %q; got %q", e, g) - } if g, e := req.URL.String(), "http://example.com/path?a=b"; e != g { t.Errorf("expected URL %q; got %q", e, g) } @@ -81,9 +78,6 @@ func TestRequestWithoutHost(t *testing.T) { if err != nil { t.Fatalf("RequestFromMap: %v", err) } - if g, e := req.RawURL, "/path?a=b"; e != g { - t.Errorf("expected RawURL %q; got %q", e, g) - } if req.URL == nil { t.Fatalf("unexpected nil URL") } diff --git a/src/pkg/http/cgi/host.go b/src/pkg/http/cgi/host.go index 1d63821416d..9ea4c9d8bf2 100644 --- a/src/pkg/http/cgi/host.go +++ b/src/pkg/http/cgi/host.go @@ -322,7 +322,6 @@ func (h *Handler) handleInternalRedirect(rw http.ResponseWriter, req *http.Reque newReq := &http.Request{ Method: "GET", URL: url, - RawURL: path, Proto: "HTTP/1.1", ProtoMajor: 1, ProtoMinor: 1, diff --git a/src/pkg/http/readrequest_test.go b/src/pkg/http/readrequest_test.go index f6dc99e2e08..6d9042aceb6 100644 --- a/src/pkg/http/readrequest_test.go +++ b/src/pkg/http/readrequest_test.go @@ -40,7 +40,6 @@ var reqTests = []reqTest{ &Request{ Method: "GET", - RawURL: "http://www.techcrunch.com/", URL: &url.URL{ Raw: "http://www.techcrunch.com/", Scheme: "http", @@ -83,7 +82,6 @@ var reqTests = []reqTest{ &Request{ Method: "GET", - RawURL: "/", URL: &url.URL{ Raw: "/", Path: "/", @@ -110,7 +108,6 @@ var reqTests = []reqTest{ &Request{ Method: "GET", - RawURL: "//user@host/is/actually/a/path/", URL: &url.URL{ Raw: "//user@host/is/actually/a/path/", Scheme: "", diff --git a/src/pkg/http/request.go b/src/pkg/http/request.go index dc344ca0057..4f555ff5753 100644 --- a/src/pkg/http/request.go +++ b/src/pkg/http/request.go @@ -80,9 +80,8 @@ var reqWriteExcludeHeaderDump = map[string]bool{ // A Request represents a parsed HTTP request header. type Request struct { - Method string // GET, POST, PUT, etc. - RawURL string // The raw URL given in the request. - URL *url.URL // Parsed URL. + Method string // GET, POST, PUT, etc. + URL *url.URL // The protocol version for incoming requests. // Outgoing requests always use HTTP/1.1. @@ -265,7 +264,7 @@ const defaultUserAgent = "Go http package" // Write writes an HTTP/1.1 request -- header and body -- in wire format. // This method consults the following fields of req: // Host -// RawURL, if non-empty, or else URL +// URL // Method (defaults to "GET") // Header // ContentLength @@ -282,21 +281,18 @@ func (req *Request) Write(w io.Writer) os.Error { // WriteProxy is like Write but writes the request in the form // expected by an HTTP proxy. In particular, WriteProxy writes the // initial Request-URI line of the request with an absolute URI, per -// section 5.1.2 of RFC 2616, including the scheme and host. If -// req.RawURL is non-empty, WriteProxy uses it unchanged. In either -// case, WriteProxy also writes a Host header, using either req.Host -// or req.URL.Host. +// section 5.1.2 of RFC 2616, including the scheme and host. In +// either case, WriteProxy also writes a Host header, using either +// req.Host or req.URL.Host. func (req *Request) WriteProxy(w io.Writer) os.Error { return req.write(w, true) } func (req *Request) dumpWrite(w io.Writer) os.Error { - urlStr := req.RawURL - if urlStr == "" { - urlStr = valueOrDefault(req.URL.EncodedPath(), "/") - if req.URL.RawQuery != "" { - urlStr += "?" + req.URL.RawQuery - } + // TODO(bradfitz): RawPath here? + urlStr := valueOrDefault(req.URL.EncodedPath(), "/") + if req.URL.RawQuery != "" { + urlStr += "?" + req.URL.RawQuery } bw := bufio.NewWriter(w) @@ -346,9 +342,12 @@ func (req *Request) write(w io.Writer, usingProxy bool) os.Error { host = req.URL.Host } - urlStr := req.RawURL + urlStr := req.URL.RawPath + if strings.HasPrefix(urlStr, "?") { + urlStr = "/" + urlStr // Issue 2344 + } if urlStr == "" { - urlStr = valueOrDefault(req.URL.EncodedPath(), "/") + urlStr = valueOrDefault(req.URL.RawPath, valueOrDefault(req.URL.EncodedPath(), "/")) if req.URL.RawQuery != "" { urlStr += "?" + req.URL.RawQuery } @@ -359,6 +358,7 @@ func (req *Request) write(w io.Writer, usingProxy bool) os.Error { urlStr = req.URL.Scheme + "://" + host + urlStr } } + // TODO(bradfitz): escape at least newlines in urlStr? bw := bufio.NewWriter(w) fmt.Fprintf(bw, "%s %s HTTP/1.1\r\n", valueOrDefault(req.Method, "GET"), urlStr) @@ -598,13 +598,14 @@ func ReadRequest(b *bufio.Reader) (req *Request, err os.Error) { if f = strings.SplitN(s, " ", 3); len(f) < 3 { return nil, &badStringError{"malformed HTTP request", s} } - req.Method, req.RawURL, req.Proto = f[0], f[1], f[2] + var rawurl string + req.Method, rawurl, req.Proto = f[0], f[1], f[2] var ok bool if req.ProtoMajor, req.ProtoMinor, ok = ParseHTTPVersion(req.Proto); !ok { return nil, &badStringError{"malformed HTTP version", req.Proto} } - if req.URL, err = url.ParseRequest(req.RawURL); err != nil { + if req.URL, err = url.ParseRequest(rawurl); err != nil { return nil, err } diff --git a/src/pkg/http/requestwrite_test.go b/src/pkg/http/requestwrite_test.go index 8c29c44f498..194f6dd213c 100644 --- a/src/pkg/http/requestwrite_test.go +++ b/src/pkg/http/requestwrite_test.go @@ -32,7 +32,6 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "GET", - RawURL: "http://www.techcrunch.com/", URL: &url.URL{ Raw: "http://www.techcrunch.com/", Scheme: "http", @@ -188,7 +187,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "POST", - RawURL: "http://example.com/", + URL: mustParseURL("http://example.com/"), Host: "example.com", Header: Header{ "Content-Length": []string{"10"}, // ignored @@ -198,14 +197,14 @@ var reqWriteTests = []reqWriteTest{ Body: []byte("abcdef"), - WantWrite: "POST http://example.com/ HTTP/1.1\r\n" + + WantWrite: "POST / HTTP/1.1\r\n" + "Host: example.com\r\n" + "User-Agent: Go http package\r\n" + "Content-Length: 6\r\n" + "\r\n" + "abcdef", - WantProxy: "POST http://example.com/ HTTP/1.1\r\n" + + WantProxy: "POST / HTTP/1.1\r\n" + "Host: example.com\r\n" + "User-Agent: Go http package\r\n" + "Content-Length: 6\r\n" + @@ -217,7 +216,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "GET", - RawURL: "/search", + URL: mustParseURL("/search"), Host: "www.google.com", }, @@ -225,19 +224,13 @@ var reqWriteTests = []reqWriteTest{ "Host: www.google.com\r\n" + "User-Agent: Go http package\r\n" + "\r\n", - - // Looks weird but RawURL overrides what WriteProxy would choose. - WantProxy: "GET /search HTTP/1.1\r\n" + - "Host: www.google.com\r\n" + - "User-Agent: Go http package\r\n" + - "\r\n", }, // Request with a 0 ContentLength and a 0 byte body. { Req: Request{ Method: "POST", - RawURL: "/", + URL: mustParseURL("/"), Host: "example.com", ProtoMajor: 1, ProtoMinor: 1, @@ -266,7 +259,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "POST", - RawURL: "/", + URL: mustParseURL("/"), Host: "example.com", ProtoMajor: 1, ProtoMinor: 1, @@ -292,7 +285,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "POST", - RawURL: "/", + URL: mustParseURL("/"), Host: "example.com", ProtoMajor: 1, ProtoMinor: 1, @@ -306,7 +299,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "POST", - RawURL: "/", + URL: mustParseURL("/"), Host: "example.com", ProtoMajor: 1, ProtoMinor: 1, @@ -320,7 +313,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "POST", - RawURL: "/", + URL: mustParseURL("/"), Host: "example.com", ProtoMajor: 1, ProtoMinor: 1, @@ -334,7 +327,7 @@ var reqWriteTests = []reqWriteTest{ { Req: Request{ Method: "GET", - RawURL: "/foo", + URL: mustParseURL("/foo"), ProtoMajor: 1, ProtoMinor: 0, Header: Header{ @@ -349,7 +342,10 @@ var reqWriteTests = []reqWriteTest{ // .. but we can't call Request.Write on it, due to its lack of Host header. // TODO(bradfitz): there might be an argument to allow this, but for now I'd // rather let HTTP/1.0 continue to die. - WantError: os.NewError("http: Request.Write on Request with no Host or URL set"), + WantWrite: "GET /foo HTTP/1.1\r\n" + + "Host: \r\n" + + "User-Agent: Go http package\r\n" + + "X-Foo: X-Bar\r\n\r\n", }, } @@ -464,3 +460,11 @@ func TestRequestWriteClosesBody(t *testing.T) { func chunk(s string) string { return fmt.Sprintf("%x\r\n%s\r\n", len(s), s) } + +func mustParseURL(s string) *url.URL { + u, err := url.Parse(s) + if err != nil { + panic(fmt.Sprintf("Error parsing URL %q: %v", s, err)) + } + return u +} diff --git a/src/pkg/http/transport.go b/src/pkg/http/transport.go index 8ac78324a38..a580e1f7cb4 100644 --- a/src/pkg/http/transport.go +++ b/src/pkg/http/transport.go @@ -103,9 +103,7 @@ func ProxyURL(fixedURL *url.URL) func(*Request) (*url.URL, os.Error) { // RoundTrip implements the RoundTripper interface. func (t *Transport) RoundTrip(req *Request) (resp *Response, err os.Error) { if req.URL == nil { - if req.URL, err = url.Parse(req.RawURL); err != nil { - return - } + return nil, os.NewError("http: nil Request.URL") } if req.URL.Scheme != "http" && req.URL.Scheme != "https" { t.lk.Lock() @@ -315,7 +313,7 @@ func (t *Transport) getConn(cm *connectMethod) (*persistConn, os.Error) { case cm.targetScheme == "https": connectReq := &Request{ Method: "CONNECT", - RawURL: cm.targetAddr, + URL: &url.URL{RawPath: cm.targetAddr}, Host: cm.targetAddr, Header: make(Header), } diff --git a/src/pkg/http/transport_test.go b/src/pkg/http/transport_test.go index b9ae7a36857..a5dfe5ee3ca 100644 --- a/src/pkg/http/transport_test.go +++ b/src/pkg/http/transport_test.go @@ -78,7 +78,7 @@ func TestTransportConnectionCloseOnResponse(t *testing.T) { fetch := func(n int) string { req := new(Request) var err os.Error - req.URL, err = url.Parse(ts.URL + fmt.Sprintf("?close=%v", connectionClose)) + req.URL, err = url.Parse(ts.URL + fmt.Sprintf("/?close=%v", connectionClose)) if err != nil { t.Fatalf("URL parse error: %v", err) } @@ -362,32 +362,6 @@ func TestTransportHeadChunkedResponse(t *testing.T) { } } -func TestTransportNilURL(t *testing.T) { - ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { - fmt.Fprintf(w, "Hi") - })) - defer ts.Close() - - req := new(Request) - req.URL = nil // what we're actually testing - req.Method = "GET" - req.RawURL = ts.URL - req.Proto = "HTTP/1.1" - req.ProtoMajor = 1 - req.ProtoMinor = 1 - req.Header = make(Header) - - tr := &Transport{} - res, err := tr.RoundTrip(req) - if err != nil { - t.Fatalf("unexpected RoundTrip error: %v", err) - } - body, err := ioutil.ReadAll(res.Body) - if g, e := string(body), "Hi"; g != e { - t.Fatalf("Expected response body of %q; got %q", e, g) - } -} - var roundTripTests = []struct { accept string expectAccept string @@ -484,7 +458,7 @@ func TestTransportGzip(t *testing.T) { c := &Client{Transport: &Transport{}} // First fetch something large, but only read some of it. - res, err := c.Get(ts.URL + "?body=large&chunked=" + chunked) + res, err := c.Get(ts.URL + "/?body=large&chunked=" + chunked) if err != nil { t.Fatalf("large get: %v", err) } @@ -504,7 +478,7 @@ func TestTransportGzip(t *testing.T) { } // Then something small. - res, err = c.Get(ts.URL + "?chunked=" + chunked) + res, err = c.Get(ts.URL + "/?chunked=" + chunked) if err != nil { t.Fatal(err) } diff --git a/src/pkg/websocket/hixie_test.go b/src/pkg/websocket/hixie_test.go index a480b6608c5..98a0de4d6f4 100644 --- a/src/pkg/websocket/hixie_test.go +++ b/src/pkg/websocket/hixie_test.go @@ -72,13 +72,13 @@ Sec-WebSocket-Protocol: sample } req, err := http.ReadRequest(bufio.NewReader(b)) if err != nil { - t.Errorf("read request: %v", err) + t.Fatalf("read request: %v", err) } if req.Method != "GET" { t.Errorf("request method expected GET, but got %q", req.Method) } - if req.RawURL != "/demo" { - t.Errorf("request path expected /demo, but got %q", req.RawURL) + if req.URL.Path != "/demo" { + t.Errorf("request path expected /demo, but got %q", req.URL.Path) } if req.Proto != "HTTP/1.1" { t.Errorf("request proto expected HTTP/1.1, but got %q", req.Proto) diff --git a/src/pkg/websocket/hybi_test.go b/src/pkg/websocket/hybi_test.go index 0814c08015f..71d1893b301 100644 --- a/src/pkg/websocket/hybi_test.go +++ b/src/pkg/websocket/hybi_test.go @@ -63,13 +63,13 @@ Sec-WebSocket-Protocol: chat } req, err := http.ReadRequest(bufio.NewReader(b)) if err != nil { - t.Errorf("read request: %v", err) + t.Fatalf("read request: %v", err) } if req.Method != "GET" { t.Errorf("request method expected GET, but got %q", req.Method) } - if req.RawURL != "/chat" { - t.Errorf("request path expected /chat, but got %q", req.RawURL) + if req.URL.Path != "/chat" { + t.Errorf("request path expected /chat, but got %q", req.URL.Path) } if req.Proto != "HTTP/1.1" { t.Errorf("request proto expected HTTP/1.1, but got %q", req.Proto) @@ -125,13 +125,13 @@ Sec-WebSocket-Protocol: chat } req, err := http.ReadRequest(bufio.NewReader(b)) if err != nil { - t.Errorf("read request: %v", err) + t.Fatalf("read request: %v", err) } if req.Method != "GET" { t.Errorf("request method expected GET, but got %q", req.Method) } - if req.RawURL != "/chat" { - t.Errorf("request path expected /demo, but got %q", req.RawURL) + if req.URL.Path != "/chat" { + t.Errorf("request path expected /demo, but got %q", req.URL.Path) } if req.Proto != "HTTP/1.1" { t.Errorf("request proto expected HTTP/1.1, but got %q", req.Proto)