From eb7c6dc151227456a04f7c2d5d023f4111bb36a4 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Fri, 29 Sep 2017 17:06:28 -0700 Subject: [PATCH 1/2] errors: make properties mutable Userland code can break if it depends on a mutable `code` property for errors. Allow users to change the `code` property but do not propagate changes to the error `name`. Additionally, make `message` and `name` consistent with `Error` object (non-enumerable). Test that `console.log()` and `.toString()` calls on internal `Error` objects with mutated properties have analogous results with the standard ECMAScript `Error` objects. PR-URL: https://github.com/nodejs/node/pull/15694 Fixes: https://github.com/nodejs/node/issues/15658 Reviewed-By: Evan Lucas Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- lib/internal/errors.js | 16 ++++------ test/parallel/test-internal-errors.js | 46 +++++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 496f00e4cebae5..535defff6975cf 100755 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -18,15 +18,13 @@ function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { super(message(key, args)); - this[kCode] = key; - } - - get name() { - return `${super.name} [${this[kCode]}]`; - } - - get code() { - return this[kCode]; + this[kCode] = this.code = key; + Object.defineProperty(this, 'name', { + configurable: true, + enumerable: false, + value: `${super.name} [${this[kCode]}]`, + writable: true + }); } }; } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index cfe523496a8ce4..6bdadd8018f707 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -1,9 +1,9 @@ // Flags: --expose-internals 'use strict'; - const common = require('../common'); -const errors = require('internal/errors'); + const assert = require('assert'); +const errors = require('internal/errors'); function invalidKey(key) { return new RegExp(`^An invalid error message key was used: ${key}\\.$`); @@ -235,3 +235,45 @@ assert.throws( code: 'ERR_ASSERTION', message: /^At least one arg needs to be specified$/ })); + + +// Test that `code` property is mutable and that changing it does not change the +// name. +{ + const myError = new errors.Error('ERR_MISSING_ARGS', ['name']); + assert.strictEqual(myError.code, 'ERR_MISSING_ARGS'); + const initialName = myError.name; + myError.code = 'FHQWHGADS'; + assert.strictEqual(myError.code, 'FHQWHGADS'); + assert.strictEqual(myError.name, initialName); + assert.ok(myError.name.includes('ERR_MISSING_ARGS')); + assert.ok(!myError.name.includes('FHQWHGADS')); +} + +// Test that `name` and `message` are mutable and that changing them alters +// `toString()` but not `console.log()` results, which is the behavior of +// `Error` objects in the browser. +{ + function test(prop) { + let initialConsoleLog = ''; + common.hijackStdout((data) => { initialConsoleLog += data; }); + const myError = new errors.Error('ERR_MISSING_ARGS', ['name']); + const initialToString = myError.toString(); + console.log(myError); + assert.notStrictEqual(initialConsoleLog, ''); + + common.restoreStdout(); + + let subsequentConsoleLog = ''; + common.hijackStdout((data) => { subsequentConsoleLog += data; }); + myError[prop] = 'Fhqwhgads'; + assert.notStrictEqual(myError.toString(), initialToString); + console.log(myError); + assert.strictEqual(subsequentConsoleLog, initialConsoleLog); + + common.restoreStdout(); + } + + test('name'); + test('message'); +} From d4436a06fd41c6490e5148c948743e7aebc41e0f Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Sun, 1 Oct 2017 16:14:45 -0700 Subject: [PATCH 2/2] errors: make `code` and `name` properties settable For internal errors, make `code` and `name` settable while keeping them non-own properties by default. PR-URL: https://github.com/nodejs/node/pull/15694 Fixes: https://github.com/nodejs/node/issues/15658 Reviewed-By: Evan Lucas Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- lib/internal/errors.js | 33 +++++++++++++-- test/parallel/test-internal-errors.js | 58 +++++++++++++++++---------- 2 files changed, 66 insertions(+), 25 deletions(-) diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 535defff6975cf..2a3e8c8f19b6c6 100755 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -11,6 +11,8 @@ const kCode = Symbol('code'); const messages = new Map(); +const { defineProperty } = Object; + // Lazily loaded var util = null; @@ -18,11 +20,36 @@ function makeNodeError(Base) { return class NodeError extends Base { constructor(key, ...args) { super(message(key, args)); - this[kCode] = this.code = key; - Object.defineProperty(this, 'name', { + defineProperty(this, kCode, { configurable: true, enumerable: false, - value: `${super.name} [${this[kCode]}]`, + value: key, + writable: true + }); + } + + get name() { + return `${super.name} [${this[kCode]}]`; + } + + set name(value) { + defineProperty(this, 'name', { + configurable: true, + enumerable: true, + value, + writable: true + }); + } + + get code() { + return this[kCode]; + } + + set code(value) { + defineProperty(this, 'code', { + configurable: true, + enumerable: true, + value, writable: true }); } diff --git a/test/parallel/test-internal-errors.js b/test/parallel/test-internal-errors.js index 6bdadd8018f707..d8e2640f12b0e6 100644 --- a/test/parallel/test-internal-errors.js +++ b/test/parallel/test-internal-errors.js @@ -240,40 +240,54 @@ assert.throws( // Test that `code` property is mutable and that changing it does not change the // name. { - const myError = new errors.Error('ERR_MISSING_ARGS', ['name']); + const myError = new errors.Error('ERR_MISSING_ARGS', ['Sterrance']); assert.strictEqual(myError.code, 'ERR_MISSING_ARGS'); + assert.strictEqual(myError.hasOwnProperty('code'), false); + assert.strictEqual(myError.hasOwnProperty('name'), false); + assert.deepStrictEqual(Object.keys(myError), []); const initialName = myError.name; myError.code = 'FHQWHGADS'; assert.strictEqual(myError.code, 'FHQWHGADS'); assert.strictEqual(myError.name, initialName); + assert.deepStrictEqual(Object.keys(myError), ['code']); assert.ok(myError.name.includes('ERR_MISSING_ARGS')); assert.ok(!myError.name.includes('FHQWHGADS')); } -// Test that `name` and `message` are mutable and that changing them alters -// `toString()` but not `console.log()` results, which is the behavior of -// `Error` objects in the browser. +// Test that `name` is mutable and that changing it alters `toString()` but not +// `console.log()` results, which is the behavior of `Error` objects in the +// browser. Note that `name` becomes enumerable after being assigned. { - function test(prop) { - let initialConsoleLog = ''; - common.hijackStdout((data) => { initialConsoleLog += data; }); - const myError = new errors.Error('ERR_MISSING_ARGS', ['name']); - const initialToString = myError.toString(); - console.log(myError); - assert.notStrictEqual(initialConsoleLog, ''); + const myError = new errors.Error('ERR_MISSING_ARGS', ['Sterrance']); + assert.deepStrictEqual(Object.keys(myError), []); + const initialToString = myError.toString(); - common.restoreStdout(); + myError.name = 'Fhqwhgads'; + assert.deepStrictEqual(Object.keys(myError), ['name']); + assert.notStrictEqual(myError.toString(), initialToString); +} + +// Test that `message` is mutable and that changing it alters `toString()` but +// not `console.log()` results, which is the behavior of `Error` objects in the +// browser. Note that `message` remains non-enumerable after being assigned. +{ + let initialConsoleLog = ''; + common.hijackStdout((data) => { initialConsoleLog += data; }); + const myError = new errors.Error('ERR_MISSING_ARGS', ['Sterrance']); + assert.deepStrictEqual(Object.keys(myError), []); + const initialToString = myError.toString(); + console.log(myError); + assert.notStrictEqual(initialConsoleLog, ''); - let subsequentConsoleLog = ''; - common.hijackStdout((data) => { subsequentConsoleLog += data; }); - myError[prop] = 'Fhqwhgads'; - assert.notStrictEqual(myError.toString(), initialToString); - console.log(myError); - assert.strictEqual(subsequentConsoleLog, initialConsoleLog); + common.restoreStdout(); - common.restoreStdout(); - } + let subsequentConsoleLog = ''; + common.hijackStdout((data) => { subsequentConsoleLog += data; }); + myError.message = 'Fhqwhgads'; + assert.deepStrictEqual(Object.keys(myError), []); + assert.notStrictEqual(myError.toString(), initialToString); + console.log(myError); + assert.strictEqual(subsequentConsoleLog, initialConsoleLog); - test('name'); - test('message'); + common.restoreStdout(); }