From 2787e183145a94ffe4ee402f2c92d96427a58539 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 30 Aug 2022 19:04:15 +0200 Subject: [PATCH 1/4] Fix serialization of broken objects in the driver logs Trying to logging broken objects was causing the driver fails in unexpected moments. This broken behaviour makes the driver fail different when the `debug` log is enabled. --- packages/core/src/internal/util.ts | 41 ++++++++++++++-- packages/core/src/json.ts | 19 +++++-- .../core/test/__snapshots__/json.test.ts.snap | 7 +++ packages/core/test/internal/util.test.ts | 49 ++++++++++++++++++- packages/core/test/json.test.ts | 49 +++++++++++++++++++ 5 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 packages/core/test/__snapshots__/json.test.ts.snap create mode 100644 packages/core/test/json.test.ts diff --git a/packages/core/src/internal/util.ts b/packages/core/src/internal/util.ts index ecad93e7e..867f3abeb 100644 --- a/packages/core/src/internal/util.ts +++ b/packages/core/src/internal/util.ts @@ -24,7 +24,10 @@ import { stringify } from '../json' const ENCRYPTION_ON: EncryptionLevel = 'ENCRYPTION_ON' const ENCRYPTION_OFF: EncryptionLevel = 'ENCRYPTION_OFF' - +// eslint-disable-next-line @typescript-eslint/naming-convention +const __isBrokenObject__ = '__isBrokenObject__' +// eslint-disable-next-line @typescript-eslint/naming-convention +const __reason__ = '__reason__' /** * Verifies if the object is null or empty * @param obj The subject object @@ -236,7 +239,16 @@ function createBrokenObject (error: Error, object: any = {}): } return new Proxy(object, { - get: fail, + get: (_: T, p: string | Symbol): any => { + if (p === __isBrokenObject__) { + return true + } else if (p === __reason__) { + return error + } else if (p === 'toJSON') { + return undefined + } + fail() + }, set: fail, apply: fail, construct: fail, @@ -252,6 +264,27 @@ function createBrokenObject (error: Error, object: any = {}): }) } +/** + * Verifies if it is a Broken Object + * @param {any} object The object + * @returns {boolean} If it was created with createBrokenObject + */ +function isBrokenObject (object: any): boolean { + return object !== null && typeof object === 'object' && object[__isBrokenObject__] === true +} + +/** + * Returns if the reason the object is broken. + * + * This method should only be called with instances create with {@link createBrokenObject} + * + * @param {any} object The object + * @returns {Error} The reason the object is broken + */ +function getBrokenObjectReason (object: any): Error { + return object[__reason__] +} + export { isEmptyObjectOrNull, isObject, @@ -264,5 +297,7 @@ export { validateQueryAndParameters, ENCRYPTION_ON, ENCRYPTION_OFF, - createBrokenObject + createBrokenObject, + isBrokenObject, + getBrokenObjectReason } diff --git a/packages/core/src/json.ts b/packages/core/src/json.ts index a1eeebf91..67fd90c42 100644 --- a/packages/core/src/json.ts +++ b/packages/core/src/json.ts @@ -17,14 +17,25 @@ * limitations under the License. */ +import { isBrokenObject, getBrokenObjectReason } from './internal/util' + /** * Custom version on JSON.stringify that can handle values that normally don't support serialization, such as BigInt. * @private * @param val A JavaScript value, usually an object or array, to be converted. * @returns A JSON string representing the given value. */ -export function stringify (val: any) { - return JSON.stringify(val, (_, value) => - typeof value === 'bigint' ? `${value}n` : value - ) +export function stringify (val: any): string { + return JSON.stringify(val, (_, value) => { + if (isBrokenObject(value)) { + return { + __isBrokenObject__: true, + __reason__: getBrokenObjectReason(value) + } + } + if (typeof value === 'bigint') { + return `${value}n` + } + return value + }) } diff --git a/packages/core/test/__snapshots__/json.test.ts.snap b/packages/core/test/__snapshots__/json.test.ts.snap new file mode 100644 index 000000000..fec2a2ba5 --- /dev/null +++ b/packages/core/test/__snapshots__/json.test.ts.snap @@ -0,0 +1,7 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`json .stringify should handle objects created with createBrokenObject 1`] = `"{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\"}}"`; + +exports[`json .stringify should handle objects created with createBrokenObject in list 1`] = `"[{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\"}}]"`; + +exports[`json .stringify should handle objects created with createBrokenObject inside other object 1`] = `"{\\"number\\":1,\\"broken\\":{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\"}}}"`; diff --git a/packages/core/test/internal/util.test.ts b/packages/core/test/internal/util.test.ts index df218ae20..b3c88df19 100644 --- a/packages/core/test/internal/util.test.ts +++ b/packages/core/test/internal/util.test.ts @@ -16,6 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ +import { newError } from '../../src' import Integer, { int } from '../../src/integer' import { isEmptyObjectOrNull, @@ -28,7 +29,10 @@ import { assertValidDate, validateQueryAndParameters, ENCRYPTION_ON, - ENCRYPTION_OFF + ENCRYPTION_OFF, + createBrokenObject, + isBrokenObject, + getBrokenObjectReason } from '../../src/internal/util' /* eslint-disable no-new-wrappers */ @@ -250,4 +254,47 @@ describe('Util', () => { expect(ENCRYPTION_ON).toBe('ENCRYPTION_ON')) test('should ENCRYPTION_OFF toBe "ENCRYPTION_OFF"', () => expect(ENCRYPTION_OFF).toBe('ENCRYPTION_OFF')) + + describe('isBrokenObject', () => { + it('should return true when object created with createBrokenObject', () => { + const object = createBrokenObject(newError('error'), {}) + + expect(isBrokenObject(object)).toBe(true) + }) + + it('should return false for regular objects', () => { + const object = {} + + expect(isBrokenObject(object)).toBe(false) + }) + + it('should return false for non-objects', () => { + expect(isBrokenObject(null)).toBe(false) + expect(isBrokenObject(undefined)).toBe(false) + expect(isBrokenObject(1)).toBe(false) + expect(isBrokenObject(() => {})).toBe(false) + expect(isBrokenObject('string')).toBe(false) + }) + }) + + describe('getBrokenObjectReason', () => { + it('should return the reason the object is broken', () => { + const reason = newError('error') + const object = createBrokenObject(reason, {}) + + expect(getBrokenObjectReason(object)).toBe(reason) + }) + }) + + describe('createBrokenObject', () => { + describe('toJSON', () => { + it('should return undefined', () => { + const reason = newError('error') + const object = createBrokenObject(reason, {}) + + // @ts-expect-error + expect(object.toJSON).toBeUndefined() + }) + }) + }) }) diff --git a/packages/core/test/json.test.ts b/packages/core/test/json.test.ts new file mode 100644 index 000000000..2c2f9239e --- /dev/null +++ b/packages/core/test/json.test.ts @@ -0,0 +1,49 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { json, newError } from '../src' +import { createBrokenObject } from '../src/internal/util' + +describe('json', () => { + describe('.stringify', () => { + it('should handle objects created with createBrokenObject', () => { + const reason = newError('some error') + const broken = createBrokenObject(reason, { }) + + expect(json.stringify(broken)).toMatchSnapshot() + }) + + it('should handle objects created with createBrokenObject in list', () => { + const reason = newError('some error') + const broken = createBrokenObject(reason, { }) + + expect(json.stringify([broken])).toMatchSnapshot() + }) + + it('should handle objects created with createBrokenObject inside other object', () => { + const reason = newError('some error') + const broken = createBrokenObject(reason, { }) + + expect(json.stringify({ + number: 1, + broken + })).toMatchSnapshot() + }) + }) +}) From 05ade645737fc4357dba0ed9a0527317fcbfccb7 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 2 Sep 2022 12:01:20 +0200 Subject: [PATCH 2/4] Remove circular dependency between `json` and `util` Extracting `object-util` resolve this circular dependency issue. --- .../src/packstream/packstream-v1.js | 4 +- packages/core/src/internal/index.ts | 4 +- packages/core/src/internal/object-util.ts | 87 +++++++++++++++++++ packages/core/src/internal/util.ts | 68 +-------------- packages/core/src/json.ts | 2 +- .../core/test/internal/object-util.test.ts | 67 ++++++++++++++ packages/core/test/internal/util.test.ts | 49 +---------- packages/core/test/json.test.ts | 2 +- 8 files changed, 163 insertions(+), 120 deletions(-) create mode 100644 packages/core/src/internal/object-util.ts create mode 100644 packages/core/test/internal/object-util.test.ts diff --git a/packages/bolt-connection/src/packstream/packstream-v1.js b/packages/bolt-connection/src/packstream/packstream-v1.js index 20073231e..25e0e6b04 100644 --- a/packages/bolt-connection/src/packstream/packstream-v1.js +++ b/packages/bolt-connection/src/packstream/packstream-v1.js @@ -32,7 +32,7 @@ import { internal } from 'neo4j-driver-core' -const { util } = internal +const { objectUtil } = internal const { PROTOCOL_ERROR } = error const TINY_STRING = 0x80 @@ -575,7 +575,7 @@ class Unpacker { return null } } catch (error) { - return util.createBrokenObject(error) + return objectUtil.createBrokenObject(error) } } diff --git a/packages/core/src/internal/index.ts b/packages/core/src/internal/index.ts index 9e3150329..db094a3b8 100644 --- a/packages/core/src/internal/index.ts +++ b/packages/core/src/internal/index.ts @@ -31,6 +31,7 @@ import * as urlUtil from './url-util' import * as serverAddress from './server-address' import * as resolver from './resolver' import * as retryStrategy from './retry-strategy' +import * as objectUtil from './object-util' export { util, @@ -46,5 +47,6 @@ export { urlUtil, serverAddress, resolver, - retryStrategy + retryStrategy, + objectUtil } diff --git a/packages/core/src/internal/object-util.ts b/packages/core/src/internal/object-util.ts new file mode 100644 index 000000000..77016038c --- /dev/null +++ b/packages/core/src/internal/object-util.ts @@ -0,0 +1,87 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +// eslint-disable-next-line @typescript-eslint/naming-convention +const __isBrokenObject__ = '__isBrokenObject__' +// eslint-disable-next-line @typescript-eslint/naming-convention +const __reason__ = '__reason__' + +/** + * Creates a object which all method call will throw the given error + * + * @param {Error} error The error + * @param {any} object The object. Default: {} + * @returns {any} A broken object + */ +function createBrokenObject (error: Error, object: any = {}): T { + const fail: () => T = () => { + throw error + } + + return new Proxy(object, { + get: (_: T, p: string | Symbol): any => { + if (p === __isBrokenObject__) { + return true + } else if (p === __reason__) { + return error + } else if (p === 'toJSON') { + return undefined + } + fail() + }, + set: fail, + apply: fail, + construct: fail, + defineProperty: fail, + deleteProperty: fail, + getOwnPropertyDescriptor: fail, + getPrototypeOf: fail, + has: fail, + isExtensible: fail, + ownKeys: fail, + preventExtensions: fail, + setPrototypeOf: fail + }) +} + +/** + * Verifies if it is a Broken Object + * @param {any} object The object + * @returns {boolean} If it was created with createBrokenObject + */ +function isBrokenObject (object: any): boolean { + return object !== null && typeof object === 'object' && object[__isBrokenObject__] === true +} + +/** + * Returns if the reason the object is broken. + * + * This method should only be called with instances create with {@link createBrokenObject} + * + * @param {any} object The object + * @returns {Error} The reason the object is broken + */ +function getBrokenObjectReason (object: any): Error { + return object[__reason__] +} + +export { + createBrokenObject, + isBrokenObject, + getBrokenObjectReason +} diff --git a/packages/core/src/internal/util.ts b/packages/core/src/internal/util.ts index 867f3abeb..3434884c9 100644 --- a/packages/core/src/internal/util.ts +++ b/packages/core/src/internal/util.ts @@ -24,10 +24,6 @@ import { stringify } from '../json' const ENCRYPTION_ON: EncryptionLevel = 'ENCRYPTION_ON' const ENCRYPTION_OFF: EncryptionLevel = 'ENCRYPTION_OFF' -// eslint-disable-next-line @typescript-eslint/naming-convention -const __isBrokenObject__ = '__isBrokenObject__' -// eslint-disable-next-line @typescript-eslint/naming-convention -const __reason__ = '__reason__' /** * Verifies if the object is null or empty * @param obj The subject object @@ -226,65 +222,6 @@ function isString(str: any): str is string { return Object.prototype.toString.call(str) === '[object String]' } -/** - * Creates a object which all method call will throw the given error - * - * @param {Error} error The error - * @param {any} object The object. Default: {} - * @returns {any} A broken object - */ -function createBrokenObject (error: Error, object: any = {}): T { - const fail = () => { - throw error - } - - return new Proxy(object, { - get: (_: T, p: string | Symbol): any => { - if (p === __isBrokenObject__) { - return true - } else if (p === __reason__) { - return error - } else if (p === 'toJSON') { - return undefined - } - fail() - }, - set: fail, - apply: fail, - construct: fail, - defineProperty: fail, - deleteProperty: fail, - getOwnPropertyDescriptor: fail, - getPrototypeOf: fail, - has: fail, - isExtensible: fail, - ownKeys: fail, - preventExtensions: fail, - setPrototypeOf: fail, - }) -} - -/** - * Verifies if it is a Broken Object - * @param {any} object The object - * @returns {boolean} If it was created with createBrokenObject - */ -function isBrokenObject (object: any): boolean { - return object !== null && typeof object === 'object' && object[__isBrokenObject__] === true -} - -/** - * Returns if the reason the object is broken. - * - * This method should only be called with instances create with {@link createBrokenObject} - * - * @param {any} object The object - * @returns {Error} The reason the object is broken - */ -function getBrokenObjectReason (object: any): Error { - return object[__reason__] -} - export { isEmptyObjectOrNull, isObject, @@ -296,8 +233,5 @@ export { assertValidDate, validateQueryAndParameters, ENCRYPTION_ON, - ENCRYPTION_OFF, - createBrokenObject, - isBrokenObject, - getBrokenObjectReason + ENCRYPTION_OFF } diff --git a/packages/core/src/json.ts b/packages/core/src/json.ts index 67fd90c42..247b9b9c9 100644 --- a/packages/core/src/json.ts +++ b/packages/core/src/json.ts @@ -17,7 +17,7 @@ * limitations under the License. */ -import { isBrokenObject, getBrokenObjectReason } from './internal/util' +import { isBrokenObject, getBrokenObjectReason } from './internal/object-util' /** * Custom version on JSON.stringify that can handle values that normally don't support serialization, such as BigInt. diff --git a/packages/core/test/internal/object-util.test.ts b/packages/core/test/internal/object-util.test.ts new file mode 100644 index 000000000..792fcecae --- /dev/null +++ b/packages/core/test/internal/object-util.test.ts @@ -0,0 +1,67 @@ +/** + * Copyright (c) "Neo4j" + * Neo4j Sweden AB [http://neo4j.com] + * + * This file is part of Neo4j. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { newError } from '../../src' +import { + createBrokenObject, + isBrokenObject, + getBrokenObjectReason +} from '../../src/internal/object-util' + +describe('isBrokenObject', () => { + it('should return true when object created with createBrokenObject', () => { + const object = createBrokenObject(newError('error'), {}) + + expect(isBrokenObject(object)).toBe(true) + }) + + it('should return false for regular objects', () => { + const object = {} + + expect(isBrokenObject(object)).toBe(false) + }) + + it('should return false for non-objects', () => { + expect(isBrokenObject(null)).toBe(false) + expect(isBrokenObject(undefined)).toBe(false) + expect(isBrokenObject(1)).toBe(false) + expect(isBrokenObject(() => {})).toBe(false) + expect(isBrokenObject('string')).toBe(false) + }) +}) + +describe('getBrokenObjectReason', () => { + it('should return the reason the object is broken', () => { + const reason = newError('error') + const object = createBrokenObject(reason, {}) + + expect(getBrokenObjectReason(object)).toBe(reason) + }) +}) + +describe('createBrokenObject', () => { + describe('toJSON', () => { + it('should return undefined', () => { + const reason = newError('error') + const object = createBrokenObject(reason, {}) + + // @ts-expect-error + expect(object.toJSON).toBeUndefined() + }) + }) +}) diff --git a/packages/core/test/internal/util.test.ts b/packages/core/test/internal/util.test.ts index b3c88df19..df218ae20 100644 --- a/packages/core/test/internal/util.test.ts +++ b/packages/core/test/internal/util.test.ts @@ -16,7 +16,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { newError } from '../../src' import Integer, { int } from '../../src/integer' import { isEmptyObjectOrNull, @@ -29,10 +28,7 @@ import { assertValidDate, validateQueryAndParameters, ENCRYPTION_ON, - ENCRYPTION_OFF, - createBrokenObject, - isBrokenObject, - getBrokenObjectReason + ENCRYPTION_OFF } from '../../src/internal/util' /* eslint-disable no-new-wrappers */ @@ -254,47 +250,4 @@ describe('Util', () => { expect(ENCRYPTION_ON).toBe('ENCRYPTION_ON')) test('should ENCRYPTION_OFF toBe "ENCRYPTION_OFF"', () => expect(ENCRYPTION_OFF).toBe('ENCRYPTION_OFF')) - - describe('isBrokenObject', () => { - it('should return true when object created with createBrokenObject', () => { - const object = createBrokenObject(newError('error'), {}) - - expect(isBrokenObject(object)).toBe(true) - }) - - it('should return false for regular objects', () => { - const object = {} - - expect(isBrokenObject(object)).toBe(false) - }) - - it('should return false for non-objects', () => { - expect(isBrokenObject(null)).toBe(false) - expect(isBrokenObject(undefined)).toBe(false) - expect(isBrokenObject(1)).toBe(false) - expect(isBrokenObject(() => {})).toBe(false) - expect(isBrokenObject('string')).toBe(false) - }) - }) - - describe('getBrokenObjectReason', () => { - it('should return the reason the object is broken', () => { - const reason = newError('error') - const object = createBrokenObject(reason, {}) - - expect(getBrokenObjectReason(object)).toBe(reason) - }) - }) - - describe('createBrokenObject', () => { - describe('toJSON', () => { - it('should return undefined', () => { - const reason = newError('error') - const object = createBrokenObject(reason, {}) - - // @ts-expect-error - expect(object.toJSON).toBeUndefined() - }) - }) - }) }) diff --git a/packages/core/test/json.test.ts b/packages/core/test/json.test.ts index 2c2f9239e..aba214df4 100644 --- a/packages/core/test/json.test.ts +++ b/packages/core/test/json.test.ts @@ -18,7 +18,7 @@ */ import { json, newError } from '../src' -import { createBrokenObject } from '../src/internal/util' +import { createBrokenObject } from '../src/internal/object-util' describe('json', () => { describe('.stringify', () => { From 492b2d44e66980708144a49b3e23fd425e52643f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Mon, 5 Sep 2022 15:03:52 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Robsdedude --- packages/core/src/internal/object-util.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/internal/object-util.ts b/packages/core/src/internal/object-util.ts index 77016038c..f4c21cfb9 100644 --- a/packages/core/src/internal/object-util.ts +++ b/packages/core/src/internal/object-util.ts @@ -22,7 +22,7 @@ const __isBrokenObject__ = '__isBrokenObject__' const __reason__ = '__reason__' /** - * Creates a object which all method call will throw the given error + * Creates a object on which all method calls will throw the given error * * @param {Error} error The error * @param {any} object The object. Default: {} From 299d987336a27693c6e77fd8a21ab1ee00eff23d Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 5 Sep 2022 15:09:20 +0200 Subject: [PATCH 4/4] Add tests for BigInt serialization --- .../core/test/__snapshots__/json.test.ts.snap | 6 ++++++ packages/core/test/json.test.ts | 20 +++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/packages/core/test/__snapshots__/json.test.ts.snap b/packages/core/test/__snapshots__/json.test.ts.snap index fec2a2ba5..40919422a 100644 --- a/packages/core/test/__snapshots__/json.test.ts.snap +++ b/packages/core/test/__snapshots__/json.test.ts.snap @@ -1,5 +1,11 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`json .stringify should handle BigInt 1`] = `"\\"42n\\""`; + +exports[`json .stringify should handle BigInt in a list 1`] = `"[\\"42n\\",\\"-24n\\"]"`; + +exports[`json .stringify should handle BigInt in a object 1`] = `"{\\"theResponse\\":\\"42n\\"}"`; + exports[`json .stringify should handle objects created with createBrokenObject 1`] = `"{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\"}}"`; exports[`json .stringify should handle objects created with createBrokenObject in list 1`] = `"[{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\"}}]"`; diff --git a/packages/core/test/json.test.ts b/packages/core/test/json.test.ts index aba214df4..305382902 100644 --- a/packages/core/test/json.test.ts +++ b/packages/core/test/json.test.ts @@ -45,5 +45,25 @@ describe('json', () => { broken })).toMatchSnapshot() }) + + it('should handle BigInt', () => { + const bigint = BigInt(42) + + expect(json.stringify(bigint)).toMatchSnapshot() + }) + + it('should handle BigInt in a list', () => { + const bigintList = [BigInt(42), BigInt(-24)] + + expect(json.stringify(bigintList)).toMatchSnapshot() + }) + + it('should handle BigInt in a object', () => { + const bigintInObject = { + theResponse: BigInt(42) + } + + expect(json.stringify(bigintInObject)).toMatchSnapshot() + }) }) })