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

File upload with multipart/form-data #1873

Open
diegolacarta opened this Issue Oct 18, 2018 · 17 comments

Comments

Projects
None yet
7 participants
@diegolacarta

diegolacarta commented Oct 18, 2018

Is there any way to handle an upload with multipart/form-data? I'm trying to migrate a simple piece of code that works in express to loopback, and can't find any docs for v4.

The issue is it never gets to the controller, and returns an error saying multipart/form-data is not supported.

This is the code in expressjs:

  app.post('/upload', (request, response) => {
    const upload = multer().single('file')
    upload(request, response, () => {
      const {file} = request
      ...
    })
  })

Acceptance criteria

  • A set of extension points in @loopback/rest allowing applications & extensions to provide custom request body parsers. See #1936
  • An official extension (a new npm package developed inside loopback-next monorepo) providing a reference implementation of file-upload (multi-part) body parser.
  • An example application (in loopback-next's examples directory) showing file uploads in practice, using the official extension on the server and providing a simple HTML-based UI on the client side.
  • (Should have:) The OpenAPI spec generated for file-upload endpoints should be recognized by swagger-ui so that our REST API Explorer provides file-upload controls too. (Assuming swagger-ui does support multi-part requests and file uploads.)

@bajtos bajtos added the feature label Oct 18, 2018

@bajtos

This comment has been minimized.

Member

bajtos commented Oct 18, 2018

Hi @diegolacarta, thank you for starting this discussion! Incidentally, multipart/form-data was mentioned very recently in #1838. I guess it's time to start looking into ways how to add support for file uploads.

@raymondfeng @marioestradarosa do you have any opinions on the implementation?

Few questions that comes to my mind:

  • Which npm module should we use to handle processing of multipart/form-data for us?
  • Can we offer a stream-based API to controller methods/route handler so that we don't have to store the uploaded files anywhere? (We definitely don't want to cache them in memory!)
  • If not, then where are we going to store uploaded files? I remember there were security issues around storing the uploaded files in a temp folder, we must be careful here.
@diegolacarta

This comment has been minimized.

diegolacarta commented Oct 18, 2018

Thanks for the answer @bajtos , is there any way to plug that code into loopback in the mean time?

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 19, 2018

I recommend to use https://github.com/expressjs/multer.

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Oct 19, 2018

We probably should implement a custom storage engine based on the controller method, allowing injections of fields/files to the method arguments. For example,

export class FileUploadController {
  upload(@file({name: 'my-file}) file: File) {
    // ...
  }
}

export class FileUploadController {
  upload(
    @file.handle() handle: (file: File) => Promise<void>,  // handle a file
    @file.remove() remove: (file: File) => Promise<void> // clean up a file
) {
    // ...
  }
}
@dhmlau

This comment has been minimized.

Contributor

dhmlau commented Oct 19, 2018

@strongloop/sq-lb-apex @hacksparrow @bajtos , are we good with proposal from @raymondfeng above?
Moving it to Needs Discussion until we have acceptance criteria.

@marioestradarosa

This comment has been minimized.

Contributor

marioestradarosa commented Oct 19, 2018

I believe multer is the best option since it covers both scenarios (with and without a file upload).

@hacksparrow

This comment has been minimized.

Member

hacksparrow commented Oct 19, 2018

+1 for multer. Very customizable. Its storage engine concept will help with easy cloud integration too, if and when required.

@bajtos bajtos referenced this issue Oct 25, 2018

Open

[EPIC] REST layer improvements (post-GA) #1452

2 of 6 tasks complete

@goodexpert goodexpert referenced this issue Oct 25, 2018

Closed

Support multipart form data #1880

3 of 7 tasks complete
@bajtos

This comment has been minimized.

Member

bajtos commented Oct 25, 2018

For better or worse, the pull request #1880 is showing how to implement support for file uploads using multer.

As I see it, adding support for file uploads is a big feature that will live with us for many months or years and will be difficult to significantly change. I'd like us to carefully design this figure in a way that will be robust, secure and extensible to support future needs.

A question for all of us to consider: should we implement file-upload as a built-in feature in @loopback/rest, or make it an extension that's 1. versioned independently from @loopback/rest and 2. can have different implementations, each optimizing for different needs and tradeoffs?

Having file-uploads outside of @loopback/rest will greatly benefit people NOT using this feature. The size of their dependencies will be smaller and as an important consequence, their applications will have less sources of potential security vulnerabilities.

Another concern to consider: multipart content can mix (binary) files with structured (JSON) data. For example, an HTML form can provide input fields "description" and "category" in addition to file inputs. The design of our file-upload feature needs to support this scenario too. Ideally, we should support arbitrarily structured multipart requests. See Special Considerations for multipart Content.

export class FileUploadController {
  upload(
    @file.handle() handle: (file: File) => Promise<void>,  // handle a file
    @file.remove() remove: (file: File) => Promise<void> // clean up a file
) {
    // ...
  }
}

Let's not use multiple method arguments for a single endpoint parameter ("files") please, it is difficult to extend in the future. I'd prefer to group handle and remove into an interface, making it easy to add additional methods and/or metadata in the future.

export class FileUploadController {
  upload(
    @multipart() multipart: MultipartRequestBody,
  ) {
    multipart.handle();
    multipart.remove();
    // ...
  }
}
@bajtos

This comment has been minimized.

Member

bajtos commented Oct 25, 2018

Ping @strongloop/loopback-next. In this GitHub issue, we are discussing how to best implement support for receiving file uploads in LB4 applications. Please let us know if this is something you are interested in. If you are just interested in seeing the feature implemented, then upvote this issue by pressing the thumb-up button just below the issue description. If you can help us with relevant information or experience that can make our implementation better, then please leave a comment.

@bajtos bajtos changed the title from Upload with multipart/form-data to File upload with multipart/form-data Oct 26, 2018

@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 1, 2018

As a developer that needs to create a REST API to accept file uploads, I would like to implement the following minimum logic:

  1. Accept one or more file streams and provide code to process them (handle/remove).
  2. Access one or more fields (get values, such as {f1: 'text', f2: ['val1', 'val2']})
  3. Expose an endpoint that allows multipart/form-data media type with http post

Please note a multipart/form-data request body should be ideally parsed as a series of events representing the different parts - field or file. So 'push mode works better.

A possible style: Use @upload decorator to configure multer parser and @upload.files/fields to access files/fields.

export class FileUploadController {

  @post('/uploads')
  @upload({
    limits: {...},
    fileFilter: ...,
    storage: ...,
  })
  upload(
    @upload.files() files: File[], @upload.fields([
      { name: 'avatar', maxCount: 1 },
      { name: 'gallery', maxCount: 8 }
    ]) fields: Field[]) {
      // return a response;
    }
  )
}
@raymondfeng

This comment has been minimized.

Member

raymondfeng commented Nov 5, 2018

What about even simpler solution - 5c2051d?

Basically, we allow controller methods to skip body parsing by configuring x-skip-body-parsing and give the implementation the full control to handle multipart/form-data.

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 6, 2018

What about even simpler solution - 5c2051d?

Basically, we allow controller methods to skip body parsing by configuring x-skip-body-parsing and give the implementation the full control to handle multipart/form-data.

I like this idea as a short-term workaround allowing LB4 app developers to build their own solutions for parsing multipart/form-data requests, buying us more time to implement the official support in a robust way.

I am proposing the following approach:

