From 28fc814562cab94f25c30bf95593b4eda32ddc38 Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Fri, 20 Dec 2019 17:18:52 +0100 Subject: [PATCH] fs: ignore prototype properties This is mainly a minor refactoring to reduce the code overhead with the side effect of removing support for prototype properties in passed through options. Copying options is mostly done for enumerable own properties and prototype properties are ignored. --- lib/fs.js | 19 +- lib/internal/fs/promises.js | 6 +- lib/internal/fs/streams.js | 7 +- lib/internal/fs/utils.js | 85 ++++---- test/parallel/test-fs-read-stream-inherit.js | 205 ------------------- 5 files changed, 47 insertions(+), 275 deletions(-) delete mode 100644 test/parallel/test-fs-read-stream-inherit.js diff --git a/lib/fs.js b/lib/fs.js index 66113899a5bfdc..c6e16a35eec2df 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -67,7 +67,6 @@ const { FSReqCallback, statValues } = binding; const { toPathIfFileURL } = require('internal/url'); const internalUtil = require('internal/util'); const { - copyObject, Dirent, getDirents, getOptions, @@ -1321,12 +1320,9 @@ function appendFile(path, data, options, callback) { callback = maybeCallback(callback || options); options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - // Don't make changes directly on options object - options = copyObject(options); - // Force append behavior when using a supplied file descriptor if (!options.flag || isFd(path)) - options.flag = 'a'; + options = { ...options, flag: 'a' }; fs.writeFile(path, data, options, callback); } @@ -1334,12 +1330,10 @@ function appendFile(path, data, options, callback) { function appendFileSync(path, data, options) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - // Don't make changes directly on options object - options = copyObject(options); - // Force append behavior when using a supplied file descriptor - if (!options.flag || isFd(path)) - options.flag = 'a'; + if (!options.flag || isFd(path)) { + options = { ...options, flag: 'a' }; + } fs.writeFileSync(path, data, options); } @@ -1348,10 +1342,7 @@ function watch(filename, options, listener) { if (typeof options === 'function') { listener = options; } - options = getOptions(options, {}); - - // Don't make changes directly on options object - options = copyObject(options); + options = { ...getOptions(options, {}) }; if (options.persistent === undefined) options.persistent = true; if (options.recursive === undefined) options.recursive = false; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 5f8be113f44c0c..896d8025bdaec6 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -25,7 +25,6 @@ const { const { isUint8Array } = require('internal/util/types'); const { rimrafPromises } = require('internal/fs/rimraf'); const { - copyObject, getDirents, getOptions, getStatsFromBinding, @@ -496,8 +495,9 @@ async function writeFile(path, data, options) { async function appendFile(path, data, options) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'a' }); - options = copyObject(options); - options.flag = options.flag || 'a'; + if (!options.flag) { + options = { ...options, flag: 'a' }; + } return writeFile(path, data, options); } diff --git a/lib/internal/fs/streams.js b/lib/internal/fs/streams.js index d1603f6a024b81..c86fb7d3fab4a0 100644 --- a/lib/internal/fs/streams.js +++ b/lib/internal/fs/streams.js @@ -20,7 +20,6 @@ const { validateNumber } = require('internal/validators'); const fs = require('fs'); const { Buffer } = require('buffer'); const { - copyObject, getOptions, } = require('internal/fs/utils'); const { Readable, Writable } = require('stream'); @@ -69,7 +68,7 @@ function ReadStream(path, options) { return new ReadStream(path, options); // A little bit bigger buffer and water marks by default - options = copyObject(getOptions(options, {})); + options = { ...getOptions(options, {}) }; if (options.highWaterMark === undefined) options.highWaterMark = 64 * 1024; @@ -292,10 +291,8 @@ function WriteStream(path, options) { if (!(this instanceof WriteStream)) return new WriteStream(path, options); - options = copyObject(getOptions(options, {})); - // Only buffers are supported. - options.decodeStrings = true; + options = { ...getOptions(options, {}), decodeStrings: true }; // For backwards compat do not emit close on destroy. if (options.emitClose === undefined) { diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 18d8e07d78e3cb..1c7cda24e0edd8 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -166,49 +166,38 @@ for (const name of ReflectOwnKeys(Dirent.prototype)) { }; } -function copyObject(source) { - const target = {}; - for (const key in source) - target[key] = source[key]; - return target; -} - function getDirents(path, [names, types], callback) { - let i; - if (typeof callback === 'function') { - const len = names.length; - let toFinish = 0; - callback = once(callback); - for (i = 0; i < len; i++) { - const type = types[i]; - if (type === UV_DIRENT_UNKNOWN) { - const name = names[i]; - const idx = i; - toFinish++; - lazyLoadFs().lstat(pathModule.join(path, name), (err, stats) => { - if (err) { - callback(err); - return; - } - names[idx] = new DirentFromStats(name, stats); - if (--toFinish === 0) { - callback(null, names); - } - }); - } else { - names[i] = new Dirent(names[i], types[i]); - } - } - if (toFinish === 0) { - callback(null, names); - } - } else { - const len = names.length; - for (i = 0; i < len; i++) { + if (typeof callback !== 'function') { + for (let i = 0; i < names.length; i++) { names[i] = getDirent(path, names[i], types[i]); } return names; } + let toFinish = 0; + callback = once(callback); + for (let i = 0; i < names.length; i++) { + const type = types[i]; + if (type === UV_DIRENT_UNKNOWN) { + const name = names[i]; + const idx = i; + toFinish++; + lazyLoadFs().lstat(pathModule.join(path, name), (err, stats) => { + if (err) { + callback(err); + return; + } + names[idx] = new DirentFromStats(name, stats); + if (--toFinish === 0) { + callback(null, names); + } + }); + } else { + names[i] = new Dirent(names[i], types[i]); + } + } + if (toFinish === 0) { + callback(null, names); + } } function getDirent(path, name, type, callback) { @@ -239,9 +228,7 @@ function getOptions(options, defaultOptions) { } if (typeof options === 'string') { - defaultOptions = { ...defaultOptions }; - defaultOptions.encoding = options; - options = defaultOptions; + options = { ...defaultOptions, encoding: options }; } else if (typeof options !== 'object') { throw new ERR_INVALID_ARG_TYPE('options', ['string', 'Object'], options); } @@ -271,13 +258,16 @@ function handleErrorFromBinding(ctx) { // Check if the path contains null types if it is a string nor Uint8Array, // otherwise return silently. const nullCheck = hideStackFrames((path, propName, throwError = true) => { - const pathIsString = typeof path === 'string'; - const pathIsUint8Array = isUint8Array(path); - // We can only perform meaningful checks on strings and Uint8Arrays. - if ((!pathIsString && !pathIsUint8Array) || - (pathIsString && !path.includes('\u0000')) || - (pathIsUint8Array && !path.includes(0))) { + if (typeof path === 'string') { + if (!path.includes('\u0000')) { + return; + } + } else if (isUint8Array(path)) { + if (!path.includes(0)) { + return; + } + } else { return; } @@ -654,7 +644,6 @@ const getValidMode = hideStackFrames((mode, type) => { module.exports = { assertEncoding, BigIntStats, // for testing - copyObject, Dirent, getDirent, getDirents, diff --git a/test/parallel/test-fs-read-stream-inherit.js b/test/parallel/test-fs-read-stream-inherit.js deleted file mode 100644 index 5c8bbef0e499c0..00000000000000 --- a/test/parallel/test-fs-read-stream-inherit.js +++ /dev/null @@ -1,205 +0,0 @@ -'use strict'; - -const common = require('../common'); - -const assert = require('assert'); -const fs = require('fs'); -const fixtures = require('../common/fixtures'); - -const fn = fixtures.path('elipses.txt'); -const rangeFile = fixtures.path('x.txt'); - -{ - let paused = false; - - const file = fs.ReadStream(fn); - - file.on('open', common.mustCall(function(fd) { - file.length = 0; - assert.strictEqual(typeof fd, 'number'); - assert.ok(file.readable); - - // GH-535 - file.pause(); - file.resume(); - file.pause(); - file.resume(); - })); - - file.on('data', common.mustCallAtLeast(function(data) { - assert.ok(data instanceof Buffer); - assert.ok(!paused); - file.length += data.length; - - paused = true; - file.pause(); - - setTimeout(function() { - paused = false; - file.resume(); - }, 10); - })); - - - file.on('end', common.mustCall()); - - - file.on('close', common.mustCall(function() { - assert.strictEqual(file.length, 30000); - })); -} - -{ - const file = fs.createReadStream(fn, Object.create({ encoding: 'utf8' })); - file.length = 0; - file.on('data', function(data) { - assert.strictEqual(typeof data, 'string'); - file.length += data.length; - - for (let i = 0; i < data.length; i++) { - // http://www.fileformat.info/info/unicode/char/2026/index.htm - assert.strictEqual(data[i], '\u2026'); - } - }); - - file.on('close', common.mustCall(function() { - assert.strictEqual(file.length, 10000); - })); -} - -{ - const options = Object.create({ bufferSize: 1, start: 1, end: 2 }); - const file = fs.createReadStream(rangeFile, options); - assert.strictEqual(file.start, 1); - assert.strictEqual(file.end, 2); - let contentRead = ''; - file.on('data', function(data) { - contentRead += data.toString('utf-8'); - }); - file.on('end', common.mustCall(function() { - assert.strictEqual(contentRead, 'yz'); - })); -} - -{ - const options = Object.create({ bufferSize: 1, start: 1 }); - const file = fs.createReadStream(rangeFile, options); - assert.strictEqual(file.start, 1); - file.data = ''; - file.on('data', function(data) { - file.data += data.toString('utf-8'); - }); - file.on('end', common.mustCall(function() { - assert.strictEqual(file.data, 'yz\n'); - })); -} - -// https://github.com/joyent/node/issues/2320 -{ - const options = Object.create({ bufferSize: 1.23, start: 1 }); - const file = fs.createReadStream(rangeFile, options); - assert.strictEqual(file.start, 1); - file.data = ''; - file.on('data', function(data) { - file.data += data.toString('utf-8'); - }); - file.on('end', common.mustCall(function() { - assert.strictEqual(file.data, 'yz\n'); - })); -} - -{ - const message = - 'The value of "start" is out of range. It must be <= "end" (here: 2).' + - ' Received 10'; - - assert.throws( - () => { - fs.createReadStream(rangeFile, Object.create({ start: 10, end: 2 })); - }, - { - code: 'ERR_OUT_OF_RANGE', - message, - name: 'RangeError' - }); -} - -{ - const options = Object.create({ start: 0, end: 0 }); - const stream = fs.createReadStream(rangeFile, options); - assert.strictEqual(stream.start, 0); - assert.strictEqual(stream.end, 0); - stream.data = ''; - - stream.on('data', function(chunk) { - stream.data += chunk; - }); - - stream.on('end', common.mustCall(function() { - assert.strictEqual(stream.data, 'x'); - })); -} - -// Pause and then resume immediately. -{ - const pauseRes = fs.createReadStream(rangeFile); - pauseRes.pause(); - pauseRes.resume(); -} - -{ - let data = ''; - let file = - fs.createReadStream(rangeFile, Object.create({ autoClose: false })); - assert.strictEqual(file.autoClose, false); - file.on('data', (chunk) => { data += chunk; }); - file.on('end', common.mustCall(function() { - process.nextTick(common.mustCall(function() { - assert(!file.closed); - assert(!file.destroyed); - assert.strictEqual(data, 'xyz\n'); - fileNext(); - })); - })); - - function fileNext() { - // This will tell us if the fd is usable again or not. - file = fs.createReadStream(null, Object.create({ fd: file.fd, start: 0 })); - file.data = ''; - file.on('data', function(data) { - file.data += data; - }); - file.on('end', common.mustCall(function() { - assert.strictEqual(file.data, 'xyz\n'); - })); - } - process.on('exit', function() { - assert(file.closed); - assert(file.destroyed); - }); -} - -// Just to make sure autoClose won't close the stream because of error. -{ - const options = Object.create({ fd: 13337, autoClose: false }); - const file = fs.createReadStream(null, options); - file.on('data', common.mustNotCall()); - file.on('error', common.mustCall()); - process.on('exit', function() { - assert(!file.closed); - assert(!file.destroyed); - assert(file.fd); - }); -} - -// Make sure stream is destroyed when file does not exist. -{ - const file = fs.createReadStream('/path/to/file/that/does/not/exist'); - file.on('data', common.mustNotCall()); - file.on('error', common.mustCall()); - - process.on('exit', function() { - assert(!file.closed); - assert(file.destroyed); - }); -}