Reclassify Transaction.LockClientStopped and Transaction.Terminated as ClientError (#945)

bigmontz · fbiville · web-flow · commit f2312dab1c45 · 2022-09-12T14:17:50.000+02:00
* Reclassify `Transaction.LockClientStopped` and `Transaction.Terminated` as `ClientError`

The server 5.x already fix this classification error, but the driver has re-classify errors from previous version of the server for a more convenience for the user.

Co-authored-by: Florent Biville &lt;florent.biville@neo4j.com&gt;
diff --git a/packages/bolt-connection/src/bolt/response-handler.js b/packages/bolt-connection/src/bolt/response-handler.js
@@ -117,7 +117,8 @@ export default class ResponseHandler {
           this._log.debug(`S: FAILURE ${json.stringify(msg)}`)
         }
         try {
-          const error = newError(payload.message, payload.code)
+          const standardizedCode = _standardizeCode(payload.code)
+          const error = newError(payload.message, standardizedCode)
           this._currentFailure = this._observer.onErrorApplyTransformation(
             error
           )
@@ -194,3 +195,21 @@ export default class ResponseHandler {
     this._currentFailure = null
   }
 }
+
+/**
+ * Standardize error classification that are different between 5.x and previous versions.
+ *
+ * The transient error were clean-up for being retrieable and because of this
+ * `Terminated` and `LockClientStopped` were reclassified as `ClientError`.
+ *
+ * @param {string} code
+ * @returns {string} the standardized error code
+ */
+function _standardizeCode (code) {
+  if (code === 'Neo.TransientError.Transaction.Terminated') {
+    return 'Neo.ClientError.Transaction.Terminated'
+  } else if (code === 'Neo.TransientError.Transaction.LockClientStopped') {
+    return 'Neo.ClientError.Transaction.LockClientStopped'
+  }
+  return code
+}
diff --git a/packages/bolt-connection/test/bolt/response-handler.test.js b/packages/bolt-connection/test/bolt/response-handler.test.js
@@ -0,0 +1,74 @@
+/**
+ * Copyright (c) "Neo4j"
+ * Neo4j Sweden AB [https://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
+ *
+ *     https://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 ResponseHandler from '../../src/bolt/response-handler'
+import { internal, newError } from 'neo4j-driver-core'
+
+const {
+  logger: { Logger }
+} = internal
+
+const FAILURE = 0x7f // 0111 1111 // FAILURE <metadata>
+
+describe('response-handler', () => {
+  describe('.handleResponse()', () => {
+    it.each([
+      {
+        code: 'Neo.TransientError.Transaction.Terminated',
+        expectedErrorCode: 'Neo.ClientError.Transaction.Terminated'
+      },
+      {
+        code: 'Neo.TransientError.Transaction.LockClientStopped',
+        expectedErrorCode: 'Neo.ClientError.Transaction.LockClientStopped'
+      },
+      {
+        code: 'Neo.ClientError.Transaction.LockClientStopped',
+        expectedErrorCode: 'Neo.ClientError.Transaction.LockClientStopped'
+      },
+      {
+        code: 'Neo.ClientError.Transaction.Terminated',
+        expectedErrorCode: 'Neo.ClientError.Transaction.Terminated'
+      },
+      {
+        code: 'Neo.TransientError.Security.NotYourBusiness',
+        expectedErrorCode: 'Neo.TransientError.Security.NotYourBusiness'
+      }
+    ])('should fix wrong classified error codes', ({ code, expectedErrorCode }) => {
+      const observer = {
+        capturedErrors: [],
+        onFailure: error => observer.capturedErrors.push(error)
+      }
+      const message = 'Something gets wrong'
+      const expectedError = newError(message, expectedErrorCode)
+      const responseHandler = new ResponseHandler({ observer, log: Logger.noOp() })
+      responseHandler._queueObserver({})
+
+      const errorMessage = {
+        signature: FAILURE,
+        fields: [{ message, code }]
+      }
+      responseHandler.handleResponse(errorMessage)
+
+      expect(observer.capturedErrors.length).toBe(1)
+      const [receivedError] = observer.capturedErrors
+      expect(receivedError.message).toBe(expectedError.message)
+      expect(receivedError.code).toBe(expectedError.code)
+    })
+  })
+})
diff --git a/packages/core/src/error.ts b/packages/core/src/error.ts
@@ -129,30 +129,16 @@ function _isRetriableCode (code?: Neo4jErrorCode): boolean {
   return code === SERVICE_UNAVAILABLE ||
     code === SESSION_EXPIRED ||
     _isAuthorizationExpired(code) ||
-    _isRetriableTransientError(code)
+    _isTransientError(code)
 }
 
 /**
  * @private
  * @param {string} code the error to check
  * @return {boolean} true if the error is a transient error
  */
-function _isRetriableTransientError (code?: Neo4jErrorCode): boolean {
-  // Retries should not happen when transaction was explicitly terminated by the user.
-  // Termination of transaction might result in two different error codes depending on where it was
-  // terminated. These are really client errors but classification on the server is not entirely correct and
-  // they are classified as transient.
-
-  if (code?.includes('TransientError') === true) {
-    if (
-      code === 'Neo.TransientError.Transaction.Terminated' ||
-      code === 'Neo.TransientError.Transaction.LockClientStopped'
-    ) {
-      return false
-    }
-    return true
-  }
-  return false
+function _isTransientError (code?: Neo4jErrorCode): boolean {
+  return code?.includes('TransientError') === true
 }
 
 /**
diff --git a/packages/core/test/error.test.ts b/packages/core/test/error.test.ts
@@ -165,15 +165,17 @@ function getRetriableCodes (): string[] {
     SESSION_EXPIRED,
     'Neo.ClientError.Security.AuthorizationExpired',
     'Neo.TransientError.Transaction.DeadlockDetected',
-    'Neo.TransientError.Network.CommunicationError'
+    'Neo.TransientError.Network.CommunicationError',
+    'Neo.TransientError.Transaction.Terminated',
+    'Neo.TransientError.Transaction.LockClientStopped'
   ]
 }
 
 function getNonRetriableCodes (): string[] {
   return [
-    'Neo.TransientError.Transaction.Terminated',
     'Neo.DatabaseError.General.UnknownError',
-    'Neo.TransientError.Transaction.LockClientStopped',
-    'Neo.DatabaseError.General.OutOfMemoryError'
+    'Neo.DatabaseError.General.OutOfMemoryError',
+    'Neo.ClientError.Transaction.Terminated',
+    'Neo.ClientError.Transaction.LockClientStopped'
   ]
 }
diff --git a/packages/neo4j-driver/test/internal/retry-logic-rx.test.js b/packages/neo4j-driver/test/internal/retry-logic-rx.test.js
@@ -65,7 +65,7 @@ describe('#unit-rx retrylogic', () => {
       verifyNoRetry(
         newError(
           'transaction terminated',
-          'Neo.TransientError.Transaction.Terminated'
+          'Neo.ClientError.Transaction.Terminated'
         )
       )
     })
@@ -74,7 +74,7 @@ describe('#unit-rx retrylogic', () => {
       verifyNoRetry(
         newError(
           'lock client stopped',
-          'Neo.TransientError.Transaction.LockClientStopped'
+          'Neo.ClientError.Transaction.LockClientStopped'
         )
       )
     })
diff --git a/packages/neo4j-driver/test/internal/transaction-executor.test.js b/packages/neo4j-driver/test/internal/transaction-executor.test.js
@@ -30,9 +30,9 @@ const { SERVICE_UNAVAILABLE, SESSION_EXPIRED } = err
 const TRANSIENT_ERROR_1 = 'Neo.TransientError.Transaction.DeadlockDetected'
 const TRANSIENT_ERROR_2 = 'Neo.TransientError.Network.CommunicationError'
 const UNKNOWN_ERROR = 'Neo.DatabaseError.General.UnknownError'
-const TX_TERMINATED_ERROR = 'Neo.TransientError.Transaction.Terminated'
+const TX_TERMINATED_ERROR = 'Neo.ClientError.Transaction.Terminated'
 const LOCKS_TERMINATED_ERROR =
-  'Neo.TransientError.Transaction.LockClientStopped'
+  'Neo.ClientError.Transaction.LockClientStopped'
 const OOM_ERROR = 'Neo.DatabaseError.General.OutOfMemoryError'
 
 describe('#unit TransactionExecutor', () => {

Original file line number	Diff line number	Diff line change
`@@ -165,15 +165,17 @@ function getRetriableCodes (): string[] {`
`165`	`165`	`SESSION_EXPIRED,`
`166`	`166`	`'Neo.ClientError.Security.AuthorizationExpired',`
`167`	`167`	`'Neo.TransientError.Transaction.DeadlockDetected',`
`168`		`- 'Neo.TransientError.Network.CommunicationError'`
	`168`	`+ 'Neo.TransientError.Network.CommunicationError',`
	`169`	`+ 'Neo.TransientError.Transaction.Terminated',`
	`170`	`+ 'Neo.TransientError.Transaction.LockClientStopped'`
`169`	`171`	`]`
`170`	`172`	`}`
`171`	`173`
`172`	`174`	`function getNonRetriableCodes (): string[] {`
`173`	`175`	`return [`
`174`		`- 'Neo.TransientError.Transaction.Terminated',`
`175`	`176`	`'Neo.DatabaseError.General.UnknownError',`
`176`		`- 'Neo.TransientError.Transaction.LockClientStopped',`
`177`		`- 'Neo.DatabaseError.General.OutOfMemoryError'`
	`177`	`+ 'Neo.DatabaseError.General.OutOfMemoryError',`
	`178`	`+ 'Neo.ClientError.Transaction.Terminated',`
	`179`	`+ 'Neo.ClientError.Transaction.LockClientStopped'`
`178`	`180`	`]`
`179`	`181`	`}`
Original file line number	Diff line number	Diff line change
`@@ -65,7 +65,7 @@ describe('#unit-rx retrylogic', () => {`
`65`	`65`	`verifyNoRetry(`
`66`	`66`	`newError(`
`67`	`67`	`'transaction terminated',`
`68`		`- 'Neo.TransientError.Transaction.Terminated'`
	`68`	`+ 'Neo.ClientError.Transaction.Terminated'`
`69`	`69`	`)`
`70`	`70`	`)`
`71`	`71`	`})`
`@@ -74,7 +74,7 @@ describe('#unit-rx retrylogic', () => {`
`74`	`74`	`verifyNoRetry(`
`75`	`75`	`newError(`
`76`	`76`	`'lock client stopped',`
`77`		`- 'Neo.TransientError.Transaction.LockClientStopped'`
	`77`	`+ 'Neo.ClientError.Transaction.LockClientStopped'`
`78`	`78`	`)`
`79`	`79`	`)`
`80`	`80`	`})`