Add .retriable to the Neo4jError

bigmontz · bigmontz · commit 634a0b1527bb · 2022-03-16T12:13:29.000+01:00
This property should helps users to implement custom retries logics.

Along with the property, the static function `Neo4jError.isRetriable(error)` and
the function `isRetriableError(error)` were added for saving boilerplates and avoid
the error typechecking to be spread accross the client and driver code base.

The internal function `canRetryOn(error)` and the `retry-strategy` file were removed
since the logic implemented there was moved to the `error` module replaced by
the `isRetriableError(error)` usage.
diff --git a/packages/bolt-connection/test/connection/connection-channel.test.js b/packages/bolt-connection/test/connection/connection-channel.test.js
@@ -169,7 +169,7 @@ describe('ChannelConnection', () => {
         expect(loggerFunction).toHaveBeenCalledWith(
           'error',
           `${connection} experienced a fatal error caused by Neo4jError: some error ` +
-            '({"code":"C","name":"Neo4jError"})'
+            '({"code":"C","name":"Neo4jError","retriable":false})'
         )
       })
     })
@@ -217,7 +217,7 @@ describe('ChannelConnection', () => {
         expect(loggerFunction).toHaveBeenCalledWith(
           'error',
           `${connection} experienced a fatal error caused by Neo4jError: current failure ` +
-            '({"code":"ongoing","name":"Neo4jError"})'
+            '({"code":"ongoing","name":"Neo4jError","retriable":false})'
         )
       })
     })
diff --git a/packages/core/src/error.ts b/packages/core/src/error.ts
@@ -62,6 +62,7 @@ class Neo4jError extends Error {
    * Optional error code. Will be populated when error originates in the database.
    */
   code: Neo4jErrorCode
+  retriable: boolean
   __proto__: Neo4jError
 
   /**
@@ -76,6 +77,24 @@ class Neo4jError extends Error {
     this.__proto__ = Neo4jError.prototype
     this.code = code
     this.name = 'Neo4jError'
+    /**
+     * Indicates if the error is retriable.
+     * @type {boolean} - true if the error is retriable
+     */
+    this.retriable = _isRetriableCode(code)
+  }
+
+  /**
+   * Verifies if the given error is retriable.
+   *
+   * @param {object|undefined|null} error the error object
+   * @returns {boolean} true if the error is retriable
+   */
+  static isRetriable(error?: any | null): boolean {
+    return error !== null &&
+      error !== undefined &&
+      error instanceof Neo4jError &&
+      error.retriable
   }
 }
 
@@ -90,8 +109,62 @@ function newError (message: string, code?: Neo4jErrorCode): Neo4jError {
   return new Neo4jError(message, code ?? NOT_AVAILABLE)
 }
 
