code review 1963044: http: parse query string always, not just in GET

http: parse query string always, not just in GET

Fixes issue 985.

[rsc, 2010-08-17 14:43:57]

Hello dsymonds (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[dsymonds, 2010-08-17 22:15:58]

LGTM

http://codereview.appspot.com/1963044/diff/2001/3001
File src/pkg/http/request.go (right):

http://codereview.appspot.com/1963044/diff/2001/3001#newcode649
src/pkg/http/request.go:649: if err == nil {
err can only be nil here because a non-nil err would return back on line 646.
These four lines can just be
  err = parseQuery(...)

http://codereview.appspot.com/1963044/diff/2001/3002
File src/pkg/http/request_test.go (right):

http://codereview.appspot.com/1963044/diff/2001/3002#newcode82
src/pkg/http/request_test.go:82: }
It would also be worth checking the behaviour if a parameter is in both the URL
and the POST body.

[rsc, 2010-08-17 22:43:59]

> http://codereview.appspot.com/1963044/diff/2001/3001#newcode649
> src/pkg/http/request.go:649: if err == nil {
> err can only be nil here because a non-nil err would return back on line
> 646. These four lines can just be
>  err = parseQuery(...)

Nice catch, but different fix.

> http://codereview.appspot.com/1963044/diff/2001/3002
> File src/pkg/http/request_test.go (right):
>
> http://codereview.appspot.com/1963044/diff/2001/3002#newcode82
> src/pkg/http/request_test.go:82: }
> It would also be worth checking the behaviour if a parameter is in both
> the URL and the POST body.

What's the behavior?  I don't know which should be first.

Russ

[rsc, 2010-08-17 22:44:17]

Hello dsymonds, dsymonds1 (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2010-08-17 22:46:30]

Hello dsymonds, dsymonds1 (cc: golang-dev@googlegroups.com),

Please take another look.

[dsymonds, 2010-08-17 23:20:29]

On Wed, Aug 18, 2010 at 8:43 AM, Russ Cox <rsc@golang.org> wrote:

>> http://codereview.appspot.com/1963044/diff/2001/3002#newcode82
>> src/pkg/http/request_test.go:82: }
>> It would also be worth checking the behaviour if a parameter is in both
>> the URL and the POST body.
>
> What's the behavior?  I don't know which should be first.

My instinct would be that the URL parameters would come first, since
that's the order they come in the request. I don't think it matters
too much; it's a multimap anyway.


Dave.

[rsc, 2010-08-17 23:33:55]

> My instinct would be that the URL parameters would come first, since
> that's the order they come in the request. I don't think it matters
> too much; it's a multimap anyway.

changed

PTAL

[dsymonds, 2010-08-17 23:48:11]

LGTM

http://codereview.appspot.com/1963044/diff/14001/15002
File src/pkg/http/request_test.go (right):

http://codereview.appspot.com/1963044/diff/14001/15002#newcode84
src/pkg/http/request_test.go:84: if both := req.Form["both"];
!reflect.DeepEqual(both, []string{"x", "y"}) {
It would probably be most "correct" to sort the list and compare that way. The
current way is over-constraining the implementation. That's just nitpicking,
though. I think what you have here is fine.

[rsc, 2010-08-18 00:02:02]

*** Submitted as http://code.google.com/p/go/source/detail?r=768afe3769a8 ***

http: parse query string always, not just in GET

Fixes issue 985.

R=dsymonds, dsymonds1
CC=golang-dev
http://codereview.appspot.com/1963044