-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
errors: validate input arguments #19924
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -416,20 +416,30 @@ function internalAssert(condition, message) { | |
} | ||
} | ||
|
||
function message(key, args) { | ||
function message(key, args = []) { | ||
const msg = messages.get(key); | ||
internalAssert(msg, `An invalid error message key was used: ${key}.`); | ||
let fmt; | ||
if (util === undefined) util = require('util'); | ||
|
||
if (typeof msg === 'function') { | ||
fmt = msg; | ||
} else { | ||
fmt = util.format; | ||
if (args === undefined || args.length === 0) | ||
return msg; | ||
args.unshift(msg); | ||
internalAssert( | ||
msg.length <= args.length, // Default options do not count. | ||
`Code: ${key}; The provided arguments length (${args.length}) does not ` + | ||
`match the required ones (${msg.length}).` | ||
); | ||
return msg.apply(null, args); | ||
} | ||
return String(fmt.apply(null, args)); | ||
|
||
const expectedLength = (msg.match(/%[dfijoOs]/g) || []).length; | ||
internalAssert( | ||
expectedLength === args.length, | ||
`Code: ${key}; The provided arguments length (${args.length}) does not ` + | ||
`match the required ones (${expectedLength}).` | ||
); | ||
if (args.length === 0) | ||
return msg; | ||
|
||
args.unshift(msg); | ||
return util.format.apply(null, args); | ||
} | ||
|
||
/** | ||
|
@@ -739,7 +749,7 @@ E('ERR_HTTP2_INVALID_SETTING_VALUE', | |
'Invalid value for setting "%s": %s', TypeError, RangeError); | ||
E('ERR_HTTP2_INVALID_STREAM', 'The stream has been destroyed', Error); | ||
E('ERR_HTTP2_MAX_PENDING_SETTINGS_ACK', | ||
'Maximum number of pending settings acknowledgements (%s)', Error); | ||
'Maximum number of pending settings acknowledgements', Error); | ||
E('ERR_HTTP2_NO_SOCKET_MANIPULATION', | ||
'HTTP/2 sockets should not be directly manipulated (e.g. read and written)', | ||
Error); | ||
|
@@ -792,7 +802,7 @@ E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => { | |
}, TypeError, RangeError); | ||
E('ERR_INVALID_ARRAY_LENGTH', | ||
(name, len, actual) => { | ||
internalAssert(typeof actual === 'number', 'actual must be a number'); | ||
internalAssert(typeof actual === 'number', 'actual must be of type number'); | ||
return `The array "${name}" (length ${actual}) must be of length ${len}.`; | ||
}, TypeError); | ||
E('ERR_INVALID_ASYNC_ID', 'Invalid %s value: %s', RangeError); | ||
|
@@ -924,7 +934,7 @@ E('ERR_UNCAUGHT_EXCEPTION_CAPTURE_ALREADY_SET', | |
Error); | ||
E('ERR_UNESCAPED_CHARACTERS', '%s contains unescaped characters', TypeError); | ||
E('ERR_UNHANDLED_ERROR', | ||
(err) => { | ||
(err = undefined) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is truly needed. Or is it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sadly that is necessary. Default arguments do not count towards function a(a) {}
function b(a = undefined) {}
assert.strictEqual(a.length, 1);
assert.strictEqual(b.length, 0); Otherwise it is difficult to validate a couple of functions, but I think it is worth it because it makes sure we make less errors while implementing the errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a comment before the function then? Otherwise it won't be clear to the reader. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
const msg = 'Unhandled error.'; | ||
if (err === undefined) return msg; | ||
return `${msg} (${err})`; | ||
|
@@ -959,7 +969,6 @@ E('ERR_VM_MODULE_STATUS', 'Module status %s', Error); | |
E('ERR_ZLIB_INITIALIZATION_FAILED', 'Initialization failed', Error); | ||
|
||
function invalidArgType(name, expected, actual) { | ||
internalAssert(arguments.length === 3, 'Exactly 3 arguments are required'); | ||
internalAssert(typeof name === 'string', 'name must be a string'); | ||
|
||
// determiner: 'must be' or 'must not be' | ||
|
@@ -1006,8 +1015,7 @@ function missingArgs(...args) { | |
} | ||
|
||
function oneOf(expected, thing) { | ||
internalAssert(expected, 'expected is required'); | ||
internalAssert(typeof thing === 'string', 'thing is required'); | ||
internalAssert(typeof thing === 'string', '`thing` has to be of type string'); | ||
if (Array.isArray(expected)) { | ||
const len = expected.length; | ||
internalAssert(len > 0, | ||
|
@@ -1026,25 +1034,24 @@ function oneOf(expected, thing) { | |
} | ||
} | ||
|
||
function bufferOutOfBounds(name, isWriting) { | ||
if (isWriting) { | ||
return 'Attempt to write outside buffer bounds'; | ||
} else { | ||
function bufferOutOfBounds(name = undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the default argument is truly needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See above. |
||
if (name) { | ||
return `"${name}" is outside of buffer bounds`; | ||
} | ||
return 'Attempt to write outside buffer bounds'; | ||
} | ||
|
||
function invalidChar(name, field) { | ||
function invalidChar(name, field = undefined) { | ||
let msg = `Invalid character in ${name}`; | ||
if (field) { | ||
if (field !== undefined) { | ||
msg += ` ["${field}"]`; | ||
} | ||
return msg; | ||
} | ||
|
||
function outOfRange(name, range, value) { | ||
let msg = `The value of "${name}" is out of range.`; | ||
if (range) msg += ` It must be ${range}.`; | ||
if (value !== undefined) msg += ` Received ${value}`; | ||
if (range !== undefined) msg += ` It must be ${range}.`; | ||
msg += ` Received ${value}`; | ||
return msg; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain this and the change to
fs
? I didn't expect to see those here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
length
was never used in this case. It was a weird API that I fixed.Originally the implementation was like:
Now it is: