From 3ba8b5c7b5975c3ffdcc984aa65de38c8194f584 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Tue, 22 Nov 2022 14:35:14 +0100 Subject: [PATCH 1/8] Update Bookmark manager for no longer tracking per database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes in the BookmarkManagerConfig: * `initialBookmarks` type changed from `Map>` to `Iterable` * `bookmarksSupplier` type changed from `(db?: string) => Promise>` to `() => Promise>` * `bookmarksConsumer` type changed from `(db: string, bookmarks: Iterable) => Promise` to `(bookmarks: Iterable) => Promise` Changes in the BookmarkManager: * `getAllBookmarks` and `forget` were removed * `updateBookmarks` signature changed to `updateBookmarks (previousBookmarks: Iterable, newBookmarks: Iterable): Promise` * `getBookmarks` signature changed to `getBookmarks (): Promise>` ⚠️ This is a experimental feature. --- packages/core/src/bookmark-manager.ts | 101 ++------- packages/core/src/driver.ts | 6 +- packages/core/src/session.ts | 5 +- packages/core/test/bookmark-manager.test.ts | 196 ++---------------- packages/core/test/session.test.ts | 141 +++++-------- .../lib/core/bookmark-manager.ts | 101 ++------- packages/neo4j-driver-deno/lib/core/driver.ts | 6 +- .../neo4j-driver-deno/lib/core/session.ts | 5 +- .../neo4j-driver-lite/test/unit/index.test.ts | 10 +- .../testkit-backend/src/request-handlers.js | 11 +- 10 files changed, 126 insertions(+), 456 deletions(-) diff --git a/packages/core/src/bookmark-manager.ts b/packages/core/src/bookmark-manager.ts index 3d2a36168..f74849bfd 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -36,54 +36,30 @@ export default class BookmarkManager { * Method called when the bookmarks get updated when a transaction finished. * * This method will be called when auto-commit queries finish and when explicit transactions - * get commited. + * get committed. * - * @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. * @returns {void} */ - async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { + async updateBookmarks (previousBookmarks: Iterable, newBookmarks: Iterable): Promise { throw new Error('Not implemented') } /** - * Method called by the driver to get the bookmarks for one specific database + * Method called by the driver to get the bookmarks. * - * @param {string} database The database which the bookmarks belong to * @returns {Iterable} The set of bookmarks */ - async getBookmarks (database: string): Promise> { - throw new Error('Not implemented') - } - - /** - * Method called by the driver for getting all the bookmarks. - * - * This method should return all bookmarks for all databases present in the BookmarkManager. - * - * @returns {Iterable} The set of bookmarks - */ - async getAllBookmarks (): Promise> { - 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 {Iterable} databases The databases which the bookmarks will be removed for. - */ - async forget (databases: Iterable): Promise { + async getBookmarks (): Promise> { throw new Error('Not implemented') } } export interface BookmarkManagerConfig { - initialBookmarks?: Map> - bookmarksSupplier?: (database?: string) => Promise> - bookmarksConsumer?: (database: string, bookmarks: Iterable) => Promise + initialBookmarks?: Iterable + bookmarksSupplier?: () => Promise> + bookmarksConsumer?: (bookmarks: Iterable) => Promise } /** @@ -91,11 +67,9 @@ export interface BookmarkManagerConfig { * * @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 - * @property {function(database: string, bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks for database get updated + * @property {Iterable} [initialBookmarks@experimental] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {function():Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager + * @property {function(bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks get updated */ /** * Provides an configured {@link BookmarkManager} instance. @@ -106,9 +80,7 @@ export interface BookmarkManagerConfig { * @returns {BookmarkManager} */ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { - const initialBookmarks = new Map>() - - config.initialBookmarks?.forEach((v, k) => initialBookmarks.set(k, new Set(v))) + const initialBookmarks = new Set(config.initialBookmarks) return new Neo4jBookmarkManager( initialBookmarks, @@ -119,15 +91,15 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa class Neo4jBookmarkManager implements BookmarkManager { constructor ( - private readonly _bookmarksPerDb: Map>, - private readonly _bookmarksSupplier?: (database?: string) => Promise>, - private readonly _bookmarksConsumer?: (database: string, bookmark: Iterable) => Promise + private readonly _bookmarks: Set, + private readonly _bookmarksSupplier?: () => Promise>, + private readonly _bookmarksConsumer?: (bookmark: Iterable) => Promise ) { } - async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { - const bookmarks = this._getOrInitializeBookmarks(database) + async updateBookmarks (previousBookmarks: Iterable, newBookmarks: Iterable): Promise { + const bookmarks = this._bookmarks for (const bm of previousBookmarks) { bookmarks.delete(bm) } @@ -135,40 +107,13 @@ class Neo4jBookmarkManager implements BookmarkManager { bookmarks.add(bm) } if (typeof this._bookmarksConsumer === 'function') { - await this._bookmarksConsumer(database, [...bookmarks]) - } - } - - private _getOrInitializeBookmarks (database: string): Set { - let maybeBookmarks = this._bookmarksPerDb.get(database) - if (maybeBookmarks === undefined) { - maybeBookmarks = new Set() - this._bookmarksPerDb.set(database, maybeBookmarks) - } - return maybeBookmarks - } - - async getBookmarks (database: string): Promise> { - const bookmarks = new Set(this._bookmarksPerDb.get(database)) - - if (typeof this._bookmarksSupplier === 'function') { - const suppliedBookmarks = await this._bookmarksSupplier(database) ?? [] - for (const bm of suppliedBookmarks) { - bookmarks.add(bm) - } + await this._bookmarksConsumer([...bookmarks]) } - - return [...bookmarks] } - async getAllBookmarks (): Promise> { - const bookmarks = new Set() + async getBookmarks (): Promise> { + const bookmarks = new Set(this._bookmarks) - for (const [, dbBookmarks] of this._bookmarksPerDb) { - for (const bm of dbBookmarks) { - bookmarks.add(bm) - } - } if (typeof this._bookmarksSupplier === 'function') { const suppliedBookmarks = await this._bookmarksSupplier() ?? [] for (const bm of suppliedBookmarks) { @@ -176,12 +121,6 @@ class Neo4jBookmarkManager implements BookmarkManager { } } - return bookmarks - } - - async forget (databases: Iterable): Promise { - for (const database of databases) { - this._bookmarksPerDb.delete(database) - } + return [...bookmarks] } } diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index c2174ebc9..5cc5bdc1a 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -194,10 +194,10 @@ class SessionConfig { * 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 + * **Warning**: Share the same BookmarkManager instance across 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. + * For keeping consistency between a group of sessions, use {@link BookmarkManager} instance for grouping them. * * @example * const bookmarkManager = neo4j.bookmarkManager() @@ -214,7 +214,7 @@ class SessionConfig { * * // 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'}) + * const linkedResult = await linkedSession2.run('CREATE (p:Person {name: $name}) RETURN p', { name: 'Driver User'}) * * await linkedSession1.close() * await linkedSession2.close() diff --git a/packages/core/src/session.ts b/packages/core/src/session.ts index 09c5a1892..feedd0cc1 100644 --- a/packages/core/src/session.ts +++ b/packages/core/src/session.ts @@ -343,7 +343,7 @@ class Session { } private async _bookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getAllBookmarks() + const bookmarks = await this._bookmarkManager?.getBookmarks() if (bookmarks === undefined) { return this._lastBookmarks } @@ -489,7 +489,7 @@ class Session { } private async _getConnectionAcquistionBookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getBookmarks('system') + const bookmarks = await this._bookmarkManager?.getBookmarks() if (bookmarks === undefined) { return this._lastBookmarks } @@ -505,7 +505,6 @@ class Session { _updateBookmarks (newBookmarks?: Bookmarks, previousBookmarks?: Bookmarks, database?: string): void { if ((newBookmarks != null) && !newBookmarks.isEmpty()) { this._bookmarkManager?.updateBookmarks( - database ?? this._database, previousBookmarks?.values() ?? [], newBookmarks?.values() ?? [] ) diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 0ff6ae73f..10d97b9b3 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -30,106 +30,33 @@ describe('BookmarkManager', () => { }) describe('getBookmarks()', () => { - it('should return empty if db doesnt exists', async () => { - const manager = bookmarkManager({}) - - const bookmarks = await manager.getBookmarks('neo4j') - - expect(bookmarks).toEqual([]) - }) - - it('should return bookmarks for the given db', async () => { - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) - }) - - const bookmarks = await manager.getBookmarks('neo4j') - - expect(bookmarks).toBeSortedEqual(neo4jBookmarks) - }) - - it('should return get bookmarks from bookmarkSupplier', async () => { - const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] - - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), - bookmarksSupplier: async () => await Promise.resolve(extraBookmarks) - }) - - const bookmarks = await manager.getBookmarks('neo4j') - - expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...extraBookmarks]) - }) - - it('should return not duplicate bookmarks if bookmarkSupplier returns existing bm', async () => { - const extraBookmarks = ['neo4j:bm03', 'neo4j:bm04'] - - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), - bookmarksSupplier: async () => await Promise.resolve([...extraBookmarks, ...neo4jBookmarks]) - }) - - const bookmarks = await manager.getBookmarks('neo4j') - - expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...extraBookmarks]) - }) - - it('should return call from bookmarkSupplier with correct database', async () => { - const bookmarksSupplier = jest.fn() - - const manager = bookmarkManager({ - bookmarksSupplier - }) - - await manager.getBookmarks('neo4j') - - expect(bookmarksSupplier).toBeCalledWith('neo4j') - }) - }) - - describe('getAllBookmarks()', () => { it('should return all bookmarks ', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) - const bookmarks = await manager.getAllBookmarks() + const bookmarks = await manager.getBookmarks() expect(bookmarks).toBeSortedEqual([...neo4jBookmarks, ...systemBookmarks]) }) - it('should return empty if there are no bookmarks for any db', async () => { + it('should return empty if there are no bookmarks', async () => { const manager = bookmarkManager({}) - const bookmarks = await manager.getAllBookmarks() + const bookmarks = await manager.getBookmarks() expect(bookmarks).toBeSortedEqual([]) }) it('should return enriched bookmarks list with supplied bookmarks', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(async (database?: string) => await Promise.resolve(extraBookmarks)) + const bookmarksSupplier = jest.fn(async () => await Promise.resolve(extraBookmarks)) const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksSupplier }) - const bookmarks = await manager.getAllBookmarks() + const bookmarks = await manager.getBookmarks() expect(bookmarks).toBeSortedEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks] @@ -138,16 +65,13 @@ describe('BookmarkManager', () => { it('should return duplicate bookmarks if bookmarksSupplier returns already existing bm', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(async (database?: string) => await Promise.resolve([...extraBookmarks, ...systemBookmarks])) + const bookmarksSupplier = jest.fn(async () => await Promise.resolve([...extraBookmarks, ...systemBookmarks])) const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksSupplier }) - const bookmarks = await manager.getAllBookmarks() + const bookmarks = await manager.getBookmarks() expect(bookmarks).toBeSortedEqual( [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks] @@ -157,136 +81,62 @@ describe('BookmarkManager', () => { it('should call bookmarkSupplier for getting all bookmarks', async () => { const bookmarksSupplier = jest.fn() const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksSupplier }) - await manager.getAllBookmarks() + await manager.getBookmarks() expect(bookmarksSupplier).toBeCalledWith() }) }) describe('updateBookmarks()', () => { - it('should remove previous bookmarks and new bookmarks for an existing db', async () => { + it('should remove previous bookmarks and new bookmarks', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) await manager.updateBookmarks( - 'neo4j', - await manager.getAllBookmarks(), + + await manager.getBookmarks(), newBookmarks ) - await expect(manager.getBookmarks('neo4j')).resolves.toBeSortedEqual(newBookmarks) - await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) + await expect(manager.getBookmarks()).resolves.toBeSortedEqual(newBookmarks) }) it('should not remove bookmarks not present in the original list', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) - const [bookmarkNotUsedInTx, ...bookmarksUsedInTx] = neo4jBookmarks + const [bookmarkNotUsedInTx, ...bookmarksUsedInTx] = [...neo4jBookmarks, ...systemBookmarks] await manager.updateBookmarks( - 'neo4j', bookmarksUsedInTx, newBookmarks ) - await expect(manager.getBookmarks('neo4j')) + await expect(manager.getBookmarks()) .resolves.toBeSortedEqual([bookmarkNotUsedInTx, ...newBookmarks]) - await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) - }) - - it('should add bookmarks to a non-existing database', async () => { - const newBookmarks = ['neo4j:bm03'] - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['system', systemBookmarks] - ]) - }) - - await manager.updateBookmarks( - 'neo4j', - [], - newBookmarks - ) - - await expect(manager.getBookmarks('neo4j')).resolves.toBeSortedEqual(newBookmarks) - await expect(manager.getBookmarks('system')).resolves.toBeSortedEqual(systemBookmarks) }) it('should notify new bookmarks', async () => { const bookmarksConsumer = jest.fn() const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksConsumer }) await manager.updateBookmarks( - 'neo4j', - await manager.getAllBookmarks(), + await manager.getBookmarks(), newBookmarks ) - expect(bookmarksConsumer).toBeCalledWith('neo4j', newBookmarks) - }) - }) - - describe('forget()', () => { - it('should forget database', async () => { - const extraBookmarks = ['system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(async () => extraBookmarks) - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), - bookmarksSupplier - }) - - await manager.forget(['neo4j', 'adb']) - const bookmarks = await manager.getAllBookmarks() - - expect(bookmarks).toBeSortedEqual( - [...systemBookmarks, ...extraBookmarks] - ) - }) - - it('should forget what never reminded', async () => { - const extraBookmarks = ['system:bmextra', 'adb:bmextra'] - const bookmarksSupplier = jest.fn(async () => extraBookmarks) - const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]), - bookmarksSupplier - }) - - await manager.forget(['unexisting-db']) - const bookmarks = await manager.getAllBookmarks() - - expect(bookmarks).toBeSortedEqual( - [...systemBookmarks, ...neo4jBookmarks, ...extraBookmarks] - ) + expect(bookmarksConsumer).toBeCalledWith(newBookmarks) }) }) }) diff --git a/packages/core/test/session.test.ts b/packages/core/test/session.test.ts index 6d1d1f417..58795e4f4 100644 --- a/packages/core/test/session.test.ts +++ b/packages/core/test/session.test.ts @@ -315,12 +315,9 @@ describe('session', () => { expect(tx).toBeDefined() }) - it('should acquire connection with system bookmarks from the bookmark manager', async () => { + it('should acquire connection with bookmarks from the bookmark manager', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = mockBeginWithSuccess(newFakeConnection()) @@ -334,16 +331,13 @@ describe('session', () => { await session.beginTransaction() expect(connectionProvider.acquireConnection).toBeCalledWith( - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) }) + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks]) }) ) }) - it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks', async () => { + it('should acquire connection with bookmarks from the bookmark manager + lastBookmarks', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = mockBeginWithSuccess(newFakeConnection()) @@ -358,7 +352,7 @@ describe('session', () => { await session.beginTransaction() expect(connectionProvider.acquireConnection).toBeCalledWith( - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...neo4jBookmarks, ...systemBookmarks]) }) ) }) @@ -367,12 +361,9 @@ describe('session', () => { [customBookmarks] ])('should call getAllBookmarks for the relevant database', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) - const getAllBookmarksSpy = jest.spyOn(manager, 'getAllBookmarks') + const getAllBookmarksSpy = jest.spyOn(manager, 'getBookmarks') const connection = mockBeginWithSuccess(newFakeConnection()) @@ -394,10 +385,7 @@ describe('session', () => { [customBookmarks] ])('should call begin query with getAllBookmarks + lastBookmarks', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = mockBeginWithSuccess(newFakeConnection()) @@ -425,10 +413,7 @@ describe('session', () => { ['adb'] ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') @@ -455,10 +440,7 @@ describe('session', () => { ['adb', 'adb'] ])('should call updateBookmarks when server returns non-empty bookmarks', async (metaDb, updateDb) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') @@ -476,7 +458,7 @@ describe('session', () => { await tx.commit() expect(updateBookmarksSpy).toBeCalledTimes(1) - expect(updateBookmarksSpy).toBeCalledWith(updateDb, [...neo4jBookmarks, ...systemBookmarks], customBookmarks) + expect(updateBookmarksSpy).toBeCalledWith([...neo4jBookmarks, ...systemBookmarks], customBookmarks) }) it.each([ @@ -485,10 +467,7 @@ describe('session', () => { ['adb'] ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') @@ -548,12 +527,9 @@ describe('session', () => { }) describe('.run()', () => { - it('should acquire connection with system bookmarks from the bookmark manager', async () => { + it('should acquire connection with bookmarks from the bookmark manager', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -567,16 +543,13 @@ describe('session', () => { await session.run('query') expect(connectionProvider.acquireConnection).toBeCalledWith( - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) }) + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks]) }) ) }) - it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks', async () => { + it('should acquire connection with bookmarks from the bookmark manager + lastBookmarks', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -591,16 +564,13 @@ describe('session', () => { await session.run('query') expect(connectionProvider.acquireConnection).toBeCalledWith( - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...neo4jBookmarks, ...systemBookmarks]) }) ) }) - it('should acquire connection with system bookmarks from the bookmark manager when bookmarks already updated', async () => { + it('should acquire connection with bookmarks from the bookmark manager when bookmarks already updated', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -618,16 +588,17 @@ describe('session', () => { await session.run('query') - expect(connectionProvider.acquireConnection).toHaveBeenNthCalledWith(2, - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(systemBookmarks) })) + expect(connectionProvider.acquireConnection).toHaveBeenCalledTimes(2) + expect(connectionProvider.acquireConnection).toHaveBeenNthCalledWith(1, + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...neo4jBookmarks, ...systemBookmarks]) })) + + expect(connectionProvider.acquireConnection).toHaveBeenLastCalledWith( + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks(['other-bookmark']) })) }) - it('should acquire connection with system bookmarks from the bookmark manager + lastBookmarks when bookmarks not updated', async () => { + it('should acquire connection with bookmarks from the bookmark manager + lastBookmarks when bookmarks not updated', async () => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -646,21 +617,18 @@ describe('session', () => { await session.run('query') expect(connectionProvider.acquireConnection).toHaveBeenNthCalledWith(2, - expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...systemBookmarks]) }) + expect.objectContaining({ bookmarks: new bookmarks.Bookmarks([...customBookmarks, ...neo4jBookmarks, ...systemBookmarks]) }) ) }) it.each([ [[]], [customBookmarks] - ])('should call getAllBookmarks for the relevant database', async (lastBookmarks) => { + ])('should call getBookmarks', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) - const getAllBookmarksSpy = jest.spyOn(manager, 'getAllBookmarks') + const getAllBookmarksSpy = jest.spyOn(manager, 'getBookmarks') const connection = newFakeConnection() @@ -680,12 +648,9 @@ describe('session', () => { it.each([ [[]], [customBookmarks] - ])('should call run query with getAllBookmarks + lastBookmarks', async (lastBookmarks) => { + ])('should call run query with getBookmarks + lastBookmarks', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -710,12 +675,9 @@ describe('session', () => { it.each([ [[]], [customBookmarks] - ])('should call run query with getAllBookmarks + lastBookmarks when bookmarks not updated', async (lastBookmarks) => { + ])('should call run query with getBookmarks + lastBookmarks when bookmarks not updated', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -744,12 +706,9 @@ describe('session', () => { it.each([ [[]], [customBookmarks] - ])('should call run query with getAllBookmarks when bookmarks updated', async (lastBookmarks) => { + ])('should call run query with getBookmarks when bookmarks updated', async (lastBookmarks) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const connection = newFakeConnection() @@ -765,13 +724,13 @@ describe('session', () => { await session.run('query') const { afterComplete } = connection.seenProtocolOptions[0] afterComplete({ db: 'neo4j', bookmark: 'abc' }) - await manager.updateBookmarks('neo4j', ['abc'], neo4jBookmarks) + await manager.updateBookmarks(['abc'], neo4jBookmarks) await session.run('query') expect(connection.seenProtocolOptions[1]).toEqual( expect.objectContaining({ - bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks, ...systemBookmarks]) + bookmarks: new bookmarks.Bookmarks([...neo4jBookmarks]) }) ) }) @@ -782,10 +741,7 @@ describe('session', () => { ['adb', 'adb'] ])('should call updateBookmarks when server returns non-empty bookmarks', async (metaDb, updateDb) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') @@ -805,7 +761,7 @@ describe('session', () => { afterComplete({ db: metaDb, bookmark: customBookmarks }) - expect(updateBookmarksSpy).toBeCalledWith(updateDb, [...neo4jBookmarks, ...systemBookmarks], customBookmarks) + expect(updateBookmarksSpy).toBeCalledWith([...neo4jBookmarks, ...systemBookmarks], customBookmarks) }) it.each([ @@ -814,10 +770,7 @@ describe('session', () => { ['adb'] ])('should not call updateBookmarks when server returns no bookmarks', async (metaDb) => { const manager = bookmarkManager({ - initialBookmarks: new Map([ - ['neo4j', neo4jBookmarks], - ['system', systemBookmarks] - ]) + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] }) const updateBookmarksSpy = jest.spyOn(manager, 'updateBookmarks') @@ -847,7 +800,7 @@ function mockBeginWithSuccess (connection: FakeConnection): FakeConnection { connection.protocol = () => { return { ...protocol, - beginTransaction: (params: { afterComplete: () => {}}, ...args: any[]) => { + beginTransaction: (params: { afterComplete: () => {} }, ...args: any[]) => { protocol.beginTransaction([params, ...args]) params.afterComplete() } @@ -861,7 +814,7 @@ function mockCommitWithSuccess (connection: FakeConnection, metadata: any): Fake connection.protocol = () => { return { ...protocol, - commitTransaction: (params: { afterComplete: (metadata: any) => {}}, ...args: any[]) => { + commitTransaction: (params: { afterComplete: (metadata: any) => {} }, ...args: any[]) => { const observer = protocol.commitTransaction(...[params, ...args]) params.afterComplete(metadata) return observer @@ -915,7 +868,7 @@ function setupSession ({ }) if (beginTx) { - session.beginTransaction().catch(e => {}) // force session to acquire new connection + session.beginTransaction().catch(e => { }) // force session to acquire new connection } return { session, connectionProvider } } diff --git a/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts b/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts index 3d2a36168..f74849bfd 100644 --- a/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts +++ b/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts @@ -36,54 +36,30 @@ export default class BookmarkManager { * Method called when the bookmarks get updated when a transaction finished. * * This method will be called when auto-commit queries finish and when explicit transactions - * get commited. + * get committed. * - * @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. * @returns {void} */ - async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { + async updateBookmarks (previousBookmarks: Iterable, newBookmarks: Iterable): Promise { throw new Error('Not implemented') } /** - * Method called by the driver to get the bookmarks for one specific database + * Method called by the driver to get the bookmarks. * - * @param {string} database The database which the bookmarks belong to * @returns {Iterable} The set of bookmarks */ - async getBookmarks (database: string): Promise> { - throw new Error('Not implemented') - } - - /** - * Method called by the driver for getting all the bookmarks. - * - * This method should return all bookmarks for all databases present in the BookmarkManager. - * - * @returns {Iterable} The set of bookmarks - */ - async getAllBookmarks (): Promise> { - 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 {Iterable} databases The databases which the bookmarks will be removed for. - */ - async forget (databases: Iterable): Promise { + async getBookmarks (): Promise> { throw new Error('Not implemented') } } export interface BookmarkManagerConfig { - initialBookmarks?: Map> - bookmarksSupplier?: (database?: string) => Promise> - bookmarksConsumer?: (database: string, bookmarks: Iterable) => Promise + initialBookmarks?: Iterable + bookmarksSupplier?: () => Promise> + bookmarksConsumer?: (bookmarks: Iterable) => Promise } /** @@ -91,11 +67,9 @@ export interface BookmarkManagerConfig { * * @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 - * @property {function(database: string, bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks for database get updated + * @property {Iterable} [initialBookmarks@experimental] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {function():Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager + * @property {function(bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks get updated */ /** * Provides an configured {@link BookmarkManager} instance. @@ -106,9 +80,7 @@ export interface BookmarkManagerConfig { * @returns {BookmarkManager} */ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkManager { - const initialBookmarks = new Map>() - - config.initialBookmarks?.forEach((v, k) => initialBookmarks.set(k, new Set(v))) + const initialBookmarks = new Set(config.initialBookmarks) return new Neo4jBookmarkManager( initialBookmarks, @@ -119,15 +91,15 @@ export function bookmarkManager (config: BookmarkManagerConfig = {}): BookmarkMa class Neo4jBookmarkManager implements BookmarkManager { constructor ( - private readonly _bookmarksPerDb: Map>, - private readonly _bookmarksSupplier?: (database?: string) => Promise>, - private readonly _bookmarksConsumer?: (database: string, bookmark: Iterable) => Promise + private readonly _bookmarks: Set, + private readonly _bookmarksSupplier?: () => Promise>, + private readonly _bookmarksConsumer?: (bookmark: Iterable) => Promise ) { } - async updateBookmarks (database: string, previousBookmarks: Iterable, newBookmarks: Iterable): Promise { - const bookmarks = this._getOrInitializeBookmarks(database) + async updateBookmarks (previousBookmarks: Iterable, newBookmarks: Iterable): Promise { + const bookmarks = this._bookmarks for (const bm of previousBookmarks) { bookmarks.delete(bm) } @@ -135,40 +107,13 @@ class Neo4jBookmarkManager implements BookmarkManager { bookmarks.add(bm) } if (typeof this._bookmarksConsumer === 'function') { - await this._bookmarksConsumer(database, [...bookmarks]) - } - } - - private _getOrInitializeBookmarks (database: string): Set { - let maybeBookmarks = this._bookmarksPerDb.get(database) - if (maybeBookmarks === undefined) { - maybeBookmarks = new Set() - this._bookmarksPerDb.set(database, maybeBookmarks) - } - return maybeBookmarks - } - - async getBookmarks (database: string): Promise> { - const bookmarks = new Set(this._bookmarksPerDb.get(database)) - - if (typeof this._bookmarksSupplier === 'function') { - const suppliedBookmarks = await this._bookmarksSupplier(database) ?? [] - for (const bm of suppliedBookmarks) { - bookmarks.add(bm) - } + await this._bookmarksConsumer([...bookmarks]) } - - return [...bookmarks] } - async getAllBookmarks (): Promise> { - const bookmarks = new Set() + async getBookmarks (): Promise> { + const bookmarks = new Set(this._bookmarks) - for (const [, dbBookmarks] of this._bookmarksPerDb) { - for (const bm of dbBookmarks) { - bookmarks.add(bm) - } - } if (typeof this._bookmarksSupplier === 'function') { const suppliedBookmarks = await this._bookmarksSupplier() ?? [] for (const bm of suppliedBookmarks) { @@ -176,12 +121,6 @@ class Neo4jBookmarkManager implements BookmarkManager { } } - return bookmarks - } - - async forget (databases: Iterable): Promise { - for (const database of databases) { - this._bookmarksPerDb.delete(database) - } + return [...bookmarks] } } diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index 5244aa67a..bf8933ca0 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -194,10 +194,10 @@ class SessionConfig { * 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 + * **Warning**: Share the same BookmarkManager instance across 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. + * For keeping consistency between a group of sessions, use {@link BookmarkManager} instance for grouping them. * * @example * const bookmarkManager = neo4j.bookmarkManager() @@ -214,7 +214,7 @@ class SessionConfig { * * // 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'}) + * const linkedResult = await linkedSession2.run('CREATE (p:Person {name: $name}) RETURN p', { name: 'Driver User'}) * * await linkedSession1.close() * await linkedSession2.close() diff --git a/packages/neo4j-driver-deno/lib/core/session.ts b/packages/neo4j-driver-deno/lib/core/session.ts index fd6770345..ab8cd9750 100644 --- a/packages/neo4j-driver-deno/lib/core/session.ts +++ b/packages/neo4j-driver-deno/lib/core/session.ts @@ -343,7 +343,7 @@ class Session { } private async _bookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getAllBookmarks() + const bookmarks = await this._bookmarkManager?.getBookmarks() if (bookmarks === undefined) { return this._lastBookmarks } @@ -489,7 +489,7 @@ class Session { } private async _getConnectionAcquistionBookmarks (): Promise { - const bookmarks = await this._bookmarkManager?.getBookmarks('system') + const bookmarks = await this._bookmarkManager?.getBookmarks() if (bookmarks === undefined) { return this._lastBookmarks } @@ -505,7 +505,6 @@ class Session { _updateBookmarks (newBookmarks?: Bookmarks, previousBookmarks?: Bookmarks, database?: string): void { if ((newBookmarks != null) && !newBookmarks.isEmpty()) { this._bookmarkManager?.updateBookmarks( - database ?? this._database, previousBookmarks?.values() ?? [], newBookmarks?.values() ?? [] ) diff --git a/packages/neo4j-driver-lite/test/unit/index.test.ts b/packages/neo4j-driver-lite/test/unit/index.test.ts index c19f37ae6..8b7476756 100644 --- a/packages/neo4j-driver-lite/test/unit/index.test.ts +++ b/packages/neo4j-driver-lite/test/unit/index.test.ts @@ -93,16 +93,10 @@ describe('index', () => { it('should treat BookmarkManager as an interface', () => { const bookmarkManager: BookmarkManager = { - async getAllBookmarks (): Promise { + async getBookmarks (): Promise { return [] }, - async getBookmarks (database: string): Promise { - return [] - }, - async updateBookmarks (database: string, previousBookmarks: string[], newBookmarks: string[]): Promise { - - }, - async forget (databases: string[]): Promise { + async updateBookmarks (previousBookmarks: string[], newBookmarks: string[]): Promise { } } diff --git a/packages/testkit-backend/src/request-handlers.js b/packages/testkit-backend/src/request-handlers.js index 49b8fb0a2..3a030f7e4 100644 --- a/packages/testkit-backend/src/request-handlers.js +++ b/packages/testkit-backend/src/request-handlers.js @@ -428,21 +428,18 @@ export function NewBookmarkManager ( const id = context.addBookmarkManager((bookmarkManagerId) => { let bookmarksSupplier let bookmarksConsumer - if (initialBookmarks != null) { - initialBookmarks = new Map(Object.entries(initialBookmarks)) - } if (bookmarksSupplierRegistered === true) { - bookmarksSupplier = (database) => + bookmarksSupplier = () => new Promise((resolve, reject) => { const id = context.addBookmarkSupplierRequest(resolve, reject) - wire.writeResponse(responses.BookmarksSupplierRequest({ id, bookmarkManagerId, database })) + wire.writeResponse(responses.BookmarksSupplierRequest({ id, bookmarkManagerId })) }) } if (bookmarksConsumerRegistered === true) { - bookmarksConsumer = (database, bookmarks) => + bookmarksConsumer = (bookmarks) => new Promise((resolve, reject) => { const id = context.addNotifyBookmarksRequest(resolve, reject) - wire.writeResponse(responses.BookmarksConsumerRequest({ id, bookmarkManagerId, database, bookmarks })) + wire.writeResponse(responses.BookmarksConsumerRequest({ id, bookmarkManagerId, bookmarks })) }) } bookmarkManager = neo4j.bookmarkManager({ From e57624dc525a1aa9ee523e812f2c3ee18b96366a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Wed, 23 Nov 2022 11:21:35 +0100 Subject: [PATCH 2/8] Apply suggestions from code review --- 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 f74849bfd..9ebee832f 100644 --- a/packages/core/src/bookmark-manager.ts +++ b/packages/core/src/bookmark-manager.ts @@ -67,7 +67,7 @@ export interface BookmarkManagerConfig { * * @since 5.0 * @experimental - * @property {Iterable} [initialBookmarks@experimental] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {Iterable} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. * @property {function():Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * @property {function(bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks get updated */ From 2f3b4514027a0d09820385db7a2bc301ba4883ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Wed, 23 Nov 2022 11:56:04 +0100 Subject: [PATCH 3/8] Apply suggestions from code review Co-authored-by: Robsdedude --- packages/core/test/bookmark-manager.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 10d97b9b3..90dd6cb51 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -92,7 +92,7 @@ describe('BookmarkManager', () => { }) describe('updateBookmarks()', () => { - it('should remove previous bookmarks and new bookmarks', async () => { + it('should replace previous bookmarks with new bookmarks', async () => { const newBookmarks = ['neo4j:bm03'] const manager = bookmarkManager({ initialBookmarks: [...neo4jBookmarks, ...systemBookmarks] From 7bbbc4eca51289c15b9699a2ffc0422d2b45421b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Wed, 23 Nov 2022 11:56:37 +0100 Subject: [PATCH 4/8] Apply suggestions from code review Co-authored-by: grant lodge <6323995+thelonelyvulpes@users.noreply.github.com> --- packages/core/src/driver.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/driver.ts b/packages/core/src/driver.ts index 5cc5bdc1a..8de1de9b2 100644 --- a/packages/core/src/driver.ts +++ b/packages/core/src/driver.ts @@ -194,7 +194,7 @@ class SessionConfig { * 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 across all session can have a negative impact + * **Warning**: Sharing the same BookmarkManager instance across multiple sessions 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 grouping them. From 5e1bfb7639a3cc3d8295d184fd89338b18a48a43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Wed, 23 Nov 2022 08:06:48 -0300 Subject: [PATCH 5/8] Addressing Rouven comment about bookmarking leak --- packages/core/test/bookmark-manager.test.ts | 22 +++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 90dd6cb51..e1fe64e27 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -63,6 +63,28 @@ describe('BookmarkManager', () => { ) }) + it('should return not leak bookmarks from bookmarks supplier', async () => { + const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] + const bookmarksSupplier = jest.fn() + bookmarkManager.mockReturnValueOnce(Promise.resolve(extraBookmarks)).mockReturnValue(Promise.resolve([])) + const manager = bookmarkManager({ + initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], + bookmarksSupplier + }) + + const bookmarksWithExtraBookmarks = await manager.getBookmarks() + + expect(bookmarksWithExtraBookmarks).toBeSortedEqual( + [...neo4jBookmarks, ...systemBookmarks, ...extraBookmarks] + ) + + const internalBookmarks = await manager.getBookmarks() + + expect(bookmarksWithExtraBookmarks).toBeSortedEqual( + [...neo4jBookmarks, ...systemBookmarks] + ) + }) + it('should return duplicate bookmarks if bookmarksSupplier returns already existing bm', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] const bookmarksSupplier = jest.fn(async () => await Promise.resolve([...extraBookmarks, ...systemBookmarks])) From 42a84ae0d80a66bd456b13e03bbfc4e2745550f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Antonio=20Barc=C3=A9los?= Date: Wed, 23 Nov 2022 08:25:38 -0300 Subject: [PATCH 6/8] Fix test --- packages/core/test/bookmark-manager.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index e1fe64e27..5e82d1f8e 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -63,10 +63,10 @@ describe('BookmarkManager', () => { ) }) - it('should return not leak bookmarks from bookmarks supplier', async () => { + it('should not leak bookmarks from bookmarks supplier to the internal state', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] const bookmarksSupplier = jest.fn() - bookmarkManager.mockReturnValueOnce(Promise.resolve(extraBookmarks)).mockReturnValue(Promise.resolve([])) + bookmarksSupplier.mockReturnValueOnce(Promise.resolve(extraBookmarks)).mockReturnValue(Promise.resolve([])) const manager = bookmarkManager({ initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksSupplier From c6e5098390172577cbec26e2a6e68668cf0627ea Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 23 Nov 2022 13:32:26 +0100 Subject: [PATCH 7/8] Adjust test --- packages/core/test/bookmark-manager.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/test/bookmark-manager.test.ts b/packages/core/test/bookmark-manager.test.ts index 5e82d1f8e..1b71ad9a2 100644 --- a/packages/core/test/bookmark-manager.test.ts +++ b/packages/core/test/bookmark-manager.test.ts @@ -66,7 +66,9 @@ describe('BookmarkManager', () => { it('should not leak bookmarks from bookmarks supplier to the internal state', async () => { const extraBookmarks = ['neo4j:bmextra', 'system:bmextra', 'adb:bmextra'] const bookmarksSupplier = jest.fn() - bookmarksSupplier.mockReturnValueOnce(Promise.resolve(extraBookmarks)).mockReturnValue(Promise.resolve([])) + bookmarksSupplier + .mockReturnValueOnce(Promise.resolve(extraBookmarks)) + .mockReturnValue(Promise.resolve([])) const manager = bookmarkManager({ initialBookmarks: [...neo4jBookmarks, ...systemBookmarks], bookmarksSupplier @@ -80,7 +82,7 @@ describe('BookmarkManager', () => { const internalBookmarks = await manager.getBookmarks() - expect(bookmarksWithExtraBookmarks).toBeSortedEqual( + expect(internalBookmarks).toBeSortedEqual( [...neo4jBookmarks, ...systemBookmarks] ) }) From 528210f0cb5f305755ed0168a9ab0b47d38098c8 Mon Sep 17 00:00:00 2001 From: Antonio Barcelos Date: Wed, 23 Nov 2022 13:33:01 +0100 Subject: [PATCH 8/8] Update DenoJS implementation --- packages/neo4j-driver-deno/lib/core/bookmark-manager.ts | 2 +- packages/neo4j-driver-deno/lib/core/driver.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts b/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts index f74849bfd..9ebee832f 100644 --- a/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts +++ b/packages/neo4j-driver-deno/lib/core/bookmark-manager.ts @@ -67,7 +67,7 @@ export interface BookmarkManagerConfig { * * @since 5.0 * @experimental - * @property {Iterable} [initialBookmarks@experimental] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. + * @property {Iterable} [initialBookmarks] Defines the initial set of bookmarks. The key is the database name and the values are the bookmarks. * @property {function():Promise>} [bookmarksSupplier] Called for supplying extra bookmarks to the BookmarkManager * @property {function(bookmarks: Iterable): Promise} [bookmarksConsumer] Called when the set of bookmarks get updated */ diff --git a/packages/neo4j-driver-deno/lib/core/driver.ts b/packages/neo4j-driver-deno/lib/core/driver.ts index bf8933ca0..b7850ddd8 100644 --- a/packages/neo4j-driver-deno/lib/core/driver.ts +++ b/packages/neo4j-driver-deno/lib/core/driver.ts @@ -194,7 +194,7 @@ class SessionConfig { * 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 across all session can have a negative impact + * **Warning**: Sharing the same BookmarkManager instance across multiple sessions 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 grouping them.