Skip to content

Deprecate isRetriableError and ensure isRetryableError functions #1305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jul 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ export default class PooledConnectionProvider extends ConnectionProvider {
const handled = this._authenticationProvider.handleError({ connection, code: error.code })

if (handled) {
error.retriable = true
error.retriable = true // remove in 7.0
error.retryable = true
}

if (error.code === 'Neo.ClientError.Security.AuthorizationExpired') {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ it('should call authenticationAuthProvider.handleError when TokenExpired happens
expect(handleError).toBeCalledWith({ connection: conn, code: 'Neo.ClientError.Security.TokenExpired' })
})

it('should change error to retriable when error when TokenExpired happens and staticAuthTokenManager is not being used', async () => {
it('should change error to retryable when error when TokenExpired happens and staticAuthTokenManager is not being used', async () => {
const address = ServerAddress.fromUrl('localhost:123')
const pool = newPool()
const connectionProvider = newDirectConnectionProvider(address, pool, authTokenManagers.bearer({ tokenProvider: () => null }))
Expand All @@ -223,9 +223,10 @@ it('should change error to retriable when error when TokenExpired happens and st
const error = conn.handleAndTransformError(expectedError, address)

expect(error.retriable).toBe(true)
expect(error.retryable).toBe(true)
})

it('should not change error to retriable when error when TokenExpired happens and staticAuthTokenManager is being used', async () => {
it('should not change error to retryable when error when TokenExpired happens and staticAuthTokenManager is being used', async () => {
const address = ServerAddress.fromUrl('localhost:123')
const pool = newPool()
const connectionProvider = newDirectConnectionProvider(address, pool, staticAuthTokenManager({ authToken: null }))
Expand All @@ -243,9 +244,10 @@ it('should not change error to retriable when error when TokenExpired happens an
const error = conn.handleAndTransformError(expectedError, address)

expect(error.retriable).toBe(false)
expect(error.retryable).toBe(false)
})

it('should not change error to retriable when error when TokenExpired happens and authTokenManagers.basic is being used', async () => {
it('should not change error to retryable when error when TokenExpired happens and authTokenManagers.basic is being used', async () => {
const address = ServerAddress.fromUrl('localhost:123')
const pool = newPool()
const connectionProvider = newDirectConnectionProvider(address, pool, authTokenManagers.basic({ tokenProvider: () => null }))
Expand All @@ -263,6 +265,7 @@ it('should not change error to retriable when error when TokenExpired happens an
const error = conn.handleAndTransformError(expectedError, address)

expect(error.retriable).toBe(false)
expect(error.retryable).toBe(false)
})

describe('constructor', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1704,7 +1704,7 @@ describe.each([
expect(error).toBe(expectedError)
})

it.each(usersDataSet)('should change error to retriable when error when TokenExpired happens and staticAuthTokenManager is not being used [user=%s]', async (user) => {
it.each(usersDataSet)('should change error to retryable when error when TokenExpired happens and staticAuthTokenManager is not being used [user=%s]', async (user) => {
const pool = newPool()
const connectionProvider = newRoutingConnectionProvider(
[
Expand Down Expand Up @@ -1741,10 +1741,12 @@ describe.each([
const error2 = server2Connection.handleAndTransformError(error, server2)

expect(error1.retriable).toBe(true)
expect(error1.retryable).toBe(true)
expect(error2.retriable).toBe(true)
expect(error2.retryable).toBe(true)
})

it.each(usersDataSet)('should not change error to retriable when error when TokenExpired happens and staticAuthTokenManager is being used [user=%s]', async (user) => {
it.each(usersDataSet)('should not change error to retryable when error when TokenExpired happens and staticAuthTokenManager is being used [user=%s]', async (user) => {
const pool = newPool()
const connectionProvider = newRoutingConnectionProvider(
[
Expand Down Expand Up @@ -1782,10 +1784,12 @@ describe.each([
const error2 = server2Connection.handleAndTransformError(error, server2)

expect(error1.retriable).toBe(false)
expect(error1.retryable).toBe(false)
expect(error2.retriable).toBe(false)
expect(error2.retryable).toBe(false)
})

it.each(usersDataSet)('should not change error to retriable when error when TokenExpired happens and authTokenManagers.basic is being used [user=%s]', async (user) => {
it.each(usersDataSet)('should not change error to retryable when error when TokenExpired happens and authTokenManagers.basic is being used [user=%s]', async (user) => {
const pool = newPool()
const connectionProvider = newRoutingConnectionProvider(
[
Expand Down Expand Up @@ -1823,7 +1827,9 @@ describe.each([
const error2 = server2Connection.handleAndTransformError(error, server2)

expect(error1.retriable).toBe(false)
expect(error1.retryable).toBe(false)
expect(error2.retriable).toBe(false)
expect(error2.retryable).toBe(false)
})

it.each(usersDataSet)('should use resolved seed router after accepting table with no writers [user=%s]', (user, done) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ describe('ChannelConnection', () => {
expect(loggerFunction).toHaveBeenCalledWith(
'error',
`${connection} experienced a fatal error caused by Neo4jError: some error ` +
'({"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"C","retriable":false})'
'({"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"C","retriable":false,"retryable":false})'
)
})
})
Expand Down Expand Up @@ -419,7 +419,7 @@ describe('ChannelConnection', () => {
expect(loggerFunction).toHaveBeenCalledWith(
'error',
`${connection} experienced a fatal error caused by Neo4jError: current failure ` +
'({"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. current failure","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"ongoing","retriable":false})'
'({"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. current failure","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"ongoing","retriable":false,"retryable":false})'
)
})
})
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/driver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -536,7 +536,7 @@ class Driver {
}

/**
* Executes a query in a retriable context and returns a {@link EagerResult}.
* Executes a query in a retryable context and returns a {@link EagerResult}.
*
* This method is a shortcut for a {@link Session#executeRead} and {@link Session#executeWrite}.
*
Expand Down
47 changes: 44 additions & 3 deletions packages/core/src/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,17 @@ class Neo4jError extends GQLError {
/**
* Whether the request that caused this error can be safely retried without duplicate commits on the server.
* This does not apply when running auto-commit transactions using {@link Session#run}
*
* @deprecated members using the spelling 'retriable' will be removed in 7.0. Use {@link retryable} instead.
*/
retriable: boolean

/**
* Whether the request that caused this error can be safely retried without duplicate commits on the server.
* This does not apply when running auto-commit transactions using {@link Session#run}
*/
retryable: boolean

/**
* @constructor
* @param {string} message - the error message
Expand Down Expand Up @@ -207,27 +215,49 @@ class Neo4jError extends GQLError {
*/
this.name = 'Neo4jError'

const isRetryableCode = _isRetryableCode(code)
/**
* If the error is considered retriable.
* This does not apply when running auto-commit transactions using {@link Session#run}
*
* @deprecated members using the spelling 'retriable' will be removed in 7.0. Use {@link retryable} instead.
* @type {boolean}
* @public
*/
this.retriable = isRetryableCode

/**
* If the error is considered retryable.
* This does not apply when running auto-commit transactions using {@link Session#run}
*
* @type {boolean}
* @public
*/
this.retriable = _isRetriableCode(code)
this.retryable = isRetryableCode
}

/**
* Verifies if the given error is retriable.
*
* @deprecated members using the spelling 'retriable' will be removed in 7.0. Use {@link isRetryable} instead.
* @param {object|undefined|null} error the error object
* @returns {boolean} true if the error is retriable
*/
static isRetriable (error?: any | null): boolean {
return this.isRetryable(error)
}

/**
* Verifies if the given error is retryable.
*
* @param {object|undefined|null} error the error object
* @returns {boolean} true if the error is retryable
*/
static isRetryable (error?: any | null): boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can imagine user code that writes to error.retriable. With this PR the driver starts to basically ignore that field all together (users now have to write to error.retryable to achieve the same behavior). I'm not sure there's a legitimate use-case for such a thing or whether we'd want to support this at all, but it's a subtle breakage worth considering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is true and to some extent a breaking change. However I can't see any real use-case for writing to retriable, and I'm okay with such usage needing to do a small bit of migration since it's a major version release. The alternative would be to still use the value of retriable internally, but then the user would need to use the deprecated member for their code to work, which is worse to me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convinced ✔️

Just make sure that the changelog, migration guide, etc. clearly highlights this subtle breakage.

return error !== null &&
error !== undefined &&
error instanceof Neo4jError &&
error.retriable
error.retryable
}
}

Expand Down Expand Up @@ -263,18 +293,28 @@ function newGQLError (message: string, cause?: Error, gqlStatus?: string, gqlSta
/**
* Verifies if the given error is retriable.
*
* @deprecated members using the spelling 'retriable' will be removed in 7.0. Use {@link isRetryableError} instead.
* @public
* @param {object|undefined|null} error the error object
* @returns {boolean} true if the error is retriable
*/
const isRetriableError = Neo4jError.isRetriable

/**
* Verifies if the given error is retryable.
*
* @public
* @param {object|undefined|null} error the error object
* @returns {boolean} true if the error is retryable
*/
const isRetryableError = Neo4jError.isRetryable

/**
* @private
* @param {string} code the error code
* @returns {boolean} true if the error is a retriable error
*/
function _isRetriableCode (code?: Neo4jErrorCode): boolean {
function _isRetryableCode (code?: Neo4jErrorCode): boolean {
return code === SERVICE_UNAVAILABLE ||
code === SESSION_EXPIRED ||
_isAuthorizationExpired(code) ||
Expand Down Expand Up @@ -313,6 +353,7 @@ export {
newError,
newGQLError,
isRetriableError,
isRetryableError,
Neo4jError,
GQLError,
SERVICE_UNAVAILABLE,
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
Neo4jError,
newGQLError,
GQLError,
isRetryableError,
isRetriableError,
PROTOCOL_ERROR,
SERVICE_UNAVAILABLE,
Expand Down Expand Up @@ -121,6 +122,7 @@ const forExport = {
Neo4jError,
newGQLError,
GQLError,
isRetryableError,
isRetriableError,
error,
Integer,
Expand Down Expand Up @@ -196,6 +198,7 @@ export {
Neo4jError,
newGQLError,
GQLError,
isRetryableError,
isRetriableError,
error,
Integer,
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/internal/transaction-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
* limitations under the License.
*/
/* eslint-disable @typescript-eslint/promise-function-async */
import { newError, isRetriableError } from '../error'
import { newError, isRetryableError } from '../error'
import Transaction, { NonAutoCommitApiTelemetryConfig, NonAutoCommitTelemetryApis } from '../transaction'
import TransactionPromise from '../transaction-promise'
import { TELEMETRY_APIS } from './constants'
Expand Down Expand Up @@ -151,7 +151,7 @@ export class TransactionExecutor {
): Promise<T> {
const elapsedTimeMs = Date.now() - retryStartTime

if (elapsedTimeMs > this._maxRetryTimeMs || !isRetriableError(error)) {
if (elapsedTimeMs > this._maxRetryTimeMs || !isRetryableError(error)) {
return Promise.reject(error)
}

Expand Down
6 changes: 3 additions & 3 deletions packages/core/test/__snapshots__/json.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ exports[`json .stringify should handle object with custom toString in list 1`] =

exports[`json .stringify should handle object with custom toString in object 1`] = `"{"key":{"identity":"1"}}"`;

exports[`json .stringify should handle objects created with createBrokenObject 1`] = `"{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false}}"`;
exports[`json .stringify should handle objects created with createBrokenObject 1`] = `"{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false,"retryable":false}}"`;

exports[`json .stringify should handle objects created with createBrokenObject in list 1`] = `"[{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false}}]"`;
exports[`json .stringify should handle objects created with createBrokenObject in list 1`] = `"[{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false,"retryable":false}}]"`;

exports[`json .stringify should handle objects created with createBrokenObject inside other object 1`] = `"{"number":1,"broken":{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false}}}"`;
exports[`json .stringify should handle objects created with createBrokenObject inside other object 1`] = `"{"number":1,"broken":{"__isBrokenObject__":true,"__reason__":{"gqlStatus":"50N42","gqlStatusDescription":"error: general processing exception - unexpected error. some error","diagnosticRecord":{"OPERATION":"","OPERATION_CODE":"0","CURRENT_SCHEMA":"/"},"classification":"UNKNOWN","name":"Neo4jError","code":"N/A","retriable":false,"retryable":false}}}"`;

exports[`json .stringify should handle string 1`] = `""my string""`;

Expand Down
40 changes: 21 additions & 19 deletions packages/core/test/error.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
import {
Neo4jError,
isRetriableError,
isRetryableError,
newError,
newGQLError,
PROTOCOL_ERROR,
Expand Down Expand Up @@ -123,13 +123,13 @@ describe('newError', () => {
})
})

describe('isRetriableError()', () => {
it.each(getRetriableErrorsFixture())('should return true for error with code %s', error => {
expect(isRetriableError(error)).toBe(true)
describe('isRetryableError()', () => {
it.each(getRetryableErrorsFixture())('should return true for error with code %s', error => {
expect(isRetryableError(error)).toBe(true)
})

it.each(getNonRetriableErrorsFixture())('should return false for error with code %s', error => {
expect(isRetriableError(error)).toBe(false)
it.each(getNonRetryableErrorsFixture())('should return false for error with code %s', error => {
expect(isRetryableError(error)).toBe(false)
})
})

Expand Down Expand Up @@ -178,45 +178,47 @@ describe('Neo4jError', () => {
expect(error.constructor).toEqual(Neo4jError)
})

test.each(getRetriableCodes())('should define retriable as true for error with code %s', code => {
test.each(getRetryableCodes())('should define retryable as true for error with code %s', code => {
const error = new Neo4jError('message', code, 'gqlStatus', 'gqlStatusDescription')

expect(error.retriable).toBe(true)
expect(error.retryable).toBe(true)
})

test.each(getNonRetriableCodes())('should define retriable as false for error with code %s', code => {
test.each(getNonRetryableCodes())('should define retryable as false for error with code %s', code => {
const error = new Neo4jError('message', code, 'gqlStatus', 'gqlStatusDescription')

expect(error.retriable).toBe(false)
expect(error.retryable).toBe(false)
})

describe('.isRetriable()', () => {
it.each(getRetriableErrorsFixture())('should return true for error with code %s', error => {
expect(Neo4jError.isRetriable(error)).toBe(true)
describe('.isRetryable()', () => {
it.each(getRetryableErrorsFixture())('should return true for error with code %s', error => {
expect(Neo4jError.isRetryable(error)).toBe(true)
})

it.each(getNonRetriableErrorsFixture())('should return false for error with code %s', error => {
expect(Neo4jError.isRetriable(error)).toBe(false)
it.each(getNonRetryableErrorsFixture())('should return false for error with code %s', error => {
expect(Neo4jError.isRetryable(error)).toBe(false)
})
})
})

function getRetriableErrorsFixture (): Array<[Neo4jError]> {
return getRetriableCodes().map(code => [newError('message', code)])
function getRetryableErrorsFixture (): Array<[Neo4jError]> {
return getRetryableCodes().map(code => [newError('message', code)])
}

function getNonRetriableErrorsFixture (): any[] {
function getNonRetryableErrorsFixture (): any[] {
return [
null,
undefined,
'',
'Neo.TransientError.Transaction.DeadlockDetected',
new Error('Neo.ClientError.Security.AuthorizationExpired'),
...getNonRetriableCodes().map(code => [newError('message', code)])
...getNonRetryableCodes().map(code => [newError('message', code)])
]
}

function getRetriableCodes (): string[] {
function getRetryableCodes (): string[] {
return [
SERVICE_UNAVAILABLE,
SESSION_EXPIRED,
Expand All @@ -228,7 +230,7 @@ function getRetriableCodes (): string[] {
]
}

function getNonRetriableCodes (): string[] {
function getNonRetryableCodes (): string[] {
return [
'Neo.DatabaseError.General.UnknownError',
'Neo.DatabaseError.General.OutOfMemoryError',
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading