Skip to content

Commit

Permalink
lib/fs: convert to using internal/errors
Browse files Browse the repository at this point in the history
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in GH-12926

- throw the ERR_INVALID_CALLBACK error
  when the the callback is invalid
- replace the ['object', 'string'] with
  ['string', 'object'] in the error constructor call,
  to better match the previous err msg
  in the getOptions() function
- add error ERR_VALUE_OUT_OF_RANGE in lib/internal/errors.js,
  this error is thrown when a numeric value is out of range
- document the ERR_VALUE_OUT_OF_RANGE err in errors.md
- correct the expected args, in the error thrown in the function
  fs._toUnixTimestamp() to ['Date', 'time in seconds'] (lib/fs.js)
- update the listener error type in the fs.watchFile() function,
  from Error to TypeError (lib/fs.js)
- update errors from ERR_INVALID_OPT_VALUE to ERR_INVALID_ARG_TYPE
  in the functions fs.ReadStream() and fs.WriteStream(),
  for the cases of range errors use the new error:
  ERR_VALUE_OUT_OF_RANGE (lib/fs.js)

PR-URL: #15043
Refs: #11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
matzavinos authored and refack committed Aug 31, 2017
1 parent a517466 commit 0b1285c
Show file tree
Hide file tree
Showing 17 changed files with 246 additions and 116 deletions.
5 changes: 5 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1136,6 +1136,11 @@ Used when an attempt is made to launch a Node.js process with an unknown
by errors in user code, although it is not impossible. Occurrences of this error
are most likely an indication of a bug within Node.js itself.

<a id="ERR_VALUE_OUT_OF_RANGE"></a>
### ERR_VALUE_OUT_OF_RANGE

Used when a number value is out of range.

<a id="ERR_V8BREAKITERATOR"></a>
### ERR_V8BREAKITERATOR

Expand Down
73 changes: 55 additions & 18 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const { isUint8Array, createPromise, promiseResolve } = process.binding('util');
const binding = process.binding('fs');
const fs = exports;
const Buffer = require('buffer').Buffer;
const errors = require('internal/errors');
const Stream = require('stream').Stream;
const EventEmitter = require('events');
const FSReqWrap = binding.FSReqWrap;
Expand Down Expand Up @@ -72,8 +73,10 @@ function getOptions(options, 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.');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options',
['string', 'object'],
options);
}

if (options.encoding !== 'buffer')
Expand Down Expand Up @@ -128,7 +131,7 @@ function makeCallback(cb) {
}

if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

