From 692a7cc1855191909e86e3bd56413a6503aad4da Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 23 Apr 2021 19:39:35 +0200 Subject: [PATCH 1/5] feat: make circular input errors for EJSON expressive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Give errors roughly in the style of those thrown by `JSON.stringify()` on modern Node.js/V8 versions, where the path to the property and its circularity are visualized instead of just recursive indefinitely. This is just one suggested solution – it would be nice to have *some* kind of better error in these cases, and I think actually displaying the path would be nice in terms of UX, but I can also see an argument for avoiding the extra bits of complexity here. NODE-3226 --- src/extended_json.ts | 54 +++++++++++++++++++++++++++----- test/node/extended_json_tests.js | 15 +++++++++ 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/src/extended_json.ts b/src/extended_json.ts index a4d22444..6bdcf76e 100644 --- a/src/extended_json.ts +++ b/src/extended_json.ts @@ -140,9 +140,20 @@ function deserializeValue(value: any, options: EJSON.Options = {}) { return value; } +type EJSONSerializeOptions = EJSON.Options & { + seenObjects: { obj: unknown; propertyName: string }[]; +}; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -function serializeArray(array: any[], options: EJSON.Options): any[] { - return array.map((v: unknown) => serializeValue(v, options)); +function serializeArray(array: any[], options: EJSONSerializeOptions): any[] { + return array.map((v: unknown, index: number) => { + options.seenObjects.push({ propertyName: `index ${index}`, obj: null }); + try { + return serializeValue(v, options); + } finally { + options.seenObjects.pop(); + } + }); } function getISOString(date: Date) { @@ -152,7 +163,29 @@ function getISOString(date: Date) { } // eslint-disable-next-line @typescript-eslint/no-explicit-any -function serializeValue(value: any, options: EJSON.Options): any { +function serializeValue(value: any, options: EJSONSerializeOptions): any { + if ((typeof value === 'object' || typeof value === 'function') && value !== null) { + const index = options.seenObjects.findIndex(entry => entry.obj === value); + if (index !== -1) { + const props = options.seenObjects.map(entry => entry.propertyName); + const leadingPart = `${props.slice(0, index).join(' -> ')} -> `; + const alreadySeen = props[index]; + const circularPart = ` -> ${props.slice(index + 1, props.length - 1).join(' -> ')} -> `; + const current = props[props.length - 1]; + const leadingSpace = ' '.repeat(leadingPart.length + alreadySeen.length / 2); + const dashes = '-'.repeat( + circularPart.length + (alreadySeen.length + current.length) / 2 - 1 + ); + + throw new TypeError( + 'Converting circular structure to EJSON:\n' + + ` ${leadingPart}${alreadySeen}${circularPart}${current}\n` + + ` ${leadingSpace}\\${dashes}/` + ); + } + options.seenObjects[options.seenObjects.length - 1].obj = value; + } + if (Array.isArray(value)) return serializeArray(value, options); if (value === undefined) return null; @@ -232,7 +265,7 @@ const BSON_TYPE_MAPPINGS = { } as const; // eslint-disable-next-line @typescript-eslint/no-explicit-any -function serializeDocument(doc: any, options: EJSON.Options) { +function serializeDocument(doc: any, options: EJSONSerializeOptions) { if (doc == null || typeof doc !== 'object') throw new Error('not an object instance'); const bsontype: BSONType['_bsontype'] = doc._bsontype; @@ -240,7 +273,12 @@ function serializeDocument(doc: any, options: EJSON.Options) { // It's a regular object. Recursively serialize its property values. const _doc: Document = {}; for (const name in doc) { - _doc[name] = serializeValue(doc[name], options); + options.seenObjects.push({ propertyName: name, obj: null }); + try { + _doc[name] = serializeValue(doc[name], options); + } finally { + options.seenObjects.pop(); + } } return _doc; } else if (isBSONType(doc)) { @@ -365,9 +403,11 @@ export namespace EJSON { replacer = undefined; space = 0; } - options = Object.assign({}, { relaxed: true, legacy: false }, options); + const serializeOptions = Object.assign({ relaxed: true, legacy: false }, options, { + seenObjects: [{ propertyName: '(input)', obj: null }] + }); - const doc = serializeValue(value, options); + const doc = serializeValue(value, serializeOptions); return JSON.stringify(doc, replacer as Parameters[1], space); } diff --git a/test/node/extended_json_tests.js b/test/node/extended_json_tests.js index 8208956c..97b70237 100644 --- a/test/node/extended_json_tests.js +++ b/test/node/extended_json_tests.js @@ -495,6 +495,21 @@ describe('Extended JSON', function () { // expect(() => EJSON.serialize(badMap)).to.throw(); // uncomment when EJSON supports ES6 Map }); + it('should throw a helpful error message for input with circular references', function () { + const obj = { + some: { + property: { + array: [] + } + } + }; + obj.some.property.array.push(obj.some); + expect(() => EJSON.serialize(obj)).to.throw(`\ +Converting circular structure to EJSON: + (input) -> some -> property -> array -> index 0 + \\-----------------------------/`); + }); + context('when dealing with legacy extended json', function () { describe('.stringify', function () { context('when serializing binary', function () { From 4b5176543eabe1d00a46378e73fb423b38aa5537 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 May 2021 19:45:50 +0200 Subject: [PATCH 2/5] fixup! feat: make circular input errors for EJSON expressive --- src/extended_json.ts | 4 +-- test/node/extended_json_tests.js | 47 ++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/extended_json.ts b/src/extended_json.ts index 6bdcf76e..75cd3c79 100644 --- a/src/extended_json.ts +++ b/src/extended_json.ts @@ -168,9 +168,9 @@ function serializeValue(value: any, options: EJSONSerializeOptions): any { const index = options.seenObjects.findIndex(entry => entry.obj === value); if (index !== -1) { const props = options.seenObjects.map(entry => entry.propertyName); - const leadingPart = `${props.slice(0, index).join(' -> ')} -> `; + const leadingPart = props.slice(0, index).map(prop => `${prop} -> `).join(''); const alreadySeen = props[index]; - const circularPart = ` -> ${props.slice(index + 1, props.length - 1).join(' -> ')} -> `; + const circularPart = ' -> ' + props.slice(index + 1, props.length - 1).map(prop => `${prop} -> `).join(''); const current = props[props.length - 1]; const leadingSpace = ' '.repeat(leadingPart.length + alreadySeen.length / 2); const dashes = '-'.repeat( diff --git a/test/node/extended_json_tests.js b/test/node/extended_json_tests.js index 97b70237..b7780c67 100644 --- a/test/node/extended_json_tests.js +++ b/test/node/extended_json_tests.js @@ -495,19 +495,48 @@ describe('Extended JSON', function () { // expect(() => EJSON.serialize(badMap)).to.throw(); // uncomment when EJSON supports ES6 Map }); - it('should throw a helpful error message for input with circular references', function () { - const obj = { - some: { - property: { - array: [] + context('circular references', () => { + it('should throw a helpful error message for input with circular references', function () { + const obj = { + some: { + property: { + array: [] + } } - } - }; - obj.some.property.array.push(obj.some); - expect(() => EJSON.serialize(obj)).to.throw(`\ + }; + obj.some.property.array.push(obj.some); + expect(() => EJSON.serialize(obj)).to.throw(`\ Converting circular structure to EJSON: (input) -> some -> property -> array -> index 0 \\-----------------------------/`); + }); + + it('should throw a helpful error message for input with circular references, one-level nested', function () { + const obj = {}; + obj.obj = obj; + expect(() => EJSON.serialize(obj)).to.throw(`\ +Converting circular structure to EJSON: + (input) -> obj + \\--------/`); + }); + + it('should throw a helpful error message for input with circular references, one-level nested inside base object', function () { + const obj = {}; + obj.obj = obj; + expect(() => EJSON.serialize({ foo: obj })).to.throw(`\ +Converting circular structure to EJSON: + (input) -> foo -> obj + \\------/`); + }); + + it('should throw a helpful error message for input with circular references, pointing back to base object', function () { + const obj = { foo: {} }; + obj.foo.obj = obj; + expect(() => EJSON.serialize(obj)).to.throw(`\ +Converting circular structure to EJSON: + (input) -> foo -> obj + \\---------------/`); + }); }); context('when dealing with legacy extended json', function () { From 3bba7b0d23fee29dc15dbe8f3daa841ab1d380cc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 May 2021 21:14:42 +0200 Subject: [PATCH 3/5] fixup: review suggestion Co-authored-by: Neal Beeken --- src/extended_json.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/extended_json.ts b/src/extended_json.ts index 75cd3c79..03c6e422 100644 --- a/src/extended_json.ts +++ b/src/extended_json.ts @@ -404,7 +404,7 @@ export namespace EJSON { space = 0; } const serializeOptions = Object.assign({ relaxed: true, legacy: false }, options, { - seenObjects: [{ propertyName: '(input)', obj: null }] + seenObjects: [{ propertyName: '(root)', obj: null }] }); const doc = serializeValue(value, serializeOptions); From e9278ebf18b0d6aa5accd0432425857f0ccb473d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 May 2021 21:16:47 +0200 Subject: [PATCH 4/5] fixup: test updates for review suggestion --- test/node/extended_json_tests.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/node/extended_json_tests.js b/test/node/extended_json_tests.js index b7780c67..79624c14 100644 --- a/test/node/extended_json_tests.js +++ b/test/node/extended_json_tests.js @@ -507,8 +507,8 @@ describe('Extended JSON', function () { obj.some.property.array.push(obj.some); expect(() => EJSON.serialize(obj)).to.throw(`\ Converting circular structure to EJSON: - (input) -> some -> property -> array -> index 0 - \\-----------------------------/`); + (root) -> some -> property -> array -> index 0 + \\-----------------------------/`); }); it('should throw a helpful error message for input with circular references, one-level nested', function () { @@ -516,8 +516,8 @@ Converting circular structure to EJSON: obj.obj = obj; expect(() => EJSON.serialize(obj)).to.throw(`\ Converting circular structure to EJSON: - (input) -> obj - \\--------/`); + (root) -> obj + \\-------/`); }); it('should throw a helpful error message for input with circular references, one-level nested inside base object', function () { @@ -525,8 +525,8 @@ Converting circular structure to EJSON: obj.obj = obj; expect(() => EJSON.serialize({ foo: obj })).to.throw(`\ Converting circular structure to EJSON: - (input) -> foo -> obj - \\------/`); + (root) -> foo -> obj + \\------/`); }); it('should throw a helpful error message for input with circular references, pointing back to base object', function () { @@ -534,8 +534,8 @@ Converting circular structure to EJSON: obj.foo.obj = obj; expect(() => EJSON.serialize(obj)).to.throw(`\ Converting circular structure to EJSON: - (input) -> foo -> obj - \\---------------/`); + (root) -> foo -> obj + \\--------------/`); }); }); From 89f431880242859683d26afd16ec70a6f476c486 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 7 May 2021 21:44:42 +0200 Subject: [PATCH 5/5] fixup: linter error --- src/extended_json.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/extended_json.ts b/src/extended_json.ts index 03c6e422..239b6caa 100644 --- a/src/extended_json.ts +++ b/src/extended_json.ts @@ -168,9 +168,17 @@ function serializeValue(value: any, options: EJSONSerializeOptions): any { const index = options.seenObjects.findIndex(entry => entry.obj === value); if (index !== -1) { const props = options.seenObjects.map(entry => entry.propertyName); - const leadingPart = props.slice(0, index).map(prop => `${prop} -> `).join(''); + const leadingPart = props + .slice(0, index) + .map(prop => `${prop} -> `) + .join(''); const alreadySeen = props[index]; - const circularPart = ' -> ' + props.slice(index + 1, props.length - 1).map(prop => `${prop} -> `).join(''); + const circularPart = + ' -> ' + + props + .slice(index + 1, props.length - 1) + .map(prop => `${prop} -> `) + .join(''); const current = props[props.length - 1]; const leadingSpace = ' '.repeat(leadingPart.length + alreadySeen.length / 2); const dashes = '-'.repeat(