diff --git a/lib/fileFactory.js b/lib/fileFactory.js index a84924f..02b82df 100644 --- a/lib/fileFactory.js +++ b/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 { diff --git a/lib/processMultipart.js b/lib/processMultipart.js index e8a815b..1561c0b 100644 --- a/lib/processMultipart.js +++ b/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); }; diff --git a/lib/tempFileHandler.js b/lib/tempFileHandler.js index 2eea3b8..bfaca8b 100644 --- a/lib/tempFileHandler.js +++ b/lib/tempFileHandler.js @@ -18,23 +18,16 @@ 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) => { @@ -42,7 +35,6 @@ module.exports = (options, fieldname, filename) => { 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 }; diff --git a/lib/utilities.js b/lib/utilities.js index ced0863..9f12820 100644 --- a/lib/utilities.js +++ b/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 }; diff --git a/test/fileFactory.spec.js b/test/fileFactory.spec.js index 5e357ee..350158e 100644 --- a/test/fileFactory.spec.js +++ b/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', () => { diff --git a/test/files/emptyfile.txt b/test/files/emptyfile.txt new file mode 100644 index 0000000..e69de29 diff --git a/test/multipartUploads.spec.js b/test/multipartUploads.spec.js index e2a0806..4cefd60 100644 --- a/test/multipartUploads.spec.js +++ b/test/multipartUploads.spec.js @@ -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', diff --git a/test/server.js b/test/server.js index c384dfa..ae399e0 100644 --- a/test/server.js +++ b/test/server.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);