return function() {
Expand All @@ -145,7 +148,7 @@ function makeStatsCallback(cb) {
}

if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

return function(err) {
Expand All @@ -156,8 +159,11 @@ function makeStatsCallback(cb) {

function nullCheck(path, callback) {
if (('' + path).indexOf('\u0000') !== -1) {
var er = new Error('Path must be a string without null bytes');
er.code = 'ENOENT';
const er = new errors.Error('ERR_INVALID_ARG_TYPE',
'path',
'string without null bytes',
path);

if (typeof callback !== 'function')
throw er;
process.nextTick(callback, er);
Expand Down Expand Up @@ -274,7 +280,7 @@ fs.access = function(path, mode, callback) {
callback = mode;
mode = fs.F_OK;
} else if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');
}

if (handleError((path = getPathFromURL(path)), callback))
Expand Down Expand Up @@ -1193,7 +1199,10 @@ function toUnixTimestamp(time) {
// convert to 123.456 UNIX timestamp
return time.getTime() / 1000;
}
throw new Error('Cannot parse time: ' + time);
throw new errors.Error('ERR_INVALID_ARG_TYPE',
'time',
['Date', 'time in seconds'],
time);
}

// exported for unit tests, not for public consumption
Expand Down Expand Up @@ -1495,7 +1504,10 @@ fs.watchFile = function(filename, options, listener) {
}

if (typeof listener !== 'function') {
throw new Error('"watchFile()" requires a listener function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'listener',
'function',
listener);
}

stat = statWatchers.get(filename);
Expand Down Expand Up @@ -1842,8 +1854,12 @@ 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');
if (!prefix || typeof prefix !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'prefix',
'string',
prefix);
}
if (!nullCheck(prefix, callback)) {
return;
}
Expand All @@ -1856,8 +1872,12 @@ fs.mkdtemp = function(prefix, options, callback) {


fs.mkdtempSync = function(prefix, options) {
if (!prefix || typeof prefix !== 'string')
throw new TypeError('filename prefix is required');
if (!prefix || typeof prefix !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'prefix',
'string',
prefix);
}
options = getOptions(options, {});
nullCheck(prefix);
return binding.mkdtemp(prefix + 'XXXXXX', options.encoding);
Expand Down Expand Up @@ -1903,16 +1923,26 @@ function ReadStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new TypeError('"start" option must be a Number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'start',
'number',
this.start);
}
if (this.end === undefined) {
this.end = Infinity;
} else if (typeof this.end !== 'number') {
throw new TypeError('"end" option must be a Number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'end',
'number',
this.end);
}

if (this.start > this.end) {
throw new Error('"start" option must be <= "end" option');
const errVal = `{start: ${this.start}, end: ${this.end}}`;
throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE',
'start',
'<= "end"',
errVal);
}

this.pos = this.start;
Expand Down Expand Up @@ -2069,10 +2099,17 @@ function WriteStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new TypeError('"start" option must be a Number');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'start',
'number',
this.start);
}
if (this.start < 0) {
throw new Error('"start" must be >= zero');
const errVal = `{start: ${this.start}}`;
throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE',
'start',
'>= 0',
errVal);
}

