From e8e969a5c1a64513b28dd07748787dec04b90d82 Mon Sep 17 00:00:00 2001 From: Sakthipriyan Vairamani Date: Sun, 26 Jun 2016 00:03:05 +0530 Subject: [PATCH] fs: refactor "options" processing as a function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As it is, the "options" processing is repeated in all the functions which need it. That introduces checks which are inconsistent with other functions and produces slightly different error messages. This patch moves the basic "options" validation and processing to a seperate function. PR-URL: https://github.com/nodejs/node/pull/7165 Reviewed-By: Benjamin Gruenbaum Reviewed-By: Roman Reiss Reviewed-By: Trevor Norris Reviewed-By: Nicu Micleușanu Reviewed-By: Yorkie Liu Reviewed-By: James M Snell --- lib/fs.js | 218 ++++-------------- .../test-fs-read-stream-throw-type-error.js | 14 +- .../test-fs-write-stream-throw-type-error.js | 14 +- 3 files changed, 65 insertions(+), 181 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 17860ec05affcb..9cab1e9e25a877 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -36,9 +36,24 @@ const isWindows = process.platform === 'win32'; const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG); const errnoException = util._errnoException; -function throwOptionsError(options) { - throw new TypeError('Expected options to be either an object or a string, ' + - 'but got ' + typeof options + ' instead'); +function getOptions(options, defaultOptions) { + if (options === null || options === undefined || + typeof options === 'function') { + return defaultOptions; + } + + if (typeof options === 'string') { + defaultOptions = util._extend({}, defaultOptions); + defaultOptions.encoding = options; + options = defaultOptions; + } else if (typeof options !== 'object') { + throw new TypeError('"options" must be a string or an object, got ' + + typeof options + ' instead.'); + } + + if (options.encoding !== 'buffer') + assertEncoding(options.encoding); + return options; } function rethrow() { @@ -228,26 +243,14 @@ fs.existsSync = function(path) { } }; -fs.readFile = function(path, options, callback_) { - var callback = maybeCallback(arguments[arguments.length - 1]); - - if (!options || typeof options === 'function') { - options = { encoding: null, flag: 'r' }; - } else if (typeof options === 'string') { - options = { encoding: options, flag: 'r' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } - - var encoding = options.encoding; - assertEncoding(encoding); - - var flag = options.flag || 'r'; +fs.readFile = function(path, options, callback) { + callback = maybeCallback(arguments[arguments.length - 1]); + options = getOptions(options, { flag: 'r' }); if (!nullCheck(path, callback)) return; - var context = new ReadFileContext(callback, encoding); + var context = new ReadFileContext(callback, options.encoding); context.isUserFd = isFd(path); // file descriptor ownership var req = new FSReqWrap(); req.context = context; @@ -261,7 +264,7 @@ fs.readFile = function(path, options, callback_) { } binding.open(pathModule._makeLong(path), - stringToFlags(flag), + stringToFlags(options.flag || 'r'), 0o666, req); }; @@ -452,20 +455,9 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { } fs.readFileSync = function(path, options) { - if (!options) { - options = { encoding: null, flag: 'r' }; - } else if (typeof options === 'string') { - options = { encoding: options, flag: 'r' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } - - var encoding = options.encoding; - assertEncoding(encoding); - - var flag = options.flag || 'r'; + options = getOptions(options, { flag: 'r' }); var isUserFd = isFd(path); // file descriptor ownership - var fd = isUserFd ? path : fs.openSync(path, flag, 0o666); + var fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666); var st = tryStatSync(fd, isUserFd); var size = st.isFile() ? st.size : 0; @@ -509,7 +501,7 @@ fs.readFileSync = function(path, options) { buffer = buffer.slice(0, pos); } - if (encoding) buffer = buffer.toString(encoding); + if (options.encoding) buffer = buffer.toString(options.encoding); return buffer; }; @@ -849,17 +841,8 @@ fs.mkdirSync = function(path, mode) { }; fs.readdir = function(path, options, callback) { - options = options || {}; - if (typeof options === 'function') { - callback = options; - options = {}; - } else if (typeof options === 'string') { - options = {encoding: options}; - } - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); - - callback = makeCallback(callback); + callback = makeCallback(typeof options === 'function' ? options : callback); + options = getOptions(options, {}); if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -867,11 +850,7 @@ fs.readdir = function(path, options, callback) { }; fs.readdirSync = function(path, options) { - options = options || {}; - if (typeof options === 'string') - options = {encoding: options}; - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); + options = getOptions(options, {}); nullCheck(path); return binding.readdir(pathModule._makeLong(path), options.encoding); }; @@ -913,16 +892,8 @@ fs.statSync = function(path) { }; fs.readlink = function(path, options, callback) { - options = options || {}; - if (typeof options === 'function') { - callback = options; - options = {}; - } else if (typeof options === 'string') { - options = {encoding: options}; - } - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); - callback = makeCallback(callback); + callback = makeCallback(typeof options === 'function' ? options : callback); + options = getOptions(options, {}); if (!nullCheck(path, callback)) return; var req = new FSReqWrap(); req.oncomplete = callback; @@ -930,11 +901,7 @@ fs.readlink = function(path, options, callback) { }; fs.readlinkSync = function(path, options) { - options = options || {}; - if (typeof options === 'string') - options = {encoding: options}; - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); + options = getOptions(options, {}); nullCheck(path); return binding.readlink(pathModule._makeLong(path), options.encoding); }; @@ -1205,20 +1172,10 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) { }); } -fs.writeFile = function(path, data, options, callback_) { - var callback = maybeCallback(arguments[arguments.length - 1]); - - if (!options || typeof options === 'function') { - options = { encoding: 'utf8', mode: 0o666, flag: 'w' }; - } else if (typeof options === 'string') { - options = { encoding: options, mode: 0o666, flag: 'w' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } - - assertEncoding(options.encoding); - - var flag = options.flag || 'w'; +fs.writeFile = function(path, data, options, callback) { + callback = maybeCallback(arguments[arguments.length - 1]); + options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); + const flag = options.flag || 'w'; if (isFd(path)) { writeFd(path, true); @@ -1243,17 +1200,9 @@ fs.writeFile = function(path, data, options, callback_) { }; fs.writeFileSync = function(path, data, options) { - if (!options) { - options = { encoding: 'utf8', mode: 0o666, flag: 'w' }; - } else if (typeof options === 'string') { - options = { encoding: options, mode: 0o666, flag: 'w' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } - - assertEncoding(options.encoding); + options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); + const flag = options.flag || 'w'; - var flag = options.flag || 'w'; var isUserFd = isFd(path); // file descriptor ownership var fd = isUserFd ? path : fs.openSync(path, flag, options.mode); @@ -1277,16 +1226,9 @@ fs.writeFileSync = function(path, data, options) { } }; -fs.appendFile = function(path, data, options, callback_) { - var callback = maybeCallback(arguments[arguments.length - 1]); - - if (!options || typeof options === 'function') { - options = { encoding: 'utf8', mode: 0o666, flag: 'a' }; - } else if (typeof options === 'string') { - options = { encoding: options, mode: 0o666, flag: 'a' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } +fs.appendFile = function(path, data, options, callback) { + callback = maybeCallback(arguments[arguments.length - 1]); + options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); if (!options.flag) options = util._extend({ flag: 'a' }, options); @@ -1299,13 +1241,7 @@ fs.appendFile = function(path, data, options, callback_) { }; fs.appendFileSync = function(path, data, options) { - if (!options) { - options = { encoding: 'utf8', mode: 0o666, flag: 'a' }; - } else if (typeof options === 'string') { - options = { encoding: options, mode: 0o666, flag: 'a' }; - } else if (typeof options !== 'object') { - throwOptionsError(options); - } + options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); if (!options.flag) options = util._extend({ flag: 'a' }, options); @@ -1364,15 +1300,10 @@ FSWatcher.prototype.close = function() { fs.watch = function(filename, options, listener) { nullCheck(filename); - options = options || {}; if (typeof options === 'function') { listener = options; - options = {}; - } else if (typeof options === 'string') { - options = {encoding: options}; } - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); + options = getOptions(options, {}); if (options.persistent === undefined) options.persistent = true; if (options.recursive === undefined) options.recursive = false; @@ -1519,12 +1450,7 @@ function encodeRealpathResult(result, options, err) { const realpathCacheKey = fs.realpathCacheKey = Symbol('realpathCacheKey'); fs.realpathSync = function realpathSync(p, options) { - if (!options) - options = {}; - else if (typeof options === 'string') - options = {encoding: options}; - else if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); + options = getOptions(options, {}); nullCheck(p); p = p.toString('utf8'); @@ -1625,20 +1551,8 @@ fs.realpathSync = function realpathSync(p, options) { fs.realpath = function realpath(p, options, callback) { - if (typeof callback !== 'function') { - callback = maybeCallback(options); - options = {}; - } - - if (!options) { - options = {}; - } else if (typeof options === 'function') { - options = {}; - } else if (typeof options === 'string') { - options = {encoding: options}; - } else if (typeof options !== 'object') { - throw new TypeError('"options" must be a string or an object'); - } + callback = maybeCallback(typeof options === 'function' ? options : callback); + options = getOptions(options, {}); if (!nullCheck(p, callback)) return; @@ -1747,20 +1661,10 @@ fs.realpath = function realpath(p, options, callback) { }; fs.mkdtemp = function(prefix, options, callback) { + callback = makeCallback(typeof options === 'function' ? options : callback); + options = getOptions(options, {}); if (!prefix || typeof prefix !== 'string') throw new TypeError('filename prefix is required'); - - options = options || {}; - if (typeof options === 'function') { - callback = options; - options = {}; - } else if (typeof options === 'string') { - options = {encoding: options}; - } - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); - - callback = makeCallback(callback); if (!nullCheck(prefix, callback)) { return; } @@ -1775,14 +1679,8 @@ fs.mkdtemp = function(prefix, options, callback) { fs.mkdtempSync = function(prefix, options) { if (!prefix || typeof prefix !== 'string') throw new TypeError('filename prefix is required'); - - options = options || {}; - if (typeof options === 'string') - options = {encoding: options}; - if (typeof options !== 'object') - throw new TypeError('"options" must be a string or an object'); + options = getOptions(options, {}); nullCheck(prefix); - return binding.mkdtemp(prefix + 'XXXXXX', options.encoding); }; @@ -1806,15 +1704,8 @@ function ReadStream(path, options) { if (!(this instanceof ReadStream)) return new ReadStream(path, options); - if (options === undefined) - options = {}; - else if (typeof options === 'string') - options = { encoding: options }; - else if (options === null || typeof options !== 'object') - throw new TypeError('"options" argument must be a string or an object'); - // a little bit bigger buffer and water marks by default - options = Object.create(options); + options = Object.create(getOptions(options, {})); if (options.highWaterMark === undefined) options.highWaterMark = 64 * 1024; @@ -1980,14 +1871,7 @@ function WriteStream(path, options) { if (!(this instanceof WriteStream)) return new WriteStream(path, options); - if (options === undefined) - options = {}; - else if (typeof options === 'string') - options = { encoding: options }; - else if (options === null || typeof options !== 'object') - throw new TypeError('"options" argument must be a string or an object'); - - options = Object.create(options); + options = Object.create(getOptions(options, {})); Writable.call(this, options); diff --git a/test/parallel/test-fs-read-stream-throw-type-error.js b/test/parallel/test-fs-read-stream-throw-type-error.js index 81f924d355b91c..bc1fca3a3c3e6a 100644 --- a/test/parallel/test-fs-read-stream-throw-type-error.js +++ b/test/parallel/test-fs-read-stream-throw-type-error.js @@ -9,6 +9,9 @@ const example = path.join(common.fixturesDir, 'x.txt'); assert.doesNotThrow(function() { fs.createReadStream(example, undefined); }); +assert.doesNotThrow(function() { + fs.createReadStream(example, null); +}); assert.doesNotThrow(function() { fs.createReadStream(example, 'utf8'); }); @@ -16,18 +19,15 @@ assert.doesNotThrow(function() { fs.createReadStream(example, {encoding: 'utf8'}); }); -assert.throws(function() { - fs.createReadStream(example, null); -}, /"options" argument must be a string or an object/); assert.throws(function() { fs.createReadStream(example, 123); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createReadStream(example, 0); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createReadStream(example, true); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createReadStream(example, false); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); diff --git a/test/parallel/test-fs-write-stream-throw-type-error.js b/test/parallel/test-fs-write-stream-throw-type-error.js index 007ac03f1bb342..996b22119cda5e 100644 --- a/test/parallel/test-fs-write-stream-throw-type-error.js +++ b/test/parallel/test-fs-write-stream-throw-type-error.js @@ -11,6 +11,9 @@ common.refreshTmpDir(); assert.doesNotThrow(function() { fs.createWriteStream(example, undefined); }); +assert.doesNotThrow(function() { + fs.createWriteStream(example, null); +}); assert.doesNotThrow(function() { fs.createWriteStream(example, 'utf8'); }); @@ -18,18 +21,15 @@ assert.doesNotThrow(function() { fs.createWriteStream(example, {encoding: 'utf8'}); }); -assert.throws(function() { - fs.createWriteStream(example, null); -}, /"options" argument must be a string or an object/); assert.throws(function() { fs.createWriteStream(example, 123); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createWriteStream(example, 0); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createWriteStream(example, true); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/); assert.throws(function() { fs.createWriteStream(example, false); -}, /"options" argument must be a string or an object/); +}, /"options" must be a string or an object/);