Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib/fs: convert to using internal/errors #15043

Merged
merged 1 commit into from
Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this could be changed to TypeError.
Feedback anyone?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's really difficult to say. The coercion to a string means that any value can be used, it's not really a TypeError. The real check here is the null-character bit, which doesn't feel like a TypeError to me. I could live with it tho.

'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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. These a disparity with the docs (but out of scope for this PR)
  2. AFAICT date string will not parse

tl;dr 3rd arg should be ['Date', 'time in seconds']

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2.You are right I will go with ['Date', 'time in seconds']
The date string was (inaccurate) for a string timestamp arg like '1504020561954'

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to ['Date', 'time in seconds'] (commit: 2f0738a)

'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