Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mime/multipart: add 'Size int64' to FileHeader struct #19501

Closed
itsjamie opened this issue Mar 10, 2017 · 6 comments
Closed

mime/multipart: add 'Size int64' to FileHeader struct #19501

itsjamie opened this issue Mar 10, 2017 · 6 comments

Comments

@itsjamie
Copy link
Contributor

itsjamie commented Mar 10, 2017

When receiving a file from a form upload, it is sometimes required to be able to grab the individual file content length, rather than limiting the size of the entire request body.

However, when using http.Request.FormFile() you are given a multipart.File, which is implemented as either an io.SectionReader or os.File depending on whether the file was larger than the defined max multipart memory size. The SectionReader is wrapped in a little struct sectionReadCloser for a no-op close.

After looking at and implementing this size check. I was wishing to have a simpler method to implement checking the size, where right now to check, you can upgrade the sectionReadCloser into a "sizer". When it's a os.File, you could either Stat() and check the size or seek to the end, grab the number of bytes read, seek to the start, and then proceed.

Example:

func uploadHandler(w http.ResponseWriter, r *http.Request) {
	if err := r.ParseMultipartForm(); err != nil {
		http.Error(w, http.StatusText(http.InternalServerError()), http.InternalServerError())
		return
	}

	file, _, err := r.FormFile("fileupload")

	type sizer interface {
		Size() int64
	}

	var size int64
	sz, ok := file.(sizer)
	if !ok {
		// we likely have a os.File here
		// we can Stat()
		//   - FileInfo has Size()
		// alternatively
		//   - size, err = Seek(0, 2) (bytes to end)
		//   - _, err = Seek(0, 0)
	} else {
		size = sz.Size()
	}

	// use the size for setting Content-Length when forwarding upload to a object store
	// use it for logging time/size ratio to determine upload connection
	// etc...
}

If possible, adding Size() directly to the multipart.File interface would be great, but in some conversation on Gopher Slack, it was pointed out this would violate the Go 1 Compat. Since this is the case, I'd be open to other ideas here, but at the very least I would like to propose wrapping the os.File that is returned in a private struct similar to sectionReadCloser to add a Size() method to make multipart.File a single interface upgrade to get the file size.

@rsc rsc changed the title proposal: Simpler size detection for multipart.File proposal: mime/multipart: simpler size detection for File Mar 13, 2017
@rsc
Copy link
Contributor

rsc commented Mar 13, 2017

It seems like the first thing we'd need to do is decide whether we're going to enumerate the possible interfaces in a multipart.File. To date, the only thing we've guaranteed is that when it's on disk it is a *os.File:

$ go doc multipart.File
package multipart // import "mime/multipart"

type File interface {
	io.Reader
	io.ReaderAt
	io.Seeker
	io.Closer
}
    File is an interface to access the file part of a multipart message. Its
    contents may be either stored in memory or on disk. If stored on disk, the
    File's underlying concrete type will be an *os.File.

It's guaranteed to be a Seeker, so you can Seek to the end and look at the resulting offset. That's what http.ServeContent does.

@itsjamie
Copy link
Contributor Author

Yeah, using Seek in a style similar to http.ServeContent is what I ended up doing for the actual code I wrote for what triggered the proposal. So that is how I handled the case where the underlying implementation doesn't already have a Size() method that would be quicker.

I don't think we need to enumerate every possible interface of multipart.File. The intention here was that I had a case where knowing the Content-Length for a subsequent request ahead of time was going to prevent having to read the data from one buffer into another purely to get the length.

@gopherbot gopherbot added this to the Proposal milestone Mar 20, 2017
@bradfitz
Copy link
Contributor

Seems like a Size int64 field could be added to https://golang.org/pkg/mime/multipart/#FileHeader ?

Somebody want to try that?

@rsc rsc changed the title proposal: mime/multipart: simpler size detection for File proposal: mime/multipart: add 'Size int64' to FileHeader struct Mar 27, 2017
@rsc
Copy link
Contributor

rsc commented Mar 27, 2017

Approving for adding Size int64 to FileHeader.

@rsc rsc changed the title proposal: mime/multipart: add 'Size int64' to FileHeader struct mime/multipart: add 'Size int64' to FileHeader struct Mar 27, 2017
@gopherbot
Copy link

CL https://golang.org/cl/39223 mentions this issue.

@itsjamie
Copy link
Contributor Author

itsjamie commented Apr 1, 2017

@rsc @bradfitz Thank you!

lparth pushed a commit to lparth/go that referenced this issue Apr 13, 2017
This change makes it possible to retrieve the size of a file part
without having to Seek to determine file-size.

Resolves golang#19501

Change-Id: I7b9994c4cf41c9b06a046eb7046f8952ae1f15e9
Reviewed-on: https://go-review.googlesource.com/39223
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Apr 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants