code review 44040043: net/http: Return ErrNotMultipart from ParseMultipartFor...

net/http: Return ErrNotMultipart from ParseMultipartForm if content-type isn't multipart/form-data.

Add test for multipart form requests with an invalid content-type to ensure
ErrNotMultipart is returned.

Change ParseMultipartForm to return ErrNotMultipart when it is returned by multipartReader.

Modify test for empty multipart request handling to use POST so that the body is checked.

Fixes issue 6334.

This is the first changeset working on multipart request handling. Further changesets
could add more tests and clean up the TODO.

[mattcottingham, 2013-12-18 20:30:35]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[gobot, 2013-12-19 17:08:16]

R=bradfitz@golang.org (assigned by rsc@google.com)

[gobot, 2013-12-20 16:26:17]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2013-12-26 20:51:11]

https://codereview.appspot.com/44040043/diff/40001/src/pkg/net/http/request.go
File src/pkg/net/http/request.go (right):

https://codereview.appspot.com/44040043/diff/40001/src/pkg/net/http/request.g...
src/pkg/net/http/request.go:784: return err
This looks very intentional.

If this were an acceptable change (and it might be a lot to convince us all it
is), the proper way to write this would be:

   if err != nil {
      return err
   }

But I'm not convinced this is a safe change.

See the TODO comment in parsePostForm which says how all this form parsing code
is too twisty.

[mattcottingham, 2013-12-27 14:45:10]

On 2013/12/26 20:51:11, bradfitz wrote:
> https://codereview.appspot.com/44040043/diff/40001/src/pkg/net/http/request.go
> File src/pkg/net/http/request.go (right):
> 
>
https://codereview.appspot.com/44040043/diff/40001/src/pkg/net/http/request.g...
> src/pkg/net/http/request.go:784: return err
> This looks very intentional.
> 
> If this were an acceptable change (and it might be a lot to convince us all it
> is), the proper way to write this would be:
> 
>    if err != nil {
>       return err
>    }
> 
> But I'm not convinced this is a safe change.
> 

Agreed. I added a test but it's likely not sufficient by itself.

> See the TODO comment in parsePostForm which says how all this form parsing
code
> is too twisty.

Yes, I saw that when reading through. I'd like to add more tests and simplify
the call graph. Does that sound okay?

[bradfitz, 2013-12-27 16:47:03]

I do want to see this code simplified, so go for it.

It will have to be simple, readable, documented, and well-tested. And any
weird behavior is likely frozen at this point, unfortunately.
On Dec 27, 2013 6:45 AM, <MattCottingham@gmail.com> wrote:

> On 2013/12/26 20:51:11, bradfitz wrote:
>
> https://codereview.appspot.com/44040043/diff/40001/src/
> pkg/net/http/request.go
>
>> File src/pkg/net/http/request.go (right):
>>
>
>
> https://codereview.appspot.com/44040043/diff/40001/src/
> pkg/net/http/request.go#newcode784
>
>> src/pkg/net/http/request.go:784: return err
>> This looks very intentional.
>>
>
>  If this were an acceptable change (and it might be a lot to convince
>>
> us all it
>
>> is), the proper way to write this would be:
>>
>
>      if err != nil {
>>        return err
>>     }
>>
>
>  But I'm not convinced this is a safe change.
>>
>
>
> Agreed. I added a test but it's likely not sufficient by itself.
>
>  See the TODO comment in parsePostForm which says how all this form
>>
> parsing code
>
>> is too twisty.
>>
>
> Yes, I saw that when reading through. I'd like to add more tests and
> simplify the call graph. Does that sound okay?
>
> https://codereview.appspot.com/44040043/
>

[mattcottingham_gmail.com, 2013-12-27 17:44:58]

On 27 December 2013 16:47, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> I do want to see this code simplified, so go for it.
>
I'll start the work in a new CL and mail a work-in-progress in a few days.

> It will have to be simple, readable, documented, and well-tested. And any
> weird behavior is likely frozen at this point, unfortunately.
>
Sure. In that case, probably best to abandon this CL (once I work out how)
and document it in the refactor.


> On Dec 27, 2013 6:45 AM, <MattCottingham@gmail.com> wrote:
>
>> On 2013/12/26 20:51:11, bradfitz wrote:
>>
>> https://codereview.appspot.com/44040043/diff/40001/src/
>> pkg/net/http/request.go
>>
>>> File src/pkg/net/http/request.go (right):
>>>
>>
>>
>> https://codereview.appspot.com/44040043/diff/40001/src/
>> pkg/net/http/request.go#newcode784
>>
>>> src/pkg/net/http/request.go:784: return err
>>> This looks very intentional.
>>>
>>
>>  If this were an acceptable change (and it might be a lot to convince
>>>
>> us all it
>>
>>> is), the proper way to write this would be:
>>>
>>
>>      if err != nil {
>>>        return err
>>>     }
>>>
>>
>>  But I'm not convinced this is a safe change.
>>>
>>
>>
>> Agreed. I added a test but it's likely not sufficient by itself.
>>
>>  See the TODO comment in parsePostForm which says how all this form
>>>
>> parsing code
>>
>>> is too twisty.
>>>
>>
>> Yes, I saw that when reading through. I'd like to add more tests and
>> simplify the call graph. Does that sound okay?
>>
>> https://codereview.appspot.com/44040043/
>>
>

