Merge pull request #233 from RomanBurunkov/master

Support empty files

Author: RomanBurunkov

lib/fileFactory.js

@@ -39,7 +39,7 @@ module.exports = (options, fileUploadOptions = {}) => {
   // see: https://github.com/richardgirges/express-fileupload/issues/14
   // firefox uploads empty file in case of cache miss when f5ing page.
   // resulting in unexpected behavior. if there is no file data, the file is invalid.
-  if (!fileUploadOptions.useTempFiles && !options.buffer.length) return;
+  // if (!fileUploadOptions.useTempFiles && !options.buffer.length) return;
 
   // Create and return file object.
   return {

lib/processMultipart.js

@@ -59,6 +59,8 @@ module.exports = (options, req, res, next) => {
       : memHandler(options, field, filename);     // Upload into RAM.
     // Define upload timer.
     const uploadTimer = new UploadTimer(options.uploadTimeout, () => {
+      file.removeAllListeners('data');
+      file.resume();
       // After destroy an error event will be emitted and file clean up will be done.
       file.destroy(new Error(`Upload timeout ${field}->${filename}, bytes:${getFileSize()}`));
     });
@@ -88,9 +90,12 @@ module.exports = (options, req, res, next) => {
       debugLog(options, `Upload finished ${field}->${filename}, bytes:${size}`);
       // Reset upload timer in case of end event.
       uploadTimer.clear();
+      // See https://github.com/richardgirges/express-fileupload/issues/191
       // Do not add file instance to the req.files if original name and size are empty.
       // Empty name and zero size indicates empty file field in the posted form.
-      if (!name && size === 0) return;
+      if (!name && size === 0) {
+        return debugLog(options, `Don't add file instance if original name and size are empty`);
+      }
       req.files = buildFields(req.files, field, fileFactory({
         buffer: complete(),
         name: filename,
@@ -122,6 +127,7 @@ module.exports = (options, req, res, next) => {
   });
 
   busboy.on('finish', () => {
+    debugLog(options, `Busboy finished parsing request.`);
     if (options.parseNested) {
       req.body = processNested(req.body);
       req.files = processNested(req.files);
@@ -139,7 +145,10 @@ module.exports = (options, req, res, next) => {
       });
   });
 
-  busboy.on('error', next);
+  busboy.on('error', (err) => {
+    debugLog(options, `Busboy error`);
+    next(err);
+  });
 
   req.pipe(busboy);
 };

lib/tempFileHandler.js

@@ -18,31 +18,23 @@ module.exports = (options, fieldname, filename) => {
   const hash = crypto.createHash('md5');
   let fileSize = 0;
   let completed = false;
-  
-  let writeStream = false;
-  let writePromise = Promise.resolve();
 
-  const createWriteStream = () => {
-    debugLog(options, `Opening write stream for ${fieldname}->${filename}...`);
-    writeStream = fs.createWriteStream(tempFilePath);
-    writePromise = new Promise((resolve, reject) => {
-      writeStream.on('finish', () => {
-        resolve();
-      });
-      writeStream.on('error', (err) => {
-        debugLog(options, `Error write temp file: ${err}`);
-        reject(err);
-      });
+  debugLog(options, `Opening write stream for ${fieldname}->${filename}...`);
+  const writeStream = fs.createWriteStream(tempFilePath);
+  const writePromise = new Promise((resolve, reject) => {
+    writeStream.on('finish', () => resolve());
+    writeStream.on('error', (err) => {
+      debugLog(options, `Error write temp file: ${err}`);
+      reject(err);
     });
-  };
+  });
 
   return {
     dataHandler: (data) => {
       if (completed === true) {
         debugLog(options, `Error: got ${fieldname}->${filename} data chunk for completed upload!`);
         return;
       }
-      if (writeStream === false) createWriteStream();
       writeStream.write(data);
       hash.update(data);
       fileSize += data.length;
@@ -60,14 +52,12 @@ module.exports = (options, fieldname, filename) => {
     },
     cleanup: () => {
       completed = true;
-      if (writeStream !== false) {
-        debugLog(options, `Cleaning up temporary file ${tempFilePath}...`);
-        writeStream.end();
-        deleteFile(tempFilePath, err => (err 
-          ? debugLog(options, `Cleaning up temporary file ${tempFilePath} failed: ${err}`)
-          : debugLog(options, `Cleaning up temporary file ${tempFilePath} done.`)
-        ));
-      }
+      debugLog(options, `Cleaning up temporary file ${tempFilePath}...`);
+      writeStream.end();
+      deleteFile(tempFilePath, err => (err 
+        ? debugLog(options, `Cleaning up temporary file ${tempFilePath} failed: ${err}`)
+        : debugLog(options, `Cleaning up temporary file ${tempFilePath} done.`)
+      ));
     },
     getWritePromise: () => writePromise
   };

lib/utilities.js

@@ -2,7 +2,7 @@
 
 const fs = require('fs');
 const path = require('path');
-const Readable = require('stream').Readable;
+const { Readable } = require('stream');
 
 // Parameters for safe file name parsing.
 const SAFE_FILE_NAME_REGEX = /[^\w-]/g;
@@ -27,17 +27,17 @@ const debugLog = (options, msg) => {
 };
 
 /**
- * Generates unique temporary file name like: tmp-5000-156788789789.
+ * Generates unique temporary file name. e.g. tmp-5000-156788789789.
  * @param {string} prefix - a prefix for generated unique file name.
  * @returns {string}
  */
-const getTempFilename = (prefix) => {
+const getTempFilename = (prefix = TEMP_PREFIX) => {
   tempCounter = tempCounter >= TEMP_COUNTER_MAX ? 1 : tempCounter + 1;
-  return `${prefix || TEMP_PREFIX}-${tempCounter}-${Date.now()}`;
+  return `${prefix}-${tempCounter}-${Date.now()}`;
 };
 
 /**
- * isFunc- check if argument is a function.
+ * isFunc: Checks if argument is a function.
  * @returns {boolean} - Returns true if argument is a function.
  */
 const isFunc = func => func && func.constructor && func.call && func.apply ? true: false;
@@ -60,7 +60,7 @@ const promiseCallback = (resolve, reject) => {
  * Builds instance options from arguments objects(can't be arrow function).
  * @returns {Object} - result options.
  */
-const buildOptions = function(){
+const buildOptions = function() {
   const result = {};
   [...arguments].forEach(options => {
     if (!options || typeof options !== 'object') return;
@@ -107,17 +107,18 @@ const checkAndMakeDir = (fileUploadOptions, filePath) => {
   // Check whether folder for the file exists.
   if (!filePath) return false;
   const parentPath = path.dirname(filePath);
-  // Create folder if it is not exists.
+  // Create folder if it doesn't exist.
   if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true }); 
   // Checks folder again and return a results.
   return fs.existsSync(parentPath);
 };
 
 /**
- * Delete file.
+ * Deletes a file.
  * @param {string} file - Path to the file to delete.
+ * @param {Function} callback
  */
-const deleteFile = (file, callback) => fs.unlink(file, err => err ? callback(err) : callback());
+const deleteFile = (file, callback) => fs.unlink(file, callback);
 
 /**
  * Copy file via streams
@@ -147,15 +148,15 @@ const copyFile = (src, dst, callback) => {
 };
 
 /**
- * moveFile - moves the file from src to dst.
+ * moveFile: moves the file from src to dst.
  * Firstly trying to rename the file if no luck copying it to dst and then deleteing src.
  * @param {string} src - Path to the source file
  * @param {string} dst - Path to the destination file.
  * @param {Function} callback - A callback function.
  */
-const moveFile = (src, dst, callback) => fs.rename(src, dst, err => (!err
-  ? callback()
-  : copyFile(src, dst, err => err ? callback(err) : deleteFile(src, callback))
+const moveFile = (src, dst, callback) => fs.rename(src, dst, err => (err
+  ? copyFile(src, dst, err => err ? callback(err) : deleteFile(src, callback))
+  : callback()
 ));
 
 /**
@@ -176,7 +177,7 @@ const saveBufferToFile = (buffer, filePath, callback) => {
   };
   // Setup file system writable stream.
   let fstream = fs.createWriteStream(filePath);
-  fstream.on('error', error => callback(error));
+  fstream.on('error', err => callback(err));
   fstream.on('close', () => callback());
   // Copy file via piping streams.
   readStream.pipe(fstream);
@@ -252,18 +253,18 @@ const parseFileName = (opts, fileName) => {
 };
 
 module.exports = {
-  debugLog,
   isFunc,
+  debugLog,
+  copyFile, // For testing purpose.
+  moveFile,
   errorFunc,
-  promiseCallback,
-  buildOptions,
+  deleteFile, // For testing purpose.
   buildFields,
-  checkAndMakeDir,
-  deleteFile,         // For testing purpose.
-  copyFile,           // For testing purpose.
-  moveFile,
-  saveBufferToFile,
+  buildOptions,
   parseFileName,
   getTempFilename,
+  promiseCallback,
+  checkAndMakeDir,
+  saveBufferToFile,
   uriDecodeFileName
 };

test/fileFactory.spec.js

@@ -26,14 +26,6 @@ describe('Test of the fileFactory factory', function() {
   beforeEach(() => server.clearUploadsDir());
 
   it('return a file object', () => assert.ok(fileFactory(mockFileOpts)));
-  it('return void if buffer is empty and useTempFiles is false.', () => {
-    assert.equal(fileFactory({
-      name: mockFileName,
-      buffer: Buffer.concat([])
-    }, {
-      useTempFiles: false
-    }), null);
-  });
 
   describe('Properties', function() {
     it('contains the name property', () => {

test/files/emptyfile.txt

@@ -12,7 +12,7 @@ const uploadDir = server.uploadDir;
 const clearTempDir = server.clearTempDir;
 const clearUploadsDir = server.clearUploadsDir;
 
-const mockFiles = ['car.png', 'tree.png', 'basketball.png'];
+const mockFiles = ['car.png', 'tree.png', 'basketball.png', 'emptyfile.txt'];
 
 const mockUser = {
   firstName: 'Joe',

test/multipartUploads.spec.js

@@ -40,7 +40,7 @@ const setup = (fileUploadOptions) => {
     const testFile = req.files.testFile;
     const fileData = getUploadedFileData(testFile);
 
-    testFile.mv(fileData.uploadPath, function(err) {
+    testFile.mv(fileData.uploadPath, (err) => {
       if (err) {
         console.log('ERR', err); // eslint-disable-line
         return res.status(500).send(err);