From 1fb9470101f0feb5b6c4ded0cad95f5d09695820 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 29 Aug 2022 19:06:59 +0200 Subject: [PATCH 1/4] Fix RxSession in Testkit Backend Testkit environment variables without `TEST_` prefix are not forward by the Testkit to the scripts in the driver. So we need change the session type variable expected by the `backend.py` to be `TEST_SESSION_TYPE` and then remove the prefix before redirect to the backend. This PR also fix the `SessionRun` and `TransactionRun` to not initate the stream. --- packages/testkit-backend/src/feature/common.js | 1 - packages/testkit-backend/src/request-handlers-rx.js | 8 +++----- testkit/backend.py | 4 ++++ 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/testkit-backend/src/feature/common.js b/packages/testkit-backend/src/feature/common.js index 26d7e4e12..7e8e554bc 100644 --- a/packages/testkit-backend/src/feature/common.js +++ b/packages/testkit-backend/src/feature/common.js @@ -33,7 +33,6 @@ const features = [ 'Feature:API:ConnectionAcquisitionTimeout', 'Feature:API:Driver:GetServerInfo', 'Feature:API:Driver.VerifyConnectivity', - 'Feature:API:Result.Peek', 'Optimization:ImplicitDefaultArguments', 'Optimization:MinimalResets', ...SUPPORTED_TLS diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index 36a47d76d..91c61b105 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -76,8 +76,7 @@ export function SessionRun (context, data, wire) { return } - const it = toAsyncIterator(result) - result[Symbol.asyncIterator] = () => it + result[Symbol.asyncIterator] = () => toAsyncIterator(result) const id = context.addResult(result) @@ -125,8 +124,7 @@ export function TransactionRun (context, data, wire) { } const result = tx.tx.run(cypher, params) - const it = toAsyncIterator(result) - result[Symbol.asyncIterator] = () => it + result[Symbol.asyncIterator] = () => toAsyncIterator(result) const id = context.addResult(result) @@ -305,7 +303,7 @@ function toAsyncIterator (result) { return: async (value) => { state.finished = true state.summary = value - return { done: true, value: value } + return { done: true, value } } } } diff --git a/testkit/backend.py b/testkit/backend.py index 304f08a57..4acdce471 100644 --- a/testkit/backend.py +++ b/testkit/backend.py @@ -16,6 +16,10 @@ print("Testkit should test browser") os.environ["TEST_ENVIRONMENT"] = "REMOTE" + session_type = os.environ.get("TEST_SESSION_TYPE", None) + if session_type is not None: + os.environ["SESSION_TYPE"] = session_type + print("npm run start-testkit-backend") with open_proccess_in_driver_repo([ "npm", "run", "start-testkit-backend" From 67b418968716be6149bc27d6852346b5b7b066ab Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 30 Aug 2022 14:50:13 +0200 Subject: [PATCH 2/4] Fix TransactionRun and SessionRun in Rx backend Improves the RxSession.beginTransaction by make it failing in the correct observable. --- packages/neo4j-driver/src/result-rx.js | 30 ++++++++++++++++-- packages/neo4j-driver/src/session-rx.js | 14 +++++---- packages/neo4j-driver/test/index.test.js | 8 +++++ packages/testkit-backend/src/feature/async.js | 1 - .../testkit-backend/src/feature/common.js | 1 + .../src/request-handlers-rx.js | 31 +++++++++++++------ .../testkit-backend/src/skipped-tests/rx.js | 9 ++++++ 7 files changed, 76 insertions(+), 18 deletions(-) diff --git a/packages/neo4j-driver/src/result-rx.js b/packages/neo4j-driver/src/result-rx.js index b9f229da3..c1f2eab0b 100644 --- a/packages/neo4j-driver/src/result-rx.js +++ b/packages/neo4j-driver/src/result-rx.js @@ -35,8 +35,9 @@ export default class RxResult { * @constructor * @protected * @param {Observable} result - An observable of single Result instance to relay requests. + * @param {number} state - The streamming state */ - constructor (result) { + constructor (result, state) { const replayedResult = result.pipe(publishReplay(1), refCount()) this._result = replayedResult @@ -48,7 +49,7 @@ export default class RxResult { this._records = undefined this._controls = new StreamControl() this._summary = new ReplaySubject() - this._state = States.READY + this._state = state || States.READY } /** @@ -184,6 +185,31 @@ export default class RxResult { } } + /** + * Create a {@link Observable} for the current {@link RxResult} + * + * + * @package + * @experimental + * @since 5.0 + * @return {Observable} + */ + _toObservable () { + function wrap (result) { + return new Observable(observer => { + observer.next(result) + observer.complete() + }) + } + return new Observable(observer => { + this._result.subscribe({ + complete: () => observer.complete(), + next: result => observer.next(new RxResult(wrap(result)), this._state), + error: e => observer.error(e) + }) + }) + } + _setupRecordsStream (result) { if (this._records) { return this._records diff --git a/packages/neo4j-driver/src/session-rx.js b/packages/neo4j-driver/src/session-rx.js index 6acec5b34..ef9576202 100644 --- a/packages/neo4j-driver/src/session-rx.js +++ b/packages/neo4j-driver/src/session-rx.js @@ -205,12 +205,14 @@ export default class RxSession { return new Observable(observer => { try { - observer.next( - new RxTransaction( - this._session._beginTransaction(accessMode, txConfig) - ) - ) - observer.complete() + this._session._beginTransaction(accessMode, txConfig) + .then(tx => { + observer.next( + new RxTransaction(tx) + ) + observer.complete() + }) + .catch(err => observer.error(err)) } catch (err) { observer.error(err) } diff --git a/packages/neo4j-driver/test/index.test.js b/packages/neo4j-driver/test/index.test.js index bb2ee567f..2fb6115c8 100644 --- a/packages/neo4j-driver/test/index.test.js +++ b/packages/neo4j-driver/test/index.test.js @@ -152,6 +152,10 @@ describe('#unit index', () => { function subject () { const driver = neo4j.driver('bolt://localhost') + driver._createSession = () => ({ + _mode: 'READ', + _beginTransaction: async () => new Transaction({}) + }) return driver.rxSession().beginTransaction().toPromise() } }) @@ -202,6 +206,10 @@ describe('#unit index', () => { async function subject () { const driver = neo4j.driver('bolt://localhost') + driver._createSession = () => ({ + _mode: 'READ', + _beginTransaction: async () => new Transaction({}) + }) const tx = await driver.rxSession().beginTransaction().toPromise() return InternalRxManagedTransaction.fromTransaction(tx) } diff --git a/packages/testkit-backend/src/feature/async.js b/packages/testkit-backend/src/feature/async.js index 3c637a19f..a9986ddd8 100644 --- a/packages/testkit-backend/src/feature/async.js +++ b/packages/testkit-backend/src/feature/async.js @@ -1,7 +1,6 @@ const features = [ 'Feature:API:Result.List', 'Feature:API:Result.Peek', - 'Optimization:EagerTransactionBegin', 'Optimization:PullPipelining' ] diff --git a/packages/testkit-backend/src/feature/common.js b/packages/testkit-backend/src/feature/common.js index 7e8e554bc..4cddbf559 100644 --- a/packages/testkit-backend/src/feature/common.js +++ b/packages/testkit-backend/src/feature/common.js @@ -33,6 +33,7 @@ const features = [ 'Feature:API:ConnectionAcquisitionTimeout', 'Feature:API:Driver:GetServerInfo', 'Feature:API:Driver.VerifyConnectivity', + 'Optimization:EagerTransactionBegin', 'Optimization:ImplicitDefaultArguments', 'Optimization:MinimalResets', ...SUPPORTED_TLS diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index 91c61b105..db3305c1d 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -67,20 +67,27 @@ export function SessionRun (context, data, wire) { } } - let result + let rxResult try { - result = session.run(cypher, params, { metadata, timeout }) + rxResult = session.run(cypher, params, { metadata, timeout }) } catch (e) { console.log('got some err: ' + JSON.stringify(e)) wire.writeError(e) return } - result[Symbol.asyncIterator] = () => toAsyncIterator(result) + rxResult + ._toObservable() + .subscribe({ + error: e => wire.writeError(e), + next: result => { + result[Symbol.asyncIterator] = () => toAsyncIterator(result) - const id = context.addResult(result) + const id = context.addResult(result) - wire.writeResponse(responses.Result({ id })) + wire.writeResponse(responses.Result({ id })) + } + }) } export function ResultConsume (context, data, wire) { @@ -122,13 +129,19 @@ export function TransactionRun (context, data, wire) { params[key] = cypherToNative(value) } } - const result = tx.tx.run(cypher, params) - result[Symbol.asyncIterator] = () => toAsyncIterator(result) + tx.tx.run(cypher, params) + ._toObservable() + .subscribe({ + error: e => wire.writeError(e), + next: result => { + result[Symbol.asyncIterator] = () => toAsyncIterator(result) - const id = context.addResult(result) + const id = context.addResult(result) - wire.writeResponse(responses.Result({ id })) + wire.writeResponse(responses.Result({ id })) + } + }) } export function TransactionRollback (context, data, wire) { diff --git a/packages/testkit-backend/src/skipped-tests/rx.js b/packages/testkit-backend/src/skipped-tests/rx.js index 0d0be9514..a1c2931ab 100644 --- a/packages/testkit-backend/src/skipped-tests/rx.js +++ b/packages/testkit-backend/src/skipped-tests/rx.js @@ -4,6 +4,15 @@ const skippedTests = [ skip( 'Throws after run insted of the first next because of the backend implementation', ifEquals('stub.disconnects.test_disconnects.TestDisconnects.test_disconnect_on_tx_begin') + ), + skip( + 'Backend could not support this test', + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_zoned_date_time'), + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_zoned_date_time_patched'), + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_then_known_zoned_date_time'), + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_then_known_zoned_date_time_patched'), + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV5x0.test_unknown_then_known_zoned_date_time'), + ifEquals('stub.types.test_temporal_types.TestTemporalTypesV5x0.test_unknown_zoned_date_time') ) ] From 9d64e5321ce0a7f9498f7a59b5715420e800ae95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Tue, 30 Aug 2022 16:31:12 +0200 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Florent Biville <445792+fbiville@users.noreply.github.com> --- packages/neo4j-driver/src/result-rx.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/neo4j-driver/src/result-rx.js b/packages/neo4j-driver/src/result-rx.js index c1f2eab0b..f887dc5c2 100644 --- a/packages/neo4j-driver/src/result-rx.js +++ b/packages/neo4j-driver/src/result-rx.js @@ -35,7 +35,7 @@ export default class RxResult { * @constructor * @protected * @param {Observable} result - An observable of single Result instance to relay requests. - * @param {number} state - The streamming state + * @param {number} state - The streaming state */ constructor (result, state) { const replayedResult = result.pipe(publishReplay(1), refCount()) From 91c8084a9af97fa36f9fe226edf18167c0b4bfce Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 30 Aug 2022 19:04:15 +0200 Subject: [PATCH 4/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 | 17 +++++-- .../core/test/__snapshots__/json.test.ts.snap | 7 +++ packages/core/test/internal/util.test.ts | 49 ++++++++++++++++++- packages/core/test/json.test.ts | 49 +++++++++++++++++++ .../testkit-backend/src/skipped-tests/rx.js | 9 ---- 6 files changed, 156 insertions(+), 16 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 2b6b3e722..3b7ad1525 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 @@ -237,7 +240,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, @@ -253,6 +265,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, @@ -265,5 +298,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 1142d59c2..67fd90c42 100644 --- a/packages/core/src/json.ts +++ b/packages/core/src/json.ts @@ -17,6 +17,8 @@ * 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 @@ -24,7 +26,16 @@ * @returns A JSON string representing the given value. */ export function stringify (val: any): string { - return JSON.stringify(val, (_, value) => - typeof value === 'bigint' ? `${value}n` : value - ) + 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..2eb678f6e --- /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\\",\\"retriable\\":false}}"`; + +exports[`json .stringify should handle objects created with createBrokenObject in list 1`] = `"[{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\",\\"retriable\\":false}}]"`; + +exports[`json .stringify should handle objects created with createBrokenObject inside other object 1`] = `"{\\"number\\":1,\\"broken\\":{\\"__isBrokenObject__\\":true,\\"__reason__\\":{\\"code\\":\\"N/A\\",\\"name\\":\\"Neo4jError\\",\\"retriable\\":false}}}"`; 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() + }) + }) +}) diff --git a/packages/testkit-backend/src/skipped-tests/rx.js b/packages/testkit-backend/src/skipped-tests/rx.js index a1c2931ab..0d0be9514 100644 --- a/packages/testkit-backend/src/skipped-tests/rx.js +++ b/packages/testkit-backend/src/skipped-tests/rx.js @@ -4,15 +4,6 @@ const skippedTests = [ skip( 'Throws after run insted of the first next because of the backend implementation', ifEquals('stub.disconnects.test_disconnects.TestDisconnects.test_disconnect_on_tx_begin') - ), - skip( - 'Backend could not support this test', - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_zoned_date_time'), - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_zoned_date_time_patched'), - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_then_known_zoned_date_time'), - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV4x4.test_unknown_then_known_zoned_date_time_patched'), - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV5x0.test_unknown_then_known_zoned_date_time'), - ifEquals('stub.types.test_temporal_types.TestTemporalTypesV5x0.test_unknown_zoned_date_time') ) ]