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

File upload with multipart/form-data #1873

Closed
1 of 4 tasks
diegolacarta opened this issue Oct 18, 2018 · 26 comments · Fixed by #4882
Closed
1 of 4 tasks

File upload with multipart/form-data #1873

diegolacarta opened this issue Oct 18, 2018 · 26 comments · Fixed by #4882
Labels
feature parity feature help wanted REST Issues related to @loopback/rest package and REST transport in general

Comments

@diegolacarta
Copy link

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 feat(rest): switch to body parser and add extensibility for parsers #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.) See Upload binary files via multipart/form-data from REST API Explorer #2666
@bajtos bajtos added the feature label Oct 18, 2018
@bajtos
Copy link
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.

@bajtos bajtos added help wanted REST Issues related to @loopback/rest package and REST transport in general labels Oct 18, 2018
@diegolacarta
Copy link
Author

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

@raymondfeng
Copy link
Contributor

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

@raymondfeng
Copy link
Contributor

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

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

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

@hacksparrow
Copy link
Contributor

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

@bajtos
Copy link
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
Copy link
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 Upload with multipart/form-data File upload with multipart/form-data Oct 26, 2018
@raymondfeng
Copy link
Contributor

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

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

@bajtos
Copy link
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
Copy link

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

@bajtos
Copy link
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
Copy link
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).

@straeger
Copy link

+1 for a LB4 server + HTML5 front-end Example Repo
The Link https://github.com/strongloop/loopback-next/tree/master/packages/rest/test/acceptance/file-upload doesn't work anymore.

@dhmlau
Copy link
Member

dhmlau commented Feb 11, 2020

There is documentation showing how to do file upload: https://loopback.io/doc/en/lb4/Parsing-requests.html#specify-custom-parser-by-controller-methods.

What we might be able to do is to:

@jannyHou
Copy link
Contributor

jannyHou commented Feb 27, 2020

Updated Acceptance Criteria:

@dhmlau
Copy link
Member

dhmlau commented Mar 6, 2020

@raymondfeng created a PR #4801 to add an example.

@jannyHou
Copy link
Contributor

some update: PR #4801 merged. Next we need update the docs mentioned in #1873 (comment)

@raymondfeng
Copy link
Contributor

One more PR for further improvement - #4882

@VishalSingh28
Copy link

How can we upload file (Single and Multiple)
in MySQL database with id and forigen key

@raymondfeng
Copy link
Contributor

raymondfeng commented Jun 18, 2021

@VishalSingh28 You will have to follow https://github.com/expressjs/multer/blob/master/StorageEngine.md to create a multer storage engine for MySQL and configure the file uploader to use it. See an example at https://github.com/strongloop/loopback-next/blob/master/examples/file-transfer/src/application.ts#L59.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature parity feature help wanted REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

Successfully merging a pull request may close this issue.