golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.14k stars 17.69k forks source link

mime/multipart: add FileContentDisposition #46771

Open robtimus opened 3 years ago

robtimus commented 3 years ago

For uploading files as part of multipart/form-data requests, the specification allows the content type to be specified, e.g. application/pdf. However, the current version of Go always uses application/octet-stream. While it's possible to work around this, it requires duplicating code like the Sprintf command and the escaping. An example can be found at for example, see https://github.com/Ingenico-ePayments/connect-sdk-go/blob/master/defaultimpl/DefaultConnection.go#L345. Lines 345-348 could all be replaced by a single line if the Go API supported custom content types, and the escaping of quotes could be removed completely.

I've already created a PR for this which: https://github.com/golang/go/pull/29140. It's fully backward compatible because it introduces a new function; the existing function delegates to the new one.

ianlancetaylor commented 3 years ago

For the record, the new API proposed in https://golang.org/cl/153178 is

// CreateFormFileWithContentType is a convenience wrapper around CreatePart.
// It creates a new form-data header with the provided field name, file name and content type.
func (w *Writer) CreateFormFileWithContentType(fieldname, filename, contentType string) (io.Writer, error)
rsc commented 3 years ago

What if instead we add:

func FileContentDisposition(fieldname, filename string) string {
    return fmt.Sprintf(`form-data; name="%s"; filename="%s"`,
        escapeQuotes(fieldname), escapeQuotes(filename)))
}

?

Then CreateFormFile is very simple:

func (w *Writer) CreateFormFile(fieldname, filename string) (io.Writer, error) {
        h := make(textproto.MIMEHeader)
        h.Set("Content-Disposition", FileContentDisposition(fieldname, filename)
        h.Set("Content-Type", "application/octet-stream")
        return w.CreatePart(h)
}

and at that point if you want a custom Content-Type or any other header, it's reasonable to just write your own version.

rsc commented 3 years ago

This proposal has been added to the active column of the proposals project and will now be reviewed at the weekly proposal review meetings. — rsc for the proposal review group

robtimus commented 3 years ago

The latter would work, but it still would require 4 lines of code (excluding any function declaration) just to use a different content type. And I think that form files mostly come in just one variant - what the existing CreateFormFile produces, but with often different content types. And that's where CreateFormFileWithContentType comes in.

With your proposal, for most file uploads you'd make developers write this:

h := make(textproto.MIMEHeader)
h.Set("Content-Disposition", FileContentDisposition(fieldname, filename)
h.Set("Content-Type", "application/pdf")
writer, error := w.CreatePart(h)
// error check omitted
// now write to writer

instead of this:

writer, error := CreateFormFileWithContentType(fieldname, filename, "application/pdf")

Perhaps FileContentDisposition is a good addition for those developers that need to add more headers like perhaps a content encoding (base64). Personally, I'd add both functions.

rsc commented 3 years ago

This is an unusual enough case that it's OK to have to write 4 lines. The important part is not having to copy-paste the complex Sprintf. Retitled to be about the FileContentDisposition function.

robtimus commented 3 years ago

I have to disagree with you that a content type that is not application/octet-stream is an unusual case. Several services verify the provided content type first as a shortcut; only if it's valid (e.g. an image or PDF) is the actual content inspected.

rsc commented 3 years ago

I only said it was OK to write 4 lines for that case. The part that is bad to copy-paste is the Sprintf call, hence the function.

rsc commented 3 years ago

Based on the discussion above, this proposal seems like a likely accept. — rsc for the proposal review group

neild commented 3 years ago

Copying a possible alternative from #49329:

// NewFormFieldHeader creates a form-data header with the provided field name.
func NewFormFieldHeader(fieldname) textproto.MIMEHeader {
  h := make(textproto.MIMEHeader)
  h.Set("Content-Disposition", fmt.Sprintf(`form-data; name="%s"`, escapeQuotes(fieldname)))
  return h
}

// NewFormFileHeader creates a form-data header with the provided field and file names.
func NewFormFileHeader(fieldname, filename string) textproto.MIMEHeader {
  h := make(textproto.MIMEHeader)
  h.Set("Content-Disposition", fmt.Sprintf(`form-data; name="%s"; filename="%s"`, escapeQuotes(fieldname), escapeQuotes(filename)))
  return h
}

This pushes the header name (in addition to the value) into the multipart package API.

rsc commented 2 years ago

As soon as there's a second field to write a helper for, having helpers that make the map is not going to scale. It seems like we should probably stick with the field-specific helper.

rsc commented 2 years ago

No change in consensus, so accepted. 🎉 This issue now tracks the work of implementing the proposal. — rsc for the proposal review group

gopherbot commented 1 year ago

Change https://go.dev/cl/531995 mentions this issue: mime/multipart: add helpers to build content-disposition header contents