Difficulty with handler for file upload

[mikestead]

Running into a similar issue to https://github.com/krakenjs/swaggerize-express/issues/82 with an empty body on form data. This is specific to file upload however.

I'm wanting a handler to upload a file and have the following route specified:

post:
  description: Upload a zip
  operationId: uploadZip
  consumes: multipart/form-data
  parameters:
    - name: zip
      in: formData
      type: file
      required: true
  responses:
    '204':
      description: OK

For file upload it seems that adding a global middleware is not a great option in express (see Upload Per Route), so instead it's recommended to add it to handlers which require it.

The issue I'm having is that validation appears to run before any route middleware so I have no chance to process the stream and update req.body.

The validator also assumes that form data will reside in the req.body property, however this may not be the case, for example with https://github.com/expressjs/multer it's dropped in a req.files property. I guess if I could add middleware to the route ahead of validation I could move these into the body.

Edit: Pull request https://github.com/krakenjs/swaggerize-express/pull/90 moves middleware before validation.

[kenjones-cisco]

@mikestead For my use case where I have mutter, I only accept a single file so the PR I just create might not solve the same for you.

I'm assuming there would be another branch in the if statement where it is req.files but since that is an array would it be safe to just check req.files[0].fieldname?

Any input would be great so hopefully we have one update the resolves the issue for all of us dealing with file upload issues.

As you will see referenced in the PR, there is an issue within swaggerize-routes as well that performs the actual validation that was making things hard as well. I submitted a PR for that issue as well.

[mikestead]

@kenjones-cisco I think the best (only?) way to solve this for all scenarios is to allow route middleware to run before validation. That then allows us to process any request and format it inline with the requirements of this lib (i.e. populate body correctly). I'm not sure if this lib should know/care about how third partly libs like multer custom format the request.

Moving middleware before validation may be a breaking change, and there could well be something I'm not aware of that prevents this. Given global middleware can run before validation it would make sense that the same should be said for a route?

[kenjones-cisco]

Should body even need to be updated for file uploads? As this is an express framework, and pre Express 4, there was req.files, and their documentation notes that if you still want to access req.files then leverage one of the listed middlewares.

In Express 4, req.files is no longer available on the req object by default. 
To access uploaded files on the req.files object, use a multipart-handling middleware like 
busboy, multer, formidable, multiparty, connect-multiparty, or pez.

Source: http://expressjs.com/en/4x/api.html#req

As such an express framework that expects all parsing to be done prior to the validation and routing, should be expected to handle file upload. (In my opinion)

[mikestead]

Fair point. If those libs format req.files in a standard way then it would make sense. Looks like your pull request references req.file though. Is that standard?

If there is a standard then putting middleware before validation, AND adding handling for files like your pull request does, would mean we could use libs like multer as it's recommended. e.g.

var multer  = require('multer')
var upload = multer({ dest: 'uploads/' })

module.exports = {
    post: [
        upload.single('zip'),
        function handler(req, res) {
            console.log(req.file)
        }
    ]
}

[kenjones-cisco]

multer which is by the expressjs team uses req.file if you say .single or req.files if you use .array As such, after looking through each of the list middleware on expressjs site it seemed to be a mix but since my preference and use case uses .single I was looking for feedback from those using multiple files.

Should vauleAccessor just look at files[0] or what should be the proper response?

[mikestead]

Looking at the swagger spec it looks like you can't have an array of files as a parameter so single file support may well be enough? I guess the fallback you mentioned would be fine.

var file = req.file || Array.isArray(req.files) && req.files[0]

[mikestead]

Oops I guess you can have multiple params that are each a file. In that case you would have an array of files and you'd need to scan them to find the one that corresponds to the key.

[tlivings]

Why can't global middleware be used?

Similar to body-parser for json(), global middleware for multipart makes sense.

[kenjones-cisco]

Global middleware is being used, but the assumption in the validation here will only validate against body, but uploaded files in express are stored in files.