[bradfitz, 2013-12-27 17:48:31]

On Fri, Dec 27, 2013 at 9:44 AM, Matt Cottingham
<mattcottingham@gmail.com>wrote:

> On 27 December 2013 16:47, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>> I do want to see this code simplified, so go for it.
>>
> I'll start the work in a new CL and mail a work-in-progress in a few days.
>
>> It will have to be simple, readable, documented, and well-tested. And any
>> weird behavior is likely frozen at this point, unfortunately.
>>
> Sure. In that case, probably best to abandon this CL (once I work out how)
> and document it in the refactor.
>

This CL might be acceptable. The method does return an error, and we
already have that error value (even though it was mostly for internal use),
and we don't document what the return value might be.  So you might still
be able to make this change, but I'd want to verify that any internal
callers aren't surprised by the change.

The call graph is crazy and I can never remember it.  Everything has weird
side effects too.

[mattcottingham_gmail.com, 2013-12-27 18:13:28]

On 27 December 2013 17:48, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> On Fri, Dec 27, 2013 at 9:44 AM, Matt Cottingham <mattcottingham@gmail.com
> > wrote:
>
>> On 27 December 2013 16:47, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>>
>>> I do want to see this code simplified, so go for it.
>>>
>> I'll start the work in a new CL and mail a work-in-progress in a few days.
>>
>>> It will have to be simple, readable, documented, and well-tested. And
>>> any weird behavior is likely frozen at this point, unfortunately.
>>>
>> Sure. In that case, probably best to abandon this CL (once I work out
>> how) and document it in the refactor.
>>
>
> This CL might be acceptable. The method does return an error, and we
> already have that error value (even though it was mostly for internal use),
> and we don't document what the return value might be.  So you might still
> be able to make this change, but I'd want to verify that any internal
> callers aren't surprised by the change.
>

I'll keep this open for now but start the other work from a blank slate.


> The call graph is crazy and I can never remember it.  Everything has weird
> side effects too.
>

Well, I have a good excuse to use Go oracle now :)

[bradfitz, 2014-01-14 23:30:16]

Did you determine whether this breaks any publicly-visible behavior or side
effects?

[mattcottingham, 2014-01-16 22:38:38]

On 2014/01/14 23:30:16, bradfitz wrote:
> Did you determine whether this breaks any publicly-visible behavior or side
> effects?

Sorry, I've been away for a while, picking things up again now.

I need to take another look and simplify/add more tests to the form handling
before I can be completely sure. Also, as you pointed out the error value might
be better kept for internal use.

[rsc, 2014-03-05 20:00:20]

matt, if you'd like this to be in go 1.3 we need to wrap up this review in the
next week or so. or else we can postpone to 1.4.

[mattcottingham, 2014-03-09 20:06:21]

Hello golang-codereviews@googlegroups.com, gobot@golang.org,
bradfitz@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[mattcottingham, 2014-03-09 20:07:29]

On 2014/03/09 20:06:21, mattcottingham wrote:
> Hello mailto:golang-codereviews@googlegroups.com, mailto:gobot@golang.org,
> mailto:bradfitz@golang.org, mailto:rsc@golang.org (cc:
mailto:golang-codereviews@googlegroups.com),
> 
> Please take another look.

I've made a couple of passes at this and I'm cautiously optimistic it doesn't
break anything. Since this has been open for a while I'll summarise:

- ParseMultipartForm now returns the same error value as MultipartReader when
the content-type is not multipart/form-data, ErrNotMultipart.

- FormValue and PostFormValue call and depend upon side-effects of
ParseMultipartForm. But ParseForm is called before multipartReader (which may
return ErrNotMultipart) so it shouldn't matter.

- FormFile also calls ParseMultipartForm. In this case the return value is
(correctly) used.

- It looks like the `return nil` was a placeholder (see comment on line 706) for
cleaning up parseForm etc.

- The tests added here and in 46750043 should lock in the call graph for a later
refactor.

It might be good to get this in for 1.3, but if we are too close to the cut I'm
happy to postpone too. Let me know what you think.

[rsc, 2014-03-13 02:32:25]

thanks. brad?

[bradfitz, 2014-04-11 05:50:00]

LGTM

Sorry for the delay. This turned out less scary than it sounded.

Thanks for the tests.

[bradfitz, 2014-04-11 05:50:12]

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

net/http: Return ErrNotMultipart from ParseMultipartForm if content-type isn't
multipart/form-data.

Add test for multipart form requests with an invalid content-type to ensure
ErrNotMultipart is returned.

Change ParseMultipartForm to return ErrNotMultipart when it is returned by
multipartReader.

Modify test for empty multipart request handling to use POST so that the body is
checked.

Fixes issue 6334.

This is the first changeset working on multipart request handling. Further
changesets
could add more tests and clean up the TODO.

LGTM=bradfitz
R=golang-codereviews, gobot, bradfitz, rsc
CC=golang-codereviews
https://codereview.appspot.com/44040043

Committer: Brad Fitzpatrick <bradfitz@golang.org>