keystonejs / keystone-classic

Node.js CMS and web app framework
http://v4.keystonejs.com
MIT License
14.64k stars 2.21k forks source link

add enctype='multipart/form-data' to all forms #309

Closed fscz closed 10 years ago

fscz commented 10 years ago

The plan behind this is to use multiparty as the new form parser, since bodyparser will be removed from express. The above changes would allow to write a form parser, that could handle all signin/singout, forms with and without files.

////////////////////////////////////////////////////////////

app.use ( function( req, res, next) {
  if (req.method == 'GET') {
  req.body = {};
  next();
} else {
  var form = new multiparty.Form({
    autoFields: true,
    autoFiles: true,
    hash: 'sha256',
    uploadDir: file_store.getTmpDir()
  });

  form.parse(req, function (err, fields, files) {
    if (err) {
      next(err);
    } else {
      req.body = fields || {};
      for (var field in req.body) {
        if (req.body[field].length == 1) req.body[field] = req.body[field][0];
      }
      req.files = files || {};
      next();
    }
  });
}
});

////////////////////////////////////////////////////////////

JedWatson commented 10 years ago

This has been on my mind for a while, thanks for picking it up, @fscz.

When we upgrade to the new version of Express we should be sure to bump to a new minor version of Keystone so that everyone can deal with any breaking changes to the express API without being auto-upgraded by the default settings in the package.json that the yeoman generator provides.

There are apps in production (that I know of) that rely on the current req.files API provided by express; from the look of your example above they should continue to work as before, is that correct?

fscz commented 10 years ago

I've checked this. bodyParser uses formidable under the hood and produces file objects in req.files of the following structure:


{ size: 74643, path: '/tmp/8ef9c52abe857867fd0a4e9a819d1876', name: 'edge.png', type: 'image/png', hash: false, lastModifiedDate: Thu Aug 09 2012 20:07:51 GMT-0700 (PDT), _writeStream: { path: '/tmp/8ef9c52abe857867fd0a4e9a819d1876', fd: 13, writable: false, flags: 'w', encoding: 'binary', mode: 438, bytesWritten: 74643, busy: false, _queue: [], _open: [Function], drainable: true }, length: [Getter], filename: [Getter], mime: [Getter] }


multiparty's structure is a bit different: { "fieldName":"e5af46414ed484ebccad390d3e002e340b84524598cb7e704b05746587924a22","originalFilename":"e5af46414ed484ebccad390d3e002e340b84524598cb7e704b05746587924a22","path":"/var/folders/sj/mptbdmx97855jqn08z7kb8900000gn/T/9260-1ww70om", "headers":{ "content-disposition":"form-data;name=\"e5af46414ed484ebccad390d3e002e340b84524598cb7e704b05746587924a22\"; filename=\"e5af46414ed484ebccad390d3e002e340b84524598cb7e704b05746587924a22\"", "content-type":"application/octet-stream" }, "ws":{"_writableState":{"highWaterMark":16384,"objectMode":false,"needDrain":true,"ending":true,"ended":true,"finished":true,"decodeStrings":true,"defaultEncoding":"utf8","length":0,"writing":false,"sync":false,"bufferProcessing":false,"writecb":null,"writelen":0,"buffer":[]},"writable":true,"domain":null,"_events":{"error":[null],"close":[null]},"_maxListeners":10,"path":"/var/folders/sj/mptbdmx97855jqn08z7kb8900000gn/T/9260-1ww70om","fd":null,"flags":"w","mode":438,"bytesWritten":192829,"closed":true},"hash":"e5af46414ed484ebccad390d3e002e340b84524598cb7e704b05746587924a22","size":192829}

if the exact structure as defined by bodyParser is crucial, Keystone should probably use formidable instead of multiparty. multiparty however offers nice features, like automatically writing files to disc and supports computation of more different hashes (f.e. sha256). formidable only supports [sha1, md5].

I see 2 options:

  1. Use formidable under the hood and provide some options to offer similar features as multiparty
  2. Use multiparty and add some options to the file objects structure to make it as similar as possible

I recommend using the 2. approach, as the important part of the file is really just the 'path' attribute and the code the people use write now: /////////// fs.readFile(req.files.displayImage.path, function (err, data) { // ... var newPath = __dirname + "/uploads/uploadedFileName"; fs.writeFile(newPath, data, function (err) { res.redirect("back"); }); }); ////////////

works also with multiparty .

JedWatson commented 10 years ago

It looks like multer is a drop-in replacement using busboy, which would resolve this more easily and facilitate the move to Express 4 with minimal breaking changes, so I'm closing this issue in favor of #549