code review 7062052: net/http/cgi: make it work without REQUEST_URI environm...

net/http/cgi: make it work without REQUEST_URI environment variable

Fixes issue 4367.

[brainman, 2013-01-08 01:35:49]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2013-01-08 03:36:46]

LGTM

On Mon, Jan 7, 2013 at 5:35 PM, <alex.brainman@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http/cgi: make it work without REQUEST_URI environment variable
>
> Fixes issue 4367.
>
> Please review this at
https://codereview.appspot.**com/7062052/<https://codereview.appspot.com/7062...
>
> Affected files:
>   M src/pkg/net/http/cgi/child.go
>   M src/pkg/net/http/cgi/child_**test.go
>
>
> Index: src/pkg/net/http/cgi/child.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/cgi/child.**go
> +++ b/src/pkg/net/http/cgi/child.**go
> @@ -91,10 +91,19 @@
>
>         // TODO: cookies.  parsing them isn't exported, though.
>
> +       uriStr := params["REQUEST_URI"]
> +       if uriStr == "" {
> +               // Fallback to SCRIPT_NAME, PATH_INFO and QUERY_STRING.
> +               uriStr = params["SCRIPT_NAME"] + params["PATH_INFO"]
> +               s := params["QUERY_STRING"]
> +               if s != "" {
> +                       uriStr += "?" + s
> +               }
> +       }
>         if r.Host != "" {
>                 // Hostname is provided, so we can reasonably construct a
> URL,
>                 // even if we have to assume 'http' for the scheme.
> -               rawurl := "http://" + r.Host + params["REQUEST_URI"]
> +               rawurl := "http://" + r.Host + uriStr
>                 url, err := url.Parse(rawurl)
>                 if err != nil {
>                         return nil, errors.New("cgi: failed to parse host
> and REQUEST_URI into a URL: " + rawurl)
> @@ -104,7 +113,6 @@
>         // Fallback logic if we don't have a Host header or the URL
>         // failed to parse
>         if r.URL == nil {
> -               uriStr := params["REQUEST_URI"]
>                 url, err := url.Parse(uriStr)
>                 if err != nil {
>                         return nil, errors.New("cgi: failed to parse
> REQUEST_URI into a URL: " + uriStr)
> Index: src/pkg/net/http/cgi/child_**test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/cgi/child_**test.go
> +++ b/src/pkg/net/http/cgi/child_**test.go
> @@ -85,3 +85,25 @@
>                 t.Errorf("expected URL %q; got %q", e, g)
>         }
>  }
> +
> +func TestRequestWithoutRequestURI(t *testing.T) {
> +       env := map[string]string{
> +               "SERVER_PROTOCOL": "HTTP/1.1",
> +               "HTTP_HOST":       "example.com",
> +               "REQUEST_METHOD":  "GET",
> +               "SCRIPT_NAME":     "/dir/scriptname",
> +               "PATH_INFO":       "/p1/p2",
> +               "QUERY_STRING":    "a=1&b=2",
> +               "CONTENT_LENGTH":  "123",
> +       }
> +       req, err := RequestFromMap(env)
> +       if err != nil {
> +               t.Fatalf("RequestFromMap: %v", err)
> +       }
> +       if req.URL == nil {
> +               t.Fatalf("unexpected nil URL")
> +       }
> +       if g, e := req.URL.String(), "http://example.com/dir/**
> scriptname/p1/p2?a=1&b=2 <http://example.com/dir/scriptname/p1/p2?a=1&b=2>";
> e != g {
> +               t.Errorf("expected URL %q; got %q", e, g)
> +       }
> +}
>
>
>

[bradfitz, 2013-01-08 03:37:54]

LGTM

https://codereview.appspot.com/7062052/diff/5001/src/pkg/net/http/cgi/child_t...
File src/pkg/net/http/cgi/child_test.go (right):

https://codereview.appspot.com/7062052/diff/5001/src/pkg/net/http/cgi/child_t...
src/pkg/net/http/cgi/child_test.go:107: t.Errorf("expected URL %q; got %q", e,
g)
Go style error message & order is:

Errorf("URL = %q; want %q", g, e)

[brainman, 2013-01-08 06:23:56]

*** Submitted as https://code.google.com/p/go/source/detail?r=08a1396e9aa7 ***

net/http/cgi: make it work without REQUEST_URI environment variable

Fixes issue 4367.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/7062052

https://codereview.appspot.com/7062052/diff/5001/src/pkg/net/http/cgi/child_t...
File src/pkg/net/http/cgi/child_test.go (right):

https://codereview.appspot.com/7062052/diff/5001/src/pkg/net/http/cgi/child_t...
src/pkg/net/http/cgi/child_test.go:107: t.Errorf("expected URL %q; got %q", e,
g)
On 2013/01/08 03:37:54, bradfitz wrote:
> Go style error message & order is:
> 
> Errorf("URL = %q; want %q", g, e)

Done. But I have also changed the original line in TestRequestWithoutHost now.