From 48b9f72397c8a82b4795283c03c12a7e0ce36a53 Mon Sep 17 00:00:00 2001 From: Carsten Klein Date: Sat, 6 Oct 2018 17:49:34 +0200 Subject: [PATCH] fix gh-176: fail early if there is no tmp dir specified, add rimraf dependency that somehow got lost --- lib/tmp.js | 99 +++++++++++++++++++++++++++++------------- package.json | 3 +- test/dir-test.js | 1 - test/name-sync-test.js | 68 ++++++++++++++++++++++++++++- test/name-test.js | 76 +++++++++++++++++++++++++++++++- 5 files changed, 211 insertions(+), 36 deletions(-) diff --git a/lib/tmp.js b/lib/tmp.js index 33d59bd..4e38f6c 100644 --- a/lib/tmp.js +++ b/lib/tmp.js @@ -22,12 +22,6 @@ const rimraf = require('rimraf'); * The working inner variables. */ const - /** - * The temporary directory. - * @type {string} - */ - tmpDir = os.tmpdir(), - // the random characters to choose from RANDOM_CHARS = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz', @@ -123,12 +117,21 @@ function _parseArguments(options, callback) { * @private */ function _generateTmpName(opts) { + + const tmpDir = _getTmpDir(); + + // fail early on missing tmp dir + if (isBlank(opts.dir) && isBlank(tmpDir)) { + throw new Error('No tmp dir specified'); + } + /* istanbul ignore else */ - if (opts.name) { + if (!isBlank(opts.name)) { return path.join(opts.dir || tmpDir, opts.name); } // mkstemps like template + // opts.template has already been guarded in tmpName() below /* istanbul ignore else */ if (opts.template) { var template = opts.template; @@ -141,10 +144,10 @@ function _generateTmpName(opts) { // prefix and postfix const name = [ - opts.prefix || 'tmp-', + (isBlank(opts.prefix) ? 'tmp-' : opts.prefix), process.pid, _randomChars(12), - opts.postfix || '' + (isBlank(opts.postfix) ? '' : opts.postfix) ].join(''); return path.join(opts.dir || tmpDir, name); @@ -161,7 +164,7 @@ function tmpName(options, callback) { args = _parseArguments(options, callback), opts = args[0], cb = args[1], - tries = opts.name ? 1 : opts.tries || DEFAULT_TRIES; + tries = !isBlank(opts.name) ? 1 : opts.tries || DEFAULT_TRIES; /* istanbul ignore else */ if (isNaN(tries) || tries < 0) @@ -172,20 +175,24 @@ function tmpName(options, callback) { return cb(new Error('Invalid template provided')); (function _getUniqueName() { - const name = _generateTmpName(opts); + try { + const name = _generateTmpName(opts); - // check whether the path exists then retry if needed - fs.stat(name, function (err) { - /* istanbul ignore else */ - if (!err) { + // check whether the path exists then retry if needed + fs.stat(name, function (err) { /* istanbul ignore else */ - if (tries-- > 0) return _getUniqueName(); + if (!err) { + /* istanbul ignore else */ + if (tries-- > 0) return _getUniqueName(); - return cb(new Error('Could not get a unique tmp filename, max tries reached ' + name)); - } + return cb(new Error('Could not get a unique tmp filename, max tries reached ' + name)); + } - cb(null, name); - }); + cb(null, name); + }); + } catch (err) { + cb(err); + } }()); } @@ -200,7 +207,7 @@ function tmpNameSync(options) { var args = _parseArguments(options), opts = args[0], - tries = opts.name ? 1 : opts.tries || DEFAULT_TRIES; + tries = !isBlank(opts.name) ? 1 : opts.tries || DEFAULT_TRIES; /* istanbul ignore else */ if (isNaN(tries) || tries < 0) @@ -234,7 +241,7 @@ function file(options, callback) { opts = args[0], cb = args[1]; - opts.postfix = (_isUndefined(opts.postfix)) ? '.tmp' : opts.postfix; + opts.postfix = isBlank(opts.postfix) ? '.tmp' : opts.postfix; // gets a temporary filename tmpName(opts, function _tmpNameCreated(err, name) { @@ -287,7 +294,7 @@ function fileSync(options) { args = _parseArguments(options), opts = args[0]; - opts.postfix = (_isUndefined(opts.postfix)) ? '.tmp' : opts.postfix; + opts.postfix = isBlank(opts.postfix) ? '.tmp' : opts.postfix; const discardOrDetachDescriptor = opts.discardDescriptor || opts.detachDescriptor; const name = tmpNameSync(opts); @@ -536,25 +543,36 @@ function isENOENT(error) { * which will differ between the supported node versions. * * - Node >= 7.0: - * error.code {String} - * error.errno {String|Number} any numerical value will be negated + * error.code {string} + * error.errno {string|number} any numerical value will be negated * * - Node >= 6.0 < 7.0: - * error.code {String} - * error.errno {Number} negated + * error.code {string} + * error.errno {number} negated * * - Node >= 4.0 < 6.0: introduces SystemError - * error.code {String} - * error.errno {Number} negated + * error.code {string} + * error.errno {number} negated * * - Node >= 0.10 < 4.0: - * error.code {Number} negated + * error.code {number} negated * error.errno n/a */ function isExpectedError(error, code, errno) { return error.code === code || error.code === errno; } +/** + * Helper which determines whether a string s is blank, that is undefined, or empty or null. + * + * @private + * @param {string} s + * @returns {Boolean} true whether the string s is blank, false otherwise + */ +function isBlank(s) { + return s === null || s === undefined || !s.trim(); +} + /** * Sets the graceful cleanup. */ @@ -562,6 +580,16 @@ function setGracefulCleanup() { _gracefulCleanup = true; } +/** + * Returns the currently configured tmp dir from os.tmpdir(). + * + * @private + * @returns {string} the currently configured tmp dir + */ +function _getTmpDir() { + return os.tmpdir(); +} + /** * If there are multiple different versions of tmp in place, make sure that * we recognize the old listeners. @@ -715,7 +743,16 @@ _safely_install_sigint_listener(); */ // exporting all the needed methods -module.exports.tmpdir = tmpDir; + +// evaluate os.tmpdir() lazily, mainly for simplifying testing but it also will +// allow users to reconfigure the temporary directory +Object.defineProperty(module.exports, 'tmpdir', { + enumerable: true, + configurable: false, + get: function () { + return _getTmpDir(); + } +}); module.exports.dir = dir; module.exports.dirSync = dirSync; diff --git a/package.json b/package.json index 0c178bc..6d28fc7 100644 --- a/package.json +++ b/package.json @@ -22,9 +22,8 @@ "node": ">=6" }, "dependencies": { - "rimraf": "^2.6.2" + "rimraf": "^2.6.3" }, - "dependencies": {}, "devDependencies": { "eslint": "^4.19.1", "eslint-plugin-mocha": "^5.0.0", diff --git a/test/dir-test.js b/test/dir-test.js index 406175a..8cee952 100644 --- a/test/dir-test.js +++ b/test/dir-test.js @@ -54,7 +54,6 @@ describe('tmp', function () { }); describe('when running issue specific inband tests', function () { - // add your issue specific tests here }); describe('when running standard outband tests', function () { diff --git a/test/name-sync-test.js b/test/name-sync-test.js index e784668..8921150 100644 --- a/test/name-sync-test.js +++ b/test/name-sync-test.js @@ -1,8 +1,9 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), + os = require('os'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); @@ -35,6 +36,71 @@ describe('tmp', function () { }); describe('when running issue specific inband tests', function () { + describe('on issue #176', function () { + const origfn = os.tmpdir; + + function _generateSpecName(optsDir, osTmpDir) { + return 'opts.dir = "$1", os.tmpdir() = "$2"'.replace('$1', optsDir).replace('$2', osTmpDir); + } + + const failing = ['', ' ', undefined, null]; + const nonFailing = ['tmp']; // the origfn cannot be trusted as the os may or may not have a valid tmp dir + + describe('must fail on invalid os.tmpdir() and invalid opts.dir', function () { + // test all failing permutations + for (let oidx = 0; oidx < failing.length; oidx++) { + for (let iidx = 0; iidx < failing.length; iidx++) { + it(_generateSpecName(failing[iidx], failing[oidx]), function () { + os.tmpdir = function () { return failing[oidx]; }; + try { + tmp.tmpNameSync({ dir: failing[iidx] }); + assert.fail('expected this to fail'); + } catch (err) { + assert.ok(err instanceof Error, 'error expected'); + } finally { + os.tmpdir = origfn; + } + }); + } + } + }); + + describe('must not fail on invalid os.tmpdir() and valid opts.dir', function () { + // test all non failing permutations for non failing opts.dir and failing osTmpDir + for (let oidx = 0; oidx < failing.length; oidx++) { + for (let iidx = 0; iidx < nonFailing.length; iidx++) { + it(_generateSpecName(nonFailing[iidx], failing[oidx]), function () { + os.tmpdir = function () { return failing[oidx]; }; + try { + tmp.tmpNameSync({ dir: nonFailing[iidx] }); + } catch (err) { + assert.fail(err); + } finally { + os.tmpdir = origfn; + } + }); + } + } + }); + + describe('must not fail on valid os.tmpdir() and invalid opts.dir', function () { + // test all non failing permutations for failing opts.dir and non failing osTmpDir + for (let oidx = 0; oidx < nonFailing.length; oidx++) { + for (let iidx = 0; iidx < failing.length; iidx++) { + it(_generateSpecName(failing[iidx], nonFailing[oidx]), function () { + os.tmpdir = function () { return nonFailing[oidx]; }; + try { + tmp.tmpNameSync({ dir: failing[iidx] }); + } catch (err) { + assert.fail(err); + } finally { + os.tmpdir = origfn; + } + }); + } + } + }); + }); }); describe('when running standard outband tests', function () { diff --git a/test/name-test.js b/test/name-test.js index 564ad32..bee989e 100644 --- a/test/name-test.js +++ b/test/name-test.js @@ -1,8 +1,9 @@ /* eslint-disable no-octal */ // vim: expandtab:ts=2:sw=2 -var +const assert = require('assert'), + os = require('os'), inbandStandardTests = require('./name-inband-standard'), tmp = require('../lib/tmp'); @@ -45,6 +46,79 @@ describe('tmp', function () { }); describe('when running issue specific inband tests', function () { + describe('on issue #176', function () { + const origfn = os.tmpdir; + + function _generateSpecName(optsDir, osTmpDir) { + return 'opts.dir = "$1", os.tmpdir() = "$2"'.replace('$1', optsDir).replace('$2', osTmpDir); + } + + const failing = ['', ' ', undefined, null]; + const nonFailing = ['tmp']; // the origfn cannot be trusted as the os may or may not have a valid tmp dir + + describe('must fail on invalid os.tmpdir() and invalid opts.dir', function () { + // test all failing permutations + for (let oidx = 0; oidx < failing.length; oidx++) { + for (let iidx = 0; iidx < failing.length; iidx++) { + it(_generateSpecName(failing[iidx], failing[oidx]), function (done) { + os.tmpdir = function () { return failing[oidx]; }; + tmp.tmpName({ dir: failing[iidx] }, function (err) { + try { + assert.ok(err instanceof Error, 'should have failed'); + } catch (err) { + return done(err); + } finally { + os.tmpdir = origfn; + } + done(); + }); + }); + } + } + }); + + describe('must not fail on invalid os.tmpdir() and valid opts.dir', function () { + // test all non failing permutations for non failing opts.dir and failing osTmpDir + for (let oidx = 0; oidx < failing.length; oidx++) { + for (let iidx = 0; iidx < nonFailing.length; iidx++) { + it(_generateSpecName(nonFailing[iidx], failing[oidx]), function (done) { + os.tmpdir = function () { return failing[oidx]; }; + tmp.tmpName({ dir: nonFailing[iidx] }, function (err) { + try { + assert.ok(err === null || err === undefined, 'should not have failed'); + } catch (err) { + return done(err); + } finally { + os.tmpdir = origfn; + } + done(); + }); + }); + } + } + }); + + describe('must not fail on valid os.tmpdir() and invalid opts.dir', function () { + // test all non failing permutations for failing opts.dir and non failing osTmpDir + for (let oidx = 0; oidx < nonFailing.length; oidx++) { + for (let iidx = 0; iidx < failing.length; iidx++) { + it(_generateSpecName(failing[iidx], nonFailing[oidx]), function (done) { + os.tmpdir = function () { return nonFailing[oidx]; }; + tmp.tmpName({ dir: failing[iidx] }, function (err) { + try { + assert.ok(err === null || err === undefined, 'should not have failed'); + } catch (err) { + return done(err); + } finally { + os.tmpdir = origfn; + } + done(); + }); + }); + } + } + }); + }); }); describe('when running standard outband tests', function () {