From e10d8101cb36eadb3945efcb992f71ceef5d43ba Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Fri, 3 Jun 2022 09:20:01 +0200 Subject: [PATCH 1/2] Adjust Retryable Errors * Deprecate `is_retriable()` in favor of `is_retryable` * Rewrite errors for backwards compatibility * Neo.TransientError.Transaction.Terminated -> Neo.ClientError.Transaction.Terminated * Neo.TransientError.Transaction.LockClientStopped -> Neo.ClientError.Transaction.LockClientStopped * Add unit tests --- CHANGELOG.md | 2 + docs/source/api.rst | 4 +- neo4j/_async/work/session.py | 2 +- neo4j/_async/work/transaction.py | 2 +- neo4j/_sync/work/session.py | 2 +- neo4j/_sync/work/transaction.py | 2 +- neo4j/exceptions.py | 77 ++++++++++++++++++++++------ tests/unit/common/test_exceptions.py | 65 ++++++++++++++++------- 8 files changed, 114 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffe36b39f..dd30687de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,8 @@ - ANSI colour codes for log output are now opt-in - Prepend log format with log-level (if colours are disabled) - Prepend log format with thread name and id +- Deprecated `neo4j.exceptions.Neo4jError.is_retriable()`. + Use `neo4j.exceptions.Neo4jError.is_retryable()` instead. ## Version 4.4 diff --git a/docs/source/api.rst b/docs/source/api.rst index 30a2e92f9..95ef0cb9f 100644 --- a/docs/source/api.rst +++ b/docs/source/api.rst @@ -1186,7 +1186,7 @@ Neo4j Execution Errors .. autoclass:: neo4j.exceptions.Neo4jError - :members: message, code, is_retriable + :members: message, code, is_retriable, is_retryable .. autoclass:: neo4j.exceptions.ClientError @@ -1244,7 +1244,7 @@ Connectivity Errors .. autoclass:: neo4j.exceptions.DriverError - :members: is_retriable + :members: is_retryable .. autoclass:: neo4j.exceptions.TransactionError :show-inheritance: diff --git a/neo4j/_async/work/session.py b/neo4j/_async/work/session.py index fc0e045b5..329d39261 100644 --- a/neo4j/_async/work/session.py +++ b/neo4j/_async/work/session.py @@ -420,7 +420,7 @@ async def _run_transaction( await tx._commit() except (DriverError, Neo4jError) as error: await self._disconnect() - if not error.is_retriable(): + if not error.is_retryable(): raise errors.append(error) else: diff --git a/neo4j/_async/work/transaction.py b/neo4j/_async/work/transaction.py index e293a8ecd..91a2801da 100644 --- a/neo4j/_async/work/transaction.py +++ b/neo4j/_async/work/transaction.py @@ -247,7 +247,7 @@ class AsyncManagedTransaction(_AsyncTransactionBase): Note that transaction functions have to be idempotent (i.e., the result of running the function once has to be the same as running it any number of times). This is, because the driver will retry the transaction function - if the error is classified as retriable. + if the error is classified as retryable. .. versionadded:: 5.0 diff --git a/neo4j/_sync/work/session.py b/neo4j/_sync/work/session.py index 1e8175792..8af64c55a 100644 --- a/neo4j/_sync/work/session.py +++ b/neo4j/_sync/work/session.py @@ -420,7 +420,7 @@ def _run_transaction( tx._commit() except (DriverError, Neo4jError) as error: self._disconnect() - if not error.is_retriable(): + if not error.is_retryable(): raise errors.append(error) else: diff --git a/neo4j/_sync/work/transaction.py b/neo4j/_sync/work/transaction.py index 9b6b29c4b..37cf3e790 100644 --- a/neo4j/_sync/work/transaction.py +++ b/neo4j/_sync/work/transaction.py @@ -247,7 +247,7 @@ class ManagedTransaction(_TransactionBase): Note that transaction functions have to be idempotent (i.e., the result of running the function once has to be the same as running it any number of times). This is, because the driver will retry the transaction function - if the error is classified as retriable. + if the error is classified as retryable. .. versionadded:: 5.0 diff --git a/neo4j/exceptions.py b/neo4j/exceptions.py index 1bf45def5..5bebf5e0a 100644 --- a/neo4j/exceptions.py +++ b/neo4j/exceptions.py @@ -63,14 +63,38 @@ + BoltFailure + BoltProtocolError + Bolt* - """ + +from .meta import deprecated + + CLASSIFICATION_CLIENT = "ClientError" CLASSIFICATION_TRANSIENT = "TransientError" CLASSIFICATION_DATABASE = "DatabaseError" +ERROR_REWRITE_MAP = { + # This error can be retried ed. The driver just needs to re-authenticate + # with the same credentials. + "Neo.ClientError.Security.AuthorizationExpired": ( + CLASSIFICATION_TRANSIENT, None + ), + # In 5.0, this error has been re-classified as ClientError. + # For backwards compatibility with Neo4j 4.4 and earlier, we re-map it in + # the driver, too. + "Neo.TransientError.Transaction.Terminated": ( + CLASSIFICATION_CLIENT, "Neo.ClientError.Transaction.Terminated" + ), + # In 5.0, this error has been re-classified as ClientError. + # For backwards compatibility with Neo4j 4.4 and earlier, we re-map it in + # the driver, too. + "Neo.TransientError.Transaction.LockClientStopped": ( + CLASSIFICATION_CLIENT, "Neo.ClientError.Transaction.LockClientStopped" + ), +} + + class Neo4jError(Exception): """ Raised when the Cypher engine returns an error to the client. """ @@ -93,12 +117,17 @@ def hydrate(cls, message=None, code=None, **metadata): code = code or "Neo.DatabaseError.General.UnknownError" try: _, classification, category, title = code.split(".") - if code == "Neo.ClientError.Security.AuthorizationExpired": - classification = CLASSIFICATION_TRANSIENT except ValueError: classification = CLASSIFICATION_DATABASE category = "General" title = "UnknownError" + else: + classification_override, code_override = \ + ERROR_REWRITE_MAP.get(code, (None, None)) + if classification_override is not None: + classification = classification_override + if code_override is not None: + code = code_override error_class = cls._extract_error_class(classification, code) @@ -131,11 +160,31 @@ def _extract_error_class(cls, classification, code): else: return cls + # TODO 6.0: Remove this alias + @deprecated( + "Neo4jError.is_retriable is deprecated and will be removed in a " + "future version. Please use Neo4jError.is_retryable instead." + ) def is_retriable(self): """Whether the error is retryable. + See :meth:`.is_retryable`. + + :return: :const:`True` if the error is retryable, + :const:`False` otherwise. + :rtype: bool + + .. deprecated:: 5.0 + This method will be removed in a future version. + Please use :meth:`.is_retryable` instead. + """ + return self.is_retryable() + + def is_retryable(self): + """Whether the error is retryable. + Indicated whether a transaction that yielded this error makes sense to - retry. This methods makes mostly sense when implementing a custom + retry. This method makes mostly sense when implementing a custom retry policy in conjunction with :ref:`explicit-transactions-ref`. :return: :const:`True` if the error is retryable, @@ -180,14 +229,8 @@ class TransientError(Neo4jError): """ The database cannot service the request right now, retrying later might yield a successful outcome. """ - def is_retriable(self): - # Transient errors are always retriable. - # However, there are some errors that are misclassified by the server. - # They should really be ClientErrors. - return self.code not in ( - "Neo.TransientError.Transaction.Terminated", - "Neo.TransientError.Transaction.LockClientStopped", - ) + def is_retryable(self): + return True class DatabaseUnavailable(TransientError): @@ -283,11 +326,11 @@ class TokenExpired(AuthError): class DriverError(Exception): """ Raised when the Driver raises an error. """ - def is_retriable(self): + def is_retryable(self): """Whether the error is retryable. Indicated whether a transaction that yielded this error makes sense to - retry. This methods makes mostly sense when implementing a custom + retry. This method makes mostly sense when implementing a custom retry policy in conjunction with :ref:`explicit-transactions-ref`. :return: :const:`True` if the error is retryable, @@ -305,7 +348,7 @@ class SessionExpired(DriverError): def __init__(self, session, *args, **kwargs): super(SessionExpired, self).__init__(session, *args, **kwargs) - def is_retriable(self): + def is_retryable(self): return True @@ -347,7 +390,7 @@ class ServiceUnavailable(DriverError): """ Raised when no database service is available. """ - def is_retriable(self): + def is_retryable(self): return True @@ -375,7 +418,7 @@ class IncompleteCommit(ServiceUnavailable): successfully or not. """ - def is_retriable(self): + def is_retryable(self): return False diff --git a/tests/unit/common/test_exceptions.py b/tests/unit/common/test_exceptions.py index 26e0f6151..e1c89766d 100644 --- a/tests/unit/common/test_exceptions.py +++ b/tests/unit/common/test_exceptions.py @@ -211,22 +211,49 @@ def test_neo4jerror_hydrate_with_message_and_code_client(): assert error.code == "Neo.{}.General.TestError".format(CLASSIFICATION_CLIENT) -def test_transient_error_is_retriable_case_1(): - error = Neo4jError.hydrate(message="Test error message", code="Neo.TransientError.Transaction.Terminated") - - assert isinstance(error, TransientError) - assert error.is_retriable() is False - - -def test_transient_error_is_retriable_case_2(): - error = Neo4jError.hydrate(message="Test error message", code="Neo.TransientError.Transaction.LockClientStopped") - - assert isinstance(error, TransientError) - assert error.is_retriable() is False - - -def test_transient_error_is_retriable_case_3(): - error = Neo4jError.hydrate(message="Test error message", code="Neo.TransientError.General.TestError") - - assert isinstance(error, TransientError) - assert error.is_retriable() is True +@pytest.mark.parametrize( + ("code", "expected_cls", "expected_code"), + ( + ( + "Neo.TransientError.Transaction.Terminated", + ClientError, + "Neo.ClientError.Transaction.Terminated" + ), + ( + "Neo.ClientError.Transaction.Terminated", + ClientError, + "Neo.ClientError.Transaction.Terminated" + ), + ( + "Neo.TransientError.Transaction.LockClientStopped", + ClientError, + "Neo.ClientError.Transaction.LockClientStopped" + ), + ( + "Neo.ClientError.Transaction.LockClientStopped", + ClientError, + "Neo.ClientError.Transaction.LockClientStopped" + ), + ( + "Neo.ClientError.Security.AuthorizationExpired", + TransientError, + "Neo.ClientError.Security.AuthorizationExpired" + ), + ( + "Neo.TransientError.General.TestError", + TransientError, + "Neo.TransientError.General.TestError" + ) + ) +) +def test_error_rewrite(code, expected_cls, expected_code): + message = "Test error message" + error = Neo4jError.hydrate(message=message, code=code) + + expected_retryable = expected_cls is TransientError + assert error.__class__ is expected_cls + assert error.code == expected_code + assert error.message == message + assert error.is_retryable() is expected_retryable + with pytest.warns(DeprecationWarning, match=".*is_retryable.*"): + assert error.is_retriable() is expected_retryable From 0c18892a7c7f28d66b42c1f0ef3efa18d592c7ae Mon Sep 17 00:00:00 2001 From: Rouven Bauer Date: Tue, 5 Jul 2022 12:20:39 +0200 Subject: [PATCH 2/2] Fix typo --- neo4j/exceptions.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/neo4j/exceptions.py b/neo4j/exceptions.py index 43cd3a87f..70f2fb5fd 100644 --- a/neo4j/exceptions.py +++ b/neo4j/exceptions.py @@ -183,7 +183,7 @@ def is_retriable(self): def is_retryable(self): """Whether the error is retryable. - Indicated whether a transaction that yielded this error makes sense to + Indicates whether a transaction that yielded this error makes sense to retry. This method makes mostly sense when implementing a custom retry policy in conjunction with :ref:`explicit-transactions-ref`. @@ -331,7 +331,7 @@ class DriverError(Exception): def is_retryable(self): """Whether the error is retryable. - Indicated whether a transaction that yielded this error makes sense to + Indicates whether a transaction that yielded this error makes sense to retry. This method makes mostly sense when implementing a custom retry policy in conjunction with :ref:`explicit-transactions-ref`.