Text parameters go missing depending on uploaded file's size?

[VirtualWolf]

I've written a little custom media upload tool for the Twitter client Tweetbot (see their specification here for the details of what Tweetbot sends) that takes a file attached to a tweet, saves it to disk and returns a URL for it, and saves the tweet text to Postgres. The saving and return of the URL for the filename works fine, but I've discovered that the text parameters (the source and message fields from the link above) are going missing when the file uploaded is over a certain size (and this size seems to vary somewhat, too).

I'm able to replicate it easily and have chucked it into a repository. I've also added a couple of tests demonstrating the behaviour, using the same form order that Tweetbot uses.

If DEBUG=skipper is turned on, I see this for the working, <100KB file:

  skipper Set up "maxTimeToWaitForFirstFile" timer for 10000ms +6m
Acquiring new Upstream for field `media`
  skipper passed control to app because first file was received +1ms
  skipper waiting for any text params +0ms
Upstream: Pumping incoming file through field `media`
Parser: Read a chunk of textparam through field `message`
Parser: Read a chunk of textparam through field `source`
  skipper Something is trying to read from Upstream `media`... +2ms
Parser: Done reading textparam through field `message`
Parser: Done reading textparam through field `source`
Form: emitted `close`
  skipper multiparty form closed. +0ms
  skipper Upstream: No more files will be sent through field `media`- clearing timeouts... +0ms
  skipper Passing control to app... +4ms
  skipper .upload() called on upstream +2ms
RenamerPump:
• dirname => undefined
• field => media
• fd => 59adce99-2c8f-4d3a-85fe-e2edf0a41cb5.png
A receiver is finished writing files from Upstream `media`.
(this doesn't necessarily mean any files were actually written...)

Note the Read a chunk of textparam and Done reading text param lines.

On the >300KB file, neither of those lines appear:

  skipper Set up "maxTimeToWaitForFirstFile" timer for 10000ms +0ms
Acquiring new Upstream for field `media`
  skipper passed control to app because first file was received +0ms
  skipper waiting for any text params +1ms
Upstream: Pumping incoming file through field `media`
  skipper Something is trying to read from Upstream `media`... +2ms
  skipper Passing control to app... +4ms
  skipper .upload() called on upstream +8ms
RenamerPump:
• dirname => undefined
• field => media
• fd => b9962840-64bc-4877-b1ac-d984064a91d3.png
Form: emitted `close`
  skipper multiparty form closed. +10ms
  skipper Upstream: No more files will be sent through field `media`- clearing timeouts... +0ms
A receiver is finished writing files from Upstream `media`.
(this doesn't necessarily mean any files were actually written...)

And the console.log statement shows "undefined" for message.

I realise that Skipper's docs say that the file should be last on any given form, but is what I'm seeing expected behaviour? Why does it differ depending on uploaded filesize?

The only other references I can find to this issue are this Stack Overflow question which has no answer, and #60 right here.

Sails.js is at 0.12.11, and Skipper appears to be the 0.6.5 version that comes with it. I upgraded Sails' Skipper dependency to 0.7.3 but it didn't make a difference. Node.js itself is 6.9.1 under macOS 10.12.1.

Any help or insight would be much appreciated!

[sailsbot]

@VirtualWolf Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

[VirtualWolf]

Ok, so I see the same behaviour happens even just by editing skipper's own test/req.body.test.js and replacing the two references to suite.srcFiles with suite.bigSrcFiles and moving form.append('avatar'... to the top. If the file is moved to the top of the fields but the source files are left as the small ones, it works as expected.

So I guess the next question is, what's the best solution here for when it's not possible to have the form order changed? I assume I'm not the only one who's run into this sort of problem. :)

[VirtualWolf]

So after many hours of mostly fruitless Googling, I found out how to change the body parser that Sails uses just for a specific route.

I'm not familiar with Express in general, so all of the "Just add it as middleware in config/http.js" meant very little to me, and I couldn't find any concrete examples on how it all worked. Then I stumbled on the use-xml-body-parser example repo. That should really be linked in the documentation somewhere because it was a god-send!

My solution was to uncomment the middleware object in config/http.js and replaced the bodyParser: require('skipper') with this:

bodyParser: (function() {
    var skipper = require('skipper')();
    var busboy = require('connect-busboy')();

    return function(req, res, next) {
        if (req.path === '/i' && req.method === 'POST') {
            return busboy(req, res, next);
        }
        return skipper(req, res, next);
    };
})()

And so then my controller looked like this:

uploadMedia: (req, res) => {
    let context;
    let generatedFilename;

    req.pipe(req.busboy);

    req.busboy.on('field', function(fieldName, fieldValue) {
        if (fieldName === 'message') {
            context = fieldValue;
        }
    });

    req.busboy.on('file', function(fieldname, file, filename) {
        generatedFilename = uuid.v4() + path.extname(filename);
        const fileLocation = path.join((process.env.MEDIA_ASSETS_DIR || '.tmp/uploads'), generatedFilename);
        file.pipe(fs.createWriteStream(fileLocation));
    });

    req.busboy.on('finish', function() {
        MediaContext.create({
            filename: generatedFilename,
            context,
        }).then(result => {
            return res.json({ url: `https://${process.env.DOMAIN}/i/${generatedFilename}` });
        }).catch(err => {
            return res.serverError(err);
        });
    });
},

Then everything works as expected! And skipper is still used for everything else except this one specific function.