Home Database Cache : Stripped Back Solution #1235

MaxAake · 2024-10-16T14:25:52Z

Spikes implementation of a home database cache in the driver object, used to reduce round trips associated with identifying the home database of a user when a database is not set manually.

This behavior is turned on by a flag introduced in bolt 5.8, so the PR also includes support for Bolt 5.8

This work was migrated from PR #1230

MaxAake · 2024-10-16T14:26:40Z

[JavaScript] HomeDatabase stripped back solution spike

robsdedude

Leaving the comments of today so I don't loose them in draft mode. I'll continue the review tomorrow.

robsdedude · 2024-12-19T15:15:53Z

packages/bolt-connection/src/connection/connection-channel.js

@@ -331,6 +335,16 @@ export default class ChannelConnection extends Connection {
              if (telemetryEnabledHint === true) {
                this._telemetryDisabledConnection = false
              }
+
+              const SSREnabledHint = metadata.hints['ssr.enabled']
+              if (SSREnabledHint) {


Suggested change

if (SSREnabledHint) {

if (SSREnabledHint === true) {

Realistically, we should never see any other truthy value from the server, but I'm in favor of practicing defensive input validation :)

robsdedude · 2024-12-19T15:23:44Z

packages/bolt-connection/src/connection/connection-channel.js

+                this.serversideRouting = false
+              }
+              if (this._ssrCallback !== undefined) {
+                this._ssrCallback(this.serversideRouting, true)


nit: hard to read code. I can't see what this boolean flag does without looking at some/all implementations of the callback. I suggest to either use an enum-ish construct, different callbacks for opening and closing, or a single callback that gets passed the connection as only argument and uses con.isOpen() and con.serversideRouting to determine what to do.

robsdedude · 2024-12-19T15:46:39Z

packages/bolt-connection/src/connection/connection-channel.js

@@ -43,6 +43,7 @@ export function createChannelConnection (
  log,
  clientCertificate,
  serversideRouting = null,
+  ssrCallback = (_) => {},


nit (because internal function): new parameter not in doc comment

robsdedude · 2024-12-19T15:48:45Z

packages/bolt-connection/src/connection/connection-channel.js

+              } else {
+                this.serversideRouting = false
+              }
+              if (this._ssrCallback !== undefined) {


Is this check necessary? Can one pass in undefined with the argument having a default value? (same check also in close())

It is not. The check came before the default value and weren't removed when it was added.

packages/bolt-connection/test/bolt/bolt-protocol-v5x8.test.js

robsdedude · 2024-12-19T16:01:03Z

packages/bolt-connection/src/bolt/bolt-protocol-v5x8.js

+    return BOLT_PROTOCOL_V5_8
+  }
+
+  get transformer () {


JS noob question: Why is this override that's the exact same as its parent class necessary?

It really isn't. Every 5.x release has been overriding with it's own transformer, despite no changes having been made, so effectively they're all using the same 5.0 transformer. Then at the jump to 5.6 the import wasn't bumped.

Looking at it I'm leaning towards not creating new transformer versions with every release and cleaning up empty ones, but in this PR I will just fix the last 3 bolt versions to create transformers of their own version, and leave the larger task to another PR.

robsdedude · 2024-12-19T16:22:05Z

packages/core/src/session.ts

    if (!this._databaseNameResolved) {
+      if (this._homeDatabaseCallback != null) {
+        // eslint-disable-next-line
+        this._homeDatabaseCallback((this._impersonatedUser ? 'basic:' + this._impersonatedUser : undefined)  ?? this._auth?.cacheKey ?? 'DEFAULT', database)


For better maintainability, this logic should live somewhere central (close to all the other logic that's concerned with home db caching and cache keying).

(There's a copy of this line further down - won't comment again)

robsdedude

Next batch of comments. Sorry the PR is quite extensive 😅. So I'll continue my review after the holidays 🎄. Have a lovely time 🏂!

robsdedude · 2024-12-20T14:01:52Z

packages/core/src/driver.ts

@@ -108,6 +117,7 @@ interface DriverConfig {
  logging?: LoggingConfig
  notificationFilter?: NotificationFilter
  connectionLivenessCheckTimeout?: number
+  user?: string | undefined


Is this the public driver config type? What is this new config option used for?

Very good catch! This is a remnant of older spike work that should absolutely not still be there

robsdedude · 2024-12-20T14:03:51Z

packages/core/src/connection-provider.ts

   * @property {function (databaseName:string?)} param.onDatabaseNameResolved - Callback called when the database name get resolved
+   * @property {function (databaseName:string?)} param.removeFailureFromCache - Callback for deleting lost db from cache


Documented arg type string? does not match the declared type below string. Which on is the intended way?

robsdedude · 2024-12-20T14:05:47Z

packages/core/src/connection-provider.ts

    auth?: AuthToken
+    homeDb?: any


Can a narrower type be used? Maybe string? or whatever is appropriate?

robsdedude · 2024-12-20T14:16:57Z

packages/core/src/driver.ts

@@ -55,6 +56,12 @@ const DEFAULT_MAX_CONNECTION_LIFETIME: number = 60 * 60 * 1000 // 1 hour
 */
 const DEFAULT_FETCH_SIZE: number = 1000

+/**
+ * The maximum number of entries allowed in the home database cache before pruning
+ * @type {number}


I see this is a copy pasta 📄🍝. Why declare the type of the const and put the type in the comment? Is there any benefit or is this just introducing the potential for out-of-sync information duplication?

robsdedude · 2024-12-20T14:19:43Z

packages/core/src/session.ts

@@ -101,7 +104,9 @@ class Session {
    bookmarkManager,
    notificationFilter,
    auth,
-    log
+    log,
+    homeDatabaseCallback,


Doc comment missing new (and some old 🫠) parameters.

robsdedude · 2024-12-20T14:39:08Z

packages/bolt-connection/src/connection-provider/connection-provider-routing.js

    const databaseSpecificErrorHandler = new ConnectionErrorHandler(
      SESSION_EXPIRED,
-      (error, address) => this._handleUnavailability(error, address, context.database),
-      (error, address) => this._handleWriteFailure(error, address, context.database),
+      (error, address) => {
+        if (removeFailureFromCache !== undefined) {
+          removeFailureFromCache(homeDb ?? context.database)
+        }
+        return this._handleUnavailability(error, address, context.database)
+      },
+      (error, address) => {
+        if (removeFailureFromCache !== undefined) {
+          removeFailureFromCache(homeDb ?? context.database)
+        }
+        return this._handleWriteFailure(error, address, homeDb ?? context.database)
+      },
      (error, address, conn) =>
        this._handleSecurityError(error, address, conn, context.database)
    )


I didn't see any mention of this in the ADR. Do we want to remove a cache entry on connection failure? Pros/cons? Either way, uniform behavior is desirable.

Note to myself: should we decide to go with this approach, double check the argument passed to removeFailureFromCache and convince yourself that this is the right cache entry to invalidate.

robsdedude · 2024-12-20T14:58:30Z

packages/core/src/auth.ts

I don't like this approach for 2 reasons:

it affects the public API for auth tokens (in multiple ways)

if users where using a cacheKey property, this will clash. I don't see a good use-case for this, so I think it's unlikely, but still possible

users that build their own custom auth objects (not using one of these factory methods) would have to change their code and understand what a correct cache value looks like for their custom auth tokens to work properly with the cache

the chosen cache keys are not collision free. Please find an injective mapping TOKENS → CACHE_KEYS. Where any custom token ∈ TOKENS.

packages/core/src/auth.ts

robsdedude · 2025-01-07T12:42:26Z

packages/neo4j-driver-deno/lib/core/internal/connection-holder.ts

@@ -161,9 +166,9 @@ class ConnectionHolder implements ConnectionHolderInterface {
    return this._referenceCount
  }

-  initializeConnection (): boolean {
+  initializeConnection (homeDatabaseTable?: any): boolean {


Suggested change

initializeConnection (homeDatabaseTable?: any): boolean {

initializeConnection (homeDatabaseTable?: string): boolean {

At least that's what _createConnectionPromise expects and I don't see any narrowing type checks or such here.

robsdedude · 2025-01-07T13:12:26Z

packages/core/src/internal/connection-holder.ts

    return await connectionProvider.acquireConnection({
      accessMode: this._mode,
-      database: this._database,
+      database: this._database ?? '',


To make sure I've not missed something: is this change necessary?

I see that ConnectionHolder.setDatabase would accept and set _database to null (which , but I don't think was was happening before or is happening now. I think _database should maybe be typed string, not ?: string.

robsdedude · 2025-01-07T13:35:39Z

packages/bolt-connection/src/connection-provider/connection-provider-routing.js

    const databaseSpecificErrorHandler = new ConnectionErrorHandler(
      SESSION_EXPIRED,
-      (error, address) => this._handleUnavailability(error, address, context.database),
-      (error, address) => this._handleWriteFailure(error, address, context.database),
+      (error, address) => {
+        if (removeFailureFromCache !== undefined) {
+          removeFailureFromCache(homeDb ?? context.database)
+        }
+        return this._handleUnavailability(error, address, context.database)
+      },
+      (error, address) => {
+        if (removeFailureFromCache !== undefined) {
+          removeFailureFromCache(homeDb ?? context.database)
+        }
+        return this._handleWriteFailure(error, address, homeDb ?? context.database)
+      },
      (error, address, conn) =>
        this._handleSecurityError(error, address, conn, context.database)
    )


Note to myself: should we decide to go with this approach, double check the argument passed to removeFailureFromCache and convince yourself that this is the right cache entry to invalidate.

robsdedude · 2025-01-07T14:35:09Z

packages/core/src/internal/homedb-cache.ts

+  }
+
+  /**
+   * Updates or add an entry to the cache, and prunes the cache if above the maximum allowed size


Suggested change

* Updates or add an entry to the cache, and prunes the cache if above the maximum allowed size

* Updates or adds an entry to the cache, and prunes the cache if above the maximum allowed size

robsdedude · 2025-01-07T14:46:43Z

packages/core/src/internal/homedb-cache.ts

+  private _pruneCache (): void {
+    if (this.map.size > this.maxSize) {
+      const sortedArray = Array.from(this.map.entries()).sort((a, b) => a[1].lastUsed - b[1].lastUsed)
+      for (let i = 0; i < 70; i++) { // c * max_size * logn(max_size), c is set to 0.01


I think it's quite likely that this will be forgotten should the maxSize ever be adjusted.

I suggest to either compute this dynamically in the constructor or to make sure the definition of the truncation size lives right next to the definition of the maxSize along with this comment.

packages/core/src/session.ts

robsdedude · 2025-01-07T15:48:22Z

packages/core/src/session.ts

@@ -517,6 +533,24 @@ class Session {
    }
  }

+  _beginDbCallback (database: string): void {
+    // eslint-disable-next-line
+    if (this._connectionHolderWithMode(this._mode).connectionProvider()?.SSREnabled()) { 


This is something we should probably discuss with the team to make sure the drivers are aligned: if SSR is not enabled (or we don't know because of a too old protocol version), do we still want to maintain the home db cache in the hopes that a DBMS upgrade occurs later and we're having a cache already warm and ready?

robsdedude · 2025-01-07T15:54:17Z

packages/core/src/session.ts

@@ -517,6 +533,24 @@ class Session {
    }
  }

+  _beginDbCallback (database: string): void {


I think I'm missing something here: why is there a difference between _onDatabaseNameResolved and _beginDbCallback? Can't we always safely pin the resolved home db to the session regardless of SSR or not (cache updates discussion above pending)?

The differing behavior was required when the COMMIT SUCCESS message was used, and the improvement was not made when we switched to the BEGIN SUCCESS. Good catch

robsdedude · 2025-01-07T15:56:14Z

packages/core/src/session.ts

@@ -632,6 +669,7 @@ class Session {
 */
 function _createTransactionExecutor (config?: {
  maxTransactionRetryTime: number | null
+  commitCallback: any


Unused parameter?

robsdedude · 2025-01-07T16:25:04Z

packages/core/src/session.ts

Potential for optimization (dismiss if too much hassle to implement): session.run uses afterComplete to process the resolved home db. This means the cache only gets updated after all records have been streamed even though the information is already available earlier. This makes a difference if either the result stream dies/errors out or if multiple transactions are being run concurrently.

Looking at the code again, I'm unsure: does afterComplete only get to process the metadata of the last SUCCESS (i.e., the one without has_more)? In that case, this seems to be a left-over from an earlier design. The server echoes back the resolved home db on RUN SUCCESS. I assume I'm misreading the code... TestKit should've caught such thing 🤔

Migrating work from old branch

0ea5012

MaxAake self-assigned this Oct 16, 2024

function with driver auth tokens as well as session auth

c62cdaa

MaxAake changed the title ~~Home Database Cache : Stripped Back Spike~~ Home Database Cache : Stripped Back Solution Spike Oct 17, 2024

MaxAake added 25 commits October 18, 2024 10:47

minor fixes to integration and unit tests

6e9b495

Update driver.test.js

c307fb8

expanded callback usage and fixed tests for old servers

24cf859

change checks for when to update cache

9066f24

Save round trip when running transactions too

ca50142

update to homedbtable

886b90a

deno

b4b1a48

Continued work, hard to clear cache on error :/

f38a5df

continued work, requires server side fix to commit success message

195f05f

deno

317e282

logic to turn off and on caching using connection hints

d075328

deno

81a4924

change back to caching database name

ac11804

bolt 5.8 plus reworks

458a493

deno and minor fix

6b238fa

remove debug log and fix minor issue

936233f

many small rewrites, test updates

a84543e

deno and removal off cachekey in hello

ced7c46

remove cachekey from logon and init

fafde75

protect from undefined callback

b8633aa

Update index.js

2cc2e89

Update auth.test.js

050efaa

move hash function to allow imports

19e4f36

deno

dfbdeb4

small rework

a8a675f

MaxAake added 11 commits December 4, 2024 10:02

expansion of unit tests

5786c29

integration tests on driver, session and impersonated auth

0ef51c0

check for community edition in test

8d75497

better check for impersonation and correct protocol version

64a3bd1

fix typo in tests

ab87195

Update driver.test.js

a5e97a8

improved documentation

432a34b

deno

cdb6778

clean up

968b449

post-cleanup denobuild

d40daf8

correction

47429aa

MaxAake marked this pull request as ready for review December 11, 2024 13:59

MaxAake added 2 commits December 12, 2024 14:15

Update testkit.json

ca2fef3

remove failures for read fails, add testkit flag

38e2838

robsdedude self-requested a review December 19, 2024 14:08

robsdedude reviewed Dec 19, 2024

View reviewed changes

robsdedude self-requested a review December 19, 2024 16:39

robsdedude reviewed Dec 20, 2024

View reviewed changes

robsdedude self-requested a review December 20, 2024 16:46

addressing comments on the PR

734ed23

robsdedude reviewed Jan 7, 2025

View reviewed changes

MaxAake added 9 commits January 8, 2025 13:55

continued addressing of reviews

600f028

unit tests and renamed auth-util file

837ba63

deno

451714f

import issue in unit tests

27c9711

remove removeFailureFromCache callback

e404900

restrict onDb callback to only bolt 5.8 run success

f7df396

upgrade cacheKey function to implement impersonation and default key

b3c7e9e

deno

4f33599

fixing issue with correctly identifying the lack of an impersonated user

452a09e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Home Database Cache : Stripped Back Solution #1235

Home Database Cache : Stripped Back Solution #1235

MaxAake commented Oct 16, 2024 •

edited

Loading

MaxAake commented Oct 16, 2024

robsdedude left a comment

robsdedude Dec 19, 2024

robsdedude Dec 19, 2024 •

edited

Loading

robsdedude Dec 19, 2024

robsdedude Dec 19, 2024

MaxAake Jan 7, 2025

robsdedude Dec 19, 2024

MaxAake Jan 7, 2025

robsdedude Dec 19, 2024

robsdedude left a comment

robsdedude Dec 20, 2024

MaxAake Jan 7, 2025

robsdedude Dec 20, 2024

robsdedude Dec 20, 2024

robsdedude Dec 20, 2024

robsdedude Dec 20, 2024

robsdedude Dec 20, 2024 •

edited

Loading

robsdedude Jan 7, 2025

robsdedude Dec 20, 2024 •

edited

Loading

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

MaxAake Jan 8, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

robsdedude Jan 7, 2025

		* @property {function (databaseName:string?)} param.onDatabaseNameResolved - Callback called when the database name get resolved
		* @property {function (databaseName:string?)} param.removeFailureFromCache - Callback for deleting lost db from cache

	initializeConnection (homeDatabaseTable?: any): boolean {
	initializeConnection (homeDatabaseTable?: string): boolean {

	* Updates or add an entry to the cache, and prunes the cache if above the maximum allowed size
	* Updates or adds an entry to the cache, and prunes the cache if above the maximum allowed size

Home Database Cache : Stripped Back Solution #1235

Are you sure you want to change the base?

Home Database Cache : Stripped Back Solution #1235

Conversation

MaxAake commented Oct 16, 2024 • edited Loading

MaxAake commented Oct 16, 2024

robsdedude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsdedude Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsdedude left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsdedude Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robsdedude Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxAake commented Oct 16, 2024 •

edited

Loading

robsdedude Dec 19, 2024 •

edited

Loading

robsdedude Dec 20, 2024 •

edited

Loading

robsdedude Dec 20, 2024 •

edited

Loading