code review 4431068: http: add MultipartForm, FormFile, and ParseMultipartFo...

http: add MultipartForm, FormFile, and ParseMultipartForm to Request

[adg, 2011-04-27 06:47:50]

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

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

[bradfitz, 2011-04-27 23:44:33]

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode33
src/pkg/http/request.go:33: var DefaultMultipartMaxMemory int64 = 1048576 // 1
MB
document

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode35
src/pkg/http/request.go:35: var ErrMissingField = os.ErrorString("no such
field")
"http: no such field"

document

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode590
src/pkg/http/request.go:590: // For POST requests, it also parses the request
body as a form.
should this reference ParseMultipartForm?

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode627
src/pkg/http/request.go:627: // ParseMultipartForm parses a request body as
multipart/form-data.
this isn't clear in how it differs from Request.MultipartReader.  One should be
explicit that it slurps and one should be explicit that it streams.

It also doesn't document the meaning of maxMemory.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode672
src/pkg/http/request.go:672: // FormFile returns the first file for the named
component of the query.
s/component of the query/provided form key/

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode673
src/pkg/http/request.go:673: // FormFile calls ParseMultipartForm (and therefore
ParseForm) if necessary.
I might ditch the part in parens.  Not sure that it helps explain.

[adg, 2011-04-27 23:57:27]

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode33
src/pkg/http/request.go:33: var DefaultMultipartMaxMemory int64 = 1048576 // 1
MB
On 2011/04/27 23:44:33, bradfitzgo wrote:
> document

Done.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode35
src/pkg/http/request.go:35: var ErrMissingField = os.ErrorString("no such
field")
On 2011/04/27 23:44:33, bradfitzgo wrote:
> "http: no such field"
> 
> document

Done.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode590
src/pkg/http/request.go:590: // For POST requests, it also parses the request
body as a form.
On 2011/04/27 23:44:33, bradfitzgo wrote:
> should this reference ParseMultipartForm?

Done.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode627
src/pkg/http/request.go:627: // ParseMultipartForm parses a request body as
multipart/form-data.
On 2011/04/27 23:44:33, bradfitzgo wrote:
> this isn't clear in how it differs from Request.MultipartReader.  One should
be
> explicit that it slurps and one should be explicit that it streams.
> 
> It also doesn't document the meaning of maxMemory.

Done.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode672
src/pkg/http/request.go:672: // FormFile returns the first file for the named
component of the query.
On 2011/04/27 23:44:33, bradfitzgo wrote:
> s/component of the query/provided form key/

Done.

http://codereview.appspot.com/4431068/diff/1/src/pkg/http/request.go#newcode673
src/pkg/http/request.go:673: // FormFile calls ParseMultipartForm (and therefore
ParseForm) if necessary.
On 2011/04/27 23:44:33, bradfitzgo wrote:
> I might ditch the part in parens.  Not sure that it helps explain.  

Done.

[bradfitz, 2011-04-28 00:08:40]

In the words of Rob:  "I see why your tests aren't catching the bug.  You have
no tests."

