From fe1fe4ad400a8a6a2162e5feb893226989095e57 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 1 Aug 2022 19:44:04 +0200 Subject: [PATCH 01/43] Introduce Bookmark Manager --- packages/core/src/bookmark-manager.ts | 114 ++++++++++++++++++ packages/core/src/driver.ts | 9 +- packages/core/src/index.ts | 11 +- packages/core/src/session.ts | 38 ++++-- packages/core/src/types.ts | 3 + packages/neo4j-driver-lite/src/index.ts | 15 ++- packages/neo4j-driver/src/index.js | 11 +- packages/neo4j-driver/types/index.d.ts | 14 ++- .../testkit-backend/src/feature/common.js | 1 + .../testkit-backend/src/request-handlers.js | 21 +++- 10 files changed, 208 insertions(+), 29 deletions(-) create mode 100644 packages/core/src/bookmark-manager.ts diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts new file mode 100644 index 000000000..679f6d37f --- /dev/null +++ b/packages/core/src/bookmark-manager.ts @@ -0,0 +1,114 @@ +/** + * 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. + */ + +export default interface BookmarkManager { + /** + * Method called when the bookmarks get update because of a given event + * + * @param database The database which the bookmarks belongs to + * @param previousBookmarks The bookmarks used during the session creation + * @param newBookmarks The new bookmarks resolved at the end of the session. + * @returns {void} + */ + updateBookmarks: (database: string, previousBookmarks: string[], newBookmarks: string[]) => void + + /** + * Method called by the driver to get the bookmark for one specific database + * + * @param database The database which the bookmarks belongs to + * @returns {string[]} The set of bookmarks + */ + getBookmarks: (database: string) => string[] + + /** + * Method called by the driver for getting all the bookmarks + * + * @param mustIncludedDatabases The database which must be included in the result even if they don't have be initialized yet. + * @returns {string[]} The set of bookmarks + */ + getAllBookmarks: (mustIncludedDatabases: string[]) => string[] +} + +export interface BookmarkManagerConfig { + initialBookmarks?: Map + bookmarkSupplier?: (database: string) => string[] + notifyBookmakrs?: (database: string, bookmarks: string[]) => void +} + +export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { + const initialBookmarks = new Map>() + + config.initialBookmarks?.forEach((v, k) => initialBookmarks.set(k, new Set(v))) + + return new Neo4jBookmarkManager( + initialBookmarks, + config.bookmarkSupplier, + config.notifyBookmakrs + ) +} + +class Neo4jBookmarkManager implements BookmarkManager { + constructor ( + private readonly _bookmarksPerDb: Map>, + private readonly _bookmarkSupplier?: (database: string) => string[], + private readonly _notifyBookmarks?: (database: string, bookmark: string[]) => void + ) { + + } + + updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + const bookmarks = this._getOrInitializeBookmarks(database) + previousBookmarks.forEach(bm => bookmarks.delete(bm)) + newBookmarks.forEach(bm => bookmarks.add(bm)) + + if (typeof this._notifyBookmarks === 'function') { + this._notifyBookmarks(database, [...bookmarks]) + } + } + + private _getOrInitializeBookmarks (database: string): Set { + let maybeBookmarks = this._bookmarksPerDb.get(database) + if (maybeBookmarks == null) { + maybeBookmarks = new Set() + this._bookmarksPerDb.set(database, maybeBookmarks) + } + return maybeBookmarks + } + + getBookmarks (database: string): string[] { + const bookmarks = this._bookmarksPerDb.get(database) ?? [] + + if (typeof this._bookmarkSupplier === 'function') { + return [...bookmarks, ...this._bookmarkSupplier(database)] + } + + return [...bookmarks] + } + + getAllBookmarks (mustIncludedDatabases: string[]): string[] { + const bookmarks = [] + const databases = new Set([...this._bookmarksPerDb.keys(), ...mustIncludedDatabases]) + + for (const database of databases) { + bookmarks.push(...this.getBookmarks(database)) + } + + return bookmarks + } +} diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 241115e8d..ec60e357f 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -40,6 +40,7 @@ import { SessionMode } from './types' import { ServerAddress } from './internal/server-address' +import BookmarkManager from './bookmark-manager' const DEFAULT_MAX_CONNECTION_LIFETIME: number = 60 * 60 * 1000 // 1 hour @@ -87,6 +88,7 @@ type CreateSession = (args: { reactive: boolean fetchSize: number impersonatedUser?: string + bookmarkManager?: BookmarkManager }) => Session interface DriverConfig { @@ -94,6 +96,7 @@ interface DriverConfig { trust?: TrustStrategy fetchSize?: number logging?: LoggingConfig + bookmarkManager?: BookmarkManager } /** @@ -362,6 +365,9 @@ class Driver { const bookmarks = bookmarkOrBookmarks != null ? new Bookmarks(bookmarkOrBookmarks) : Bookmarks.empty() + const bookmarkManager = bookmarkOrBookmarks == null + ? this._config.bookmarkManager + : undefined return this._createSession({ mode: sessionMode, database: database ?? '', @@ -370,7 +376,8 @@ class Driver { config: this._config, reactive, impersonatedUser, - fetchSize + fetchSize, + bookmarkManager }) } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 47cf53a9b..89d6d6a6d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -75,6 +75,7 @@ import TransactionPromise from './transaction-promise' import Session, { TransactionConfig } from './session' import Driver, * as driver from './driver' import auth from './auth' +import BookmarkManager, { BookmarkManagerConfig, bookmarkManager } from './bookmark-manager' import * as types from './types' import * as json from './json' import * as internal from './internal' // todo: removed afterwards @@ -146,7 +147,8 @@ const forExport = { types, driver, json, - auth + auth, + bookmarkManager } export { @@ -205,7 +207,8 @@ export { types, driver, json, - auth + auth, + bookmarkManager } export type { @@ -214,7 +217,9 @@ export type { NotificationPosition, QueryResult, ResultObserver, - TransactionConfig + TransactionConfig, + BookmarkManager, + BookmarkManagerConfig } export default forExport diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index f785c120a..6fa3dc8ef 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -35,6 +35,7 @@ import Connection from './connection' import { NumberOrInteger } from './graph-types' import TransactionPromise from './transaction-promise' import ManagedTransaction from './transaction-managed' +import BookmarkManager from './bookmark-manager' type ConnectionConsumer = (connection: Connection | null) => any | undefined type TransactionWork = (tx: Transaction) => Promise | T @@ -64,11 +65,11 @@ class Session { private _lastBookmarks: Bookmarks private readonly _transactionExecutor: TransactionExecutor private readonly _impersonatedUser?: string - private readonly _onComplete: (meta: any) => void private _databaseNameResolved: boolean private readonly _lowRecordWatermark: number private readonly _highRecordWatermark: number private readonly _results: Result[] + private readonly _bookmarkManager?: BookmarkManager /** * @constructor * @protected @@ -90,7 +91,8 @@ class Session { config, reactive, fetchSize, - impersonatedUser + impersonatedUser, + bookmarkManager }: { mode: SessionMode connectionProvider: ConnectionProvider @@ -100,6 +102,7 @@ class Session { reactive: boolean fetchSize: number impersonatedUser?: string + bookmarkManager?: BookmarkManager }) { this._mode = mode this._database = database @@ -127,12 +130,12 @@ class Session { this._impersonatedUser = impersonatedUser this._lastBookmarks = bookmarks ?? Bookmarks.empty() this._transactionExecutor = _createTransactionExecutor(config) - this._onComplete = this._onCompleteCallback.bind(this) this._databaseNameResolved = this._database !== '' const calculatedWatermaks = this._calculateWatermaks() this._lowRecordWatermark = calculatedWatermaks.low this._highRecordWatermark = calculatedWatermaks.high this._results = [] + this._bookmarkManager = bookmarkManager } /** @@ -159,15 +162,17 @@ class Session { ? new TxConfig(transactionConfig) : TxConfig.empty() + const bookmarks = this._bookmarks() + const result = this._run(validatedQuery, params, connection => { this._assertSessionIsOpen() return (connection as Connection).protocol().run(validatedQuery, params, { - bookmarks: this._lastBookmarks, + bookmarks, txConfig: autoCommitTxConfig, mode: this._mode, database: this._database, impersonatedUser: this._impersonatedUser, - afterComplete: this._onComplete, + afterComplete: (meta: any) => this._onCompleteCallback(meta, bookmarks), reactive: this._reactive, fetchSize: this._fetchSize, lowRecordWatermark: this._lowRecordWatermark, @@ -278,19 +283,20 @@ class Session { const connectionHolder = this._connectionHolderWithMode(mode) connectionHolder.initializeConnection() this._hasTx = true + const bookmarks = this._bookmarks() const tx = new TransactionPromise({ connectionHolder, impersonatedUser: this._impersonatedUser, onClose: this._transactionClosed.bind(this), - onBookmarks: this._updateBookmarks.bind(this), + onBookmarks: (newBookmarks) => this._updateBookmarks(newBookmarks, bookmarks), onConnection: this._assertSessionIsOpen.bind(this), reactive: this._reactive, fetchSize: this._fetchSize, lowRecordWatermark: this._lowRecordWatermark, highRecordWatermark: this._highRecordWatermark }) - tx._begin(this._lastBookmarks, txConfig) + tx._begin(bookmarks, txConfig) return tx } @@ -332,6 +338,13 @@ class Session { return this._lastBookmarks.values() } + private _bookmarks (): Bookmarks { + if (this._bookmarkManager != null) { + return new Bookmarks(this._bookmarkManager.getAllBookmarks([this._database, 'system'])) + } + return this._lastBookmarks + } + /** * Execute given unit of work in a {@link READ} transaction. * @@ -476,7 +489,12 @@ class Session { * @param {Bookmarks} newBookmarks - The new bookmarks. * @returns {void} */ - _updateBookmarks (newBookmarks?: Bookmarks): void { + _updateBookmarks (newBookmarks?: Bookmarks, previousBookmarks?: Bookmarks, database?: string): void { + this._bookmarkManager?.updateBookmarks( + database ?? this._database, + previousBookmarks?.values() ?? [], + newBookmarks?.values() ?? [] + ) if ((newBookmarks != null) && !newBookmarks.isEmpty()) { this._lastBookmarks = newBookmarks } @@ -514,8 +532,8 @@ class Session { * @param {Object} meta Connection metadatada * @returns {void} */ - _onCompleteCallback (meta: { bookmark: string | string[] }): void { - this._updateBookmarks(new Bookmarks(meta.bookmark)) + _onCompleteCallback (meta: { bookmark: string | string[], db?: string }, previousBookmarks?: Bookmarks): void { + this._updateBookmarks(new Bookmarks(meta.bookmark), previousBookmarks, meta.db) } /** diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 63140cc16..99bf36d32 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -17,6 +17,8 @@ * limitations under the License. */ +import BookmarkManager from './bookmark-manager' + /** * @private */ @@ -64,6 +66,7 @@ export interface Config { logging?: LoggingConfig resolver?: (address: string) => string[] | Promise userAgent?: string + bookmarkManager?: BookmarkManager } /** diff --git a/packages/neo4j-driver-lite/src/index.ts b/packages/neo4j-driver-lite/src/index.ts index 3456b190c..7f411af88 100644 --- a/packages/neo4j-driver-lite/src/index.ts +++ b/packages/neo4j-driver-lite/src/index.ts @@ -69,7 +69,10 @@ import { Connection, driver as coreDriver, types as coreTypes, - auth + auth, + BookmarkManager, + bookmarkManager, + BookmarkManagerConfig } from 'neo4j-driver-core' import { DirectConnectionProvider, @@ -460,7 +463,8 @@ const forExport = { LocalDateTime, DateTime, ConnectionProvider, - Connection + Connection, + bookmarkManager } export { @@ -512,7 +516,8 @@ export { LocalDateTime, DateTime, ConnectionProvider, - Connection + Connection, + bookmarkManager } export type { QueryResult, @@ -522,6 +527,8 @@ export type { TrustStrategy, SessionMode, ResultObserver, - NotificationPosition + NotificationPosition, + BookmarkManager, + BookmarkManagerConfig } export default forExport diff --git a/packages/neo4j-driver/src/index.js b/packages/neo4j-driver/src/index.js index d706e5add..101e5ab6e 100644 --- a/packages/neo4j-driver/src/index.js +++ b/packages/neo4j-driver/src/index.js @@ -60,7 +60,8 @@ import { auth, Session, Transaction, - ManagedTransaction + ManagedTransaction, + bookmarkManager } from 'neo4j-driver-core' import { DirectConnectionProvider, @@ -335,7 +336,7 @@ const USER_AGENT = 'neo4j-javascript/' + VERSION const logging = { console: level => { return { - level: level, + level, logger: (level, message) => console.log(`${global.Date.now()} ${level.toUpperCase()} ${message}`) } @@ -453,7 +454,8 @@ const forExport = { Time, Date, LocalDateTime, - DateTime + DateTime, + bookmarkManager } export { @@ -506,6 +508,7 @@ export { Time, Date, LocalDateTime, - DateTime + DateTime, + bookmarkManager } export default forExport diff --git a/packages/neo4j-driver/types/index.d.ts b/packages/neo4j-driver/types/index.d.ts index 89937a80e..755038a34 100644 --- a/packages/neo4j-driver/types/index.d.ts +++ b/packages/neo4j-driver/types/index.d.ts @@ -60,7 +60,10 @@ import { Transaction, ManagedTransaction, Session, - ConnectionProvider + ConnectionProvider, + BookmarkManager, + bookmarkManager, + BookmarkManagerConfig } from 'neo4j-driver-core' import { AuthToken, @@ -220,6 +223,7 @@ declare const forExport: { isDate: typeof isDate isLocalDateTime: typeof isLocalDateTime isDateTime: typeof isDateTime + bookmarkManager: typeof bookmarkManager } export { @@ -280,7 +284,13 @@ export { isTime, isDate, isLocalDateTime, - isDateTime + isDateTime, + bookmarkManager +} + +export type { + BookmarkManager, + BookmarkManagerConfig } export default forExport diff --git a/packages/testkit-backend/src/feature/common.js b/packages/testkit-backend/src/feature/common.js index 4cddbf559..7315b053c 100644 --- a/packages/testkit-backend/src/feature/common.js +++ b/packages/testkit-backend/src/feature/common.js @@ -17,6 +17,7 @@ const features = [ 'Feature:Auth:Custom', 'Feature:Auth:Kerberos', 'Feature:Auth:Bearer', + 'Feature:API:BookmarkManager', 'Feature:API:SSLConfig', 'Feature:API:SSLSchemes', 'Feature:API:Type.Temporal', diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 9abc93dd8..6bea50d9c 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -43,10 +43,10 @@ export function NewDriver (context, data, wire) { } const resolver = resolverRegistered ? address => - new Promise((resolve, reject) => { - const id = context.addResolverRequest(resolve, reject) - wire.writeResponse(responses.ResolverResolutionRequired({ id, address })) - }) + new Promise((resolve, reject) => { + const id = context.addResolverRequest(resolve, reject) + wire.writeResponse(responses.ResolverResolutionRequired({ id, address })) + }) : undefined const config = { userAgent, @@ -84,6 +84,17 @@ export function NewDriver (context, data, wire) { if ('maxTxRetryTimeMs' in data) { config.maxTransactionRetryTime = data.maxTxRetryTimeMs } + if ('bookmarkManager' in data) { + /// TODO: Implement supplier and notify + const bmmConfig = data.bookmarkManager + let initialBookmarks + if (bmmConfig.initialBookmarks != null) { + initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) + } + config.bookmarkManager = neo4j.bookmarkManager({ + initialBookmarks + }) + } let driver try { driver = neo4j.driver(uri, parsedAuthToken, config) @@ -439,7 +450,7 @@ export function ForcedRoutingTableUpdate (context, { driverId, database, bookmar return provider._freshRoutingTable({ accessMode: 'READ', database, - bookmarks: bookmarks, + bookmarks, onDatabaseNameResolved: () => {} }) .then(() => wire.writeResponse(responses.Driver({ id: driverId }))) From d6945b18fa5ae1366e1e5219b60e3ee6494f132a Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 2 Aug 2022 18:21:44 +0200 Subject: [PATCH 02/43] Support BMM during routing --- .../connection-provider-routing.js | 72 +++++++++++++++---- packages/core/src/connection-provider.ts | 1 + .../core/src/internal/connection-holder.ts | 22 +++++- packages/core/src/session.ts | 14 ++-- packages/core/src/transaction-promise.ts | 4 +- packages/core/src/transaction.ts | 21 +++--- 6 files changed, 98 insertions(+), 36 deletions(-) diff --git a/packages/bolt-connection/src/connection-provider/connection-provider-routing.js b/packages/bolt-connection/src/connection-provider/connection-provider-routing.js index c61f049b2..1527d85f9 100644 --- a/packages/bolt-connection/src/connection-provider/connection-provider-routing.js +++ b/packages/bolt-connection/src/connection-provider/connection-provider-routing.js @@ -130,7 +130,14 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider * See {@link ConnectionProvider} for more information about this method and * its arguments. */ - async acquireConnection ({ accessMode, database, bookmarks, impersonatedUser, onDatabaseNameResolved } = {}) { + async acquireConnection ({ + accessMode, + database, + bookmarks, + impersonatedUser, + onDatabaseNameResolved, + onRoutingTableRefresh + } = {}) { let name let address const context = { database: database || DEFAULT_DB_NAME } @@ -146,14 +153,15 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider const routingTable = await this._freshRoutingTable({ accessMode, database: context.database, - bookmarks: bookmarks, + bookmarks, impersonatedUser, onDatabaseNameResolved: (databaseName) => { context.database = context.database || databaseName if (onDatabaseNameResolved) { onDatabaseNameResolved(databaseName) } - } + }, + onRoutingTableRefresh }) // select a target server based on specified access mode @@ -301,7 +309,14 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider return this._connectionPool.acquire(address) } - _freshRoutingTable ({ accessMode, database, bookmarks, impersonatedUser, onDatabaseNameResolved } = {}) { + _freshRoutingTable ({ + accessMode, + database, + bookmarks, + impersonatedUser, + onDatabaseNameResolved, + onRoutingTableRefresh + } = {}) { const currentRoutingTable = this._routingTableRegistry.get( database, () => new RoutingTable({ database }) @@ -313,10 +328,22 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider this._log.info( `Routing table is stale for database: "${database}" and access mode: "${accessMode}": ${currentRoutingTable}` ) - return this._refreshRoutingTable(currentRoutingTable, bookmarks, impersonatedUser, onDatabaseNameResolved) + return this._refreshRoutingTable( + currentRoutingTable, + bookmarks, + impersonatedUser, + onDatabaseNameResolved, + onRoutingTableRefresh + ) } - _refreshRoutingTable (currentRoutingTable, bookmarks, impersonatedUser, onDatabaseNameResolved) { + _refreshRoutingTable ( + currentRoutingTable, + bookmarks, + impersonatedUser, + onDatabaseNameResolved, + onRoutingTableRefresh + ) { const knownRouters = currentRoutingTable.routers if (this._useSeedRouter) { @@ -325,7 +352,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved + onDatabaseNameResolved, + onRoutingTableRefresh ) } return this._fetchRoutingTableFromKnownRoutersFallbackToSeedRouter( @@ -333,7 +361,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved + onDatabaseNameResolved, + onRoutingTableRefresh ) } @@ -342,7 +371,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved + onDatabaseNameResolved, + onRoutingTableRefresh ) { // we start with seed router, no routers were probed before const seenRouters = [] @@ -372,7 +402,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, newRoutingTable, onDatabaseNameResolved, - error + error, + onRoutingTableRefresh ) } @@ -381,7 +412,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved + onDatabaseNameResolved, + onRoutingTableRefresh ) { let [newRoutingTable, error] = await this._fetchRoutingTableUsingKnownRouters( knownRouters, @@ -405,7 +437,8 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, newRoutingTable, onDatabaseNameResolved, - error + error, + onRoutingTableRefresh ) } @@ -563,7 +596,13 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider return [null, error] } - async _applyRoutingTableIfPossible (currentRoutingTable, newRoutingTable, onDatabaseNameResolved, error) { + async _applyRoutingTableIfPossible ( + currentRoutingTable, + newRoutingTable, + onDatabaseNameResolved, + error, + onRoutingTableRefresh + ) { if (!newRoutingTable) { // none of routing servers returned valid routing table, throw exception throw newError( @@ -579,12 +618,12 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider this._useSeedRouter = true } - await this._updateRoutingTable(newRoutingTable, onDatabaseNameResolved) + await this._updateRoutingTable(newRoutingTable, onDatabaseNameResolved, onRoutingTableRefresh) return newRoutingTable } - async _updateRoutingTable (newRoutingTable, onDatabaseNameResolved) { + async _updateRoutingTable (newRoutingTable, onDatabaseNameResolved, onRoutingTableRefresh) { // close old connections to servers not present in the new routing table await this._connectionPool.keepAll(newRoutingTable.allServers()) this._routingTableRegistry.removeExpired() @@ -592,6 +631,9 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider newRoutingTable ) + if (typeof onRoutingTableRefresh === 'function') { + onRoutingTableRefresh() + } onDatabaseNameResolved(newRoutingTable.database) this._log.info(`Updated routing table ${newRoutingTable}`) diff --git a/packages/core/src/connection-provider.ts b/packages/core/src/connection-provider.ts index b0775683c..8930d2086 100644 --- a/packages/core/src/connection-provider.ts +++ b/packages/core/src/connection-provider.ts @@ -50,6 +50,7 @@ class ConnectionProvider { bookmarks: bookmarks.Bookmarks impersonatedUser?: string onDatabaseNameResolved?: (databaseName?: string) => void + onRoutingTableRefresh?: () => void }): Promise { throw Error('Not implemented') } diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index 6b608fedf..2c1571831 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -24,6 +24,7 @@ import Connection from '../connection' import { ACCESS_MODE_WRITE } from './constants' import { Bookmarks } from './bookmarks' import ConnectionProvider from '../connection-provider' +import BookmarkManager from '../bookmark-manager' /** * @private @@ -83,6 +84,7 @@ class ConnectionHolder implements ConnectionHolderInterface { private _referenceCount: number private _connectionPromise: Promise private readonly _impersonatedUser?: string + private readonly _bookmarkManager?: BookmarkManager private readonly _onDatabaseNameResolved?: (databaseName?: string) => void /** @@ -101,13 +103,15 @@ class ConnectionHolder implements ConnectionHolderInterface { bookmarks, connectionProvider, impersonatedUser, - onDatabaseNameResolved + onDatabaseNameResolved, + bookmarkManager }: { mode?: string database?: string bookmarks?: Bookmarks connectionProvider?: ConnectionProvider impersonatedUser?: string + bookmarkManager?: BookmarkManager onDatabaseNameResolved?: (databaseName?: string) => void } = {}) { this._mode = mode @@ -118,6 +122,7 @@ class ConnectionHolder implements ConnectionHolderInterface { this._referenceCount = 0 this._connectionPromise = Promise.resolve(null) this._onDatabaseNameResolved = onDatabaseNameResolved + this._bookmarkManager = bookmarkManager } mode (): string | undefined { @@ -146,12 +151,16 @@ class ConnectionHolder implements ConnectionHolderInterface { initializeConnection (): boolean { if (this._referenceCount === 0 && (this._connectionProvider != null)) { + const bookmarks = this._getBookmarks() + this._connectionPromise = this._connectionProvider.acquireConnection({ accessMode: this._mode, database: this._database, - bookmarks: this._bookmarks, + bookmarks, impersonatedUser: this._impersonatedUser, - onDatabaseNameResolved: this._onDatabaseNameResolved + onDatabaseNameResolved: this._onDatabaseNameResolved, + onRoutingTableRefresh: + () => this._bookmarkManager?.updateBookmarks('system', bookmarks.values(), []) }) } else { this._referenceCount++ @@ -161,6 +170,13 @@ class ConnectionHolder implements ConnectionHolderInterface { return true } + private _getBookmarks (): Bookmarks { + if (this._bookmarkManager != null) { + return new Bookmarks(this._bookmarkManager.getBookmarks('system')) + } + return this._bookmarks + } + getConnection (): Promise { return this._connectionPromise } diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 6fa3dc8ef..3b82c79a3 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -115,7 +115,8 @@ class Session { bookmarks, connectionProvider, impersonatedUser, - onDatabaseNameResolved: this._onDatabaseNameResolved + onDatabaseNameResolved: this._onDatabaseNameResolved, + bookmarkManager }) this._writeConnectionHolder = new ConnectionHolder({ mode: ACCESS_MODE_WRITE, @@ -123,7 +124,8 @@ class Session { bookmarks, connectionProvider, impersonatedUser, - onDatabaseNameResolved: this._onDatabaseNameResolved + onDatabaseNameResolved: this._onDatabaseNameResolved, + bookmarkManager }) this._open = true this._hasTx = false @@ -162,9 +164,8 @@ class Session { ? new TxConfig(transactionConfig) : TxConfig.empty() - const bookmarks = this._bookmarks() - const result = this._run(validatedQuery, params, connection => { + const bookmarks = this._bookmarks() this._assertSessionIsOpen() return (connection as Connection).protocol().run(validatedQuery, params, { bookmarks, @@ -283,20 +284,19 @@ class Session { const connectionHolder = this._connectionHolderWithMode(mode) connectionHolder.initializeConnection() this._hasTx = true - const bookmarks = this._bookmarks() const tx = new TransactionPromise({ connectionHolder, impersonatedUser: this._impersonatedUser, onClose: this._transactionClosed.bind(this), - onBookmarks: (newBookmarks) => this._updateBookmarks(newBookmarks, bookmarks), + onBookmarks: (newBm, oldBm, db) => this._updateBookmarks(newBm, oldBm, db), onConnection: this._assertSessionIsOpen.bind(this), reactive: this._reactive, fetchSize: this._fetchSize, lowRecordWatermark: this._lowRecordWatermark, highRecordWatermark: this._highRecordWatermark }) - tx._begin(bookmarks, txConfig) + tx._begin(() => this._bookmarks(), txConfig) return tx } diff --git a/packages/core/src/transaction-promise.ts b/packages/core/src/transaction-promise.ts index 344755e82..ac3ae7526 100644 --- a/packages/core/src/transaction-promise.ts +++ b/packages/core/src/transaction-promise.ts @@ -69,7 +69,7 @@ class TransactionPromise extends Transaction implements Promise { }: { connectionHolder: ConnectionHolder onClose: () => void - onBookmarks: (bookmarks: Bookmarks) => void + onBookmarks: (newBookmarks: Bookmarks, previousBookmarks: Bookmarks, database?: string) => void onConnection: () => void reactive: boolean fetchSize: number @@ -162,7 +162,7 @@ class TransactionPromise extends Transaction implements Promise { /** * @access private */ - _begin (bookmarks: string | Bookmarks | string[], txConfig: TxConfig): void { + _begin (bookmarks: () => Bookmarks, txConfig: TxConfig): void { return super._begin(bookmarks, txConfig, { onError: this._onBeginError.bind(this), onComplete: this._onBeginMetadata.bind(this) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index d22876bf5..f2474b53e 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -48,7 +48,7 @@ class Transaction { private readonly _reactive: boolean private _state: any private readonly _onClose: () => void - private readonly _onBookmarks: (bookmarks: Bookmarks) => void + private readonly _onBookmarks: (newBookmarks: Bookmarks, previousBookmarks: Bookmarks, database?: string) => void private readonly _onConnection: () => void private readonly _onError: (error: Error) => Promise private readonly _onComplete: (metadata: any) => void @@ -57,6 +57,7 @@ class Transaction { private readonly _impersonatedUser?: string private readonly _lowRecordWatermak: number private readonly _highRecordWatermark: number + private _bookmarks: Bookmarks /** * @constructor @@ -84,7 +85,7 @@ class Transaction { }: { connectionHolder: ConnectionHolder onClose: () => void - onBookmarks: (bookmarks: Bookmarks) => void + onBookmarks: (newBookmarks: Bookmarks, previousBookmarks: Bookmarks, database?: string) => void onConnection: () => void reactive: boolean fetchSize: number @@ -99,12 +100,13 @@ class Transaction { this._onBookmarks = onBookmarks this._onConnection = onConnection this._onError = this._onErrorCallback.bind(this) - this._onComplete = this._onCompleteCallback.bind(this) this._fetchSize = fetchSize + this._onCompleteCallback = this._onCompleteCallback.bind(this) this._results = [] this._impersonatedUser = impersonatedUser this._lowRecordWatermak = lowRecordWatermark this._highRecordWatermark = highRecordWatermark + this._bookmarks = Bookmarks.empty() } /** @@ -113,17 +115,18 @@ class Transaction { * @param {TxConfig} txConfig * @returns {void} */ - _begin (bookmarks: Bookmarks | string | string[], txConfig: TxConfig, events?: { + _begin (getBookmarks: () => Bookmarks, txConfig: TxConfig, events?: { onError: (error: Error) => void onComplete: (metadata: any) => void }): void { this._connectionHolder .getConnection() .then(connection => { + this._bookmarks = getBookmarks() this._onConnection() if (connection != null) { return connection.protocol().beginTransaction({ - bookmarks: bookmarks, + bookmarks: this._bookmarks, txConfig: txConfig, mode: this._connectionHolder.mode(), database: this._connectionHolder.database(), @@ -138,7 +141,7 @@ class Transaction { if (events != null) { events.onComplete(metadata) } - return this._onComplete(metadata) + return this._onCompleteCallback(metadata, Bookmarks.empty()) } }) } else { @@ -192,7 +195,7 @@ class Transaction { const committed = this._state.commit({ connectionHolder: this._connectionHolder, onError: this._onError, - onComplete: this._onComplete, + onComplete: (meta: any) => this._onCompleteCallback(meta, this._bookmarks), onConnection: this._onConnection, pendingResults: this._results }) @@ -271,8 +274,8 @@ class Transaction { * @param {object} meta The meta with bookmarks * @returns {void} */ - _onCompleteCallback (meta: { bookmark?: string | string[] }): void { - this._onBookmarks(new Bookmarks(meta.bookmark)) + _onCompleteCallback (meta: { bookmark?: string | string[], db?: string }, previousBookmarks: Bookmarks): void { + this._onBookmarks(new Bookmarks(meta.bookmark), previousBookmarks, meta.db) } } From e4b1efe05cdd3d0d5590b75f350fab2c2d27373f Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 11:47:15 +0200 Subject: [PATCH 03/43] Adjusting tests --- packages/core/test/transaction.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/test/transaction.test.ts b/packages/core/test/transaction.test.ts index 9e61a5a3d..57b49730b 100644 --- a/packages/core/test/transaction.test.ts +++ b/packages/core/test/transaction.test.ts @@ -151,7 +151,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection }) - tx._begin(Bookmarks.empty(), TxConfig.empty()) + tx._begin(() => Bookmarks.empty(), TxConfig.empty()) return [tx] } @@ -266,7 +266,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection }) - tx._begin(Bookmarks.empty(), TxConfig.empty()) + tx._begin(() => Bookmarks.empty(), TxConfig.empty()) return [tx, expectedError] } }) @@ -280,7 +280,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection: undefined }) - tx._begin(Bookmarks.empty(), TxConfig.empty()) + tx._begin(() => Bookmarks.empty(), TxConfig.empty()) try { await tx @@ -300,7 +300,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { errorResolvingConnection: expectedError }) - tx._begin(Bookmarks.empty(), TxConfig.empty()) + tx._begin(() => Bookmarks.empty(), TxConfig.empty()) try { await tx From 7c7281db099a67dbdce0ef005fdd895dfd396f4a Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 12:34:42 +0200 Subject: [PATCH 04/43] Fixing driver --- packages/core/src/transaction.ts | 6 +++--- packages/testkit-backend/src/request-handlers.js | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index f2474b53e..74dc9dfba 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -101,7 +101,7 @@ class Transaction { this._onConnection = onConnection this._onError = this._onErrorCallback.bind(this) this._fetchSize = fetchSize - this._onCompleteCallback = this._onCompleteCallback.bind(this) + this._onComplete = this._onCompleteCallback.bind(this) this._results = [] this._impersonatedUser = impersonatedUser this._lowRecordWatermak = lowRecordWatermark @@ -141,7 +141,7 @@ class Transaction { if (events != null) { events.onComplete(metadata) } - return this._onCompleteCallback(metadata, Bookmarks.empty()) + return this._onComplete(metadata) } }) } else { @@ -275,7 +275,7 @@ class Transaction { * @returns {void} */ _onCompleteCallback (meta: { bookmark?: string | string[], db?: string }, previousBookmarks: Bookmarks): void { - this._onBookmarks(new Bookmarks(meta.bookmark), previousBookmarks, meta.db) + this._onBookmarks(new Bookmarks(meta.bookmark), previousBookmarks ?? Bookmarks.empty(), meta.db) } } diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 6bea50d9c..f9a74ef63 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -84,7 +84,7 @@ export function NewDriver (context, data, wire) { if ('maxTxRetryTimeMs' in data) { config.maxTransactionRetryTime = data.maxTxRetryTimeMs } - if ('bookmarkManager' in data) { + if ('bookmarkManager' in data && data.bookmarkManager != null) { /// TODO: Implement supplier and notify const bmmConfig = data.bookmarkManager let initialBookmarks From 7ff597cb51e9f3bc17ba85f1c3496c6b58757126 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 13:18:15 +0200 Subject: [PATCH 05/43] Add tests to bookmark-manager.ts --- packages/core/src/bookmark-manager.ts | 7 +- packages/core/test/bookmark-manager.test.ts | 194 ++++++++++++++++++++ 2 files changed, 198 insertions(+), 3 deletions(-) create mode 100644 packages/core/test/bookmark-manager.test.ts diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 679f6d37f..3e3571dfb 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -48,7 +48,7 @@ export default interface BookmarkManager { export interface BookmarkManagerConfig { initialBookmarks?: Map bookmarkSupplier?: (database: string) => string[] - notifyBookmakrs?: (database: string, bookmarks: string[]) => void + notifyBookmarks?: (database: string, bookmarks: string[]) => void } export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { @@ -59,7 +59,7 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa return new Neo4jBookmarkManager( initialBookmarks, config.bookmarkSupplier, - config.notifyBookmakrs + config.notifyBookmarks ) } @@ -95,7 +95,8 @@ class Neo4jBookmarkManager implements BookmarkManager { const bookmarks = this._bookmarksPerDb.get(database) ?? [] if (typeof this._bookmarkSupplier === 'function') { - return [...bookmarks, ...this._bookmarkSupplier(database)] + const suppliedBookmarks = this._bookmarkSupplier(database) ?? [] + return [...bookmarks, ...suppliedBookmarks] } return [...bookmarks] diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts new file mode 100644 index 000000000..a725b70a8 --- /dev/null +++ b/packages/core/test/bookmark-manager.test.ts @@ -0,0 +1,194 @@ +/** + * 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 { + bookmarkManager +} from '../src/bookmark-manager' + +describe('BookmarkManager', () => { + const systemBookmarks = ['system:bm01', 'system:bm02'] + const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm02'] + + describe('getBookmarks()', () => { + it('should return empty if db doesnt exists', () => { + const manager = bookmarkManager({}) + + const bookmarks = manager.getBookmarks('neo4j') + + expect(bookmarks).toEqual([]) + }) + + it('should return bookmarks for the given db', () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const bookmarks = manager.getBookmarks('neo4j') + + expect(bookmarks).toEqual(neo4jBookmarks) + }) + + it('should return get bookmarks from bookmarkSupplier', () => { + const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] + + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarkSupplier: () => extraBookmarks + }) + + const bookmarks = manager.getBookmarks('neo4j') + + expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) + }) + + it('should return call from bookmarkSupplier with correct database', () => { + const bookmarkSupplier = jest.fn() + + const manager = bookmarkManager({ + bookmarkSupplier + }) + + manager.getBookmarks('neo4j') + + expect(bookmarkSupplier).toBeCalledWith('neo4j') + }) + }) + + describe('getAllBookmarks()', () => { + it('should return all bookmarks ', () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + + expect(bookmarks).toEqual([...neo4jBookmarks, ...systemBookmarks]) + }) + + it('should return empty if there isnt bookmarks for any db', () => { + const manager = bookmarkManager({}) + + const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + + expect(bookmarks).toEqual([]) + }) + + it('should return enrich bookmarks list with supplied bookmarks', () => { + const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] + const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarkSupplier + }) + + const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + + expect(bookmarks.sort()).toEqual( + [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() + ) + }) + + it('should call bookmarkSupplier for each existing and listed databases ', () => { + const bookmarkSupplier = jest.fn() + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarkSupplier + }) + + manager.getAllBookmarks(['neo4j', 'adb']) + + expect(bookmarkSupplier).toBeCalledWith('neo4j') + expect(bookmarkSupplier).toBeCalledWith('adb') + expect(bookmarkSupplier).toBeCalledWith('system') + }) + }) + + describe('updateBookmarks()', () => { + it('should remove previous bookmarks and new bookmarks an existing db', () => { + const newBookmarks = ['neo4j:bm03'] + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + manager.updateBookmarks( + 'neo4j', + manager.getAllBookmarks(['neo4j', 'system']), + newBookmarks + ) + + expect(manager.getBookmarks('neo4j')).toEqual(newBookmarks) + expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + }) + + it('should add bookmarks to a non-existing database', () => { + const newBookmarks = ['neo4j:bm03'] + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['system', systemBookmarks] + ]) + }) + + manager.updateBookmarks( + 'neo4j', + [], + newBookmarks + ) + + expect(manager.getBookmarks('neo4j')).toEqual(newBookmarks) + expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + }) + + it('should notify new bookmarks', () => { + const notifyBookmarks = jest.fn() + const newBookmarks = ['neo4j:bm03'] + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + notifyBookmarks + }) + + manager.updateBookmarks( + 'neo4j', + manager.getAllBookmarks(['neo4j', 'system']), + newBookmarks + ) + + expect(notifyBookmarks).toBeCalledWith('neo4j', newBookmarks) + }) + }) +}) From 16d49cf8c03b1052c0fd6119d512db93eceb025d Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 13:35:29 +0200 Subject: [PATCH 06/43] Add tests to driver.ts --- packages/core/test/driver.test.ts | 53 ++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/core/test/driver.test.ts b/packages/core/test/driver.test.ts index c465167c8..0f848055f 100644 --- a/packages/core/test/driver.test.ts +++ b/packages/core/test/driver.test.ts @@ -17,7 +17,7 @@ * limitations under the License. */ /* eslint-disable @typescript-eslint/promise-function-async */ -import { ConnectionProvider, newError, ServerInfo, Session } from '../src' +import { bookmarkManager, ConnectionProvider, newError, ServerInfo, Session } from '../src' import Driver, { READ } from '../src/driver' import { Bookmarks } from '../src/internal/bookmarks' import { Logger } from '../src/internal/logger' @@ -83,6 +83,57 @@ describe('Driver', () => { expect(session?.lastBookmarks()).toEqual(expectedBookmarks.values()) }) + + describe('when bookmark manager configured', () => { + it('should create session with bookmark manager when no bookmark set', async () => { + const manager = bookmarkManager() + const driver = new Driver( + META_INFO, + { ...CONFIG, bookmarkManager: manager }, + mockCreateConnectonProvider(connectionProvider), + createSession + ) + + const session = driver.session() + + try { + expect(createSession).toBeCalledWith(expect.objectContaining({ + bookmarkManager: manager, + bookmarks: Bookmarks.empty() + })) + } finally { + await session.close() + await driver.close() + } + }) + + it.each([ + [[], Bookmarks.empty()], + ['bookmark', new Bookmarks('bookmark')], + [['bookmark'], new Bookmarks(['bookmark'])], + [['bookmark1', 'bookmark2'], new Bookmarks(['bookmark1', 'bookmark2'])] + ])('should create session without bookmark manager when bookmark set', async (bookmarks, expectedBookmarks) => { + const manager = bookmarkManager() + const driver = new Driver( + META_INFO, + { ...CONFIG, bookmarkManager: manager }, + mockCreateConnectonProvider(connectionProvider), + createSession + ) + + const session = driver.session({ bookmarks }) + + try { + expect(createSession).toBeCalledWith(expect.objectContaining({ + bookmarkManager: undefined, + bookmarks: expectedBookmarks + })) + } finally { + await session.close() + await driver.close() + } + }) + }) }) it.each([ From 309ea3470db0a14abf4cf46fc9bbefd71eed3525 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 15:38:03 +0200 Subject: [PATCH 07/43] Partially revert "Support BMM during routing" Removing update bm on acquiring connection This reverts commit fc8837e7431840a5b6d319962d0274725abb74ed. --- .../connection-provider-routing.js | 70 ++++--------------- packages/core/src/connection-provider.ts | 1 - .../core/src/internal/connection-holder.ts | 10 +-- 3 files changed, 17 insertions(+), 64 deletions(-) diff --git a/packages/bolt-connection/src/connection-provider/connection-provider-routing.js b/packages/bolt-connection/src/connection-provider/connection-provider-routing.js index 1527d85f9..dc772cfda 100644 --- a/packages/bolt-connection/src/connection-provider/connection-provider-routing.js +++ b/packages/bolt-connection/src/connection-provider/connection-provider-routing.js @@ -130,14 +130,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider * See {@link ConnectionProvider} for more information about this method and * its arguments. */ - async acquireConnection ({ - accessMode, - database, - bookmarks, - impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh - } = {}) { + async acquireConnection ({ accessMode, database, bookmarks, impersonatedUser, onDatabaseNameResolved } = {}) { let name let address const context = { database: database || DEFAULT_DB_NAME } @@ -160,8 +153,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider if (onDatabaseNameResolved) { onDatabaseNameResolved(databaseName) } - }, - onRoutingTableRefresh + } }) // select a target server based on specified access mode @@ -309,14 +301,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider return this._connectionPool.acquire(address) } - _freshRoutingTable ({ - accessMode, - database, - bookmarks, - impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh - } = {}) { + _freshRoutingTable ({ accessMode, database, bookmarks, impersonatedUser, onDatabaseNameResolved } = {}) { const currentRoutingTable = this._routingTableRegistry.get( database, () => new RoutingTable({ database }) @@ -328,22 +313,10 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider this._log.info( `Routing table is stale for database: "${database}" and access mode: "${accessMode}": ${currentRoutingTable}` ) - return this._refreshRoutingTable( - currentRoutingTable, - bookmarks, - impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh - ) + return this._refreshRoutingTable(currentRoutingTable, bookmarks, impersonatedUser, onDatabaseNameResolved) } - _refreshRoutingTable ( - currentRoutingTable, - bookmarks, - impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh - ) { + _refreshRoutingTable (currentRoutingTable, bookmarks, impersonatedUser, onDatabaseNameResolved) { const knownRouters = currentRoutingTable.routers if (this._useSeedRouter) { @@ -352,8 +325,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh + onDatabaseNameResolved ) } return this._fetchRoutingTableFromKnownRoutersFallbackToSeedRouter( @@ -361,8 +333,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh + onDatabaseNameResolved ) } @@ -371,8 +342,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh + onDatabaseNameResolved ) { // we start with seed router, no routers were probed before const seenRouters = [] @@ -402,8 +372,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, newRoutingTable, onDatabaseNameResolved, - error, - onRoutingTableRefresh + error ) } @@ -412,8 +381,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, bookmarks, impersonatedUser, - onDatabaseNameResolved, - onRoutingTableRefresh + onDatabaseNameResolved ) { let [newRoutingTable, error] = await this._fetchRoutingTableUsingKnownRouters( knownRouters, @@ -437,8 +405,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider currentRoutingTable, newRoutingTable, onDatabaseNameResolved, - error, - onRoutingTableRefresh + error ) } @@ -596,13 +563,7 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider return [null, error] } - async _applyRoutingTableIfPossible ( - currentRoutingTable, - newRoutingTable, - onDatabaseNameResolved, - error, - onRoutingTableRefresh - ) { + async _applyRoutingTableIfPossible (currentRoutingTable, newRoutingTable, onDatabaseNameResolved, error) { if (!newRoutingTable) { // none of routing servers returned valid routing table, throw exception throw newError( @@ -618,12 +579,12 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider this._useSeedRouter = true } - await this._updateRoutingTable(newRoutingTable, onDatabaseNameResolved, onRoutingTableRefresh) + await this._updateRoutingTable(newRoutingTable, onDatabaseNameResolved) return newRoutingTable } - async _updateRoutingTable (newRoutingTable, onDatabaseNameResolved, onRoutingTableRefresh) { + async _updateRoutingTable (newRoutingTable, onDatabaseNameResolved) { // close old connections to servers not present in the new routing table await this._connectionPool.keepAll(newRoutingTable.allServers()) this._routingTableRegistry.removeExpired() @@ -631,9 +592,6 @@ export default class RoutingConnectionProvider extends PooledConnectionProvider newRoutingTable ) - if (typeof onRoutingTableRefresh === 'function') { - onRoutingTableRefresh() - } onDatabaseNameResolved(newRoutingTable.database) this._log.info(`Updated routing table ${newRoutingTable}`) diff --git a/packages/core/src/connection-provider.ts b/packages/core/src/connection-provider.ts index 8930d2086..b0775683c 100644 --- a/packages/core/src/connection-provider.ts +++ b/packages/core/src/connection-provider.ts @@ -50,7 +50,6 @@ class ConnectionProvider { bookmarks: bookmarks.Bookmarks impersonatedUser?: string onDatabaseNameResolved?: (databaseName?: string) => void - onRoutingTableRefresh?: () => void }): Promise { throw Error('Not implemented') } diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index 2c1571831..90f04c44a 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -111,8 +111,8 @@ class ConnectionHolder implements ConnectionHolderInterface { bookmarks?: Bookmarks connectionProvider?: ConnectionProvider impersonatedUser?: string - bookmarkManager?: BookmarkManager onDatabaseNameResolved?: (databaseName?: string) => void + bookmarkManager?: BookmarkManager } = {}) { this._mode = mode this._database = database != null ? assertString(database, 'database') : '' @@ -151,16 +151,12 @@ class ConnectionHolder implements ConnectionHolderInterface { initializeConnection (): boolean { if (this._referenceCount === 0 && (this._connectionProvider != null)) { - const bookmarks = this._getBookmarks() - this._connectionPromise = this._connectionProvider.acquireConnection({ accessMode: this._mode, database: this._database, - bookmarks, + bookmarks: this._getBookmarks(), impersonatedUser: this._impersonatedUser, - onDatabaseNameResolved: this._onDatabaseNameResolved, - onRoutingTableRefresh: - () => this._bookmarkManager?.updateBookmarks('system', bookmarks.values(), []) + onDatabaseNameResolved: this._onDatabaseNameResolved }) } else { this._referenceCount++ From 3a672eace6e5197cf8612b69a3524e19f343e2b7 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 3 Aug 2022 16:25:14 +0200 Subject: [PATCH 08/43] only update when bookmark arrive --- packages/core/src/session.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 3b82c79a3..fc2c927ee 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -490,12 +490,12 @@ class Session { * @returns {void} */ _updateBookmarks (newBookmarks?: Bookmarks, previousBookmarks?: Bookmarks, database?: string): void { - this._bookmarkManager?.updateBookmarks( - database ?? this._database, - previousBookmarks?.values() ?? [], - newBookmarks?.values() ?? [] - ) if ((newBookmarks != null) && !newBookmarks.isEmpty()) { + this._bookmarkManager?.updateBookmarks( + database ?? this._database, + previousBookmarks?.values() ?? [], + newBookmarks?.values() ?? [] + ) this._lastBookmarks = newBookmarks } } From 0ff33115a2bee0fa3e3cfb38c4831a47e2a21e9c Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 14:29:23 +0200 Subject: [PATCH 09/43] Explicity ignore bookmark manager when ignoreBookmarkManager is true This changes also includes concatenate session bookmarks and manager bookmarks --- packages/core/src/driver.ts | 14 +++++++--- .../core/src/internal/connection-holder.ts | 6 ++--- packages/core/src/session.ts | 6 ++--- packages/core/test/driver.test.ts | 26 +++++++++++++++++-- packages/neo4j-driver/src/driver.js | 7 +++-- .../src/request-handlers-rx.js | 5 ++-- .../testkit-backend/src/request-handlers.js | 5 ++-- 7 files changed, 49 insertions(+), 20 deletions(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index ec60e357f..86e9221a5 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -293,6 +293,7 @@ class Driver { * Use {@link FETCH_ALL} to always pull all records in one batch. This will override the config value set on driver config. * @param {string} param.database - The database this session will operate on. * @param {string} param.impersonatedUser - The username which the user wants to impersonate for the duration of the session. + * @param {boolean} param.ignoreBookmarkManager - Disable the bookmark manager usage in the session. * @return {Session} new session. */ session ({ @@ -300,13 +301,15 @@ class Driver { bookmarks: bookmarkOrBookmarks, database = '', impersonatedUser, - fetchSize + fetchSize, + ignoreBookmarkManager }: { defaultAccessMode?: SessionMode bookmarks?: string | string[] database?: string impersonatedUser?: string fetchSize?: number + ignoreBookmarkManager?: boolean } = {}): Session { return this._newSession({ defaultAccessMode, @@ -315,7 +318,8 @@ class Driver { reactive: false, impersonatedUser, // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize!) + fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize!), + ignoreBookmarkManager }) } @@ -351,7 +355,8 @@ class Driver { database, reactive, impersonatedUser, - fetchSize + fetchSize, + ignoreBookmarkManager }: { defaultAccessMode: SessionMode bookmarkOrBookmarks?: string | string[] @@ -359,13 +364,14 @@ class Driver { reactive: boolean impersonatedUser?: string fetchSize: number + ignoreBookmarkManager?: boolean }): Session { const sessionMode = Session._validateSessionMode(defaultAccessMode) const connectionProvider = this._getOrCreateConnectionProvider() const bookmarks = bookmarkOrBookmarks != null ? new Bookmarks(bookmarkOrBookmarks) : Bookmarks.empty() - const bookmarkManager = bookmarkOrBookmarks == null + const bookmarkManager = ignoreBookmarkManager !== true ? this._config.bookmarkManager : undefined return this._createSession({ diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index 90f04c44a..e43719e0b 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -167,10 +167,8 @@ class ConnectionHolder implements ConnectionHolderInterface { } private _getBookmarks (): Bookmarks { - if (this._bookmarkManager != null) { - return new Bookmarks(this._bookmarkManager.getBookmarks('system')) - } - return this._bookmarks + const bookmarks = this._bookmarkManager?.getBookmarks('system') ?? [] + return new Bookmarks([...this._bookmarks.values(), ...bookmarks]) } getConnection (): Promise { diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index fc2c927ee..8c69ff514 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -339,10 +339,8 @@ class Session { } private _bookmarks (): Bookmarks { - if (this._bookmarkManager != null) { - return new Bookmarks(this._bookmarkManager.getAllBookmarks([this._database, 'system'])) - } - return this._lastBookmarks + const bookmarks = this._bookmarkManager?.getAllBookmarks([this._database, 'system']) ?? [] + return new Bookmarks([...bookmarks, ...this._lastBookmarks.values()]) } /** diff --git a/packages/core/test/driver.test.ts b/packages/core/test/driver.test.ts index 0f848055f..1a66eab02 100644 --- a/packages/core/test/driver.test.ts +++ b/packages/core/test/driver.test.ts @@ -107,12 +107,34 @@ describe('Driver', () => { } }) + it('should create session without bookmark manager when bookmark manager is ignored', async () => { + const manager = bookmarkManager() + const driver = new Driver( + META_INFO, + { ...CONFIG, bookmarkManager: manager }, + mockCreateConnectonProvider(connectionProvider), + createSession + ) + + const session = driver.session({ ignoreBookmarkManager: true }) + + try { + expect(createSession).toBeCalledWith(expect.objectContaining({ + bookmarkManager: undefined, + bookmarks: Bookmarks.empty() + })) + } finally { + await session.close() + await driver.close() + } + }) + it.each([ [[], Bookmarks.empty()], ['bookmark', new Bookmarks('bookmark')], [['bookmark'], new Bookmarks(['bookmark'])], [['bookmark1', 'bookmark2'], new Bookmarks(['bookmark1', 'bookmark2'])] - ])('should create session without bookmark manager when bookmark set', async (bookmarks, expectedBookmarks) => { + ])('should create session with bookmark manager when bookmark set', async (bookmarks, expectedBookmarks) => { const manager = bookmarkManager() const driver = new Driver( META_INFO, @@ -125,7 +147,7 @@ describe('Driver', () => { try { expect(createSession).toBeCalledWith(expect.objectContaining({ - bookmarkManager: undefined, + bookmarkManager: manager, bookmarks: expectedBookmarks })) } finally { diff --git a/packages/neo4j-driver/src/driver.js b/packages/neo4j-driver/src/driver.js index e72365ad6..8b80a1bb0 100644 --- a/packages/neo4j-driver/src/driver.js +++ b/packages/neo4j-driver/src/driver.js @@ -55,6 +55,7 @@ class Driver extends CoreDriver { * absence indicates that the bookmarks do not exist or are unknown. * @param {string} param.database - The database this session will operate on. * @param {string} param.impersonatedUser - The name of the user which should be impersonated for the duration of the session. + * @param {boolean} param.ignoreBookmarkManager - Disable the bookmark manager usage in the session. * @returns {RxSession} new reactive session. */ rxSession ({ @@ -62,7 +63,8 @@ class Driver extends CoreDriver { bookmarks, database = '', fetchSize, - impersonatedUser + impersonatedUser, + ignoreBookmarkManager } = {}) { return new RxSession({ session: this._newSession({ @@ -71,7 +73,8 @@ class Driver extends CoreDriver { database, impersonatedUser, reactive: false, - fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize) + fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize), + ignoreBookmarkManager }), config: this._config }) diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index db3305c1d..ffe43bded 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -24,7 +24,7 @@ export { } from './request-handlers.js' export function NewSession (context, data, wire) { - let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser } = data + let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, ignoreBookmarkManager } = data switch (accessMode) { case 'r': accessMode = neo4j.session.READ @@ -42,7 +42,8 @@ export function NewSession (context, data, wire) { bookmarks, database, fetchSize, - impersonatedUser + impersonatedUser, + ignoreBookmarkManager }) const id = context.addSession(session) wire.writeResponse(responses.Session({ id })) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index f9a74ef63..83e94ad92 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -118,7 +118,7 @@ export function DriverClose (context, data, wire) { } export function NewSession (context, data, wire) { - let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser } = data + let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, ignoreBookmarkManager } = data switch (accessMode) { case 'r': accessMode = neo4j.session.READ @@ -136,7 +136,8 @@ export function NewSession (context, data, wire) { bookmarks, database, fetchSize, - impersonatedUser + impersonatedUser, + ignoreBookmarkManager }) const id = context.addSession(session) wire.writeResponse(responses.Session({ id })) From a3d168bf6e797d8fd303f5c678780ba2d0e4558c Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 15:17:16 +0200 Subject: [PATCH 10/43] Add test for session run --- packages/core/test/session.test.ts | 209 ++++++++++++++++++++++++++++- 1 file changed, 203 insertions(+), 6 deletions(-) diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 6a561be52..03d50c8c4 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { ConnectionProvider, Session, Connection, TransactionPromise, Transaction } from '../src' +import { ConnectionProvider, Session, Connection, TransactionPromise, Transaction, BookmarkManager, bookmarkManager } from '../src' import { bookmarks } from '../src/internal' import { ACCESS_MODE_READ, FETCH_ALL } from '../src/internal/constants' import ManagedTransaction from '../src/transaction-managed' @@ -349,6 +349,180 @@ describe('session', () => { expect(run).toHaveBeenCalledWith(query, params) }) }) + + describe('.run()', () => { + const systemBookmarks = ['sys:bm01', 'sys:bm02'] + const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm03'] + const customBookmarks = ['neo4j:bm02'] + + it('should acquire connection with system bookmarks from the bookmark manager', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = newFakeConnection() + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + await session.run('query') + + expect(connectionProvider.acquireConnection).toBeCalledWith( + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) }) + ) + }) + + it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = newFakeConnection() + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + lastBookmarks: new bookmarks.Bookmarks(customBookmarks), + database: 'neo4j' + }) + + await session.run('query') + + expect(connectionProvider.acquireConnection).toBeCalledWith( + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + ) + }) + + it.each([ + [[]], + [customBookmarks] + ])('should call getAllBookmarks for the relevant database', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const getAllBookmarksSpy = jest.spyOn(manager, 'getAllBookmarks') + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.run('query') + + expect(getAllBookmarksSpy).toBeCalledWith(['neo4j', 'system']) + }) + + it.each([ + [[]], + [customBookmarks] + ])('should call run query with getAllBookmarks + lastBookmarks', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.run('query') + + expect(connection.seenProtocolOptions[0]).toEqual( + expect.objectContaining({ + bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks, ...lastBookmarks]) + }) + ) + }) + + it.each([ + [undefined, 'neo4j'], + ['neo4j', 'neo4j'], + ['adb', 'adb'] + ])('should call run updateBookmarks when bookmarks is not empty', async (metaDb, updateDb) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + await session.run('query') + + const { afterComplete } = connection.seenProtocolOptions[0] + + afterComplete({ db: metaDb, bookmark: customBookmarks }) + + expect(updateBookmarksSpy).toBeCalledWith(updateDb, [...neo4jBookmarks, ...systemBookmarks], customBookmarks) + }) + + it.each([ + [undefined], + ['neo4j'], + ['adb'] + ])('should not call run updateBookmarks when bookmarks is empty', async (metaDb) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + await session.run('query') + + const { afterComplete } = connection.seenProtocolOptions[0] + + afterComplete({ db: metaDb }) + + expect(updateBookmarksSpy).not.toBeCalled() + }) + }) }) function mockBeginWithSuccess (connection: FakeConnection): FakeConnection { @@ -368,26 +542,49 @@ function newSessionWithConnection ( connection: Connection, beginTx: boolean = true, fetchSize: number = 1000, - lastBookmarks: bookmarks.Bookmarks = bookmarks.Bookmarks.empty() + lastBookmarks: bookmarks.Bookmarks = bookmarks.Bookmarks.empty(), + bookmarkManager?: BookmarkManager ): Session { + const { session } = setupSession({ + connection, beginTx, fetchSize, lastBookmarks, bookmarkManager + }) + return session +} + +function setupSession ({ + connection, + beginTx = true, + fetchSize = 1000, + database = '', + lastBookmarks = bookmarks.Bookmarks.empty(), + bookmarkManager +}: { + connection: Connection + beginTx?: boolean + fetchSize?: number + lastBookmarks?: bookmarks.Bookmarks + database?: string + bookmarkManager?: BookmarkManager +}): { session: Session, connectionProvider: ConnectionProvider } { const connectionProvider = new ConnectionProvider() - connectionProvider.acquireConnection = async () => await Promise.resolve(connection) + connectionProvider.acquireConnection = jest.fn(async () => await Promise.resolve(connection)) connectionProvider.close = async () => await Promise.resolve() const session = new Session({ mode: ACCESS_MODE_READ, connectionProvider, - database: '', + database, fetchSize, config: {}, reactive: false, - bookmarks: lastBookmarks + bookmarks: lastBookmarks, + bookmarkManager }) if (beginTx) { session.beginTransaction().catch(e => {}) // force session to acquire new connection } - return session + return { session, connectionProvider } } function newFakeConnection (): FakeConnection { From 0a0b56bbf931d338e908d2f07041d9bd99a97ec1 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 15:31:16 +0200 Subject: [PATCH 11/43] Add test for begin transaction --- packages/core/test/session.test.ts | 147 +++++++++++++++++++- packages/core/test/utils/connection.fake.ts | 7 +- 2 files changed, 147 insertions(+), 7 deletions(-) diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 03d50c8c4..20d5de029 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -23,6 +23,10 @@ import ManagedTransaction from '../src/transaction-managed' import FakeConnection from './utils/connection.fake' describe('session', () => { + const systemBookmarks = ['sys:bm01', 'sys:bm02'] + const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm03'] + const customBookmarks = ['neo4j:bm02'] + it('close should return promise', done => { const connection = newFakeConnection() const session = newSessionWithConnection(connection) @@ -310,6 +314,142 @@ describe('session', () => { expect(tx).toBeDefined() }) + + it('should acquire connection with system bookmarks from the bookmark manager', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = mockBeginWithSuccess(newFakeConnection()) + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + await session.beginTransaction() + + expect(connectionProvider.acquireConnection).toBeCalledWith( + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) }) + ) + }) + + it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = mockBeginWithSuccess(newFakeConnection()) + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + lastBookmarks: new bookmarks.Bookmarks(customBookmarks), + database: 'neo4j' + }) + + await session.beginTransaction() + + expect(connectionProvider.acquireConnection).toBeCalledWith( + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + ) + }) + + it.each([ + [[]], + [customBookmarks] + ])('should call getAllBookmarks for the relevant database', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const getAllBookmarksSpy = jest.spyOn(manager, 'getAllBookmarks') + + const connection = mockBeginWithSuccess(newFakeConnection()) + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.beginTransaction() + + expect(getAllBookmarksSpy).toBeCalledWith(['neo4j', 'system']) + }) + + it.each([ + [[]], + [customBookmarks] + ])('should call begin query with getAllBookmarks + lastBookmarks', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const connection = mockBeginWithSuccess(newFakeConnection()) + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.beginTransaction() + + expect(connection.seenBeginTransaction[0]).toEqual([ + expect.objectContaining({ + bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks, ...lastBookmarks]) + }) + ]) + }) + + it.each([ + [undefined], + ['neo4j'], + ['adb'] + ])('should not call run updateBookmarks when bookmarks since it didnt have bookmarks', async (metaDb) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') + + const connection = mockBeginWithSuccess(newFakeConnection()) + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + await session.run('query') + + const { afterComplete } = connection.seenProtocolOptions[0] + + afterComplete({ db: metaDb }) + + expect(updateBookmarksSpy).not.toBeCalled() + }) }) describe.each([ @@ -351,10 +491,6 @@ describe('session', () => { }) describe('.run()', () => { - const systemBookmarks = ['sys:bm01', 'sys:bm02'] - const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm03'] - const customBookmarks = ['neo4j:bm02'] - it('should acquire connection with system bookmarks from the bookmark manager', async () => { const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -530,7 +666,8 @@ function mockBeginWithSuccess (connection: FakeConnection): FakeConnection { connection.protocol = () => { return { ...protocol, - beginTransaction: (params: { afterComplete: () => {}}) => { + beginTransaction: (params: { afterComplete: () => {}}, ...args: any[]) => { + protocol.beginTransaction([params, ...args]) params.afterComplete() } } diff --git a/packages/core/test/utils/connection.fake.ts b/packages/core/test/utils/connection.fake.ts index f1157cede..6f5a5c024 100644 --- a/packages/core/test/utils/connection.fake.ts +++ b/packages/core/test/utils/connection.fake.ts @@ -45,6 +45,7 @@ export default class FakeConnection extends Connection { public seenRequestRoutingInformation: any[] public rollbackInvoked: number public _rollbackError: Error | null + public seenBeginTransaction: any[] constructor () { super() @@ -67,6 +68,7 @@ export default class FakeConnection extends Connection { this.seenRequestRoutingInformation = [] this.rollbackInvoked = 0 this._rollbackError = null + this.seenBeginTransaction = [] } get id (): string { @@ -106,7 +108,8 @@ export default class FakeConnection extends Connection { commitTransaction: () => { return mockResultStreamObserver('COMMIT', {}) }, - beginTransaction: async () => { + beginTransaction: async (...args: any) => { + this.seenBeginTransaction.push(...args) return await Promise.resolve() }, rollbackTransaction: () => { @@ -180,7 +183,7 @@ export default class FakeConnection extends Connection { return this } - hasOngoingObservableRequests(): boolean { + hasOngoingObservableRequests (): boolean { return true } } From a968d322e0e7418fe8e86188fcdb76c2e047c74e Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 15:53:11 +0200 Subject: [PATCH 12/43] Add tests for commit tx --- packages/core/src/transaction.ts | 2 +- packages/core/test/session.test.ts | 76 ++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index 74dc9dfba..2def5278e 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -275,7 +275,7 @@ class Transaction { * @returns {void} */ _onCompleteCallback (meta: { bookmark?: string | string[], db?: string }, previousBookmarks: Bookmarks): void { - this._onBookmarks(new Bookmarks(meta.bookmark), previousBookmarks ?? Bookmarks.empty(), meta.db) + this._onBookmarks(new Bookmarks(meta?.bookmark), previousBookmarks ?? Bookmarks.empty(), meta?.db) } } diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 20d5de029..8812c3d25 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -452,6 +452,67 @@ describe('session', () => { }) }) + describe('.commit()', () => { + it.each([ + [undefined, 'neo4j'], + ['neo4j', 'neo4j'], + ['adb', 'adb'] + ])('should call run updateBookmarks when bookmarks is not empty', async (metaDb, updateDb) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') + + const connection = mockCommitWithSuccess(mockBeginWithSuccess(newFakeConnection()), { db: metaDb, bookmark: customBookmarks }) + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + const tx = await session.beginTransaction() + await tx.commit() + + expect(updateBookmarksSpy).toBeCalledTimes(1) + expect(updateBookmarksSpy).toBeCalledWith(updateDb, [...neo4jBookmarks, ...systemBookmarks], customBookmarks) + }) + + it.each([ + [undefined], + ['neo4j'], + ['adb'] + ])('should not call run updateBookmarks when bookmarks is empty', async (metaDb) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') + + const connection = mockCommitWithSuccess(mockBeginWithSuccess(newFakeConnection()), { db: metaDb }) + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j' + }) + + const tx = await session.beginTransaction() + await tx.commit() + + expect(updateBookmarksSpy).not.toBeCalled() + }) + }) + describe.each([ ['.executeWrite()', (session: Session) => session.executeWrite.bind(session)], ['.executeRead()', (session: Session) => session.executeRead.bind(session)] @@ -675,6 +736,21 @@ function mockBeginWithSuccess (connection: FakeConnection): FakeConnection { return connection } +function mockCommitWithSuccess (connection: FakeConnection, metadata: any): FakeConnection { + const protocol = connection.protocol() + connection.protocol = () => { + return { + ...protocol, + commitTransaction: (params: { afterComplete: (metadata: any) => {}}, ...args: any[]) => { + const observer = protocol.commitTransaction(...[params, ...args]) + params.afterComplete(metadata) + return observer + } + } + } + return connection +} + function newSessionWithConnection ( connection: Connection, beginTx: boolean = true, From 1c4a4dfffaf9cf098674b3408782e47d6f3e9731 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 17:00:46 +0200 Subject: [PATCH 13/43] Add tests rxSessions --- packages/neo4j-driver/test/driver.test.js | 26 ++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/packages/neo4j-driver/test/driver.test.js b/packages/neo4j-driver/test/driver.test.js index 1883a541c..153757d57 100644 --- a/packages/neo4j-driver/test/driver.test.js +++ b/packages/neo4j-driver/test/driver.test.js @@ -25,7 +25,7 @@ import { DEFAULT_MAX_SIZE } from '../../bolt-connection/lib/pool/pool-config' import testUtils from './internal/test-utils' -import { json, internal } from 'neo4j-driver-core' +import { json, internal, bookmarkManager } from 'neo4j-driver-core' const { bookmarks: { Bookmarks } @@ -125,6 +125,7 @@ describe('#unit driver', () => { }) describe('.rxSession()', () => { + const manager = bookmarkManager() ;[ [undefined, Bookmarks.empty()], [null, Bookmarks.empty()], @@ -143,6 +144,29 @@ describe('#unit driver', () => { expect(session.lastBookmarks()).toEqual(expectedBookmarks.values()) }) }) + + ;[ + [manager, undefined, manager], + [manager, false, manager], + [manager, true, undefined], + [undefined, undefined, undefined], + [undefined, false, undefined], + [undefined, true, undefined] + ].forEach(([driverBMManager, ignoreBookmarkManager, sessionBMManager]) => { + it('should create session using param bookmark manager', () => { + driver = neo4j.driver( + `neo4j+ssc://${sharedNeo4j.hostname}`, + sharedNeo4j.authToken, + { + bookmarkManager: driverBMManager + } + ) + + const session = driver.rxSession({ ignoreBookmarkManager }) + + expect(session._session._bookmarkManager).toEqual(sessionBMManager) + }) + }) }) }) From 3799568adf977ab9507dec7568023cf85321806f Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 4 Aug 2022 17:43:43 +0200 Subject: [PATCH 14/43] Add iterator capabilities to Bookmarks --- packages/core/src/internal/bookmarks.ts | 4 ++++ packages/core/src/internal/connection-holder.ts | 2 +- packages/core/src/session.ts | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/core/src/internal/bookmarks.ts b/packages/core/src/internal/bookmarks.ts index 11524191a..cb6406be0 100644 --- a/packages/core/src/internal/bookmarks.ts +++ b/packages/core/src/internal/bookmarks.ts @@ -52,6 +52,10 @@ export class Bookmarks { return this._values } + [Symbol.iterator] (): Iterable { + return this._values[Symbol.iterator]() + } + /** * Get these bookmarks as an object for begin transaction call. * @return {Object} the value of this bookmarks holder as object. diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index e43719e0b..4c458d303 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -168,7 +168,7 @@ class ConnectionHolder implements ConnectionHolderInterface { private _getBookmarks (): Bookmarks { const bookmarks = this._bookmarkManager?.getBookmarks('system') ?? [] - return new Bookmarks([...this._bookmarks.values(), ...bookmarks]) + return new Bookmarks([...this._bookmarks, ...bookmarks]) } getConnection (): Promise { diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 8c69ff514..63efa3d9b 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -340,7 +340,7 @@ class Session { private _bookmarks (): Bookmarks { const bookmarks = this._bookmarkManager?.getAllBookmarks([this._database, 'system']) ?? [] - return new Bookmarks([...bookmarks, ...this._lastBookmarks.values()]) + return new Bookmarks([...bookmarks, ...this._lastBookmarks]) } /** From 2ed6501a892a9a0f1c9429fdb92f1b990cd04df4 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 5 Aug 2022 15:50:12 +0200 Subject: [PATCH 15/43] Update docs --- packages/core/src/bookmark-manager.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 3e3571dfb..10d1d7ff4 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -18,12 +18,13 @@ */ export default interface BookmarkManager { - /** - * Method called when the bookmarks get update because of a given event + /* Method called when the bookmarks get updated when a transaction finished. * + * This method will be called during when auto-commit queries finished and explicit transactions + * get commited. * @param database The database which the bookmarks belongs to - * @param previousBookmarks The bookmarks used during the session creation - * @param newBookmarks The new bookmarks resolved at the end of the session. + * @param previousBookmarks The bookmarks used during the transaction creation + * @param newBookmarks The new bookmarks resolved at the end of the transaction. * @returns {void} */ updateBookmarks: (database: string, previousBookmarks: string[], newBookmarks: string[]) => void @@ -37,7 +38,12 @@ export default interface BookmarkManager { getBookmarks: (database: string) => string[] /** - * Method called by the driver for getting all the bookmarks + * Method called by the driver for getting all the bookmarks. + * + * The return of this method should be all the bookmarks present in the BookmarkManager for all databases. + * The databases informed in the method call will be used for enriching the bookmark set by enforcing the bookmark + * manager calls `bookmarkSupplier` for these database names even though this database are not present in the bookmark + * manager map yet. * * @param mustIncludedDatabases The database which must be included in the result even if they don't have be initialized yet. * @returns {string[]} The set of bookmarks From 733251ffde6c3b98b244df0d5758fd12fa29869d Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 8 Aug 2022 15:38:16 +0200 Subject: [PATCH 16/43] Optimization:MinimalBookmarksSet --- packages/core/src/internal/bookmarks.ts | 6 +++--- packages/testkit-backend/src/feature/common.js | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core/src/internal/bookmarks.ts b/packages/core/src/internal/bookmarks.ts index cb6406be0..0c0222e43 100644 --- a/packages/core/src/internal/bookmarks.ts +++ b/packages/core/src/internal/bookmarks.ts @@ -94,7 +94,7 @@ function asStringArray ( } if (Array.isArray(value)) { - const result = [] + const result = new Set() const flattenedValue = flattenArray(value) for (let i = 0; i < flattenedValue.length; i++) { const element = flattenedValue[i] @@ -106,10 +106,10 @@ function asStringArray ( `Bookmark value should be a string, given: '${element}'` ) } - result.push(element) + result.add(element) } } - return result + return [...result] } throw new TypeError( diff --git a/packages/testkit-backend/src/feature/common.js b/packages/testkit-backend/src/feature/common.js index 7315b053c..d941af6e2 100644 --- a/packages/testkit-backend/src/feature/common.js +++ b/packages/testkit-backend/src/feature/common.js @@ -36,6 +36,7 @@ const features = [ 'Feature:API:Driver.VerifyConnectivity', 'Optimization:EagerTransactionBegin', 'Optimization:ImplicitDefaultArguments', + 'Optimization:MinimalBookmarksSet', 'Optimization:MinimalResets', ...SUPPORTED_TLS ] From b497248dbf587938de29da67c8a9e6c43cacbc95 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 8 Aug 2022 17:05:07 +0200 Subject: [PATCH 17/43] Add BookmarkManager.forget() method --- packages/core/src/bookmark-manager.ts | 15 +++++++++ packages/core/test/bookmark-manager.test.ts | 37 +++++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 10d1d7ff4..2f0425430 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -49,6 +49,15 @@ export default interface BookmarkManager { * @returns {string[]} The set of bookmarks */ getAllBookmarks: (mustIncludedDatabases: string[]) => string[] + + /** + * Forget the databases and its bookmarks + * + * This method is not called by the driver. Forgetting unused databases is the user's responsibility. + * + * @param databases The databases which the bookmarks will be removed for. + */ + forget: (databases: string[]) => void } export interface BookmarkManagerConfig { @@ -118,4 +127,10 @@ class Neo4jBookmarkManager implements BookmarkManager { return bookmarks } + + forget (databases: string[]): void { + for (const database of databases) { + this._bookmarksPerDb.delete(database) + } + } } diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index a725b70a8..12e2265b5 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -191,4 +191,41 @@ describe('BookmarkManager', () => { expect(notifyBookmarks).toBeCalledWith('neo4j', newBookmarks) }) }) + + describe('forget()', () => { + it('should forgot database', () => { + const extraBookmarks = ['system:bmextra', 'adb:bmextra'] + const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarkSupplier + }) + + manager.forget(['neo4j', 'adb']) + const bookmarks = manager.getAllBookmarks(['system', 'adb']) + + expect(bookmarks.sort()).toEqual( + [...systemBookmarks, ...extraBookmarks].sort() + ) + }) + + it('getAllBookmarks() should not call bookmarkSupplier for the forget dbs', () => { + const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarkSupplier + }) + + manager.forget(['neo4j', 'adb']) + manager.getAllBookmarks(['system', 'adb']) + + expect(bookmarkSupplier).not.toBeCalledWith('neo4j') + }) + }) }) From 9c1baa1621a2045f952b50b0b980b8c3ce8b935e Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 9 Aug 2022 12:47:04 +0200 Subject: [PATCH 18/43] Add support for testing extensions --- packages/testkit-backend/src/context.js | 30 ++++++++++++ .../testkit-backend/src/request-handlers.js | 48 ++++++++++++++++++- packages/testkit-backend/src/responses.js | 8 ++++ .../src/skipped-tests/common.js | 4 ++ 4 files changed, 89 insertions(+), 1 deletion(-) diff --git a/packages/testkit-backend/src/context.js b/packages/testkit-backend/src/context.js index 1153c2fe1..3dc735efc 100644 --- a/packages/testkit-backend/src/context.js +++ b/packages/testkit-backend/src/context.js @@ -9,6 +9,8 @@ export default class Context { this._shouldRunTest = shouldRunTest this._getFeatures = getFeatures this._results = {} + this._bookmarkSupplierRequests = {} + this._notifyBookmarksRequests = {} } addDriver (driver) { @@ -106,6 +108,34 @@ export default class Context { return this._getFeatures() } + addBookmarkSupplierRequest (resolve, reject) { + return this._add(this._bookmarkSupplierRequests, { + resolve, reject + }) + } + + removeBookmarkSupplierRequest (id) { + delete this._bookmarkSupplierRequests[id] + } + + getBookmarkSupplierRequest (id) { + return this._bookmarkSupplierRequests[id] + } + + addNotifyBookmarksRequest (resolve, reject) { + return this._add(this._notifyBookmarksRequests, { + resolve, reject + }) + } + + removeNotifyBookmarksRequest (id) { + delete this._notifyBookmarksRequests[id] + } + + getNotifyBookmarksRequest (id) { + return this._notifyBookmarksRequests[id] + } + _add (map, object) { this._id++ map[this._id] = object diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 83e94ad92..c3957200f 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -88,11 +88,36 @@ export function NewDriver (context, data, wire) { /// TODO: Implement supplier and notify const bmmConfig = data.bookmarkManager let initialBookmarks + let bookmarkSupplier + let notifyBookmarks if (bmmConfig.initialBookmarks != null) { initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) } + if (bmmConfig.bookmarkSupplierRegistered === true) { + bookmarkSupplier = (database) => { + const supplier = () => + new Promise((resolve, reject) => { + const id = context.addBookmarkSupplierRequest(resolve, reject) + wire.writeResponse(responses.BookmarkSupplierRequest({ id, database })) + }) + supplier() + return [] + } + } + if (bmmConfig.notifyBookmarksRegistered === true) { + notifyBookmarks = (database, bookmarks) => { + const notifier = () => + new Promise((resolve, reject) => { + const id = context.addNotifyBookmarksRequest(resolve, reject) + wire.writeResponse(responses.NotifyBookmarksRequest({ id, database, bookmarks })) + }) + notifier() + } + } config.bookmarkManager = neo4j.bookmarkManager({ - initialBookmarks + initialBookmarks, + bookmarkSupplier, + notifyBookmarks }) } let driver @@ -418,6 +443,27 @@ export function ResolverResolutionCompleted ( request.resolve(addresses) } +export function BookmarkSupplierCompleted ( + context, + { + requestId, + bookmarks + } +) { + const bookmarkSupplierRequest = context.getBookmarkSupplierRequest(requestId) + bookmarkSupplierRequest.resolve(bookmarks) +} + +export function NotifyBookmarksCompleted ( + context, + { + requestId + } +) { + const notifyBookmarksRequest = context.getNotifyBookmarksRequest(requestId) + notifyBookmarksRequest.resolve() +} + export function GetRoutingTable (context, { driverId, database }, wire) { const driver = context.getDriver(driverId) const routingTable = diff --git a/packages/testkit-backend/src/responses.js b/packages/testkit-backend/src/responses.js index 03a9f0487..5bbae4621 100644 --- a/packages/testkit-backend/src/responses.js +++ b/packages/testkit-backend/src/responses.js @@ -14,6 +14,14 @@ export function ResolverResolutionRequired ({ id, address }) { return response('ResolverResolutionRequired', { id, address }) } +export function BookmarkSupplierRequest ({ id, database }) { + return response('BookmarkSupplierRequest', { id, database }) +} + +export function NotifyBookmarksRequest ({ id, database, bookmarks }) { + return response('NotifyBookmarksRequest', { id, database, bookmarks }) +} + export function Session ({ id }) { return response('Session', { id }) } diff --git a/packages/testkit-backend/src/skipped-tests/common.js b/packages/testkit-backend/src/skipped-tests/common.js index bbe75f9be..84596efef 100644 --- a/packages/testkit-backend/src/skipped-tests/common.js +++ b/packages/testkit-backend/src/skipped-tests/common.js @@ -1,6 +1,10 @@ import skip, { ifEquals, ifEndsWith, endsWith, ifStartsWith, startsWith, not } from './skip' const skippedTests = [ + skip( + 'Driver does not support async call in BookmarkManager extensions', + ifEquals('stub.driver_parameters.test_bookmark_manager.TestNeo4jBookmarkManager.test_should_enrich_bookmarks_with_bookmark_supplier_result') + ), skip( 'Driver does not return offset for old DateTime implementations', ifStartsWith('stub.types.test_temporal_types.TestTemporalTypes') From 179c2e407cef24e80f609ae6e3a08f3234010253 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 9 Aug 2022 12:48:26 +0200 Subject: [PATCH 19/43] Add support for testing extensions --- packages/testkit-backend/src/request-handlers.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index c3957200f..fc882cd7a 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -85,7 +85,6 @@ export function NewDriver (context, data, wire) { config.maxTransactionRetryTime = data.maxTxRetryTimeMs } if ('bookmarkManager' in data && data.bookmarkManager != null) { - /// TODO: Implement supplier and notify const bmmConfig = data.bookmarkManager let initialBookmarks let bookmarkSupplier From c562e66fc03e1d487177077681d5eb7774eb2821 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 11:54:57 +0200 Subject: [PATCH 20/43] Addressing issues detected in the ADR --- packages/core/src/bookmark-manager.ts | 33 +++++----- packages/core/src/session.ts | 2 +- packages/core/test/bookmark-manager.test.ts | 62 +++++++------------ packages/core/test/session.test.ts | 4 +- .../testkit-backend/src/request-handlers.js | 12 ++-- 5 files changed, 49 insertions(+), 64 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 2f0425430..31304a0b2 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -48,7 +48,7 @@ export default interface BookmarkManager { * @param mustIncludedDatabases The database which must be included in the result even if they don't have be initialized yet. * @returns {string[]} The set of bookmarks */ - getAllBookmarks: (mustIncludedDatabases: string[]) => string[] + getAllBookmarks: () => string[] /** * Forget the databases and its bookmarks @@ -62,8 +62,8 @@ export default interface BookmarkManager { export interface BookmarkManagerConfig { initialBookmarks?: Map - bookmarkSupplier?: (database: string) => string[] - notifyBookmarks?: (database: string, bookmarks: string[]) => void + bookmarksSupplier?: (database?: string) => string[] + bookmarksConsumer?: (database: string, bookmarks: string[]) => void } export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { @@ -73,16 +73,16 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa return new Neo4jBookmarkManager( initialBookmarks, - config.bookmarkSupplier, - config.notifyBookmarks + config.bookmarksSupplier, + config.bookmarksConsumer ) } class Neo4jBookmarkManager implements BookmarkManager { constructor ( private readonly _bookmarksPerDb: Map>, - private readonly _bookmarkSupplier?: (database: string) => string[], - private readonly _notifyBookmarks?: (database: string, bookmark: string[]) => void + private readonly _bookmarksSupplier?: (database?: string) => string[], + private readonly _bookmarksConsumer?: (database: string, bookmark: string[]) => void ) { } @@ -92,8 +92,8 @@ class Neo4jBookmarkManager implements BookmarkManager { previousBookmarks.forEach(bm => bookmarks.delete(bm)) newBookmarks.forEach(bm => bookmarks.add(bm)) - if (typeof this._notifyBookmarks === 'function') { - this._notifyBookmarks(database, [...bookmarks]) + if (typeof this._bookmarksConsumer === 'function') { + this._bookmarksConsumer(database, [...bookmarks]) } } @@ -109,20 +109,23 @@ class Neo4jBookmarkManager implements BookmarkManager { getBookmarks (database: string): string[] { const bookmarks = this._bookmarksPerDb.get(database) ?? [] - if (typeof this._bookmarkSupplier === 'function') { - const suppliedBookmarks = this._bookmarkSupplier(database) ?? [] + if (typeof this._bookmarksSupplier === 'function') { + const suppliedBookmarks = this._bookmarksSupplier(database) ?? [] return [...bookmarks, ...suppliedBookmarks] } return [...bookmarks] } - getAllBookmarks (mustIncludedDatabases: string[]): string[] { + getAllBookmarks (): string[] { const bookmarks = [] - const databases = new Set([...this._bookmarksPerDb.keys(), ...mustIncludedDatabases]) - for (const database of databases) { - bookmarks.push(...this.getBookmarks(database)) + for (const [, dbBookmarks] of this._bookmarksPerDb) { + bookmarks.push(...dbBookmarks) + } + if (typeof this._bookmarksSupplier === 'function') { + const suppliedBookmarks = this._bookmarksSupplier() ?? [] + bookmarks.push(...suppliedBookmarks) } return bookmarks diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 63efa3d9b..9975a2104 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -339,7 +339,7 @@ class Session { } private _bookmarks (): Bookmarks { - const bookmarks = this._bookmarkManager?.getAllBookmarks([this._database, 'system']) ?? [] + const bookmarks = this._bookmarkManager?.getAllBookmarks() ?? [] return new Bookmarks([...bookmarks, ...this._lastBookmarks]) } diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 12e2265b5..81406e2e6 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -54,7 +54,7 @@ describe('BookmarkManager', () => { ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarkSupplier: () => extraBookmarks + bookmarksSupplier: () => extraBookmarks }) const bookmarks = manager.getBookmarks('neo4j') @@ -63,15 +63,15 @@ describe('BookmarkManager', () => { }) it('should return call from bookmarkSupplier with correct database', () => { - const bookmarkSupplier = jest.fn() + const bookmarksSupplier = jest.fn() const manager = bookmarkManager({ - bookmarkSupplier + bookmarksSupplier }) manager.getBookmarks('neo4j') - expect(bookmarkSupplier).toBeCalledWith('neo4j') + expect(bookmarksSupplier).toBeCalledWith('neo4j') }) }) @@ -84,7 +84,7 @@ describe('BookmarkManager', () => { ]) }) - const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + const bookmarks = manager.getAllBookmarks() expect(bookmarks).toEqual([...neo4jBookmarks, ...systemBookmarks]) }) @@ -92,44 +92,42 @@ describe('BookmarkManager', () => { it('should return empty if there isnt bookmarks for any db', () => { const manager = bookmarkManager({}) - const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + const bookmarks = manager.getAllBookmarks() expect(bookmarks).toEqual([]) }) it('should return enrich bookmarks list with supplied bookmarks', () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] - const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) + const bookmarksSupplier = jest.fn((database?: string) => extraBookmarks) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarkSupplier + bookmarksSupplier }) - const bookmarks = manager.getAllBookmarks(['neo4j', 'adb']) + const bookmarks = manager.getAllBookmarks() expect(bookmarks.sort()).toEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() ) }) - it('should call bookmarkSupplier for each existing and listed databases ', () => { - const bookmarkSupplier = jest.fn() + it('should call bookmarkSupplier for getting all bookmarks', () => { + const bookmarksSupplier = jest.fn() const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarkSupplier + bookmarksSupplier }) - manager.getAllBookmarks(['neo4j', 'adb']) + manager.getAllBookmarks() - expect(bookmarkSupplier).toBeCalledWith('neo4j') - expect(bookmarkSupplier).toBeCalledWith('adb') - expect(bookmarkSupplier).toBeCalledWith('system') + expect(bookmarksSupplier).toBeCalledWith() }) }) @@ -145,7 +143,7 @@ describe('BookmarkManager', () => { manager.updateBookmarks( 'neo4j', - manager.getAllBookmarks(['neo4j', 'system']), + manager.getAllBookmarks(), newBookmarks ) @@ -172,60 +170,44 @@ describe('BookmarkManager', () => { }) it('should notify new bookmarks', () => { - const notifyBookmarks = jest.fn() + const bookmarksConsumer = jest.fn() const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - notifyBookmarks + bookmarksConsumer }) manager.updateBookmarks( 'neo4j', - manager.getAllBookmarks(['neo4j', 'system']), + manager.getAllBookmarks(), newBookmarks ) - expect(notifyBookmarks).toBeCalledWith('neo4j', newBookmarks) + expect(bookmarksConsumer).toBeCalledWith('neo4j', newBookmarks) }) }) describe('forget()', () => { it('should forgot database', () => { const extraBookmarks = ['system:bmextra', 'adb:bmextra'] - const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) + const bookmarksSupplier = jest.fn(() => extraBookmarks) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarkSupplier + bookmarksSupplier }) manager.forget(['neo4j', 'adb']) - const bookmarks = manager.getAllBookmarks(['system', 'adb']) + const bookmarks = manager.getAllBookmarks() expect(bookmarks.sort()).toEqual( [...systemBookmarks, ...extraBookmarks].sort() ) }) - - it('getAllBookmarks() should not call bookmarkSupplier for the forget dbs', () => { - const bookmarkSupplier = jest.fn((database: string) => [`${database}:bmextra`]) - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), - bookmarkSupplier - }) - - manager.forget(['neo4j', 'adb']) - manager.getAllBookmarks(['system', 'adb']) - - expect(bookmarkSupplier).not.toBeCalledWith('neo4j') - }) }) }) diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 8812c3d25..63b1d5bda 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -386,7 +386,7 @@ describe('session', () => { await session.beginTransaction() - expect(getAllBookmarksSpy).toBeCalledWith(['neo4j', 'system']) + expect(getAllBookmarksSpy).toBeCalledWith() }) it.each([ @@ -623,7 +623,7 @@ describe('session', () => { await session.run('query') - expect(getAllBookmarksSpy).toBeCalledWith(['neo4j', 'system']) + expect(getAllBookmarksSpy).toBeCalledWith() }) it.each([ diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index fc882cd7a..3b2b1a76c 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -87,13 +87,13 @@ export function NewDriver (context, data, wire) { if ('bookmarkManager' in data && data.bookmarkManager != null) { const bmmConfig = data.bookmarkManager let initialBookmarks - let bookmarkSupplier - let notifyBookmarks + let bookmarksSupplier + let bookmarksConsumer if (bmmConfig.initialBookmarks != null) { initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) } if (bmmConfig.bookmarkSupplierRegistered === true) { - bookmarkSupplier = (database) => { + bookmarksSupplier = (database) => { const supplier = () => new Promise((resolve, reject) => { const id = context.addBookmarkSupplierRequest(resolve, reject) @@ -104,7 +104,7 @@ export function NewDriver (context, data, wire) { } } if (bmmConfig.notifyBookmarksRegistered === true) { - notifyBookmarks = (database, bookmarks) => { + bookmarksConsumer = (database, bookmarks) => { const notifier = () => new Promise((resolve, reject) => { const id = context.addNotifyBookmarksRequest(resolve, reject) @@ -115,8 +115,8 @@ export function NewDriver (context, data, wire) { } config.bookmarkManager = neo4j.bookmarkManager({ initialBookmarks, - bookmarkSupplier, - notifyBookmarks + bookmarksConsumer, + bookmarksSupplier }) } let driver From f4c5c3e3b533b294af047127219c624534b47f0a Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 12:17:40 +0200 Subject: [PATCH 21/43] Adjust testkit protocol naming --- packages/testkit-backend/src/request-handlers.js | 12 ++++++------ packages/testkit-backend/src/responses.js | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 3b2b1a76c..aad54005e 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -92,23 +92,23 @@ export function NewDriver (context, data, wire) { if (bmmConfig.initialBookmarks != null) { initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) } - if (bmmConfig.bookmarkSupplierRegistered === true) { + if (bmmConfig.bookmarksSupplierRegistered === true) { bookmarksSupplier = (database) => { const supplier = () => new Promise((resolve, reject) => { const id = context.addBookmarkSupplierRequest(resolve, reject) - wire.writeResponse(responses.BookmarkSupplierRequest({ id, database })) + wire.writeResponse(responses.BookmarksSupplierRequest({ id, database })) }) supplier() return [] } } - if (bmmConfig.notifyBookmarksRegistered === true) { + if (bmmConfig.bookmarksConsumerRegistred === true) { bookmarksConsumer = (database, bookmarks) => { const notifier = () => new Promise((resolve, reject) => { const id = context.addNotifyBookmarksRequest(resolve, reject) - wire.writeResponse(responses.NotifyBookmarksRequest({ id, database, bookmarks })) + wire.writeResponse(responses.BookmarksConsumerRequest({ id, database, bookmarks })) }) notifier() } @@ -442,7 +442,7 @@ export function ResolverResolutionCompleted ( request.resolve(addresses) } -export function BookmarkSupplierCompleted ( +export function BookmarksSupplierCompleted ( context, { requestId, @@ -453,7 +453,7 @@ export function BookmarkSupplierCompleted ( bookmarkSupplierRequest.resolve(bookmarks) } -export function NotifyBookmarksCompleted ( +export function BookmarksConsumerCompleted ( context, { requestId diff --git a/packages/testkit-backend/src/responses.js b/packages/testkit-backend/src/responses.js index 5bbae4621..eccaff5c5 100644 --- a/packages/testkit-backend/src/responses.js +++ b/packages/testkit-backend/src/responses.js @@ -14,12 +14,12 @@ export function ResolverResolutionRequired ({ id, address }) { return response('ResolverResolutionRequired', { id, address }) } -export function BookmarkSupplierRequest ({ id, database }) { - return response('BookmarkSupplierRequest', { id, database }) +export function BookmarksSupplierRequest ({ id, database }) { + return response('BookmarksSupplierRequest', { id, database }) } -export function NotifyBookmarksRequest ({ id, database, bookmarks }) { - return response('NotifyBookmarksRequest', { id, database, bookmarks }) +export function BookmarksConsumerRequest ({ id, database, bookmarks }) { + return response('BookmarksConsumerRequest', { id, database, bookmarks }) } export function Session ({ id }) { From e3883042cad93cf361a7462866b563db4b9c0621 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 14:25:59 +0200 Subject: [PATCH 22/43] Docs --- packages/core/src/bookmark-manager.ts | 61 +++++++++++++------ packages/core/src/internal/bookmarks.ts | 2 +- .../core/src/internal/connection-holder.ts | 1 + packages/neo4j-driver-lite/src/index.ts | 15 +++++ .../neo4j-driver-lite/test/unit/index.test.ts | 34 ++++++++++- packages/neo4j-driver/src/index.js | 15 +++++ 6 files changed, 106 insertions(+), 22 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 31304a0b2..7af5b7d21 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -17,47 +17,56 @@ * limitations under the License. */ -export default interface BookmarkManager { - /* Method called when the bookmarks get updated when a transaction finished. - * - * This method will be called during when auto-commit queries finished and explicit transactions - * get commited. - * @param database The database which the bookmarks belongs to - * @param previousBookmarks The bookmarks used during the transaction creation - * @param newBookmarks The new bookmarks resolved at the end of the transaction. - * @returns {void} +/** + * Interface for the piece of software responsible for keeping track of current active bookmarks accross the driver. + * @interface + */ +export default class BookmarkManager { + /** + * Method called when the bookmarks get updated when a transaction finished. + * + * This method will be called during when auto-commit queries finished and explicit transactions + * get commited. + * @param {string} database The database which the bookmarks belongs to + * @param {string[]} previousBookmarks The bookmarks used during the transaction creation + * @param {string[]} newBookmarks The new bookmarks resolved at the end of the transaction. + * @returns {void} */ - updateBookmarks: (database: string, previousBookmarks: string[], newBookmarks: string[]) => void + updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + throw new Error('Not implemented') + } /** * Method called by the driver to get the bookmark for one specific database * - * @param database The database which the bookmarks belongs to + * @param {string} database The database which the bookmarks belongs to * @returns {string[]} The set of bookmarks */ - getBookmarks: (database: string) => string[] + getBookmarks (database: string): string[] { + throw new Error('Not implemented') + } /** * Method called by the driver for getting all the bookmarks. * * The return of this method should be all the bookmarks present in the BookmarkManager for all databases. - * The databases informed in the method call will be used for enriching the bookmark set by enforcing the bookmark - * manager calls `bookmarkSupplier` for these database names even though this database are not present in the bookmark - * manager map yet. * - * @param mustIncludedDatabases The database which must be included in the result even if they don't have be initialized yet. * @returns {string[]} The set of bookmarks */ - getAllBookmarks: () => string[] + getAllBookmarks (): string[] { + throw new Error('Not implemented') + } /** * Forget the databases and its bookmarks * * This method is not called by the driver. Forgetting unused databases is the user's responsibility. * - * @param databases The databases which the bookmarks will be removed for. + * @param {string[]} databases The databases which the bookmarks will be removed for. */ - forget: (databases: string[]) => void + forget (databases: string[]): void { + throw new Error('Not implemented') + } } export interface BookmarkManagerConfig { @@ -66,6 +75,20 @@ export interface BookmarkManagerConfig { bookmarksConsumer?: (database: string, bookmarks: string[]) => void } +/** + * @typedef {Object} BookmarkManagerConfig + * @property {Map} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {function([database]: string):string[]} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager + * 1. supplying bookmarks from the given database when the default BookmarkManager's `.getBookmarks(database)` gets called. + * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called + * @property {function(database: string, bookmarks: string[]): void} [bookmarksConsumer] Called when the set of bookmarks for database get updated + */ +/** + * Provides an configured {@link BookmarkManager} instance. + * + * @param {BookmarkManagerConfig} [config={}] + * @returns {BookmarkManager} + */ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { const initialBookmarks = new Map>() diff --git a/packages/core/src/internal/bookmarks.ts b/packages/core/src/internal/bookmarks.ts index 0c0222e43..568793fce 100644 --- a/packages/core/src/internal/bookmarks.ts +++ b/packages/core/src/internal/bookmarks.ts @@ -52,7 +52,7 @@ export class Bookmarks { return this._values } - [Symbol.iterator] (): Iterable { + [Symbol.iterator] (): IterableIterator { return this._values[Symbol.iterator]() } diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index 4c458d303..6f1a3843d 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -96,6 +96,7 @@ class ConnectionHolder implements ConnectionHolderInterface { * @property {ConnectionProvider} params.connectionProvider - the connection provider to acquire connections from. * @property {string?} params.impersonatedUser - the user which will be impersonated * @property {function(databaseName:string)} params.onDatabaseNameResolved - callback called when the database name is resolved + * @property {[BookmarkManager]} params.bookmarkManager - the bookmark manager used in the session */ constructor ({ mode = ACCESS_MODE_WRITE, diff --git a/packages/neo4j-driver-lite/src/index.ts b/packages/neo4j-driver-lite/src/index.ts index 7f411af88..39b1187e2 100644 --- a/packages/neo4j-driver-lite/src/index.ts +++ b/packages/neo4j-driver-lite/src/index.ts @@ -202,6 +202,21 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * + * // Configure a BookmarkManager for in the driver + * // + * // BookmarkManager is a piece of software responsible for keeping casual concistency between different sessions by sharing bookmarks + * // between the them. + * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. + * // A default implementation could be acquired by calling bookmarkManager factory function. + * // + * // **Warning**: Enabling Bookmark Manager can have a negative impact performance wise since all the queries will wait for the last changes + * // being propagated accross the cluster. + * // For keeping concistency between a group of queries, use Session for grouping them. + * // + * // Example: + * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) + * bookmarkManager: undefined, // Disabled + * * // Specify a custom server address resolver function used by the routing driver to resolve the initial address used to create the driver. * // Such resolution happens: * // * during the very first rediscovery when driver is created diff --git a/packages/neo4j-driver-lite/test/unit/index.test.ts b/packages/neo4j-driver-lite/test/unit/index.test.ts index 9899a1e16..195031a40 100644 --- a/packages/neo4j-driver-lite/test/unit/index.test.ts +++ b/packages/neo4j-driver-lite/test/unit/index.test.ts @@ -16,7 +16,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { +import neo4j, { Result, Record, QueryResult, @@ -49,7 +49,8 @@ import { Time, Date, LocalDateTime, - DateTime + DateTime, + BookmarkManager } from '../../' describe('index', () => { @@ -84,6 +85,35 @@ describe('index', () => { expect(resultSummary).toBeDefined() }) + it('should export neo4j.bookmarkManager', () => { + const bookmarkManager = neo4j.bookmarkManager() + + expect(bookmarkManager).toBeDefined() + }) + + it('should treat BookmarkManager as an interface', () => { + const bookmarkManager: BookmarkManager = { + getAllBookmarks (): string[] { + return [] + }, + getBookmarks (database: string): string[] { + return [] + }, + updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + + }, + forget (databases: string[]): void { + + } + } + + const driver = neo4j.driver('neo4j://localhost', neo4j.auth.basic('neo4j', 'neo4i'), { + bookmarkManager + }) + + expect(driver).toBeDefined() + }) + it('should export AuthToken', () => { const authToken: AuthToken = { scheme: 'basic', diff --git a/packages/neo4j-driver/src/index.js b/packages/neo4j-driver/src/index.js index 101e5ab6e..beb3a2962 100644 --- a/packages/neo4j-driver/src/index.js +++ b/packages/neo4j-driver/src/index.js @@ -186,6 +186,21 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * + * // Configure a BookmarkManager for in the driver + * // + * // BookmarkManager is a piece of software responsible for keeping casual concistency between different sessions by sharing bookmarks + * // between the them. + * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. + * // A default implementation could be acquired by calling bookmarkManager factory function. + * // + * // **Warning**: Enabling Bookmark Manager can have a negative impact performance wise since all the queries will wait for the last changes + * // being propagated accross the cluster. + * // For keeping concistency between a group of queries, use Session for grouping them. + * // + * // Example: + * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) + * bookmarkManager: undefined, // Disabled + * * // Specify a custom server address resolver function used by the routing driver to resolve the initial address used to create the driver. * // Such resolution happens: * // * during the very first rediscovery when driver is created From 748a8d02bcc2d0d7e7036f55a44a61700a576963 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 14:35:36 +0200 Subject: [PATCH 23/43] Fix deno issues --- packages/core/src/transaction.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index 2def5278e..46088bcc5 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -51,7 +51,7 @@ class Transaction { private readonly _onBookmarks: (newBookmarks: Bookmarks, previousBookmarks: Bookmarks, database?: string) => void private readonly _onConnection: () => void private readonly _onError: (error: Error) => Promise - private readonly _onComplete: (metadata: any) => void + private readonly _onComplete: (metadata: any, previousBookmarks?: Bookmarks) => void private readonly _fetchSize: number private readonly _results: any[] private readonly _impersonatedUser?: string @@ -274,7 +274,7 @@ class Transaction { * @param {object} meta The meta with bookmarks * @returns {void} */ - _onCompleteCallback (meta: { bookmark?: string | string[], db?: string }, previousBookmarks: Bookmarks): void { + _onCompleteCallback (meta: { bookmark?: string | string[], db?: string }, previousBookmarks?: Bookmarks): void { this._onBookmarks(new Bookmarks(meta?.bookmark), previousBookmarks ?? Bookmarks.empty(), meta?.db) } } From 9d07673ddaf5b08b5800425fa29ba339a36cda7e Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 15:55:44 +0200 Subject: [PATCH 24/43] Avoiding BookmarkManager interface instantiation --- packages/core/src/bookmark-manager.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 7af5b7d21..31b640639 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -22,6 +22,10 @@ * @interface */ export default class BookmarkManager { + constructor () { + throw new Error('Not implemented') + } + /** * Method called when the bookmarks get updated when a transaction finished. * From 66786d77eb884be0a8730b6e5efd00dd934f13c8 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 16:00:08 +0200 Subject: [PATCH 25/43] @ts-ingore Error contructor --- packages/core/src/error.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/core/src/error.ts b/packages/core/src/error.ts index d1212acc3..7b565fee6 100644 --- a/packages/core/src/error.ts +++ b/packages/core/src/error.ts @@ -71,7 +71,8 @@ class Neo4jError extends Error { * @param {string} code - Optional error code. Will be populated when error originates in the database. */ constructor (message: string, code: Neo4jErrorCode, cause?: Error) { - // @ts-expect-error + // eslint-disable-next-line + // @ts-ignore: not available in ES6 yet super(message, cause != null ? { cause } : undefined) this.constructor = Neo4jError // eslint-disable-next-line no-proto From 0ac2b57c43026eebc4dcd5377d5bf7187c81d1e4 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 11 Aug 2022 17:41:27 +0200 Subject: [PATCH 26/43] Fix typo --- packages/testkit-backend/src/request-handlers.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index aad54005e..ca93fca0e 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -103,7 +103,7 @@ export function NewDriver (context, data, wire) { return [] } } - if (bmmConfig.bookmarksConsumerRegistred === true) { + if (bmmConfig.bookmarksConsumerRegistered === true) { bookmarksConsumer = (database, bookmarks) => { const notifier = () => new Promise((resolve, reject) => { From 9354389c7d3fd0804ae165c037b140a248e34bdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 19 Aug 2022 10:12:52 +0200 Subject: [PATCH 27/43] Apply suggestions from code review Co-authored-by: Robsdedude --- packages/core/src/bookmark-manager.ts | 10 +++++----- packages/core/test/session.test.ts | 10 +++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 31b640639..9d2d6a22f 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -32,8 +32,8 @@ export default class BookmarkManager { * This method will be called during when auto-commit queries finished and explicit transactions * get commited. * @param {string} database The database which the bookmarks belongs to - * @param {string[]} previousBookmarks The bookmarks used during the transaction creation - * @param {string[]} newBookmarks The new bookmarks resolved at the end of the transaction. + * @param {string[]} previousBookmarks The bookmarks used when starting the transaction + * @param {string[]} newBookmarks The new bookmarks received at the end of the transaction. * @returns {void} */ updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { @@ -41,9 +41,9 @@ export default class BookmarkManager { } /** - * Method called by the driver to get the bookmark for one specific database + * Method called by the driver to get the bookmarks for one specific database * - * @param {string} database The database which the bookmarks belongs to + * @param {string} database The database which the bookmarks belong to * @returns {string[]} The set of bookmarks */ getBookmarks (database: string): string[] { @@ -53,7 +53,7 @@ export default class BookmarkManager { /** * Method called by the driver for getting all the bookmarks. * - * The return of this method should be all the bookmarks present in the BookmarkManager for all databases. + * This method should return all bookmarks for all databases present in the BookmarkManager. * * @returns {string[]} The set of bookmarks */ diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 63b1d5bda..34aecd067 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -423,7 +423,7 @@ describe('session', () => { [undefined], ['neo4j'], ['adb'] - ])('should not call run updateBookmarks when bookmarks since it didnt have bookmarks', async (metaDb) => { + ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -457,7 +457,7 @@ describe('session', () => { [undefined, 'neo4j'], ['neo4j', 'neo4j'], ['adb', 'adb'] - ])('should call run updateBookmarks when bookmarks is not empty', async (metaDb, updateDb) => { + ])('should call updateBookmarks when server returns non-empty bookmarks', async (metaDb, updateDb) => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -487,7 +487,7 @@ describe('session', () => { [undefined], ['neo4j'], ['adb'] - ])('should not call run updateBookmarks when bookmarks is empty', async (metaDb) => { + ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -660,7 +660,7 @@ describe('session', () => { [undefined, 'neo4j'], ['neo4j', 'neo4j'], ['adb', 'adb'] - ])('should call run updateBookmarks when bookmarks is not empty', async (metaDb, updateDb) => { + ])('should call updateBookmarks when server returns non-empty bookmarks', async (metaDb, updateDb) => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -692,7 +692,7 @@ describe('session', () => { [undefined], ['neo4j'], ['adb'] - ])('should not call run updateBookmarks when bookmarks is empty', async (metaDb) => { + ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], From d0cebe3547f440d50c8001f3cc770d96f2b164f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 19 Aug 2022 10:26:06 +0200 Subject: [PATCH 28/43] Apply suggestions from code review Co-authored-by: Robsdedude --- packages/core/src/bookmark-manager.ts | 2 +- packages/core/test/bookmark-manager.test.ts | 8 ++++---- packages/neo4j-driver-lite/src/index.ts | 12 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 9d2d6a22f..1b51cf6ac 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -84,7 +84,7 @@ export interface BookmarkManagerConfig { * @property {Map} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. * @property {function([database]: string):string[]} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * 1. supplying bookmarks from the given database when the default BookmarkManager's `.getBookmarks(database)` gets called. - * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called + * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called * @property {function(database: string, bookmarks: string[]): void} [bookmarksConsumer] Called when the set of bookmarks for database get updated */ /** diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 81406e2e6..d670bf560 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -89,7 +89,7 @@ describe('BookmarkManager', () => { expect(bookmarks).toEqual([...neo4jBookmarks, ...systemBookmarks]) }) - it('should return empty if there isnt bookmarks for any db', () => { + it('should return empty if there are no bookmarks for any db', () => { const manager = bookmarkManager({}) const bookmarks = manager.getAllBookmarks() @@ -97,7 +97,7 @@ describe('BookmarkManager', () => { expect(bookmarks).toEqual([]) }) - it('should return enrich bookmarks list with supplied bookmarks', () => { + it('should return enriched bookmarks list with supplied bookmarks', () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] const bookmarksSupplier = jest.fn((database?: string) => extraBookmarks) const manager = bookmarkManager({ @@ -132,7 +132,7 @@ describe('BookmarkManager', () => { }) describe('updateBookmarks()', () => { - it('should remove previous bookmarks and new bookmarks an existing db', () => { + it('should remove previous bookmarks and new bookmarks for an existing db', () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -191,7 +191,7 @@ describe('BookmarkManager', () => { }) describe('forget()', () => { - it('should forgot database', () => { + it('should forget database', () => { const extraBookmarks = ['system:bmextra', 'adb:bmextra'] const bookmarksSupplier = jest.fn(() => extraBookmarks) const manager = bookmarkManager({ diff --git a/packages/neo4j-driver-lite/src/index.ts b/packages/neo4j-driver-lite/src/index.ts index 39b1187e2..5a01295f2 100644 --- a/packages/neo4j-driver-lite/src/index.ts +++ b/packages/neo4j-driver-lite/src/index.ts @@ -202,16 +202,16 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * - * // Configure a BookmarkManager for in the driver + * // Configure a BookmarkManager for the driver to use * // - * // BookmarkManager is a piece of software responsible for keeping casual concistency between different sessions by sharing bookmarks + * // A BookmarkManager is a piece of software responsible for keeping casual consistency between different sessions by sharing bookmarks * // between the them. * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. - * // A default implementation could be acquired by calling bookmarkManager factory function. + * // A default implementation could be acquired by calling the factory function bookmarkManager. * // - * // **Warning**: Enabling Bookmark Manager can have a negative impact performance wise since all the queries will wait for the last changes - * // being propagated accross the cluster. - * // For keeping concistency between a group of queries, use Session for grouping them. + * // **Warning**: Enabling the BookmarkManager can have a negative impact on performance since all the queries will wait for the latest changes + * // being propagated across the cluster. + * // For keeping consistency between a group of queries, use Session for grouping them. * // * // Example: * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) From 4a138c10ee0d7acef407469f24af50cbbbe681fe Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 19 Aug 2022 12:13:26 +0200 Subject: [PATCH 29/43] Tweak the interface for not returning duplication --- packages/core/src/bookmark-manager.ts | 69 +++++++++-------- packages/core/src/internal/bookmarks.ts | 4 +- packages/core/test/bookmark-manager.test.ts | 82 ++++++++++++++++++++- packages/core/test/session.test.ts | 6 +- packages/neo4j-driver/src/index.js | 12 +-- 5 files changed, 126 insertions(+), 47 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 1b51cf6ac..29becffd9 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -32,11 +32,11 @@ export default class BookmarkManager { * This method will be called during when auto-commit queries finished and explicit transactions * get commited. * @param {string} database The database which the bookmarks belongs to - * @param {string[]} previousBookmarks The bookmarks used when starting the transaction - * @param {string[]} newBookmarks The new bookmarks received at the end of the transaction. + * @param {Iterable} previousBookmarks The bookmarks used when starting the transaction + * @param {Iterable} newBookmarks The new bookmarks received at the end of the transaction. * @returns {void} */ - updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): void { throw new Error('Not implemented') } @@ -44,9 +44,9 @@ export default class BookmarkManager { * Method called by the driver to get the bookmarks for one specific database * * @param {string} database The database which the bookmarks belong to - * @returns {string[]} The set of bookmarks + * @returns {Iterable} The set of bookmarks */ - getBookmarks (database: string): string[] { + getBookmarks (database: string): Iterable { throw new Error('Not implemented') } @@ -55,9 +55,9 @@ export default class BookmarkManager { * * This method should return all bookmarks for all databases present in the BookmarkManager. * - * @returns {string[]} The set of bookmarks + * @returns {Iterable} The set of bookmarks */ - getAllBookmarks (): string[] { + getAllBookmarks (): Iterable { throw new Error('Not implemented') } @@ -66,26 +66,26 @@ export default class BookmarkManager { * * This method is not called by the driver. Forgetting unused databases is the user's responsibility. * - * @param {string[]} databases The databases which the bookmarks will be removed for. + * @param {Iterable} databases The databases which the bookmarks will be removed for. */ - forget (databases: string[]): void { + forget (databases: Iterable): void { throw new Error('Not implemented') } } export interface BookmarkManagerConfig { - initialBookmarks?: Map - bookmarksSupplier?: (database?: string) => string[] - bookmarksConsumer?: (database: string, bookmarks: string[]) => void + initialBookmarks?: Map> + bookmarksSupplier?: (database?: string) => Iterable + bookmarksConsumer?: (database: string, bookmarks: Iterable) => void } /** * @typedef {Object} BookmarkManagerConfig - * @property {Map} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. - * @property {function([database]: string):string[]} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager + * @property {Map>} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {function([database]: string):Iterable} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * 1. supplying bookmarks from the given database when the default BookmarkManager's `.getBookmarks(database)` gets called. * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called - * @property {function(database: string, bookmarks: string[]): void} [bookmarksConsumer] Called when the set of bookmarks for database get updated + * @property {function(database: string, bookmarks: Iterable): void} [bookmarksConsumer] Called when the set of bookmarks for database get updated */ /** * Provides an configured {@link BookmarkManager} instance. @@ -108,17 +108,20 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa class Neo4jBookmarkManager implements BookmarkManager { constructor ( private readonly _bookmarksPerDb: Map>, - private readonly _bookmarksSupplier?: (database?: string) => string[], - private readonly _bookmarksConsumer?: (database: string, bookmark: string[]) => void + private readonly _bookmarksSupplier?: (database?: string) => Iterable, + private readonly _bookmarksConsumer?: (database: string, bookmark: Iterable) => void ) { } - updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): void { const bookmarks = this._getOrInitializeBookmarks(database) - previousBookmarks.forEach(bm => bookmarks.delete(bm)) - newBookmarks.forEach(bm => bookmarks.add(bm)) - + for (const bm of previousBookmarks) { + bookmarks.delete(bm) + } + for (const bm of newBookmarks) { + bookmarks.add(bm) + } if (typeof this._bookmarksConsumer === 'function') { this._bookmarksConsumer(database, [...bookmarks]) } @@ -126,39 +129,45 @@ class Neo4jBookmarkManager implements BookmarkManager { private _getOrInitializeBookmarks (database: string): Set { let maybeBookmarks = this._bookmarksPerDb.get(database) - if (maybeBookmarks == null) { + if (maybeBookmarks === undefined) { maybeBookmarks = new Set() this._bookmarksPerDb.set(database, maybeBookmarks) } return maybeBookmarks } - getBookmarks (database: string): string[] { - const bookmarks = this._bookmarksPerDb.get(database) ?? [] + getBookmarks (database: string): Iterable { + const bookmarks = new Set(this._bookmarksPerDb.get(database)) if (typeof this._bookmarksSupplier === 'function') { const suppliedBookmarks = this._bookmarksSupplier(database) ?? [] - return [...bookmarks, ...suppliedBookmarks] + for (const bm of suppliedBookmarks) { + bookmarks.add(bm) + } } return [...bookmarks] } - getAllBookmarks (): string[] { - const bookmarks = [] + getAllBookmarks (): Iterable { + const bookmarks = new Set() for (const [, dbBookmarks] of this._bookmarksPerDb) { - bookmarks.push(...dbBookmarks) + for (const bm of dbBookmarks) { + bookmarks.add(bm) + } } if (typeof this._bookmarksSupplier === 'function') { const suppliedBookmarks = this._bookmarksSupplier() ?? [] - bookmarks.push(...suppliedBookmarks) + for (const bm of suppliedBookmarks) { + bookmarks.add(bm) + } } return bookmarks } - forget (databases: string[]): void { + forget (databases: Iterable): void { for (const database of databases) { this._bookmarksPerDb.delete(database) } diff --git a/packages/core/src/internal/bookmarks.ts b/packages/core/src/internal/bookmarks.ts index 568793fce..411ee7cae 100644 --- a/packages/core/src/internal/bookmarks.ts +++ b/packages/core/src/internal/bookmarks.ts @@ -28,7 +28,7 @@ export class Bookmarks { * @constructor * @param {string|string[]} values single bookmark as string or multiple bookmarks as a string array. */ - constructor (values?: string | string[] | string[] | null) { + constructor (values?: string | string[] | null) { this._values = asStringArray(values) } @@ -83,7 +83,7 @@ const EMPTY_BOOKMARK = new Bookmarks(null) * @return {string[]} value converted to an array. */ function asStringArray ( - value?: string | string[] | string[] | null + value?: string | string[] | null ): string[] { if (value == null || value === '') { return [] diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index d670bf560..fcf630e83 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -62,6 +62,22 @@ describe('BookmarkManager', () => { expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) }) + it('should return not duplicate bookmarks if bookmarkSupplier returns existing bm', () => { + const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] + + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarksSupplier: () => [...extraBookmarks, ...neo4jBookmarks] + }) + + const bookmarks = manager.getBookmarks('neo4j') + + expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) + }) + it('should return call from bookmarkSupplier with correct database', () => { const bookmarksSupplier = jest.fn() @@ -86,7 +102,7 @@ describe('BookmarkManager', () => { const bookmarks = manager.getAllBookmarks() - expect(bookmarks).toEqual([...neo4jBookmarks, ...systemBookmarks]) + expect([...bookmarks]).toEqual([...neo4jBookmarks, ...systemBookmarks]) }) it('should return empty if there are no bookmarks for any db', () => { @@ -94,7 +110,7 @@ describe('BookmarkManager', () => { const bookmarks = manager.getAllBookmarks() - expect(bookmarks).toEqual([]) + expect([...bookmarks]).toEqual([]) }) it('should return enriched bookmarks list with supplied bookmarks', () => { @@ -110,7 +126,25 @@ describe('BookmarkManager', () => { const bookmarks = manager.getAllBookmarks() - expect(bookmarks.sort()).toEqual( + expect([...bookmarks].sort()).toEqual( + [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() + ) + }) + + it('should return duplicate bookmarks if bookmarksSupplier returns already existing bm', () => { + const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] + const bookmarksSupplier = jest.fn((database?: string) => [...extraBookmarks, ...systemBookmarks]) + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarksSupplier + }) + + const bookmarks = manager.getAllBookmarks() + + expect([...bookmarks].sort()).toEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() ) }) @@ -151,6 +185,27 @@ describe('BookmarkManager', () => { expect(manager.getBookmarks('system')).toEqual(systemBookmarks) }) + it('should not remove bookmarks not present in the original list', () => { + const newBookmarks = ['neo4j:bm03'] + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const [bookmarkNotUsedInTx, ...bookmarksUsedInTx] = neo4jBookmarks + manager.updateBookmarks( + 'neo4j', + bookmarksUsedInTx, + newBookmarks + ) + + expect(manager.getBookmarks('neo4j')) + .toEqual([bookmarkNotUsedInTx, ...newBookmarks]) + expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + }) + it('should add bookmarks to a non-existing database', () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ @@ -205,9 +260,28 @@ describe('BookmarkManager', () => { manager.forget(['neo4j', 'adb']) const bookmarks = manager.getAllBookmarks() - expect(bookmarks.sort()).toEqual( + expect([...bookmarks].sort()).toEqual( [...systemBookmarks, ...extraBookmarks].sort() ) }) + + it('should forget what never reminded', () => { + const extraBookmarks = ['system:bmextra', 'adb:bmextra'] + const bookmarksSupplier = jest.fn(() => extraBookmarks) + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]), + bookmarksSupplier + }) + + manager.forget(['unexisting-db']) + const bookmarks = manager.getAllBookmarks() + + expect([...bookmarks].sort()).toEqual( + [...systemBookmarks, ...neo4jBookmarks, ...extraBookmarks].sort() + ) + }) }) }) diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 34aecd067..6350c7d2e 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -442,11 +442,7 @@ describe('session', () => { database: 'neo4j' }) - await session.run('query') - - const { afterComplete } = connection.seenProtocolOptions[0] - - afterComplete({ db: metaDb }) + await session.beginTransaction() expect(updateBookmarksSpy).not.toBeCalled() }) diff --git a/packages/neo4j-driver/src/index.js b/packages/neo4j-driver/src/index.js index beb3a2962..810172ea6 100644 --- a/packages/neo4j-driver/src/index.js +++ b/packages/neo4j-driver/src/index.js @@ -186,16 +186,16 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * - * // Configure a BookmarkManager for in the driver + * // Configure a BookmarkManager for the driver to use * // - * // BookmarkManager is a piece of software responsible for keeping casual concistency between different sessions by sharing bookmarks + * // A BookmarkManager is a piece of software responsible for keeping casual consistency between different sessions by sharing bookmarks * // between the them. * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. - * // A default implementation could be acquired by calling bookmarkManager factory function. + * // A default implementation could be acquired by calling the factory function bookmarkManager. * // - * // **Warning**: Enabling Bookmark Manager can have a negative impact performance wise since all the queries will wait for the last changes - * // being propagated accross the cluster. - * // For keeping concistency between a group of queries, use Session for grouping them. + * // **Warning**: Enabling the BookmarkManager can have a negative impact on performance since all the queries will wait for the latest changes + * // being propagated across the cluster. + * // For keeping consistency between a group of queries, use Session for grouping them. * // * // Example: * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) From 33818186b948d8d69b851ff5f5d599370b9fa258 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Fri, 19 Aug 2022 12:17:50 +0200 Subject: [PATCH 30/43] Update packages/core/src/bookmark-manager.ts Co-authored-by: Robsdedude --- packages/core/src/bookmark-manager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 29becffd9..e8545908c 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -29,7 +29,7 @@ export default class BookmarkManager { /** * Method called when the bookmarks get updated when a transaction finished. * - * This method will be called during when auto-commit queries finished and explicit transactions + * This method will be called when auto-commit queries finish and when explicit transactions * get commited. * @param {string} database The database which the bookmarks belongs to * @param {Iterable} previousBookmarks The bookmarks used when starting the transaction From 5e26a267cccd69a49e04eb8c8b08952bae8c8a4a Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 19 Aug 2022 15:01:35 +0200 Subject: [PATCH 31/43] AsyncBookmarkManager --- packages/core/src/bookmark-manager.ts | 34 +++---- .../core/src/internal/connection-holder.ts | 22 +++-- packages/core/src/session.ts | 10 +- packages/core/src/transaction-promise.ts | 2 +- packages/core/src/transaction.ts | 6 +- packages/core/test/bookmark-manager.test.ts | 98 +++++++++---------- packages/core/test/transaction.test.ts | 8 +- .../testkit-backend/src/request-handlers.js | 27 ++--- .../src/skipped-tests/common.js | 4 - 9 files changed, 102 insertions(+), 109 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index e8545908c..3b4d2c246 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -36,7 +36,7 @@ export default class BookmarkManager { * @param {Iterable} newBookmarks The new bookmarks received at the end of the transaction. * @returns {void} */ - updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): void { + async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { throw new Error('Not implemented') } @@ -46,7 +46,7 @@ export default class BookmarkManager { * @param {string} database The database which the bookmarks belong to * @returns {Iterable} The set of bookmarks */ - getBookmarks (database: string): Iterable { + async getBookmarks (database: string): Promise> { throw new Error('Not implemented') } @@ -57,7 +57,7 @@ export default class BookmarkManager { * * @returns {Iterable} The set of bookmarks */ - getAllBookmarks (): Iterable { + async getAllBookmarks (): Promise> { throw new Error('Not implemented') } @@ -68,24 +68,24 @@ export default class BookmarkManager { * * @param {Iterable} databases The databases which the bookmarks will be removed for. */ - forget (databases: Iterable): void { + async forget (databases: Iterable): Promise { throw new Error('Not implemented') } } export interface BookmarkManagerConfig { initialBookmarks?: Map> - bookmarksSupplier?: (database?: string) => Iterable - bookmarksConsumer?: (database: string, bookmarks: Iterable) => void + bookmarksSupplier?: (database?: string) => Promise> + bookmarksConsumer?: (database: string, bookmarks: Iterable) => Promise } /** * @typedef {Object} BookmarkManagerConfig * @property {Map>} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. - * @property {function([database]: string):Iterable} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager + * @property {function([database]: string):Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * 1. supplying bookmarks from the given database when the default BookmarkManager's `.getBookmarks(database)` gets called. * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called - * @property {function(database: string, bookmarks: Iterable): void} [bookmarksConsumer] Called when the set of bookmarks for database get updated + * @property {function(database: string, bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks for database get updated */ /** * Provides an configured {@link BookmarkManager} instance. @@ -108,13 +108,13 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa class Neo4jBookmarkManager implements BookmarkManager { constructor ( private readonly _bookmarksPerDb: Map>, - private readonly _bookmarksSupplier?: (database?: string) => Iterable, - private readonly _bookmarksConsumer?: (database: string, bookmark: Iterable) => void + private readonly _bookmarksSupplier?: (database?: string) => Promise>, + private readonly _bookmarksConsumer?: (database: string, bookmark: Iterable) => Promise ) { } - updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): void { + async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { const bookmarks = this._getOrInitializeBookmarks(database) for (const bm of previousBookmarks) { bookmarks.delete(bm) @@ -123,7 +123,7 @@ class Neo4jBookmarkManager implements BookmarkManager { bookmarks.add(bm) } if (typeof this._bookmarksConsumer === 'function') { - this._bookmarksConsumer(database, [...bookmarks]) + await this._bookmarksConsumer(database, [...bookmarks]) } } @@ -136,11 +136,11 @@ class Neo4jBookmarkManager implements BookmarkManager { return maybeBookmarks } - getBookmarks (database: string): Iterable { + async getBookmarks (database: string): Promise> { const bookmarks = new Set(this._bookmarksPerDb.get(database)) if (typeof this._bookmarksSupplier === 'function') { - const suppliedBookmarks = this._bookmarksSupplier(database) ?? [] + const suppliedBookmarks = await this._bookmarksSupplier(database) ?? [] for (const bm of suppliedBookmarks) { bookmarks.add(bm) } @@ -149,7 +149,7 @@ class Neo4jBookmarkManager implements BookmarkManager { return [...bookmarks] } - getAllBookmarks (): Iterable { + async getAllBookmarks (): Promise> { const bookmarks = new Set() for (const [, dbBookmarks] of this._bookmarksPerDb) { @@ -158,7 +158,7 @@ class Neo4jBookmarkManager implements BookmarkManager { } } if (typeof this._bookmarksSupplier === 'function') { - const suppliedBookmarks = this._bookmarksSupplier() ?? [] + const suppliedBookmarks = await this._bookmarksSupplier() ?? [] for (const bm of suppliedBookmarks) { bookmarks.add(bm) } @@ -167,7 +167,7 @@ class Neo4jBookmarkManager implements BookmarkManager { return bookmarks } - forget (databases: Iterable): void { + async forget (databases: Iterable): Promise { for (const database of databases) { this._bookmarksPerDb.delete(database) } diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index 6f1a3843d..d8379b95f 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -152,13 +152,7 @@ class ConnectionHolder implements ConnectionHolderInterface { initializeConnection (): boolean { if (this._referenceCount === 0 && (this._connectionProvider != null)) { - this._connectionPromise = this._connectionProvider.acquireConnection({ - accessMode: this._mode, - database: this._database, - bookmarks: this._getBookmarks(), - impersonatedUser: this._impersonatedUser, - onDatabaseNameResolved: this._onDatabaseNameResolved - }) + this._connectionPromise = this._createConnectionPromise(this._connectionProvider) } else { this._referenceCount++ return false @@ -167,8 +161,18 @@ class ConnectionHolder implements ConnectionHolderInterface { return true } - private _getBookmarks (): Bookmarks { - const bookmarks = this._bookmarkManager?.getBookmarks('system') ?? [] + private async _createConnectionPromise (connectionProvider: ConnectionProvider): Promise { + return await connectionProvider.acquireConnection({ + accessMode: this._mode, + database: this._database, + bookmarks: await this._getBookmarks(), + impersonatedUser: this._impersonatedUser, + onDatabaseNameResolved: this._onDatabaseNameResolved + }) + } + + private async _getBookmarks (): Promise { + const bookmarks = await this._bookmarkManager?.getBookmarks('system') ?? [] return new Bookmarks([...this._bookmarks, ...bookmarks]) } diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 9975a2104..4563f6be5 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -37,7 +37,7 @@ import TransactionPromise from './transaction-promise' import ManagedTransaction from './transaction-managed' import BookmarkManager from './bookmark-manager' -type ConnectionConsumer = (connection: Connection | null) => any | undefined +type ConnectionConsumer = (connection: Connection | null) => any | undefined | Promise | Promise type TransactionWork = (tx: Transaction) => Promise | T type ManagedTransactionWork = (tx: ManagedTransaction) => Promise | T @@ -164,8 +164,8 @@ class Session { ? new TxConfig(transactionConfig) : TxConfig.empty() - const result = this._run(validatedQuery, params, connection => { - const bookmarks = this._bookmarks() + const result = this._run(validatedQuery, params, async connection => { + const bookmarks = await this._bookmarks() this._assertSessionIsOpen() return (connection as Connection).protocol().run(validatedQuery, params, { bookmarks, @@ -338,8 +338,8 @@ class Session { return this._lastBookmarks.values() } - private _bookmarks (): Bookmarks { - const bookmarks = this._bookmarkManager?.getAllBookmarks() ?? [] + private async _bookmarks (): Promise { + const bookmarks = await this._bookmarkManager?.getAllBookmarks() ?? [] return new Bookmarks([...bookmarks, ...this._lastBookmarks]) } diff --git a/packages/core/src/transaction-promise.ts b/packages/core/src/transaction-promise.ts index ac3ae7526..04798195c 100644 --- a/packages/core/src/transaction-promise.ts +++ b/packages/core/src/transaction-promise.ts @@ -162,7 +162,7 @@ class TransactionPromise extends Transaction implements Promise { /** * @access private */ - _begin (bookmarks: () => Bookmarks, txConfig: TxConfig): void { + _begin (bookmarks: () => Promise, txConfig: TxConfig): void { return super._begin(bookmarks, txConfig, { onError: this._onBeginError.bind(this), onComplete: this._onBeginMetadata.bind(this) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index 46088bcc5..ced873b9d 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -115,16 +115,16 @@ class Transaction { * @param {TxConfig} txConfig * @returns {void} */ - _begin (getBookmarks: () => Bookmarks, txConfig: TxConfig, events?: { + _begin (getBookmarks: () => Promise, txConfig: TxConfig, events?: { onError: (error: Error) => void onComplete: (metadata: any) => void }): void { this._connectionHolder .getConnection() - .then(connection => { - this._bookmarks = getBookmarks() + .then(async connection => { this._onConnection() if (connection != null) { + this._bookmarks = await getBookmarks() return connection.protocol().beginTransaction({ bookmarks: this._bookmarks, txConfig: txConfig, diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index fcf630e83..5345ee43c 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -25,15 +25,15 @@ describe('BookmarkManager', () => { const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm02'] describe('getBookmarks()', () => { - it('should return empty if db doesnt exists', () => { + it('should return empty if db doesnt exists', async () => { const manager = bookmarkManager({}) - const bookmarks = manager.getBookmarks('neo4j') + const bookmarks = await manager.getBookmarks('neo4j') expect(bookmarks).toEqual([]) }) - it('should return bookmarks for the given db', () => { + it('should return bookmarks for the given db', async () => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -41,12 +41,12 @@ describe('BookmarkManager', () => { ]) }) - const bookmarks = manager.getBookmarks('neo4j') + const bookmarks = await manager.getBookmarks('neo4j') expect(bookmarks).toEqual(neo4jBookmarks) }) - it('should return get bookmarks from bookmarkSupplier', () => { + it('should return get bookmarks from bookmarkSupplier', async () => { const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] const manager = bookmarkManager({ @@ -54,15 +54,15 @@ describe('BookmarkManager', () => { ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarksSupplier: () => extraBookmarks + bookmarksSupplier: async () => await Promise.resolve(extraBookmarks) }) - const bookmarks = manager.getBookmarks('neo4j') + const bookmarks = await manager.getBookmarks('neo4j') expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) }) - it('should return not duplicate bookmarks if bookmarkSupplier returns existing bm', () => { + it('should return not duplicate bookmarks if bookmarkSupplier returns existing bm', async () => { const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] const manager = bookmarkManager({ @@ -70,29 +70,29 @@ describe('BookmarkManager', () => { ['neo4j', neo4jBookmarks], ['system', systemBookmarks] ]), - bookmarksSupplier: () => [...extraBookmarks, ...neo4jBookmarks] + bookmarksSupplier: async () => await Promise.resolve([...extraBookmarks, ...neo4jBookmarks]) }) - const bookmarks = manager.getBookmarks('neo4j') + const bookmarks = await manager.getBookmarks('neo4j') expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) }) - it('should return call from bookmarkSupplier with correct database', () => { + it('should return call from bookmarkSupplier with correct database', async () => { const bookmarksSupplier = jest.fn() const manager = bookmarkManager({ bookmarksSupplier }) - manager.getBookmarks('neo4j') + await manager.getBookmarks('neo4j') expect(bookmarksSupplier).toBeCalledWith('neo4j') }) }) describe('getAllBookmarks()', () => { - it('should return all bookmarks ', () => { + it('should return all bookmarks ', async () => { const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -100,22 +100,22 @@ describe('BookmarkManager', () => { ]) }) - const bookmarks = manager.getAllBookmarks() + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks]).toEqual([...neo4jBookmarks, ...systemBookmarks]) }) - it('should return empty if there are no bookmarks for any db', () => { + it('should return empty if there are no bookmarks for any db', async () => { const manager = bookmarkManager({}) - const bookmarks = manager.getAllBookmarks() + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks]).toEqual([]) }) - it('should return enriched bookmarks list with supplied bookmarks', () => { + it('should return enriched bookmarks list with supplied bookmarks', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn((database?: string) => extraBookmarks) + const bookmarksSupplier = jest.fn(async (database?: string) => await Promise.resolve(extraBookmarks)) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -124,16 +124,16 @@ describe('BookmarkManager', () => { bookmarksSupplier }) - const bookmarks = manager.getAllBookmarks() + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks].sort()).toEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() ) }) - it('should return duplicate bookmarks if bookmarksSupplier returns already existing bm', () => { + it('should return duplicate bookmarks if bookmarksSupplier returns already existing bm', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn((database?: string) => [...extraBookmarks, ...systemBookmarks]) + const bookmarksSupplier = jest.fn(async (database?: string) => await Promise.resolve([...extraBookmarks, ...systemBookmarks])) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -142,14 +142,14 @@ describe('BookmarkManager', () => { bookmarksSupplier }) - const bookmarks = manager.getAllBookmarks() + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks].sort()).toEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() ) }) - it('should call bookmarkSupplier for getting all bookmarks', () => { + it('should call bookmarkSupplier for getting all bookmarks', async () => { const bookmarksSupplier = jest.fn() const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -159,14 +159,14 @@ describe('BookmarkManager', () => { bookmarksSupplier }) - manager.getAllBookmarks() + await manager.getAllBookmarks() expect(bookmarksSupplier).toBeCalledWith() }) }) describe('updateBookmarks()', () => { - it('should remove previous bookmarks and new bookmarks for an existing db', () => { + it('should remove previous bookmarks and new bookmarks for an existing db', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -175,17 +175,17 @@ describe('BookmarkManager', () => { ]) }) - manager.updateBookmarks( + await manager.updateBookmarks( 'neo4j', - manager.getAllBookmarks(), + await manager.getAllBookmarks(), newBookmarks ) - expect(manager.getBookmarks('neo4j')).toEqual(newBookmarks) - expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + await expect(manager.getBookmarks('neo4j')).resolves.toEqual(newBookmarks) + await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) }) - it('should not remove bookmarks not present in the original list', () => { + it('should not remove bookmarks not present in the original list', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -195,18 +195,18 @@ describe('BookmarkManager', () => { }) const [bookmarkNotUsedInTx, ...bookmarksUsedInTx] = neo4jBookmarks - manager.updateBookmarks( + await manager.updateBookmarks( 'neo4j', bookmarksUsedInTx, newBookmarks ) - expect(manager.getBookmarks('neo4j')) - .toEqual([bookmarkNotUsedInTx, ...newBookmarks]) - expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + await expect(manager.getBookmarks('neo4j')) + .resolves.toEqual([bookmarkNotUsedInTx, ...newBookmarks]) + await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) }) - it('should add bookmarks to a non-existing database', () => { + it('should add bookmarks to a non-existing database', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: new Map([ @@ -214,17 +214,17 @@ describe('BookmarkManager', () => { ]) }) - manager.updateBookmarks( + await manager.updateBookmarks( 'neo4j', [], newBookmarks ) - expect(manager.getBookmarks('neo4j')).toEqual(newBookmarks) - expect(manager.getBookmarks('system')).toEqual(systemBookmarks) + await expect(manager.getBookmarks('neo4j')).resolves.toEqual(newBookmarks) + await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) }) - it('should notify new bookmarks', () => { + it('should notify new bookmarks', async () => { const bookmarksConsumer = jest.fn() const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ @@ -235,9 +235,9 @@ describe('BookmarkManager', () => { bookmarksConsumer }) - manager.updateBookmarks( + await manager.updateBookmarks( 'neo4j', - manager.getAllBookmarks(), + await manager.getAllBookmarks(), newBookmarks ) @@ -246,9 +246,9 @@ describe('BookmarkManager', () => { }) describe('forget()', () => { - it('should forget database', () => { + it('should forget database', async () => { const extraBookmarks = ['system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(() => extraBookmarks) + const bookmarksSupplier = jest.fn(async () => extraBookmarks) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -257,17 +257,17 @@ describe('BookmarkManager', () => { bookmarksSupplier }) - manager.forget(['neo4j', 'adb']) - const bookmarks = manager.getAllBookmarks() + await manager.forget(['neo4j', 'adb']) + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks].sort()).toEqual( [...systemBookmarks, ...extraBookmarks].sort() ) }) - it('should forget what never reminded', () => { + it('should forget what never reminded', async () => { const extraBookmarks = ['system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(() => extraBookmarks) + const bookmarksSupplier = jest.fn(async () => extraBookmarks) const manager = bookmarkManager({ initialBookmarks: new Map([ ['neo4j', neo4jBookmarks], @@ -276,8 +276,8 @@ describe('BookmarkManager', () => { bookmarksSupplier }) - manager.forget(['unexisting-db']) - const bookmarks = manager.getAllBookmarks() + await manager.forget(['unexisting-db']) + const bookmarks = await manager.getAllBookmarks() expect([...bookmarks].sort()).toEqual( [...systemBookmarks, ...neo4jBookmarks, ...extraBookmarks].sort() diff --git a/packages/core/test/transaction.test.ts b/packages/core/test/transaction.test.ts index 57b49730b..486e09b73 100644 --- a/packages/core/test/transaction.test.ts +++ b/packages/core/test/transaction.test.ts @@ -151,7 +151,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection }) - tx._begin(() => Bookmarks.empty(), TxConfig.empty()) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) return [tx] } @@ -266,7 +266,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection }) - tx._begin(() => Bookmarks.empty(), TxConfig.empty()) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) return [tx, expectedError] } }) @@ -280,7 +280,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { connection: undefined }) - tx._begin(() => Bookmarks.empty(), TxConfig.empty()) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) try { await tx @@ -300,7 +300,7 @@ testTx('TransactionPromise', newTransactionPromise, () => { errorResolvingConnection: expectedError }) - tx._begin(() => Bookmarks.empty(), TxConfig.empty()) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) try { await tx diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index ca93fca0e..07eba7e01 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -93,25 +93,18 @@ export function NewDriver (context, data, wire) { initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) } if (bmmConfig.bookmarksSupplierRegistered === true) { - bookmarksSupplier = (database) => { - const supplier = () => - new Promise((resolve, reject) => { - const id = context.addBookmarkSupplierRequest(resolve, reject) - wire.writeResponse(responses.BookmarksSupplierRequest({ id, database })) - }) - supplier() - return [] - } + bookmarksSupplier = (database) => + new Promise((resolve, reject) => { + const id = context.addBookmarkSupplierRequest(resolve, reject) + wire.writeResponse(responses.BookmarksSupplierRequest({ id, database })) + }) } if (bmmConfig.bookmarksConsumerRegistered === true) { - bookmarksConsumer = (database, bookmarks) => { - const notifier = () => - new Promise((resolve, reject) => { - const id = context.addNotifyBookmarksRequest(resolve, reject) - wire.writeResponse(responses.BookmarksConsumerRequest({ id, database, bookmarks })) - }) - notifier() - } + bookmarksConsumer = (database, bookmarks) => + new Promise((resolve, reject) => { + const id = context.addNotifyBookmarksRequest(resolve, reject) + wire.writeResponse(responses.BookmarksConsumerRequest({ id, database, bookmarks })) + }) } config.bookmarkManager = neo4j.bookmarkManager({ initialBookmarks, diff --git a/packages/testkit-backend/src/skipped-tests/common.js b/packages/testkit-backend/src/skipped-tests/common.js index 84596efef..bbe75f9be 100644 --- a/packages/testkit-backend/src/skipped-tests/common.js +++ b/packages/testkit-backend/src/skipped-tests/common.js @@ -1,10 +1,6 @@ import skip, { ifEquals, ifEndsWith, endsWith, ifStartsWith, startsWith, not } from './skip' const skippedTests = [ - skip( - 'Driver does not support async call in BookmarkManager extensions', - ifEquals('stub.driver_parameters.test_bookmark_manager.TestNeo4jBookmarkManager.test_should_enrich_bookmarks_with_bookmark_supplier_result') - ), skip( 'Driver does not return offset for old DateTime implementations', ifStartsWith('stub.types.test_temporal_types.TestTemporalTypes') From 973a61c6e2aa8d6bcfc0eec67a9fc52af850bc86 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 19 Aug 2022 18:28:12 +0200 Subject: [PATCH 32/43] SortedEqual --- packages/core/test/bookmark-manager.test.ts | 43 ++++++++++++--------- packages/core/test/utils/matchers.ts | 43 +++++++++++++++++++++ 2 files changed, 67 insertions(+), 19 deletions(-) create mode 100644 packages/core/test/utils/matchers.ts diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 5345ee43c..0ff6ae73f 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -19,11 +19,16 @@ import { bookmarkManager } from '../src/bookmark-manager' +import { installMatchers } from './utils/matchers' describe('BookmarkManager', () => { const systemBookmarks = ['system:bm01', 'system:bm02'] const neo4jBookmarks = ['neo4j:bm01', 'neo4j:bm02'] + beforeAll(() => { + installMatchers() + }) + describe('getBookmarks()', () => { it('should return empty if db doesnt exists', async () => { const manager = bookmarkManager({}) @@ -43,7 +48,7 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getBookmarks('neo4j') - expect(bookmarks).toEqual(neo4jBookmarks) + expect(bookmarks).toBeSortedEqual(neo4jBookmarks) }) it('should return get bookmarks from bookmarkSupplier', async () => { @@ -59,7 +64,7 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getBookmarks('neo4j') - expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) + expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...extraBookmarks]) }) it('should return not duplicate bookmarks if bookmarkSupplier returns existing bm', async () => { @@ -75,7 +80,7 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getBookmarks('neo4j') - expect(bookmarks).toEqual([...neo4jBookmarks, ...extraBookmarks]) + expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...extraBookmarks]) }) it('should return call from bookmarkSupplier with correct database', async () => { @@ -102,7 +107,7 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks]).toEqual([...neo4jBookmarks, ...systemBookmarks]) + expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...systemBookmarks]) }) it('should return empty if there are no bookmarks for any db', async () => { @@ -110,7 +115,7 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks]).toEqual([]) + expect(bookmarks).toBeSortedEqual([]) }) it('should return enriched bookmarks list with supplied bookmarks', async () => { @@ -126,8 +131,8 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks].sort()).toEqual( - [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() + expect(bookmarks).toBeSortedEqual( + [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks] ) }) @@ -144,8 +149,8 @@ describe('BookmarkManager', () => { const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks].sort()).toEqual( - [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks].sort() + expect(bookmarks).toBeSortedEqual( + [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks] ) }) @@ -181,8 +186,8 @@ describe('BookmarkManager', () => { newBookmarks ) - await expect(manager.getBookmarks('neo4j')).resolves.toEqual(newBookmarks) - await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) + await expect(manager.getBookmarks('neo4j')).resolves.toBeSortedEqual(newBookmarks) + await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) }) it('should not remove bookmarks not present in the original list', async () => { @@ -202,8 +207,8 @@ describe('BookmarkManager', () => { ) await expect(manager.getBookmarks('neo4j')) - .resolves.toEqual([bookmarkNotUsedInTx, ...newBookmarks]) - await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) + .resolves.toBeSortedEqual([bookmarkNotUsedInTx, ...newBookmarks]) + await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) }) it('should add bookmarks to a non-existing database', async () => { @@ -220,8 +225,8 @@ describe('BookmarkManager', () => { newBookmarks ) - await expect(manager.getBookmarks('neo4j')).resolves.toEqual(newBookmarks) - await expect(manager.getBookmarks('system')).resolves.toEqual(systemBookmarks) + await expect(manager.getBookmarks('neo4j')).resolves.toBeSortedEqual(newBookmarks) + await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) }) it('should notify new bookmarks', async () => { @@ -260,8 +265,8 @@ describe('BookmarkManager', () => { await manager.forget(['neo4j', 'adb']) const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks].sort()).toEqual( - [...systemBookmarks, ...extraBookmarks].sort() + expect(bookmarks).toBeSortedEqual( + [...systemBookmarks, ...extraBookmarks] ) }) @@ -279,8 +284,8 @@ describe('BookmarkManager', () => { await manager.forget(['unexisting-db']) const bookmarks = await manager.getAllBookmarks() - expect([...bookmarks].sort()).toEqual( - [...systemBookmarks, ...neo4jBookmarks, ...extraBookmarks].sort() + expect(bookmarks).toBeSortedEqual( + [...systemBookmarks, ...neo4jBookmarks, ...extraBookmarks] ) }) }) diff --git a/packages/core/test/utils/matchers.ts b/packages/core/test/utils/matchers.ts new file mode 100644 index 000000000..0005280e8 --- /dev/null +++ b/packages/core/test/utils/matchers.ts @@ -0,0 +1,43 @@ +/** + * 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. + */ + +declare global { + // eslint-disable-next-line @typescript-eslint/no-namespace + namespace jest { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + interface Matchers { + toBeSortedEqual: (expected: Iterable) => CustomMatcherResult + } + } +} + +export function installMatchers (): void { + expect.extend({ + toBeSortedEqual (this: jest.MatcherContext, received: Iterable, expected: Iterable): jest.CustomMatcherResult { + const sortedReceived = [...received].sort() + const sortedExpected = [...expected].sort() + + return { + pass: this.equals(sortedReceived, sortedExpected), + message: () => + `Expected sorted:\n\n\t${JSON.stringify(sortedExpected)}\n\nGot sorted:\n\n\t${JSON.stringify(sortedReceived)}` + } + } + }) +} From 2022527331194d510641a8c74a2f2945d149f42e Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Fri, 19 Aug 2022 19:15:08 +0200 Subject: [PATCH 33/43] Do not sent configured bookmarks after receive bookmarks --- .../core/src/internal/connection-holder.ts | 16 +-- packages/core/src/session.ts | 23 +++- packages/core/test/session.test.ts | 124 ++++++++++++++++++ 3 files changed, 151 insertions(+), 12 deletions(-) diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index d8379b95f..dfb0d6960 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -24,7 +24,6 @@ import Connection from '../connection' import { ACCESS_MODE_WRITE } from './constants' import { Bookmarks } from './bookmarks' import ConnectionProvider from '../connection-provider' -import BookmarkManager from '../bookmark-manager' /** * @private @@ -84,7 +83,7 @@ class ConnectionHolder implements ConnectionHolderInterface { private _referenceCount: number private _connectionPromise: Promise private readonly _impersonatedUser?: string - private readonly _bookmarkManager?: BookmarkManager + private readonly _getConnectionAcquistionBookmarks: () => Promise private readonly _onDatabaseNameResolved?: (databaseName?: string) => void /** @@ -96,7 +95,7 @@ class ConnectionHolder implements ConnectionHolderInterface { * @property {ConnectionProvider} params.connectionProvider - the connection provider to acquire connections from. * @property {string?} params.impersonatedUser - the user which will be impersonated * @property {function(databaseName:string)} params.onDatabaseNameResolved - callback called when the database name is resolved - * @property {[BookmarkManager]} params.bookmarkManager - the bookmark manager used in the session + * @property {function():Promise} params.getConnectionAcquistionBookmarks - called for getting Bookmarks for acquiring connections */ constructor ({ mode = ACCESS_MODE_WRITE, @@ -105,7 +104,7 @@ class ConnectionHolder implements ConnectionHolderInterface { connectionProvider, impersonatedUser, onDatabaseNameResolved, - bookmarkManager + getConnectionAcquistionBookmarks }: { mode?: string database?: string @@ -113,7 +112,7 @@ class ConnectionHolder implements ConnectionHolderInterface { connectionProvider?: ConnectionProvider impersonatedUser?: string onDatabaseNameResolved?: (databaseName?: string) => void - bookmarkManager?: BookmarkManager + getConnectionAcquistionBookmarks?: () => Promise } = {}) { this._mode = mode this._database = database != null ? assertString(database, 'database') : '' @@ -123,7 +122,7 @@ class ConnectionHolder implements ConnectionHolderInterface { this._referenceCount = 0 this._connectionPromise = Promise.resolve(null) this._onDatabaseNameResolved = onDatabaseNameResolved - this._bookmarkManager = bookmarkManager + this._getConnectionAcquistionBookmarks = getConnectionAcquistionBookmarks ?? (async () => Bookmarks.empty()) } mode (): string | undefined { @@ -172,8 +171,7 @@ class ConnectionHolder implements ConnectionHolderInterface { } private async _getBookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getBookmarks('system') ?? [] - return new Bookmarks([...this._bookmarks, ...bookmarks]) + return await this._getConnectionAcquistionBookmarks() } getConnection (): Promise { @@ -245,6 +243,8 @@ export default class ReadOnlyConnectionHolder extends ConnectionHolder { mode: connectionHolder.mode(), database: connectionHolder.database(), bookmarks: connectionHolder.bookmarks(), + // @ts-expect-error + getConnectionAcquistionBookmarks: connectionHolder._getConnectionAcquistionBookmarks, connectionProvider: connectionHolder.connectionProvider() }) this._connectionHolder = connectionHolder diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 4563f6be5..dfa588d3f 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -63,6 +63,7 @@ class Session { private _open: boolean private _hasTx: boolean private _lastBookmarks: Bookmarks + private _configuredBookmarks: Bookmarks private readonly _transactionExecutor: TransactionExecutor private readonly _impersonatedUser?: string private _databaseNameResolved: boolean @@ -109,6 +110,7 @@ class Session { this._reactive = reactive this._fetchSize = fetchSize this._onDatabaseNameResolved = this._onDatabaseNameResolved.bind(this) + this._getConnectionAcquistionBookmarks = this._getConnectionAcquistionBookmarks.bind(this) this._readConnectionHolder = new ConnectionHolder({ mode: ACCESS_MODE_READ, database, @@ -116,7 +118,7 @@ class Session { connectionProvider, impersonatedUser, onDatabaseNameResolved: this._onDatabaseNameResolved, - bookmarkManager + getConnectionAcquistionBookmarks: this._getConnectionAcquistionBookmarks }) this._writeConnectionHolder = new ConnectionHolder({ mode: ACCESS_MODE_WRITE, @@ -125,12 +127,13 @@ class Session { connectionProvider, impersonatedUser, onDatabaseNameResolved: this._onDatabaseNameResolved, - bookmarkManager + getConnectionAcquistionBookmarks: this._getConnectionAcquistionBookmarks }) this._open = true this._hasTx = false this._impersonatedUser = impersonatedUser this._lastBookmarks = bookmarks ?? Bookmarks.empty() + this._configuredBookmarks = this._lastBookmarks this._transactionExecutor = _createTransactionExecutor(config) this._databaseNameResolved = this._database !== '' const calculatedWatermaks = this._calculateWatermaks() @@ -339,8 +342,11 @@ class Session { } private async _bookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getAllBookmarks() ?? [] - return new Bookmarks([...bookmarks, ...this._lastBookmarks]) + const bookmarks = await this._bookmarkManager?.getAllBookmarks() + if (bookmarks === undefined) { + return this._lastBookmarks + } + return new Bookmarks([...bookmarks, ...this._configuredBookmarks]) } /** @@ -481,6 +487,14 @@ class Session { } } + private async _getConnectionAcquistionBookmarks (): Promise { + const bookmarks = await this._bookmarkManager?.getBookmarks('system') + if (bookmarks === undefined) { + return this._lastBookmarks + } + return new Bookmarks([...this._configuredBookmarks, ...bookmarks]) + } + /** * Update value of the last bookmarks. * @private @@ -495,6 +509,7 @@ class Session { newBookmarks?.values() ?? [] ) this._lastBookmarks = newBookmarks + this._configuredBookmarks = Bookmarks.empty() } } diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 6350c7d2e..6d1d1f417 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -595,6 +595,61 @@ describe('session', () => { ) }) + it('should acquire connection with system bookmarks from the bookmark manager when bookmarks already updated', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = newFakeConnection() + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + lastBookmarks: new bookmarks.Bookmarks(customBookmarks), + database: 'neo4j' + }) + + await session.run('query') + const { afterComplete } = connection.seenProtocolOptions[0] + afterComplete({ db: 'neo4j', bookmark: ['other-bookmark'] }) + + await session.run('query') + + expect(connectionProvider.acquireConnection).toHaveBeenNthCalledWith(2, + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) })) + }) + + it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks when bookmarks not updated', async () => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + const connection = newFakeConnection() + + const { session, connectionProvider } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + lastBookmarks: new bookmarks.Bookmarks(customBookmarks), + database: 'neo4j' + }) + + await session.run('query') + const { afterComplete } = connection.seenProtocolOptions[0] + afterComplete({ db: 'neo4j', bookmark: [] }) + + await session.run('query') + + expect(connectionProvider.acquireConnection).toHaveBeenNthCalledWith(2, + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + ) + }) + it.each([ [[]], [customBookmarks] @@ -652,6 +707,75 @@ describe('session', () => { ) }) + it.each([ + [[]], + [customBookmarks] + ])('should call run query with getAllBookmarks + lastBookmarks when bookmarks not updated', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.run('query') + const { afterComplete } = connection.seenProtocolOptions[0] + afterComplete({ db: 'neo4j', bookmark: [] }) + + await session.run('query') + + expect(connection.seenProtocolOptions[1]).toEqual( + expect.objectContaining({ + bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks, ...lastBookmarks]) + }) + ) + }) + + it.each([ + [[]], + [customBookmarks] + ])('should call run query with getAllBookmarks when bookmarks updated', async (lastBookmarks) => { + const manager = bookmarkManager({ + initialBookmarks: new Map([ + ['neo4j', neo4jBookmarks], + ['system', systemBookmarks] + ]) + }) + + const connection = newFakeConnection() + + const { session } = setupSession({ + connection, + bookmarkManager: manager, + beginTx: false, + database: 'neo4j', + lastBookmarks: new bookmarks.Bookmarks(lastBookmarks) + }) + + await session.run('query') + const { afterComplete } = connection.seenProtocolOptions[0] + afterComplete({ db: 'neo4j', bookmark: 'abc' }) + await manager.updateBookmarks('neo4j', ['abc'], neo4jBookmarks) + + await session.run('query') + + expect(connection.seenProtocolOptions[1]).toEqual( + expect.objectContaining({ + bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks]) + }) + ) + }) + it.each([ [undefined, 'neo4j'], ['neo4j', 'neo4j'], From baff600288a6268ab20a1112bb2ea59574694ea3 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 23 Aug 2022 17:53:20 +0200 Subject: [PATCH 34/43] Moving configuration to the session --- packages/core/src/bookmark-manager.ts | 16 +- packages/core/src/driver.ts | 138 ++++++++++++++---- packages/core/src/index.ts | 4 +- packages/core/src/types.ts | 3 - packages/core/test/driver.test.ts | 35 +++-- packages/neo4j-driver-lite/src/index.ts | 21 +-- .../neo4j-driver-lite/test/unit/index.test.ts | 16 +- packages/neo4j-driver/src/driver.js | 12 +- packages/neo4j-driver/src/index.js | 15 -- packages/neo4j-driver/test/driver.test.js | 19 +-- .../test/internal/connection-holder.test.js | 4 +- packages/neo4j-driver/types/driver.d.ts | 10 +- packages/neo4j-driver/types/index.d.ts | 6 +- .../testkit-backend/src/request-handlers.js | 28 ---- 14 files changed, 178 insertions(+), 149 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 3b4d2c246..3d2a36168 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -20,9 +20,15 @@ /** * Interface for the piece of software responsible for keeping track of current active bookmarks accross the driver. * @interface + * @since 5.0 + * @experimental */ export default class BookmarkManager { - constructor () { + /** + * @constructor + * @private + */ + private constructor () { throw new Error('Not implemented') } @@ -31,6 +37,7 @@ export default class BookmarkManager { * * This method will be called when auto-commit queries finish and when explicit transactions * get commited. + * * @param {string} database The database which the bookmarks belongs to * @param {Iterable} previousBookmarks The bookmarks used when starting the transaction * @param {Iterable} newBookmarks The new bookmarks received at the end of the transaction. @@ -81,7 +88,10 @@ export interface BookmarkManagerConfig { /** * @typedef {Object} BookmarkManagerConfig - * @property {Map>} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * + * @since 5.0 + * @experimental + * @property {Map>} [initialBookmarks@experimental] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. * @property {function([database]: string):Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * 1. supplying bookmarks from the given database when the default BookmarkManager's `.getBookmarks(database)` gets called. * 2. supplying all the bookmarks when the default BookmarkManager's `.getAllBookmarks()` gets called @@ -90,6 +100,8 @@ export interface BookmarkManagerConfig { /** * Provides an configured {@link BookmarkManager} instance. * + * @since 5.0 + * @experimental * @param {BookmarkManagerConfig} [config={}] * @returns {BookmarkManager} */ diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 86e9221a5..7907f9529 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -96,7 +96,110 @@ interface DriverConfig { trust?: TrustStrategy fetchSize?: number logging?: LoggingConfig +} + +/** + * The session configuration + * + * @interface + * + * @param {string} defaultAccessMode=WRITE - The access mode of this session, allowed values are {@link READ} and {@link WRITE}. + * @param {string|string[]} bookmarks - The initial reference or references to some previous + * transactions. Value is optional and absence indicates that that the bookmarks do not exist or are unknown. + * @param {number} fetchSize - The record fetch size of each batch of this session. + * Use {@link FETCH_ALL} to always pull all records in one batch. This will override the config value set on driver config. + * @param {string} database - The database this session will operate on. + * @param {string} impersonatedUser - The username which the user wants to impersonate for the duration of the session. + * @param {BookmarkManager} [bookmarkManager] = The bookmark manager + */ +class SessionConfig { + defaultAccessMode?: SessionMode + bookmarks?: string | string[] + database?: string + impersonatedUser?: string + fetchSize?: number bookmarkManager?: BookmarkManager + + /** + * @constructor + * @private + */ + constructor () { + /** + * The access mode of this session, allowed values are {@link READ} and {@link WRITE}. + * **Default**: {@link WRITE} + * @type {string} + */ + this.defaultAccessMode = WRITE + /** + * The initial reference or references to some previous + * transactions. Value is optional and absence indicates that that the bookmarks do not exist or are unknown. + * @type {string|string[]|undefined} + */ + this.bookmarks = [] + + /** + * The database this session will operate on. + * + * @type {string|undefined} + */ + this.database = '' + + /** + * The username which the user wants to impersonate for the duration of the session. + * + * @type {string|undefined} + */ + this.impersonatedUser = undefined + + /** + * The record fetch size of each batch of this session. + * + * Use {@link FETCH_ALL} to always pull all records in one batch. This will override the config value set on driver config. + * + * @type {number|undefined} + */ + this.fetchSize = undefined + /** + * Configure a BookmarkManager for the session to use + * + * A BookmarkManager is a piece of software responsible for keeping casual consistency between different sessions by sharing bookmarks + * between the them. + * Enabling it is done by supplying an BookmarkManager implementation instance to this param. + * A default implementation could be acquired by calling the factory function {@link bookmarkManager}. + * + * **Warning**: Share the same BookmarkManager instance accross all session can have a negative impact + * on performance since all the queries will wait for the latest changes being propagated across the cluster. + * For keeping consistency between a group of queries, use {@link Session} for grouping them. + * For keeping consistency between a group of sessions, use {@link BookmarkManager} instance for groupping them. + * + * @example + * const bookmarkManager = neo4j.bookmarkManager() + * const linkedSession1 = driver.session({ database:'neo4j', bookmarkManager }) + * const linkedSession2 = driver.session({ database:'neo4j', bookmarkManager }) + * const unlinkedSession = driver.session({ database:'neo4j' }) + * + * // Creating Driver User + * const createUserQueryResult = await linkedSession1.run('CREATE (p:Person {name: $name})', { name: 'Driver User'}) + * + * // Reading Driver User will *NOT* wait of the changes being propagated to the server before RUN the query + * // So the 'Driver User' person might not exist in the Result + * const unlinkedReadResult = await unlinkedSession.run('CREATE (p:Person {name: $name}) RETURN p', { name: 'Driver User'}) + * + * // Reading Driver User will wait of the changes being propagated to the server before RUN the query + * // So the 'Driver User' person should exist in the Result, unless deleted. + * const linkedSesssion2 = await linkedSession2.run('CREATE (p:Person {name: $name}) RETURN p', { name: 'Driver User'}) + * + * await linkedSession1.close() + * await linkedSession2.close() + * await unlinkedSession.close() + * + * @experimental + * @type {BookmarkManager|undefined} + * @since 5.0 + */ + this.bookmarkManager = undefined + } } /** @@ -285,15 +388,7 @@ class Driver { * pool and made available for others to use. * * @public - * @param {Object} param - The object parameter - * @param {string} param.defaultAccessMode=WRITE - The access mode of this session, allowed values are {@link READ} and {@link WRITE}. - * @param {string|string[]} param.bookmarks - The initial reference or references to some previous - * transactions. Value is optional and absence indicates that that the bookmarks do not exist or are unknown. - * @param {number} param.fetchSize - The record fetch size of each batch of this session. - * Use {@link FETCH_ALL} to always pull all records in one batch. This will override the config value set on driver config. - * @param {string} param.database - The database this session will operate on. - * @param {string} param.impersonatedUser - The username which the user wants to impersonate for the duration of the session. - * @param {boolean} param.ignoreBookmarkManager - Disable the bookmark manager usage in the session. + * @param {SessionConfig} param - The session configuration * @return {Session} new session. */ session ({ @@ -302,15 +397,8 @@ class Driver { database = '', impersonatedUser, fetchSize, - ignoreBookmarkManager - }: { - defaultAccessMode?: SessionMode - bookmarks?: string | string[] - database?: string - impersonatedUser?: string - fetchSize?: number - ignoreBookmarkManager?: boolean - } = {}): Session { + bookmarkManager + }: SessionConfig = {}): Session { return this._newSession({ defaultAccessMode, bookmarkOrBookmarks, @@ -319,7 +407,7 @@ class Driver { impersonatedUser, // eslint-disable-next-line @typescript-eslint/no-non-null-assertion fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize!), - ignoreBookmarkManager + bookmarkManager }) } @@ -356,7 +444,7 @@ class Driver { reactive, impersonatedUser, fetchSize, - ignoreBookmarkManager + bookmarkManager }: { defaultAccessMode: SessionMode bookmarkOrBookmarks?: string | string[] @@ -364,16 +452,14 @@ class Driver { reactive: boolean impersonatedUser?: string fetchSize: number - ignoreBookmarkManager?: boolean + bookmarkManager?: BookmarkManager }): Session { const sessionMode = Session._validateSessionMode(defaultAccessMode) const connectionProvider = this._getOrCreateConnectionProvider() const bookmarks = bookmarkOrBookmarks != null ? new Bookmarks(bookmarkOrBookmarks) : Bookmarks.empty() - const bookmarkManager = ignoreBookmarkManager !== true - ? this._config.bookmarkManager - : undefined + return this._createSession({ mode: sessionMode, database: database ?? '', @@ -486,7 +572,7 @@ function validateFetchSizeValue ( /** * @private */ -function extractConnectionTimeout (config: any): number|null { +function extractConnectionTimeout (config: any): number | null { const configuredTimeout = parseInt(config.connectionTimeout, 10) if (configuredTimeout === 0) { // timeout explicitly configured to 0 @@ -513,5 +599,5 @@ function createHostNameResolver (config: any): ConfiguredCustomResolver { } export { Driver, READ, WRITE } - +export type { SessionConfig } export default Driver diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 89d6d6a6d..8f0bb0ff1 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -76,6 +76,7 @@ import Session, { TransactionConfig } from './session' import Driver, * as driver from './driver' import auth from './auth' import BookmarkManager, { BookmarkManagerConfig, bookmarkManager } from './bookmark-manager' +import { SessionConfig } from './driver' import * as types from './types' import * as json from './json' import * as internal from './internal' // todo: removed afterwards @@ -219,7 +220,8 @@ export type { ResultObserver, TransactionConfig, BookmarkManager, - BookmarkManagerConfig + BookmarkManagerConfig, + SessionConfig } export default forExport diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 99bf36d32..63140cc16 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -17,8 +17,6 @@ * limitations under the License. */ -import BookmarkManager from './bookmark-manager' - /** * @private */ @@ -66,7 +64,6 @@ export interface Config { logging?: LoggingConfig resolver?: (address: string) => string[] | Promise userAgent?: string - bookmarkManager?: BookmarkManager } /** diff --git a/packages/core/test/driver.test.ts b/packages/core/test/driver.test.ts index 1a66eab02..c7c4c4b71 100644 --- a/packages/core/test/driver.test.ts +++ b/packages/core/test/driver.test.ts @@ -89,12 +89,12 @@ describe('Driver', () => { const manager = bookmarkManager() const driver = new Driver( META_INFO, - { ...CONFIG, bookmarkManager: manager }, + { ...CONFIG }, mockCreateConnectonProvider(connectionProvider), createSession ) - const session = driver.session() + const session = driver.session({ bookmarkManager: manager }) try { expect(createSession).toBeCalledWith(expect.objectContaining({ @@ -107,21 +107,26 @@ describe('Driver', () => { } }) - it('should create session without bookmark manager when bookmark manager is ignored', async () => { + it.each([ + [[], Bookmarks.empty()], + ['bookmark', new Bookmarks('bookmark')], + [['bookmark'], new Bookmarks(['bookmark'])], + [['bookmark1', 'bookmark2'], new Bookmarks(['bookmark1', 'bookmark2'])] + ])('should create session with bookmark manager when bookmark set', async (bookmarks, expectedBookmarks) => { const manager = bookmarkManager() const driver = new Driver( META_INFO, - { ...CONFIG, bookmarkManager: manager }, + { ...CONFIG }, mockCreateConnectonProvider(connectionProvider), createSession ) - const session = driver.session({ ignoreBookmarkManager: true }) + const session = driver.session({ bookmarks, bookmarkManager: manager }) try { expect(createSession).toBeCalledWith(expect.objectContaining({ - bookmarkManager: undefined, - bookmarks: Bookmarks.empty() + bookmarkManager: manager, + bookmarks: expectedBookmarks })) } finally { await session.close() @@ -129,26 +134,20 @@ describe('Driver', () => { } }) - it.each([ - [[], Bookmarks.empty()], - ['bookmark', new Bookmarks('bookmark')], - [['bookmark'], new Bookmarks(['bookmark'])], - [['bookmark1', 'bookmark2'], new Bookmarks(['bookmark1', 'bookmark2'])] - ])('should create session with bookmark manager when bookmark set', async (bookmarks, expectedBookmarks) => { - const manager = bookmarkManager() + it('should create session without bookmark manager when no bookmark manager is set', async () => { const driver = new Driver( META_INFO, - { ...CONFIG, bookmarkManager: manager }, + { ...CONFIG }, mockCreateConnectonProvider(connectionProvider), createSession ) - const session = driver.session({ bookmarks }) + const session = driver.session() try { expect(createSession).toBeCalledWith(expect.objectContaining({ - bookmarkManager: manager, - bookmarks: expectedBookmarks + bookmarkManager: undefined, + bookmarks: Bookmarks.empty() })) } finally { await session.close() diff --git a/packages/neo4j-driver-lite/src/index.ts b/packages/neo4j-driver-lite/src/index.ts index 5a01295f2..ed5abc899 100644 --- a/packages/neo4j-driver-lite/src/index.ts +++ b/packages/neo4j-driver-lite/src/index.ts @@ -72,7 +72,8 @@ import { auth, BookmarkManager, bookmarkManager, - BookmarkManagerConfig + BookmarkManagerConfig, + SessionConfig } from 'neo4j-driver-core' import { DirectConnectionProvider, @@ -202,21 +203,6 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * - * // Configure a BookmarkManager for the driver to use - * // - * // A BookmarkManager is a piece of software responsible for keeping casual consistency between different sessions by sharing bookmarks - * // between the them. - * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. - * // A default implementation could be acquired by calling the factory function bookmarkManager. - * // - * // **Warning**: Enabling the BookmarkManager can have a negative impact on performance since all the queries will wait for the latest changes - * // being propagated across the cluster. - * // For keeping consistency between a group of queries, use Session for grouping them. - * // - * // Example: - * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) - * bookmarkManager: undefined, // Disabled - * * // Specify a custom server address resolver function used by the routing driver to resolve the initial address used to create the driver. * // Such resolution happens: * // * during the very first rediscovery when driver is created @@ -544,6 +530,7 @@ export type { ResultObserver, NotificationPosition, BookmarkManager, - BookmarkManagerConfig + BookmarkManagerConfig, + SessionConfig } export default forExport diff --git a/packages/neo4j-driver-lite/test/unit/index.test.ts b/packages/neo4j-driver-lite/test/unit/index.test.ts index 195031a40..c19f37ae6 100644 --- a/packages/neo4j-driver-lite/test/unit/index.test.ts +++ b/packages/neo4j-driver-lite/test/unit/index.test.ts @@ -93,25 +93,27 @@ describe('index', () => { it('should treat BookmarkManager as an interface', () => { const bookmarkManager: BookmarkManager = { - getAllBookmarks (): string[] { + async getAllBookmarks (): Promise { return [] }, - getBookmarks (database: string): string[] { + async getBookmarks (database: string): Promise { return [] }, - updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): void { + async updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): Promise { }, - forget (databases: string[]): void { + async forget (databases: string[]): Promise { } } - const driver = neo4j.driver('neo4j://localhost', neo4j.auth.basic('neo4j', 'neo4i'), { - bookmarkManager - }) + const driver = neo4j.driver('neo4j://localhost', neo4j.auth.basic('neo4j', 'neo4j')) expect(driver).toBeDefined() + + const session = driver.session({ bookmarkManager }) + + expect(session).toBeDefined() }) it('should export AuthToken', () => { diff --git a/packages/neo4j-driver/src/driver.js b/packages/neo4j-driver/src/driver.js index 8b80a1bb0..491c43f02 100644 --- a/packages/neo4j-driver/src/driver.js +++ b/packages/neo4j-driver/src/driver.js @@ -49,13 +49,7 @@ class Driver extends CoreDriver { * pool and made available for others to use. * * @public - * @param {Object} param - * @param {string} param.defaultAccessMode=WRITE - The access mode of this session, allowed values are {@link READ} and {@link WRITE}. - * @param {string|string[]} param.bookmarks - The initial reference or references to some previous transactions. Value is optional and - * absence indicates that the bookmarks do not exist or are unknown. - * @param {string} param.database - The database this session will operate on. - * @param {string} param.impersonatedUser - The name of the user which should be impersonated for the duration of the session. - * @param {boolean} param.ignoreBookmarkManager - Disable the bookmark manager usage in the session. + * @param {SessionConfig} config * @returns {RxSession} new reactive session. */ rxSession ({ @@ -64,7 +58,7 @@ class Driver extends CoreDriver { database = '', fetchSize, impersonatedUser, - ignoreBookmarkManager + bookmarkManager } = {}) { return new RxSession({ session: this._newSession({ @@ -74,7 +68,7 @@ class Driver extends CoreDriver { impersonatedUser, reactive: false, fetchSize: validateFetchSizeValue(fetchSize, this._config.fetchSize), - ignoreBookmarkManager + bookmarkManager }), config: this._config }) diff --git a/packages/neo4j-driver/src/index.js b/packages/neo4j-driver/src/index.js index 810172ea6..101e5ab6e 100644 --- a/packages/neo4j-driver/src/index.js +++ b/packages/neo4j-driver/src/index.js @@ -186,21 +186,6 @@ const { * logger: (level, message) => console.log(level + ' ' + message) * }, * - * // Configure a BookmarkManager for the driver to use - * // - * // A BookmarkManager is a piece of software responsible for keeping casual consistency between different sessions by sharing bookmarks - * // between the them. - * // Enabling it is done by supplying an BookmarkManager implementation instance to this param. - * // A default implementation could be acquired by calling the factory function bookmarkManager. - * // - * // **Warning**: Enabling the BookmarkManager can have a negative impact on performance since all the queries will wait for the latest changes - * // being propagated across the cluster. - * // For keeping consistency between a group of queries, use Session for grouping them. - * // - * // Example: - * // const driver = neo4j.driver(url, auth, { bookmarkManager: neo4j.bookmarkManager() }) - * bookmarkManager: undefined, // Disabled - * * // Specify a custom server address resolver function used by the routing driver to resolve the initial address used to create the driver. * // Such resolution happens: * // * during the very first rediscovery when driver is created diff --git a/packages/neo4j-driver/test/driver.test.js b/packages/neo4j-driver/test/driver.test.js index 153757d57..36b16c712 100644 --- a/packages/neo4j-driver/test/driver.test.js +++ b/packages/neo4j-driver/test/driver.test.js @@ -146,25 +146,18 @@ describe('#unit driver', () => { }) ;[ - [manager, undefined, manager], - [manager, false, manager], - [manager, true, undefined], - [undefined, undefined, undefined], - [undefined, false, undefined], - [undefined, true, undefined] - ].forEach(([driverBMManager, ignoreBookmarkManager, sessionBMManager]) => { + [manager, manager], + [undefined, undefined] + ].forEach(([bookmarkManager, configuredBookmarkManager]) => { it('should create session using param bookmark manager', () => { driver = neo4j.driver( `neo4j+ssc://${sharedNeo4j.hostname}`, - sharedNeo4j.authToken, - { - bookmarkManager: driverBMManager - } + sharedNeo4j.authToken ) - const session = driver.rxSession({ ignoreBookmarkManager }) + const session = driver.rxSession({ bookmarkManager }) - expect(session._session._bookmarkManager).toEqual(sessionBMManager) + expect(session._session._bookmarkManager).toEqual(configuredBookmarkManager) }) }) }) diff --git a/packages/neo4j-driver/test/internal/connection-holder.test.js b/packages/neo4j-driver/test/internal/connection-holder.test.js index 8adc4f0df..5ba4a4dd4 100644 --- a/packages/neo4j-driver/test/internal/connection-holder.test.js +++ b/packages/neo4j-driver/test/internal/connection-holder.test.js @@ -47,7 +47,7 @@ describe('#unit EmptyConnectionHolder', () => { }) describe('#unit ConnectionHolder', () => { - it('should acquire new connection during initialization', () => { + it('should acquire new connection during initialization', async () => { const connectionProvider = new RecordingConnectionProvider([ new FakeConnection() ]) @@ -58,6 +58,8 @@ describe('#unit ConnectionHolder', () => { connectionHolder.initializeConnection() + await connectionHolder.getConnection() + expect(connectionProvider.acquireConnectionInvoked).toBe(1) }) diff --git a/packages/neo4j-driver/types/driver.d.ts b/packages/neo4j-driver/types/driver.d.ts index ca6cc2094..9f48468f0 100644 --- a/packages/neo4j-driver/types/driver.d.ts +++ b/packages/neo4j-driver/types/driver.d.ts @@ -20,7 +20,8 @@ import RxSession from './session-rx' import { Driver as CoreDriver, - types + types, + SessionConfig } from 'neo4j-driver-core' declare type AuthToken = types.AuthToken @@ -34,12 +35,7 @@ declare const READ: SessionMode declare const WRITE: SessionMode declare interface Driver extends CoreDriver { - rxSession: (sessionParams?: { - defaultAccessMode?: SessionMode - bookmarks?: string | string[] - fetchSize?: number - database?: string - }) => RxSession + rxSession: (sessionParams?: SessionConfig) => RxSession } export { diff --git a/packages/neo4j-driver/types/index.d.ts b/packages/neo4j-driver/types/index.d.ts index 755038a34..16a570dfd 100644 --- a/packages/neo4j-driver/types/index.d.ts +++ b/packages/neo4j-driver/types/index.d.ts @@ -63,7 +63,8 @@ import { ConnectionProvider, BookmarkManager, bookmarkManager, - BookmarkManagerConfig + BookmarkManagerConfig, + SessionConfig } from 'neo4j-driver-core' import { AuthToken, @@ -290,7 +291,8 @@ export { export type { BookmarkManager, - BookmarkManagerConfig + BookmarkManagerConfig, + SessionConfig } export default forExport diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 07eba7e01..bd90c059d 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -84,34 +84,6 @@ export function NewDriver (context, data, wire) { if ('maxTxRetryTimeMs' in data) { config.maxTransactionRetryTime = data.maxTxRetryTimeMs } - if ('bookmarkManager' in data && data.bookmarkManager != null) { - const bmmConfig = data.bookmarkManager - let initialBookmarks - let bookmarksSupplier - let bookmarksConsumer - if (bmmConfig.initialBookmarks != null) { - initialBookmarks = new Map(Object.entries(bmmConfig.initialBookmarks)) - } - if (bmmConfig.bookmarksSupplierRegistered === true) { - bookmarksSupplier = (database) => - new Promise((resolve, reject) => { - const id = context.addBookmarkSupplierRequest(resolve, reject) - wire.writeResponse(responses.BookmarksSupplierRequest({ id, database })) - }) - } - if (bmmConfig.bookmarksConsumerRegistered === true) { - bookmarksConsumer = (database, bookmarks) => - new Promise((resolve, reject) => { - const id = context.addNotifyBookmarksRequest(resolve, reject) - wire.writeResponse(responses.BookmarksConsumerRequest({ id, database, bookmarks })) - }) - } - config.bookmarkManager = neo4j.bookmarkManager({ - initialBookmarks, - bookmarksConsumer, - bookmarksSupplier - }) - } let driver try { driver = neo4j.driver(uri, parsedAuthToken, config) From 902a28671f0fb3f19e36e57d9a0117d8f1718efd Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 24 Aug 2022 16:57:09 +0200 Subject: [PATCH 35/43] Testkit: bookmark manager in the session --- .../core/src/internal/connection-holder.ts | 2 +- packages/testkit-backend/src/context.js | 15 ++++++ .../src/request-handlers-rx.js | 17 ++++-- .../testkit-backend/src/request-handlers.js | 54 ++++++++++++++++++- packages/testkit-backend/src/responses.js | 12 +++-- 5 files changed, 90 insertions(+), 10 deletions(-) diff --git a/packages/core/src/internal/connection-holder.ts b/packages/core/src/internal/connection-holder.ts index dfb0d6960..17b5690d1 100644 --- a/packages/core/src/internal/connection-holder.ts +++ b/packages/core/src/internal/connection-holder.ts @@ -122,7 +122,7 @@ class ConnectionHolder implements ConnectionHolderInterface { this._referenceCount = 0 this._connectionPromise = Promise.resolve(null) this._onDatabaseNameResolved = onDatabaseNameResolved - this._getConnectionAcquistionBookmarks = getConnectionAcquistionBookmarks ?? (async () => Bookmarks.empty()) + this._getConnectionAcquistionBookmarks = getConnectionAcquistionBookmarks ?? (() => Promise.resolve(Bookmarks.empty())) } mode (): string | undefined { diff --git a/packages/testkit-backend/src/context.js b/packages/testkit-backend/src/context.js index 3dc735efc..fad3a5329 100644 --- a/packages/testkit-backend/src/context.js +++ b/packages/testkit-backend/src/context.js @@ -11,6 +11,7 @@ export default class Context { this._results = {} this._bookmarkSupplierRequests = {} this._notifyBookmarksRequests = {} + this._bookmarksManagers = {} } addDriver (driver) { @@ -136,6 +137,20 @@ export default class Context { return this._notifyBookmarksRequests[id] } + addBookmarkManager (bookmarkManagerFactory) { + this._id++ + this._bookmarksManagers[this._id] = bookmarkManagerFactory(this._id) + return this._id + } + + getBookmarkManager (id) { + return this._bookmarksManagers[id] + } + + removeBookmarkManager (id) { + delete this._bookmarksManagers[id] + } + _add (map, object) { this._id++ map[this._id] = object diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index ffe43bded..923046fe4 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -20,11 +20,14 @@ export { ForcedRoutingTableUpdate, ResultNext, RetryablePositive, - RetryableNegative + RetryableNegative, + NewBookmarkManager, + BookmarksSupplierCompleted, + BookmarksConsumerCompleted } from './request-handlers.js' export function NewSession (context, data, wire) { - let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, ignoreBookmarkManager } = data + let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, bookmarkManagerId } = data switch (accessMode) { case 'r': accessMode = neo4j.session.READ @@ -36,6 +39,14 @@ export function NewSession (context, data, wire) { wire.writeBackendError('Unknown accessmode: ' + accessMode) return } + let bookmarkManager + if (bookmarkManagerId != null) { + bookmarkManager = context.getBookmarkManager(bookmarkManagerId) + if (bookmarkManager == null) { + wire.writeBackendError(`Bookmark manager ${bookmarkManagerId} not found`) + return + } + } const driver = context.getDriver(driverId) const session = driver.rxSession({ defaultAccessMode: accessMode, @@ -43,7 +54,7 @@ export function NewSession (context, data, wire) { database, fetchSize, impersonatedUser, - ignoreBookmarkManager + bookmarkManager }) const id = context.addSession(session) wire.writeResponse(responses.Session({ id })) diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index bd90c059d..d3ff94181 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -107,7 +107,7 @@ export function DriverClose (context, data, wire) { } export function NewSession (context, data, wire) { - let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, ignoreBookmarkManager } = data + let { driverId, accessMode, bookmarks, database, fetchSize, impersonatedUser, bookmarkManagerId } = data switch (accessMode) { case 'r': accessMode = neo4j.session.READ @@ -119,6 +119,14 @@ export function NewSession (context, data, wire) { wire.writeBackendError('Unknown accessmode: ' + accessMode) return } + let bookmarkManager + if (bookmarkManagerId != null) { + bookmarkManager = context.getBookmarkManager(bookmarkManagerId) + if (bookmarkManager == null) { + wire.writeBackendError(`Bookmark manager ${bookmarkManagerId} not found`) + return + } + } const driver = context.getDriver(driverId) const session = driver.session({ defaultAccessMode: accessMode, @@ -126,7 +134,7 @@ export function NewSession (context, data, wire) { database, fetchSize, impersonatedUser, - ignoreBookmarkManager + bookmarkManager }) const id = context.addSession(session) wire.writeResponse(responses.Session({ id })) @@ -407,6 +415,48 @@ export function ResolverResolutionCompleted ( request.resolve(addresses) } +export function NewBookmarkManager ( + context, + { + initialBookmarks, + bookmarksSupplierRegistered, + bookmarksConsumerRegistered + }, + wire +) { + let bookmarkManager + const id = context.addBookmarkManager((bookmarkManagerId) => { + let bookmarksSupplier + let bookmarksConsumer + if (initialBookmarks != null) { + initialBookmarks = new Map(Object.entries(initialBookmarks)) + } + if (bookmarksSupplierRegistered === true) { + bookmarksSupplier = (database) => + new Promise((resolve, reject) => { + const id = context.addBookmarkSupplierRequest(resolve, reject) + wire.writeResponse(responses.BookmarksSupplierRequest({ id, bookmarkManagerId, database })) + }) + } + if (bookmarksConsumerRegistered === true) { + bookmarksConsumer = (database, bookmarks) => + new Promise((resolve, reject) => { + const id = context.addNotifyBookmarksRequest(resolve, reject) + wire.writeResponse(responses.BookmarksConsumerRequest({ id, bookmarkManagerId, database, bookmarks })) + }) + } + bookmarkManager = neo4j.bookmarkManager({ + initialBookmarks, + bookmarksConsumer, + bookmarksSupplier + }) + + return bookmarkManager + }) + + wire.writeResponse(responses.BookmarkManager({ id })) +} + export function BookmarksSupplierCompleted ( context, { diff --git a/packages/testkit-backend/src/responses.js b/packages/testkit-backend/src/responses.js index eccaff5c5..000d906e4 100644 --- a/packages/testkit-backend/src/responses.js +++ b/packages/testkit-backend/src/responses.js @@ -14,12 +14,16 @@ export function ResolverResolutionRequired ({ id, address }) { return response('ResolverResolutionRequired', { id, address }) } -export function BookmarksSupplierRequest ({ id, database }) { - return response('BookmarksSupplierRequest', { id, database }) +export function BookmarkManager ({ id }) { + return response('BookmarkManager', { id }) } -export function BookmarksConsumerRequest ({ id, database, bookmarks }) { - return response('BookmarksConsumerRequest', { id, database, bookmarks }) +export function BookmarksSupplierRequest ({ id, bookmarkManagerId, database }) { + return response('BookmarksSupplierRequest', { id, bookmarkManagerId, database }) +} + +export function BookmarksConsumerRequest ({ id, bookmarkManagerId, database, bookmarks }) { + return response('BookmarksConsumerRequest', { id, bookmarkManagerId, database, bookmarks }) } export function Session ({ id }) { From bea5884f358b025eabc85ea5c2ead0a2de286780 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 24 Aug 2022 18:40:20 +0200 Subject: [PATCH 36/43] Skipping tests --- packages/neo4j-driver/test/index.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/neo4j-driver/test/index.test.js b/packages/neo4j-driver/test/index.test.js index 2fb6115c8..c0c899055 100644 --- a/packages/neo4j-driver/test/index.test.js +++ b/packages/neo4j-driver/test/index.test.js @@ -139,7 +139,7 @@ describe('#unit index', () => { } }) - describe('await rxRession.beginTransaction().toPromise() return', () => { + xdescribe('await rxRession.beginTransaction().toPromise() return', () => { it('should be instanceof neo4j.RxTransaction', async () => { const transaction = await subject() expect(transaction).toBeInstanceOf(neo4j.RxTransaction) @@ -193,7 +193,7 @@ describe('#unit index', () => { } }) - describe('RxManagedTransaction', () => { + xdescribe('RxManagedTransaction', () => { it('should be instanceof neo4j.RxManagedTransaction', async () => { const rxManagedTransaction = await subject() expect(rxManagedTransaction).toBeInstanceOf(neo4j.RxManagedTransaction) From 19694bf434f3b82f039a6d82cfeb722703315a0d Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Mon, 29 Aug 2022 15:53:34 +0200 Subject: [PATCH 37/43] BookmarkManagerClose --- packages/testkit-backend/src/request-handlers-rx.js | 1 + packages/testkit-backend/src/request-handlers.js | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index 923046fe4..009f85b5a 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -22,6 +22,7 @@ export { RetryablePositive, RetryableNegative, NewBookmarkManager, + BookmarkManagerClose, BookmarksSupplierCompleted, BookmarksConsumerCompleted } from './request-handlers.js' diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index d3ff94181..b845c42b1 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -457,6 +457,17 @@ export function NewBookmarkManager ( wire.writeResponse(responses.BookmarkManager({ id })) } +export function BookmarkManagerClose ( + context, + { + id + }, + wire +) { + context.removeBookmarkManager(id) + wire.writeResponse(responses.BookmarkManager({ id })) +} + export function BookmarksSupplierCompleted ( context, { From f8a4f1e22222272802cc07001a6d9d56499f08dc Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 31 Aug 2022 13:58:09 +0200 Subject: [PATCH 38/43] Enable Rx support StartSubTest --- packages/testkit-backend/src/request-handlers-rx.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/testkit-backend/src/request-handlers-rx.js b/packages/testkit-backend/src/request-handlers-rx.js index 009f85b5a..088b89aff 100644 --- a/packages/testkit-backend/src/request-handlers-rx.js +++ b/packages/testkit-backend/src/request-handlers-rx.js @@ -24,7 +24,8 @@ export { NewBookmarkManager, BookmarkManagerClose, BookmarksSupplierCompleted, - BookmarksConsumerCompleted + BookmarksConsumerCompleted, + StartSubTest } from './request-handlers.js' export function NewSession (context, data, wire) { From 4eb39af9e71a1a7a47ec46d7eff8da5258fc7236 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 31 Aug 2022 16:09:20 +0200 Subject: [PATCH 39/43] Unskipping tests --- packages/neo4j-driver/test/index.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/neo4j-driver/test/index.test.js b/packages/neo4j-driver/test/index.test.js index c0c899055..2fb6115c8 100644 --- a/packages/neo4j-driver/test/index.test.js +++ b/packages/neo4j-driver/test/index.test.js @@ -139,7 +139,7 @@ describe('#unit index', () => { } }) - xdescribe('await rxRession.beginTransaction().toPromise() return', () => { + describe('await rxRession.beginTransaction().toPromise() return', () => { it('should be instanceof neo4j.RxTransaction', async () => { const transaction = await subject() expect(transaction).toBeInstanceOf(neo4j.RxTransaction) @@ -193,7 +193,7 @@ describe('#unit index', () => { } }) - xdescribe('RxManagedTransaction', () => { + describe('RxManagedTransaction', () => { it('should be instanceof neo4j.RxManagedTransaction', async () => { const rxManagedTransaction = await subject() expect(rxManagedTransaction).toBeInstanceOf(neo4j.RxManagedTransaction) From bdc483de111940c69535d07118f0cd1f2c3ee7cb Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 31 Aug 2022 16:15:35 +0200 Subject: [PATCH 40/43] remove un-necessary logs --- packages/core/src/driver.ts | 9 --------- 1 file changed, 9 deletions(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 7907f9529..3c07d3c44 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -102,15 +102,6 @@ interface DriverConfig { * The session configuration * * @interface - * - * @param {string} defaultAccessMode=WRITE - The access mode of this session, allowed values are {@link READ} and {@link WRITE}. - * @param {string|string[]} bookmarks - The initial reference or references to some previous - * transactions. Value is optional and absence indicates that that the bookmarks do not exist or are unknown. - * @param {number} fetchSize - The record fetch size of each batch of this session. - * Use {@link FETCH_ALL} to always pull all records in one batch. This will override the config value set on driver config. - * @param {string} database - The database this session will operate on. - * @param {string} impersonatedUser - The username which the user wants to impersonate for the duration of the session. - * @param {BookmarkManager} [bookmarkManager] = The bookmark manager */ class SessionConfig { defaultAccessMode?: SessionMode From a7bdd3cdf694fa2d9c81f3494e8dfb10da28e5ba Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 1 Sep 2022 15:41:32 +0200 Subject: [PATCH 41/43] Wait for BEGIN be send before send RUN, COMMIT or ROLLBACK --- packages/core/src/transaction.ts | 125 +++++++++++++++---------- packages/core/test/transaction.test.ts | 54 +++++++++++ 2 files changed, 129 insertions(+), 50 deletions(-) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index ced873b9d..6842a9733 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -58,6 +58,8 @@ class Transaction { private readonly _lowRecordWatermak: number private readonly _highRecordWatermark: number private _bookmarks: Bookmarks + private readonly _activePromise: Promise + private _acceptActive: () => void /** * @constructor @@ -107,6 +109,9 @@ class Transaction { this._lowRecordWatermak = lowRecordWatermark this._highRecordWatermark = highRecordWatermark this._bookmarks = Bookmarks.empty() + this._activePromise = new Promise((resolve, reject) => { + this._acceptActive = resolve + }) } /** @@ -154,6 +159,10 @@ class Transaction { } this._onError(error).catch(() => {}) }) + // It should make the transaction active anyway + // futher errors will be treated by the exiting + // observers + .finally(() => this._acceptActive()) } /** @@ -178,7 +187,8 @@ class Transaction { reactive: this._reactive, fetchSize: this._fetchSize, highRecordWatermark: this._highRecordWatermark, - lowRecordWatermark: this._lowRecordWatermak + lowRecordWatermark: this._lowRecordWatermak, + preparationJob: this._activePromise }) this._results.push(result) return result @@ -197,7 +207,8 @@ class Transaction { onError: this._onError, onComplete: (meta: any) => this._onCompleteCallback(meta, this._bookmarks), onConnection: this._onConnection, - pendingResults: this._results + pendingResults: this._results, + preparationJob: this._activePromise }) this._state = committed.state // clean up @@ -224,7 +235,8 @@ class Transaction { onError: this._onError, onComplete: this._onComplete, onConnection: this._onConnection, - pendingResults: this._results + pendingResults: this._results, + preparationJob: this._activePromise }) this._state = rolledback.state // clean up @@ -293,6 +305,7 @@ interface StateTransitionParams { fetchSize: number highRecordWatermark: number lowRecordWatermark: number + preparationJob?: Promise } const _states = { @@ -303,7 +316,8 @@ const _states = { onError, onComplete, onConnection, - pendingResults + pendingResults, + preparationJob }: StateTransitionParams): any => { return { result: finishTransaction( @@ -312,7 +326,8 @@ const _states = { onError, onComplete, onConnection, - pendingResults + pendingResults, + preparationJob ), state: _states.SUCCEEDED } @@ -322,7 +337,8 @@ const _states = { onError, onComplete, onConnection, - pendingResults + pendingResults, + preparationJob }: StateTransitionParams): any => { return { result: finishTransaction( @@ -331,7 +347,8 @@ const _states = { onError, onComplete, onConnection, - pendingResults + pendingResults, + preparationJob ), state: _states.ROLLED_BACK } @@ -347,31 +364,35 @@ const _states = { reactive, fetchSize, highRecordWatermark, - lowRecordWatermark + lowRecordWatermark, + preparationJob }: StateTransitionParams ): any => { // RUN in explicit transaction can't contain bookmarks and transaction configuration // No need to include mode and database name as it shall be inclued in begin - const observerPromise = connectionHolder - .getConnection() - .then(conn => { - onConnection() - if (conn != null) { - return conn.protocol().run(query, parameters, { - bookmarks: Bookmarks.empty(), - txConfig: TxConfig.empty(), - beforeError: onError, - afterComplete: onComplete, - reactive: reactive, - fetchSize: fetchSize, - highRecordWatermark: highRecordWatermark, - lowRecordWatermark: lowRecordWatermark - }) - } else { - throw newError('No connection available') - } - }) - .catch(error => new FailedObserver({ error, onError })) + const requirements = preparationJob ?? Promise.resolve() + + const observerPromise = + connectionHolder.getConnection() + .then(conn => requirements.then(() => conn)) + .then(conn => { + onConnection() + if (conn != null) { + return conn.protocol().run(query, parameters, { + bookmarks: Bookmarks.empty(), + txConfig: TxConfig.empty(), + beforeError: onError, + afterComplete: onComplete, + reactive: reactive, + fetchSize: fetchSize, + highRecordWatermark: highRecordWatermark, + lowRecordWatermark: lowRecordWatermark + }) + } else { + throw newError('No connection available') + } + }) + .catch(error => new FailedObserver({ error, onError })) return newCompletedResult( observerPromise, @@ -598,32 +619,36 @@ function finishTransaction ( onError: (err: Error) => any, onComplete: (metadata: any) => any, onConnection: () => any, - pendingResults: Result[] + pendingResults: Result[], + preparationJob?: Promise ): Result { - const observerPromise = connectionHolder - .getConnection() - .then(connection => { - onConnection() - pendingResults.forEach(r => r._cancel()) - return Promise.all(pendingResults.map(result => result.summary())).then(results => { - if (connection != null) { - if (commit) { - return connection.protocol().commitTransaction({ - beforeError: onError, - afterComplete: onComplete - }) + const requirements = preparationJob ?? Promise.resolve() + + const observerPromise = + connectionHolder.getConnection() + .then(conn => requirements.then(() => conn)) + .then(connection => { + onConnection() + pendingResults.forEach(r => r._cancel()) + return Promise.all(pendingResults.map(result => result.summary())).then(results => { + if (connection != null) { + if (commit) { + return connection.protocol().commitTransaction({ + beforeError: onError, + afterComplete: onComplete + }) + } else { + return connection.protocol().rollbackTransaction({ + beforeError: onError, + afterComplete: onComplete + }) + } } else { - return connection.protocol().rollbackTransaction({ - beforeError: onError, - afterComplete: onComplete - }) + throw newError('No connection available') } - } else { - throw newError('No connection available') - } + }) }) - }) - .catch(error => new FailedObserver({ error, onError })) + .catch(error => new FailedObserver({ error, onError })) // for commit & rollback we need result that uses real connection holder and notifies it when // connection is not needed and can be safely released to the pool diff --git a/packages/core/test/transaction.test.ts b/packages/core/test/transaction.test.ts index 486e09b73..a56c49f5e 100644 --- a/packages/core/test/transaction.test.ts +++ b/packages/core/test/transaction.test.ts @@ -325,6 +325,8 @@ function testTx (transactionName: string, newTransaction: lowRecordWatermark: 300 }) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) + await tx.run('RETURN 1') expect(connection.seenProtocolOptions[0]).toMatchObject({ @@ -343,11 +345,36 @@ function testTx (transactionName: string, newTransaction: lowRecordWatermark: 300 }) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) + const result = tx.run('RETURN 1') // @ts-expect-error expect(result._watermarks).toEqual({ high: 700, low: 300 }) }) + + it('should wait begin message be send', async () => { + const connection = newFakeConnection() + const tx = newTransaction({ + connection + }) + + const bookmarksPromise: Promise = new Promise((resolve) => { + setTimeout(() => resolve(Bookmarks.empty()), 1000) + }) + + tx._begin(async () => await bookmarksPromise, TxConfig.empty()) + + const result = tx.run('RETURN 1') + + expect(connection.seenBeginTransaction.length).toEqual(0) + expect(connection.seenQueries.length).toEqual(0) + + await result + + expect(connection.seenBeginTransaction.length).toEqual(1) + expect(connection.seenQueries.length).toEqual(1) + }) }) describe('.close()', () => { @@ -356,6 +383,7 @@ function testTx (transactionName: string, newTransaction: const connection = newFakeConnection() const tx = newTransaction({ connection }) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) await tx.run('RETURN 1') await tx.close() @@ -367,6 +395,7 @@ function testTx (transactionName: string, newTransaction: const connection = newFakeConnection().withRollbackError(expectedError) const tx = newTransaction({ connection }) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) await tx.run('RETURN 1') try { @@ -376,6 +405,29 @@ function testTx (transactionName: string, newTransaction: expect(error).toEqual(expectedError) } }) + + it('should wait begin message be send', async () => { + const connection = newFakeConnection() + const tx = newTransaction({ + connection + }) + + const bookmarksPromise: Promise = new Promise((resolve) => { + setTimeout(() => resolve(Bookmarks.empty()), 1000) + }) + + tx._begin(async () => await bookmarksPromise, TxConfig.empty()) + + const result = tx.close() + + expect(connection.seenBeginTransaction.length).toEqual(0) + expect(connection.rollbackInvoked).toEqual(0) + + await result + + expect(connection.seenBeginTransaction.length).toEqual(1) + expect(connection.rollbackInvoked).toEqual(1) + }) }) describe('when transaction is closed', () => { @@ -394,6 +446,8 @@ function testTx (transactionName: string, newTransaction: const connection = newFakeConnection() const tx = newTransaction({ connection }) + tx._begin(async () => Bookmarks.empty(), TxConfig.empty()) + await operation(tx, connection) const rollbackInvokedAfterOperation = connection.rollbackInvoked From 0fe6ccd966ce54e5ca0b8339d724e67716605160 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 1 Sep 2022 17:26:31 +0200 Subject: [PATCH 42/43] addressing pr comments --- packages/core/src/transaction.ts | 4 ++-- packages/core/test/transaction.test.ts | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/core/src/transaction.ts b/packages/core/src/transaction.ts index 6842a9733..45d3c9ba9 100644 --- a/packages/core/src/transaction.ts +++ b/packages/core/src/transaction.ts @@ -160,7 +160,7 @@ class Transaction { this._onError(error).catch(() => {}) }) // It should make the transaction active anyway - // futher errors will be treated by the exiting + // further errors will be treated by the existing // observers .finally(() => this._acceptActive()) } @@ -369,7 +369,7 @@ const _states = { }: StateTransitionParams ): any => { // RUN in explicit transaction can't contain bookmarks and transaction configuration - // No need to include mode and database name as it shall be inclued in begin + // No need to include mode and database name as it shall be included in begin const requirements = preparationJob ?? Promise.resolve() const observerPromise = diff --git a/packages/core/test/transaction.test.ts b/packages/core/test/transaction.test.ts index a56c49f5e..23e91519e 100644 --- a/packages/core/test/transaction.test.ts +++ b/packages/core/test/transaction.test.ts @@ -353,7 +353,7 @@ function testTx (transactionName: string, newTransaction: expect(result._watermarks).toEqual({ high: 700, low: 300 }) }) - it('should wait begin message be send', async () => { + it('should wait begin message be sent', async () => { const connection = newFakeConnection() const tx = newTransaction({ connection @@ -406,7 +406,7 @@ function testTx (transactionName: string, newTransaction: } }) - it('should wait begin message be send', async () => { + it('should wait begin message be sent', async () => { const connection = newFakeConnection() const tx = newTransaction({ connection From 28cd7cf1993d88448aafc694c58778c632f6b350 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Thu, 1 Sep 2022 21:21:53 +0200 Subject: [PATCH 43/43] Fix test name --- packages/core/test/transaction.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/transaction.test.ts b/packages/core/test/transaction.test.ts index 23e91519e..d61459ff3 100644 --- a/packages/core/test/transaction.test.ts +++ b/packages/core/test/transaction.test.ts @@ -353,7 +353,7 @@ function testTx (transactionName: string, newTransaction: expect(result._watermarks).toEqual({ high: 700, low: 300 }) }) - it('should wait begin message be sent', async () => { + it('should wait until begin message be sent', async () => { const connection = newFakeConnection() const tx = newTransaction({ connection @@ -406,7 +406,7 @@ function testTx (transactionName: string, newTransaction: } }) - it('should wait begin message be sent', async () => { + it('should wait until begin message be sent', async () => { const connection = newFakeConnection() const tx = newTransaction({ connection