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 FileContentDisposition #46771

Open
robtimus opened this issue Jun 15, 2021 · 11 comments
Open

mime/multipart: add FileContentDisposition #46771

robtimus opened this issue Jun 15, 2021 · 11 comments

Comments

@robtimus
Copy link

@robtimus robtimus commented Jun 15, 2021

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: #29140. It's fully backward compatible because it introduces a new function; the existing function delegates to the new one.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 15, 2021

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)

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Jun 15, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

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 rsc moved this from Incoming to Active in Proposals Oct 27, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

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
Copy link
Author

@robtimus robtimus commented Oct 27, 2021

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 rsc changed the title proposal: mime/multipart: allow custom content types to be given for form files proposal: mime/multipart: add FileContentDisposition Nov 3, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Nov 3, 2021

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
Copy link
Author

@robtimus robtimus commented Nov 3, 2021

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
Copy link
Contributor

@rsc rsc commented Nov 10, 2021

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
Copy link
Contributor

@rsc rsc commented Nov 10, 2021

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

@neild
Copy link
Contributor

@neild neild commented Nov 11, 2021

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
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

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 rsc moved this from Likely Accept to Accepted in Proposals Dec 1, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Dec 1, 2021

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

@rsc rsc changed the title proposal: mime/multipart: add FileContentDisposition mime/multipart: add FileContentDisposition Dec 1, 2021
@rsc rsc removed this from the Proposal milestone Dec 1, 2021
@rsc rsc added this to the Backlog milestone Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Development

No branches or pull requests

5 participants