As such the parsing is done as expected by the middleware prior to swaggerize-express, but then a 400 bad request is thrown because it did not find the files within the body.

[tlivings]

Ah I see - yes, this needs a fix!

[mikestead]

See third paragraph from bug report. Here's the quote from the link there.

"If you chain multiple middleware together, you will find out, that express uses streams, but this does not fully work with middleware. Events are not piped between middleware. If you use two express middlewares in a chain and each tries to catch all stream events and calls next() when all the eventing is done, the second middleware will not receive the required events anymore.

In such a case you need to optimize the middleware per route. Therefore I do not recommend to use multer as a global middleware for express. Instead I would add multer to the routes that specifically require it."

This would seem to be backed up by the official examples I've seen of multer, they're all on individual routes.

[tlivings]

Yeah but that's specific to multer right?

[mikestead]

It sounds like it could arise with any middleware which tries to catch all stream events. Not sure how common that is. As mentioned, multer is from the express team so likely the most commonly used for this task.

I guess I'm missing the reason validation happens before route middleware. Whether I place my middleware globally or against a route, the route should behave the same. Validation should happen directly before the handler is executed to remove any chance a middleware has invalidated the request. Currently route middleware could modify a request and the handler would still be fed it thinking it's valid. Add this same middleware globally and it wouldn't. Seems inconsistent.

[tlivings]

Validation happens after middle-aged you define before swagger routes.

The middleware defined as array items in a route definition are custom middleware for that specific route.

It's probably not a bad thing to move them before, but there are caveats this way too - the params will not have been validated and as a result could be detrimental, not to mention a breaking change for existing apps that may be acting on parameters they've assumed to be valid.

As for the stream events, am not sure why this would affect anything. Is there a known reproduce able issue?

[YSunLIN]

So how to upload files properly now? Could you give us some source codes as examples? I haven't upload a file successfully yet.

[kenjones-cisco]

@YSunLIN

This is what I am using:

api.yml

  /images/{id}/upload:
    x-handler: handlers/imagesUpload.js
    post:
      summary: Upload image
      tags:
        - Image
      description: |
        Upload `Image` content to the server.
      consumes:
        - 'multipart/form-data'
      parameters:
        - name: id
          in: path
          description: Call the object with this id.
          required: true
          type: string
          format: uuid
        - name: image
          in: formData
          description: The `Image` content
          required: true
          type: file
      responses:
        201:
          description: Image uploaded
        400:
          description: Bad request
        401:
          description: Unauthorized
        403:
          description: Forbidden
        404:
          description: Not found

server.js

'use strict';

var path = require('path');
var express = require('express');
var bodyParser = require('body-parser');
var swaggerize = require('swaggerize-express');
var multer = require('multer');

    var app = express();

    app.use(multer({
        // store in memory while transferring to db
        storage: multer.memoryStorage(),
        limits: {
            // .5 Mb
            fileSize: 500000
        }
    }).single('image'));

    app.use(bodyParser.json());

    app.use(swaggerize({
        api: path.resolve('./config/api.yml'),
        handlers: path.resolve('./handlers')
    }));

    app.listen(3000, function () {
            console.log('Listening on 3000');
    });

handlers/imagesUpload.js

'use strict';
var logger = require('../config/logger');
var Image = require('../models').Image;

/**
 * Operations on /images/{id}/upload
 */
module.exports = {

    /**
     * Create a new `Image` object on the server.
     *
     * parameters: image
     */
    post: function (req, res, next) {
        var image = {
            content: req.file.buffer
        };

        logger.debug('image id: ', req.params.id);
        Image.findByIdAndUpdate(req.params.id, image).exec().then(function (item) {
            if (!item) {
                res.sendStatus(404);
                return next();
            }
            res.sendStatus(201);
            return next();
        }).catch(next);

    }

};

[YSunLIN]

thanks a lot, i got it. I was confused for a while. Now i know it will pass the validation while using the middleware 'multer'. And i wanna know what other multipart-handling middleware it supports.

[tlivings]

Thanks for that example @kenjones-cisco!