I don't think this code will work.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:38: // is either present in the request or not a file
field.
missing the word "not"?

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:184: func (r *Request) MultipartReader()
(multipart.Reader, os.Error) {
I wish this could return an error if it's called multiple times, but we have no
good place to put that bit.

*Request is all public fields.

I suppose we could do something gross like:

var multipartEmptyForm = make(map[string][]string)

func (r *Request) MultipartReader() (multipart.Reader, os.Error) {
   if r.Form != nil && r.Form == multipartEmptyForm {
      return os.NewError("http: MultipartReader called twice")
   }
   r.Form = multipartEmptyForm
...
}

But I don't know if that's gross or clever.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:630: // TODO(dsymonds): Handle multipart/form-data
remove / fix comment

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:632: return &badStringError{"unknown Content-Type", ct}
won't this be returned from the ParseForm() that ParseMultipartForm() initially
calls?

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:648: return err
I think you'll always return an error here for multipart forms.  See comment
above.

[adg, 2011-04-28 01:20:18]

Tests coming.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:38: // is either present in the request or not a file
field.
On 2011/04/28 00:08:40, bradfitz wrote:
> missing the word "not"?

Done.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:184: func (r *Request) MultipartReader()
(multipart.Reader, os.Error) {
On 2011/04/28 00:08:40, bradfitz wrote:
> I wish this could return an error if it's called multiple times, but we have
no
> good place to put that bit.
> 
> *Request is all public fields.
> 
> I suppose we could do something gross like:
> 
> var multipartEmptyForm = make(map[string][]string)
> 
> func (r *Request) MultipartReader() (multipart.Reader, os.Error) {
>    if r.Form != nil && r.Form == multipartEmptyForm {
>       return os.NewError("http: MultipartReader called twice")
>    }
>    r.Form = multipartEmptyForm
> ...
> }
> 
> But I don't know if that's gross or clever.

I think it's nice. Allows us to provide helpful errors when the user tries to
mix techniques. (Done)

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:630: // TODO(dsymonds): Handle multipart/form-data
On 2011/04/28 00:08:40, bradfitz wrote:
> remove / fix comment

Done.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:632: return &badStringError{"unknown Content-Type", ct}
On 2011/04/28 00:08:40, bradfitz wrote:
> won't this be returned from the ParseForm() that ParseMultipartForm()
initially
> calls?

Added a case to fix this.

http://codereview.appspot.com/4431068/diff/6001/src/pkg/http/request.go#newco...
src/pkg/http/request.go:648: return err
On 2011/04/28 00:08:40, bradfitz wrote:
> I think you'll always return an error here for multipart forms.  See comment
> above.

Fixed.

[adg, 2011-04-28 01:38:06]

Hello brad_danga_com, rsc, bradfitz (cc: golang-dev@googlegroups.com),

Please take another look.

[adg, 2011-04-28 01:39:13]

Hello brad_danga_com, rsc, bradfitz (cc: golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2011-04-28 02:13:18]

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:180: var multipartByReader = &multipart.Form{}
This isn't safe.  If you do this, you need to also make the inner maps in it. 
The zero value isn't safe to give out to clients, even stupid ones.  Because a
halfway-smart client will check the non-nil ness of this and decide that if it's
non-nil, it's initalized.

I'd just add the two map fields to this variable, initialized.

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:180: var multipartByReader = &multipart.Form{}
also might want to document this field, that it's a sentinel value.  it's
arguably a little tricky and warrants an explanation to future readers.

[adg, 2011-04-28 02:27:47]

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:180: var multipartByReader = &multipart.Form{}
On 2011/04/28 02:13:18, bradfitz wrote:
> This isn't safe.  If you do this, you need to also make the inner maps in it. 
> The zero value isn't safe to give out to clients, even stupid ones.  Because a
> halfway-smart client will check the non-nil ness of this and decide that if
it's
> non-nil, it's initalized.
> 
> I'd just add the two map fields to this variable, initialized.

Fixed. Also split MultipartReader into public and private sections, so that
ParseMultipartForm can get a MultipartReader without setting the sentinel.

http://codereview.appspot.com/4431068/diff/14001/src/pkg/http/request.go#newc...
src/pkg/http/request.go:180: var multipartByReader = &multipart.Form{}
On 2011/04/28 02:13:18, bradfitz wrote:
> also might want to document this field, that it's a sentinel value.  it's
> arguably a little tricky and warrants an explanation to future readers.

Done.

[adg, 2011-04-28 02:28:05]

Hello brad_danga_com, rsc, bradfitz (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-04-28 02:52:11]

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:35: var DefaultMultipartMaxMemory int64 = 1048576 // 1
MB
The other maximum values are in a const block above.
It seems like this one belongs there too.  If clients
want to do something different, that's why ParseMultipartForm has an argument.

defaultMaxMemory = 1<<20

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:37: // ErrMissingField is returned by FormFile when the
provided field name
shouldn't it be ErrMissingFile and http: no such file?

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:181: // Its presence in Request.Form indicates that
parsing of the request body
s/Form/MultipartForm/

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:190: // Use this function instead of ParseMultipartForm
if you wish to
s/if you wish //

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:203: func (r *Request) getMultipartReader()
(multipart.Reader, os.Error) {
s/getM/m/

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:663: // It is indempotent.
s/ind/id/
but really that's not the right word since it doesn't
explain what happens when called twice with different
maxMemory arguments.

// After one call to ParseMultipartForm, subsequent calls
// have no effect.

[bradfitz, 2011-04-28 02:58:51]

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go
File src/pkg/http/request_test.go (right):

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:175: func TestMultipartRequest(t *testing.T) {
add one-liner comments saying what these are testing?

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:184: func TestMultipartRequestAuto(t *testing.T) {
add one-liner comment?

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:195: testBody :=
regexp.MustCompile("\n").ReplaceAllString(message, "\r\n")
strings.Replace

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:214: assertMem := func(fd multipart.File) {
This is verboten.

It will generate an error message like:

file is *os.File, should not be

... but is that from line 220 or line 223?

The reason the testing package doesn't have any of the usual expect/assert
equality helpers is because Rob, Robert, et al don't like un-debuggable tests
that are convenient for the test author to write, but useless to the debugger
when the test fails.

They prefer to shift the onus to the test writer, making their life more
difficult, but making it easier to debug later.

You can do helpers like this, but you have to be absolutely certain that when
they fail, they produce helpful output.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:231: func testMultipartFile(t *testing.T, req
*Request, key, efn, econtent string) multipart.File {
Maybe:
efn -> expectFilename
econtent -> expectContent
?

[adg, 2011-04-28 03:07:41]

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

Please take another look.

[adg, 2011-04-28 03:07:52]

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:37: // ErrMissingField is returned by FormFile when the
provided field name
On 2011/04/28 02:52:11, rsc wrote:
> shouldn't it be ErrMissingFile and http: no such file?

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:181: // Its presence in Request.Form indicates that
parsing of the request body
On 2011/04/28 02:52:11, rsc wrote:
> s/Form/MultipartForm/

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:190: // Use this function instead of ParseMultipartForm
if you wish to
On 2011/04/28 02:52:11, rsc wrote:
> s/if you wish //

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:203: func (r *Request) getMultipartReader()
(multipart.Reader, os.Error) {
On 2011/04/28 02:52:11, rsc wrote:
> s/getM/m/

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:663: // It is indempotent.
On 2011/04/28 02:52:11, rsc wrote:
> s/ind/id/
> but really that's not the right word since it doesn't
> explain what happens when called twice with different
> maxMemory arguments.
> 
> // After one call to ParseMultipartForm, subsequent calls
> // have no effect.

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go
File src/pkg/http/request_test.go (right):

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:184: func TestMultipartRequestAuto(t *testing.T) {
On 2011/04/28 02:58:51, bradfitz wrote:
> add one-liner comment?

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:195: testBody :=
regexp.MustCompile("\n").ReplaceAllString(message, "\r\n")
On 2011/04/28 02:58:51, bradfitz wrote:
> strings.Replace

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:214: assertMem := func(fd multipart.File) {
On 2011/04/28 02:58:51, bradfitz wrote:
> This is verboten.
> 
> It will generate an error message like:
> 
> file is *os.File, should not be
> 
> ... but is that from line 220 or line 223?
> 
> The reason the testing package doesn't have any of the usual expect/assert
> equality helpers is because Rob, Robert, et al don't like un-debuggable tests
> that are convenient for the test author to write, but useless to the debugger
> when the test fails.
> 
> They prefer to shift the onus to the test writer, making their life more
> difficult, but making it easier to debug later.
> 
> You can do helpers like this, but you have to be absolutely certain that when
> they fail, they produce helpful output.

Done.

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request_test.go#...
src/pkg/http/request_test.go:231: func testMultipartFile(t *testing.T, req
*Request, key, efn, econtent string) multipart.File {
On 2011/04/28 02:58:51, bradfitz wrote:
> Maybe:
> efn -> expectFilename
> econtent -> expectContent
> ?

Done.

[adg, 2011-04-28 04:38:19]

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go
File src/pkg/http/request.go (right):

http://codereview.appspot.com/4431068/diff/7003/src/pkg/http/request.go#newco...
src/pkg/http/request.go:35: var DefaultMultipartMaxMemory int64 = 1048576 // 1
MB
On 2011/04/28 02:52:11, rsc wrote:
> The other maximum values are in a const block above.
> It seems like this one belongs there too.  If clients
> want to do something different, that's why ParseMultipartForm has an argument.
> 
> defaultMaxMemory = 1<<20

Done, but set to 32<<20

[rsc, 2011-04-28 04:45:44]

LGTM

[bradfitz, 2011-04-28 04:50:29]

LGTM

[adg, 2011-04-28 05:21:58]

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

http: add MultipartForm, FormFile, and ParseMultipartForm to Request

R=rsc, bradfitz
CC=golang-dev
http://codereview.appspot.com/4431068