  • Convert 5c2051d into a pull request. Add documentation (a HOWTO guide) explaining LB4 app developers how to handle file uploads.
  • Keep this issue open to discuss how to provide official support for file uploads that works OOTB, either as a built-in feature of @loopback/rest or as an extension.

@goodexpert goodexpert referenced this issue Nov 8, 2018

Closed

Support multipart form data #1989

4 of 4 tasks complete
@bajtos

This comment has been minimized.

Member

bajtos commented Nov 8, 2018

It would be great if @loopback/rest provided an extension point allowing 3rd party extensions to provide parsers for different content type. With that extension point in place, we can have multiple extensions implementing file upload functionality using different programming models e.g. push vs pull, files as streams vs. files saved in a temp directory, etc.

@raymondfeng WDYT?

@thanhdevapp

This comment has been minimized.

thanhdevapp commented Nov 10, 2018

@bajtos anyway, can you have an example of controller upload file?

@bajtos

This comment has been minimized.

Member

bajtos commented Nov 22, 2018

@thanhdevapp The pull request #1936 is adding features that will allow applications & extensions to implement custom request parsing (e.g. using multer), see the following section for information about implementing a file-upload controller: Parsing-requests.md#specify-custom-parser-by-controller-methods. Please not this pull request was not landed yet, these new APIs are not available yet.

@bajtos

This comment has been minimized.

Member

bajtos commented Dec 10, 2018

FYI, the extensible body parsing is available, you can learn more in Raymond's blog post here: https://strongloop.com/strongblog/the-journey-to-extensible-request-body-parsing/

While working on #1936, we were discussing built-in support for file uploads too. Our conclusion was that there are different styles of consuming uploaded files and it would be too difficult to build a single component to support them all. For example, some applications may want to save the uploaded files into a temporary directory for further processing. Other applications may want to stream the uploaded files directly to a backed storage (e.g Amazon S3 or IBM Cloud Object Storage).

@raymondfeng in the light of that decision, which of the following acceptance criteria are still relevant and which should be removed?

  1. An official extension (a new npm package developed inside loopback-next monorepo) providing a reference implementation of file-upload (multi-part) body parser.
  2. An example application (in loopback-next's examples directory) showing file uploads in practice, using the official extension on the server and providing a simple HTML-based UI on the client side.
  3. (Should have:) The OpenAPI spec generated for file-upload endpoints should be recognized by swagger-ui so that our REST API Explorer provides file-upload controls too. (Assuming swagger-ui does support multi-part requests and file uploads.)

Personally, I think we should have an example application (LB4 server + HTML5 front-end) to show file uploads in practice and to validate our proposed solution in a real-world setup (end-to-end, from the browser).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment