|
|
|
Created:
12 years ago by mattcottingham Modified:
11 years, 9 months ago Reviewers:
bradfitz CC:
golang-codereviews, gobot, bradfitz, rsc Visibility:
Public. |
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.
Patch Set 1 #Patch Set 2 : diff -r 477af6a8e51f https://code.google.com/p/go #Patch Set 3 : diff -r 25cd0f8e0134 https://code.google.com/p/go #
Total comments: 1
Patch Set 4 : diff -r 1849f83423ca http://code.google.com/p/go #Patch Set 5 : diff -r 1849f83423ca http://code.google.com/p/go #Patch Set 6 : diff -r 1849f83423ca http://code.google.com/p/go #Total messages: 17
|
mattcottingham
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
|
12 years ago (2013年12月18日 20:30:35 UTC) #1 |
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go
R=bradfitz@golang.org (assigned by rsc@google.com)
Replacing golang-dev with golang-codereviews.
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.
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?
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/ >
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/ >> >
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.
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 :)
Did you determine whether this breaks any publicly-visible behavior or side effects?
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.
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.
Hello golang-codereviews@googlegroups.com, gobot@golang.org, bradfitz@golang.org, rsc@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
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.
thanks. brad?
LGTM Sorry for the delay. This turned out less scary than it sounded. Thanks for the tests.
*** 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>