+/**
+ * Verifies if the given error is retriable.
+ *
+ * @public
+ * @param {object|undefined|null} error the error object
+ * @returns {boolean} true if the error is retriable
+ */
+ const isRetriableError = Neo4jError.isRetriable
+
+/**
+ * @private
+ * @param {string} code the error code
+ * @returns {boolean} true if the error is a retriable error
+ */
+function _isRetriableCode (code?: Neo4jErrorCode): boolean {
+  return code === SERVICE_UNAVAILABLE ||
+    code === SESSION_EXPIRED ||
+    _isAuthorizationExpired(code) ||
+    _isTransientError(code)
+}
+
+/**
+ * @private
+ * @param {string} code the error to check
+ * @return {boolean} true if the error is a transient error
+ */
+function _isTransientError (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 !== undefined && code.indexOf('TransientError') >= 0) {
+    if (
+      code === 'Neo.TransientError.Transaction.Terminated' ||
+      code === 'Neo.TransientError.Transaction.LockClientStopped'
+    ) {
+      return false
+    }
+    return true
+  }
+  return false
+}
+
+/**
+ * @private
+ * @param {string} code the error to check
+ * @returns {boolean} true if the error is a service unavailable error
+ */
+function _isAuthorizationExpired (code?: Neo4jErrorCode): boolean {
+  return code === 'Neo.ClientError.Security.AuthorizationExpired'
+}
+
 export {
   newError,
+  isRetriableError,
   Neo4jError,
   SERVICE_UNAVAILABLE,
   SESSION_EXPIRED,
diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts
@@ -20,6 +20,7 @@
 import {
   newError,
   Neo4jError,
+  isRetriableError,
   PROTOCOL_ERROR,
   SERVICE_UNAVAILABLE,
   SESSION_EXPIRED
@@ -93,6 +94,7 @@ const error = {
 const forExport = {
   newError,
   Neo4jError,
+  isRetriableError,
   error,
   Integer,
   int,
@@ -150,6 +152,7 @@ const forExport = {
 export {
   newError,
   Neo4jError,
+  isRetriableError,
   error,
   Integer,
   int,
diff --git a/packages/core/src/internal/index.ts b/packages/core/src/internal/index.ts
@@ -29,7 +29,6 @@ import * as logger from './logger'
 import * as urlUtil from './url-util'
 import * as serverAddress from './server-address'
 import * as resolver from './resolver'
-import * as retryStrategy from './retry-strategy'
 
 export {
   util,
@@ -43,6 +42,5 @@ export {
   logger,
   urlUtil,
   serverAddress,
-  resolver,
-  retryStrategy
+  resolver
 }
diff --git a/packages/core/src/internal/retry-strategy.ts b/packages/core/src/internal/retry-strategy.ts
diff --git a/packages/core/src/internal/transaction-executor.ts b/packages/core/src/internal/transaction-executor.ts
@@ -17,10 +17,10 @@
  * limitations under the License.
  */
 
-import { newError } from '../error'
+import { newError, isRetriableError } from '../error'
 import Transaction from '../transaction'
 import TransactionPromise from '../transaction-promise'
-import { canRetryOn } from './retry-strategy'
+
 
 const DEFAULT_MAX_RETRY_TIME_MS = 30 * 1000 // 30 seconds
 const DEFAULT_INITIAL_RETRY_DELAY_MS = 1000 // 1 seconds
@@ -107,7 +107,7 @@ export class TransactionExecutor {
   ): Promise<T> {
     const elapsedTimeMs = Date.now() - retryStartTime
 
-    if (elapsedTimeMs > this._maxRetryTimeMs || !canRetryOn(error)) {
+    if (elapsedTimeMs > this._maxRetryTimeMs || !isRetriableError(error)) {
       return Promise.reject(error)
     }
 
diff --git a/packages/core/test/error.test.ts b/packages/core/test/error.test.ts
@@ -18,6 +18,7 @@
  */
 import {
   Neo4jError,
+  isRetriableError,
   newError,
   PROTOCOL_ERROR,
   SERVICE_UNAVAILABLE,
@@ -44,6 +45,18 @@ describe('newError', () => {
   })
 })
 
+describe('isRetriableError()', () => {
+  it.each(getRetriableErrorsFixture())
+    ('should return true for error with code %s', error => {
+      expect(isRetriableError(error)).toBe(true)
+    })
+
+  it.each(getNonRetriableErrorsFixture())
+    ('should return false for error with code %s', error => {
+      expect(isRetriableError(error)).toBe(false)
+    })
+})
+
 describe('Neo4jError', () => {
   test('should have message', () => {
     const error = new Neo4jError('message', 'code')
@@ -76,4 +89,64 @@ describe('Neo4jError', () => {
     expect(error.__proto__).toEqual(Neo4jError.prototype)
     expect(error.constructor).toEqual(Neo4jError)
   })
+
+  test.each(getRetriableCodes())
+    ('should define retriable as true for error with code %s', code => {
+      const error = new Neo4jError('message', code)
+
+      expect(error.retriable).toBe(true)
+    })
+
+  test.each(getNonRetriableCodes())
+    ('should define retriable as false for error with code %s', code => {
+      const error = new Neo4jError('message', code)
+
+      expect(error.retriable).toBe(false)
+    })
+
+  describe('.isRetriable()', () => {
+    it.each(getRetriableErrorsFixture())
+      ('should return true for error with code %s', error => {
+        expect(Neo4jError.isRetriable(error)).toBe(true)
+      })
+  
+    it.each(getNonRetriableErrorsFixture())
+      ('should return false for error with code %s', error => {
+        expect(Neo4jError.isRetriable(error)).toBe(false)
+      })
+  })
 })
+
+function getRetriableErrorsFixture () {
+  return getRetriableCodes().map(code => [newError('message', code)])
+}
+
+function getNonRetriableErrorsFixture () {
+  return [
+    null,
+    undefined,
+    '',
+    'Neo.TransientError.Transaction.DeadlockDetected',
+    new Error('Neo.ClientError.Security.AuthorizationExpired'),
+    ...getNonRetriableCodes().map(code => [newError('message', code)])
+  ]
+}
+
+function getRetriableCodes () {
+  return [
+    SERVICE_UNAVAILABLE,
+    SESSION_EXPIRED,
+    'Neo.ClientError.Security.AuthorizationExpired',
+    'Neo.TransientError.Transaction.DeadlockDetected',
+    'Neo.TransientError.Network.CommunicationError'
+  ]
+}
+
+function getNonRetriableCodes () {
+  return [
+    'Neo.TransientError.Transaction.Terminated',
+    'Neo.DatabaseError.General.UnknownError',
+    'Neo.TransientError.Transaction.LockClientStopped',
+    'Neo.DatabaseError.General.OutOfMemoryError'
+  ]
+}
diff --git a/packages/neo4j-driver-lite/src/index.ts b/packages/neo4j-driver-lite/src/index.ts
@@ -21,6 +21,7 @@ import { logging } from './logging'
 
 import {
   Neo4jError,
+  isRetriableError,
   error,
   Integer,
   inSafeRange,
@@ -403,6 +404,7 @@ const forExport = {
   isDateTime,
   integer,
   Neo4jError,
+  isRetriableError,
   auth,
   logging,
   types,
@@ -453,6 +455,7 @@ export {
   isDateTime,
   integer,
   Neo4jError,
+  isRetriableError,
   auth,
   logging,
   types,
diff --git a/packages/neo4j-driver/src/index.js b/packages/neo4j-driver/src/index.js
@@ -21,6 +21,7 @@ import VERSION from './version'
 
 import {
   Neo4jError,
+  isRetryableError,
   error,
   Integer,
   inSafeRange,
@@ -383,6 +384,7 @@ const forExport = {
   isDateTime,
   integer,
   Neo4jError,
+  isRetryableError,
   auth,
   logging,
   types,
@@ -405,6 +407,7 @@ export {
   isDateTime,
   integer,
   Neo4jError,
+  isRetryableError,
   auth,
   logging,
   types,
diff --git a/packages/neo4j-driver/src/internal/retry-logic-rx.js b/packages/neo4j-driver/src/internal/retry-logic-rx.js
diff --git a/packages/neo4j-driver/types/index.d.ts b/packages/neo4j-driver/types/index.d.ts