this.pos = this.start;
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,9 @@ E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s');
E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s');
E('ERR_UNKNOWN_STDIN_TYPE', 'Unknown stdin file type');
E('ERR_UNKNOWN_STREAM_TYPE', 'Unknown stream file type');
E('ERR_VALUE_OUT_OF_RANGE', (start, end, value) => {
return `The value of "${start}" must be ${end}. Received "${value}"`;
});
E('ERR_V8BREAKITERATOR', 'Full ICU data not installed. ' +
'See https://github.com/nodejs/node/wiki/Intl');
E('ERR_VALID_PERFORMANCE_ENTRY_TYPE',
Expand Down
15 changes: 9 additions & 6 deletions test/parallel/test-file-write-stream3.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,15 @@ function run_test_3() {

const run_test_4 = common.mustCall(function() {
// Error: start must be >= zero
assert.throws(
function() {
fs.createWriteStream(filepath, { start: -5, flags: 'r+' });
},
/"start" must be/
);
const block = () => {
fs.createWriteStream(filepath, { start: -5, flags: 'r+' });
};
const err = {
code: 'ERR_VALUE_OUT_OF_RANGE',
message: 'The value of "start" must be >= 0. Received "{start: -5}"',
type: RangeError
};
common.expectsError(block, err);
});

run_test_1();
24 changes: 17 additions & 7 deletions test/parallel/test-fs-access.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,23 @@ assert.throws(() => {
fs.access(100, fs.F_OK, common.mustNotCall());
}, /^TypeError: path must be a string or Buffer$/);

assert.throws(() => {
fs.access(__filename, fs.F_OK);
}, /^TypeError: "callback" argument must be a function$/);

assert.throws(() => {
fs.access(__filename, fs.F_OK, {});
}, /^TypeError: "callback" argument must be a function$/);
common.expectsError(
() => {
fs.access(__filename, fs.F_OK);
},
{
code: 'ERR_INVALID_CALLBACK',
type: TypeError
});

common.expectsError(
() => {
fs.access(__filename, fs.F_OK, {});
},
{
code: 'ERR_INVALID_CALLBACK',
type: TypeError
});

assert.doesNotThrow(() => {
fs.accessSync(__filename);
Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-fs-make-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];

const { sep } = require('path');
Expand All @@ -24,7 +23,10 @@ assert.doesNotThrow(testMakeCallback());

function invalidCallbackThrowsTests() {
callbackThrowValues.forEach((value) => {
assert.throws(testMakeCallback(value), cbTypeError);
common.expectsError(testMakeCallback(value), {
code: 'ERR_INVALID_CALLBACK',
type: TypeError
});
});
}

Expand Down
6 changes: 4 additions & 2 deletions test/parallel/test-fs-makeStatsCallback.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const cbTypeError = /^TypeError: "callback" argument must be a function$/;
const callbackThrowValues = [null, true, false, 0, 1, 'foo', /foo/, [], {}];
const warn = 'Calling an asynchronous function without callback is deprecated.';

Expand All @@ -23,7 +22,10 @@ assert.doesNotThrow(testMakeStatsCallback());

function invalidCallbackThrowsTests() {
callbackThrowValues.forEach((value) => {
assert.throws(testMakeStatsCallback(value), cbTypeError);
common.expectsError(testMakeStatsCallback(value), {
code: 'ERR_INVALID_CALLBACK',
type: TypeError
});
});
}

Expand Down
25 changes: 16 additions & 9 deletions test/parallel/test-fs-mkdtemp-prefix-check.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,29 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const fs = require('fs');

const expectedError = /^TypeError: filename prefix is required$/;
const prefixValues = [undefined, null, 0, true, false, 1, ''];

function fail(value) {
assert.throws(
() => fs.mkdtempSync(value, {}),
expectedError
);
common.expectsError(
() => {
fs.mkdtempSync(value, {});
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});
}

function failAsync(value) {
assert.throws(
() => fs.mkdtemp(value, common.mustNotCall()), expectedError
);
common.expectsError(
() => {
fs.mkdtemp(value, common.mustNotCall());
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});
}

prefixValues.forEach((prefixValue) => {
Expand Down
36 changes: 24 additions & 12 deletions test/parallel/test-fs-non-number-arguments-throw.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,32 @@ fs.writeFileSync(tempFile, 'abc\ndef');
const sanity = 'def';
const saneEmitter = fs.createReadStream(tempFile, { start: 4, end: 6 });

assert.throws(function() {
fs.createReadStream(tempFile, { start: '4', end: 6 });
}, /^TypeError: "start" option must be a Number$/,
"start as string didn't throw an error for createReadStream");
common.expectsError(
() => {
fs.createReadStream(tempFile, { start: '4', end: 6 });
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});

assert.throws(function() {
fs.createReadStream(tempFile, { start: 4, end: '6' });
}, /^TypeError: "end" option must be a Number$/,
"end as string didn't throw an error for createReadStream");
common.expectsError(
() => {
fs.createReadStream(tempFile, { start: 4, end: '6' });
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});

assert.throws(function() {
fs.createWriteStream(tempFile, { start: '4' });
}, /^TypeError: "start" option must be a Number$/,
"start as string didn't throw an error for createWriteStream");
common.expectsError(
() => {
fs.createWriteStream(tempFile, { start: '4' });
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});

saneEmitter.on('data', common.mustCall(function(data) {
assert.strictEqual(
Expand Down
22 changes: 16 additions & 6 deletions test/parallel/test-fs-null-bytes.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,27 @@ const fs = require('fs');
const URL = require('url').URL;

function check(async, sync) {
const expected = /Path must be a string without null bytes/;
const argsSync = Array.prototype.slice.call(arguments, 2);
const argsAsync = argsSync.concat((er) => {
assert(er && expected.test(er.message));
assert.strictEqual(er.code, 'ENOENT');
common.expectsError(
() => {
throw er;
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: Error
});
});

if (sync) {
assert.throws(() => {
sync.apply(null, argsSync);
}, expected);
common.expectsError(
() => {
sync.apply(null, argsSync);
},
{
code: 'ERR_INVALID_ARG_TYPE',
type: Error,
});
}

if (async) {
Expand Down
Loading

0 comments on commit 0b1285c

Please sign in to comment.