From 83034d79754975c692634c11623559e9f8927397 Mon Sep 17 00:00:00 2001 From: Zhao vistart Date: Wed, 11 Nov 2026 11:48:18 +0800 Subject: [PATCH 1/5] feat(errors): classify Firebird driver errors by taxonomy --- .../impl/firebird/backend/async_backend.py | 4 +- .../impl/firebird/mixins/backend_mixin.py | 265 ++++++++++++++---- .../errors/test_error_taxonomy_contract.py | 4 + .../backend/test_driver_error_mapping.py | 258 +++++++++++++++++ 4 files changed, 479 insertions(+), 52 deletions(-) create mode 100644 tests/rhosocial/activerecord_firebird_test/feature/backend/errors/test_error_taxonomy_contract.py create mode 100644 tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py diff --git a/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py b/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py index 4e111c8..de677ad 100644 --- a/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py +++ b/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py @@ -176,7 +176,9 @@ async def connect(self) -> None: self.log(logging.INFO, "Connected to Firebird (async)") track_firebird_backend(self) except Exception as e: - raise exc.ConnectionError(f"Failed to connect to Firebird: {e}") from e + if isinstance(e, exc.DatabaseError): + raise + await self._handle_error(e) async def disconnect(self) -> None: """Close the connection to the Firebird database.""" diff --git a/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py b/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py index 5a75879..3010998 100644 --- a/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py +++ b/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py @@ -57,8 +57,25 @@ def untrack_firebird_backend(backend) -> None: class FirebirdBackendMixin: + # firebird-driver 2.x exposes the legacy signed SQLCODE on ``DatabaseError`` + # (``sqlcode``) plus raw GDS status codes (``gds_codes``). Codes are + # compared on ``abs()`` because both signed and unsigned forms occur. + INTEGRITY_SQLCODES = frozenset({530, 531, 625, 803, 804, 805}) + LOCK_SQLCODES = frozenset({901, 902}) + DEADLOCK_SQLCODES = frozenset({913}) + QUERY_SQLCODES = frozenset({104, 204, 206, 207}) + + # SQLSTATE is checked before driver codes: it is stable across Firebird + # versions and driver builds. + SQLSTATE_INTEGRITY = ("23000", "23505") + SQLSTATE_NOT_NULL = ("22004", "22000") + SQLSTATE_DEADLOCK = ("40001",) + SQLSTATE_CONNECTION_LOST = ("08006",) + SQLSTATE_CONNECTION_FAILED = ("08001",) + SQLSTATE_AUTHENTICATION = ("28000",) + SQLSTATE_QUERY = ("42000", "42S02", "42S22") + CONNECTION_ERROR_CODES = { - -902: "Unable to connect to database", -904: "Connection lost", -909: "Database shutdown", -916: "Database is not started", @@ -66,6 +83,57 @@ class FirebirdBackendMixin: -923: "Connection not established", } + # Message fragments are the last resort, used only when neither SQLSTATE + # nor a driver code identifies the failure. + AUTHENTICATION_KEYWORDS = ( + "user name and password are not defined", + "login failed", + "invalid user name", + "invalid username", + "authentication failed", + ) + CONNECTION_KEYWORDS = ( + "unable to connect", + "connection lost", + "connection not established", + "database shutdown", + "broken pipe", + "connection reset", + "network error", + "connection refused", + "database already in use", + "server closed", + ) + DEADLOCK_KEYWORDS = ("deadlock",) + LOCK_KEYWORDS = ( + "lock conflict", + "update conflict", + "concurrent update", + "lock timeout", + "lock time-out", + ) + INTEGRITY_KEYWORDS = ( + "unique key", + "duplicate key", + "duplicate value", + "violation of primary key", + "foreign key", + "referential integrity", + "not null", + "null value", + "validation error for column", + ) + QUERY_KEYWORDS = ( + "syntax error", + "token unknown", + "unknown table", + "table unknown", + "unknown column", + "column unknown", + "no such table", + "unsuccessful metadata update", + ) + _dialect = None _default_suggestions_cache = None @@ -100,60 +168,155 @@ def _register_firebird_adapters(self): def requires_manual_commit(self) -> bool: return True + @staticmethod + def _error_sqlstate(error: Exception): + """Return a normalized SQLSTATE string, or ``None`` when absent.""" + sqlstate = getattr(error, 'sqlstate', None) + if sqlstate is None: + return None + if isinstance(sqlstate, (list, tuple)): + if not sqlstate: + return None + sqlstate = sqlstate[0] + text = str(sqlstate).strip().upper() + return text or None + + @staticmethod + def _raw_driver_codes(error: Exception): + """Return the raw Firebird codes carried by a driver error. + + ``firebird-driver`` sets ``sqlcode`` and ``gds_codes``; some driver + wrappers and test doubles expose a plain ``code`` instead. Zero is + treated as "not reported" by the driver. + """ + values = [] + for attr in ('sqlcode', 'code'): + value = getattr(error, attr, None) + if value is not None: + values.append(value) + gds_codes = getattr(error, 'gds_codes', None) + if gds_codes is not None: + if isinstance(gds_codes, (list, tuple, set)): + values.extend(gds_codes) + else: + values.append(gds_codes) + + codes = [] + for value in values: + try: + code = int(value) + except (TypeError, ValueError): + continue + if code != 0: + codes.append(code) + return codes + + @classmethod + def _driver_error_codes(cls, error: Exception): + """Return deduplicated absolute Firebird codes for classification.""" + codes = [] + for raw in cls._raw_driver_codes(error): + code = abs(raw) + if code not in codes: + codes.append(code) + return codes + + @classmethod + def _report_driver_code(cls, error: Exception, matched_code): + """Pick the driver code to attach as diagnostics.""" + raw_codes = cls._raw_driver_codes(error) + if matched_code is not None: + for raw in raw_codes: + if abs(raw) == matched_code: + return raw + return raw_codes[0] if raw_codes else None + def _is_connection_error(self, error: Exception) -> bool: - error_code = getattr(error, 'sqlcode', None) - if error_code is not None and error_code in self.CONNECTION_ERROR_CODES: + """Return ``True`` when the error describes a connection failure.""" + sqlstate = self._error_sqlstate(error) + if sqlstate and sqlstate.startswith( + self.SQLSTATE_CONNECTION_LOST + self.SQLSTATE_CONNECTION_FAILED + ): return True - - error_msg = str(error).lower() - connection_keywords = [ - 'unable to connect', 'connection lost', 'connection not established', - 'database shutdown', 'broken pipe', 'connection reset', - 'network error', 'connection refused', 'database already in use', - ] - return any(kw in error_msg for kw in connection_keywords) + connection_codes = {abs(code) for code in self.CONNECTION_ERROR_CODES} + if any(code in connection_codes for code in self._driver_error_codes(error)): + return True + message = str(error).lower() + return any(keyword in message for keyword in self.CONNECTION_KEYWORDS) + + def _classify_error(self, error: Exception): + """Classify a driver error as ``(error_class, transient, code)``. + + Recognition order follows the backend mapping contract: SQLSTATE + first, then the Firebird SQLCODE/GDS code, then message text. + """ + sqlstate = self._error_sqlstate(error) + codes = self._driver_error_codes(error) + message = str(error).lower() + + if sqlstate: + if sqlstate.startswith(self.SQLSTATE_INTEGRITY): + # Unique/primary-key violation. + return exc.IntegrityError, False, None + if sqlstate.startswith(self.SQLSTATE_NOT_NULL): + return exc.IntegrityError, False, None + if sqlstate.startswith(self.SQLSTATE_DEADLOCK): + # Serialization failure or update conflict: another writer won + # the race, so a retry after re-reading may succeed. + return exc.DeadlockError, True, None + if sqlstate.startswith(self.SQLSTATE_CONNECTION_LOST): + return exc.OperationalError, True, None + if sqlstate.startswith(self.SQLSTATE_CONNECTION_FAILED): + return exc.ConnectionError, True, None + if sqlstate.startswith(self.SQLSTATE_AUTHENTICATION): + # Bad credentials do not improve on retry. + return exc.ConnectionError, False, None + if sqlstate.startswith(self.SQLSTATE_QUERY): + return exc.QueryError, False, None + + connection_codes = {abs(code) for code in self.CONNECTION_ERROR_CODES} + for code in codes: + if code in self.LOCK_SQLCODES: + return exc.LockError, True, code + if code in self.DEADLOCK_SQLCODES: + return exc.DeadlockError, True, code + if code in self.INTEGRITY_SQLCODES: + return exc.IntegrityError, False, code + if code in connection_codes: + return exc.ConnectionError, True, code + if code in self.QUERY_SQLCODES: + return exc.QueryError, False, code + + if any(keyword in message for keyword in self.AUTHENTICATION_KEYWORDS): + return exc.ConnectionError, False, None + if any(keyword in message for keyword in self.CONNECTION_KEYWORDS): + return exc.ConnectionError, True, None + if any(keyword in message for keyword in self.DEADLOCK_KEYWORDS): + return exc.DeadlockError, True, None + if any(keyword in message for keyword in self.LOCK_KEYWORDS): + return exc.LockError, True, None + if any(keyword in message for keyword in self.INTEGRITY_KEYWORDS): + return exc.IntegrityError, False, None + if any(keyword in message for keyword in self.QUERY_KEYWORDS): + return exc.QueryError, False, None + return exc.DatabaseError, False, None def _handle_error(self, error: Exception) -> None: - error_msg = str(error) - error_code = getattr(error, 'sqlcode', None) - gds_code = getattr(error, 'gds_codes', None) - sqlstate = getattr(error, 'sqlstate', None) - - if self._is_connection_error(error): - raise exc.ConnectionError(error_msg) from error - - if sqlstate and sqlstate.startswith('23000'): - # Integrity constraint violation (SQLSTATE 23000) covers NOT NULL, - # unique and foreign key violations across Firebird versions. - raise exc.IntegrityError(error_msg) from error - - if gds_code: - for code in gds_code if isinstance(gds_code, (list, tuple)) else [gds_code]: - if abs(code) in (803, 804, 805): - raise exc.IntegrityError(error_msg) from error - if abs(code) in (901,): - raise exc.LockError(error_msg) from error - if abs(code) in (902,): - raise exc.DeadlockError(error_msg) from error - - if error_code: - if error_code in (-803, -804, -805): - raise exc.IntegrityError(error_msg) from error - if error_code == -625: - # "validation error for column ..." (NOT NULL violation) - raise exc.IntegrityError(error_msg) from error - if error_code == -901: - raise exc.DeadlockError(error_msg) from error - if error_code in (-902, -904, -909, -916, -917, -923): - raise exc.ConnectionError(error_msg) from error - if error_code in (-530, -531): - raise exc.IntegrityError(error_msg) from error - if error_code in (-104, -206, -207): - raise exc.QueryError(error_msg) from error - if error_code in (-204,): - raise exc.DatabaseError(error_msg) from error - - raise exc.DatabaseError(error_msg) from error + """Translate a firebird-driver error into the shared taxonomy.""" + if isinstance(error, exc.DatabaseError): + # Already translated (e.g. the "no database specified" guard in + # connect()); keep its type and original cause. + raise error + + error_class, transient, matched_code = self._classify_error(error) + raise exc.attach_diagnostics( + error_class(str(error)), + operation="execute", + sqlstate=self._error_sqlstate(error), + driver_code=self._report_driver_code(error, matched_code), + backend="firebird", + transient=transient, + ) from error def get_default_adapter_suggestions(self) -> Dict[Type, Tuple[SQLTypeAdapter, Type]]: if self._default_suggestions_cache is not None: diff --git a/tests/rhosocial/activerecord_firebird_test/feature/backend/errors/test_error_taxonomy_contract.py b/tests/rhosocial/activerecord_firebird_test/feature/backend/errors/test_error_taxonomy_contract.py new file mode 100644 index 0000000..4913400 --- /dev/null +++ b/tests/rhosocial/activerecord_firebird_test/feature/backend/errors/test_error_taxonomy_contract.py @@ -0,0 +1,4 @@ +# tests/rhosocial/activerecord_firebird_test/feature/backend/errors/test_error_taxonomy_contract.py +"""Run the testsuite's semantic error taxonomy contract for the Firebird backend.""" + +from rhosocial.activerecord.testsuite.feature.backend.errors.test_error_taxonomy import * # noqa: F401,F403 diff --git a/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py new file mode 100644 index 0000000..07e2748 --- /dev/null +++ b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py @@ -0,0 +1,258 @@ +# tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py +"""Firebird driver exceptions map to the shared semantic error taxonomy. + +Every case builds a firebird-driver ``DatabaseError`` (or a tiny double that +only exposes ``sqlstate``/``code``) and calls the backend mapping function +directly. No database connection is opened, so the module runs offline; if the +driver is not installed the whole module is skipped. +""" + +import pytest + +pytest.importorskip("firebird.driver") + +from firebird.driver import DatabaseError as FirebirdDriverError # noqa: E402 + +from rhosocial.activerecord.backend.errors import ( # noqa: E402 + ConcurrencyError, + ConnectionError, + DataError, + DatabaseError, + DeadlockError, + InfrastructureError, + IntegrityError, + LockError, + OperationalError, + QueryError, +) +from rhosocial.activerecord.backend.impl.firebird.backend.backend import ( # noqa: E402 + FirebirdBackend, +) + + +@pytest.fixture +def backend(): + """Backend constructed without a connection.""" + return FirebirdBackend(database="taxonomy.fdb") + + +def translate(backend, error): + """Return the translated error while checking the original cause.""" + with pytest.raises(DatabaseError) as info: + backend._handle_error(error) + assert info.value.__cause__ is error + return info.value + + +def test_unique_sqlstate_maps_to_integrity(backend): + error = FirebirdDriverError( + "violation of PRIMARY KEY constraint", + sqlstate="23000", + sqlcode=-803, + gds_codes=(335544665,), + ) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert isinstance(translated, DataError) + assert translated.sqlstate == "23000" + assert translated.driver_code == -803 + assert translated.backend == "firebird" + assert translated.operation == "execute" + assert translated.transient is False + assert translated.retryable is False + + +def test_unique_sqlstate_23505_maps_to_integrity(backend): + error = FirebirdDriverError("duplicate key value", sqlstate="23505", sqlcode=-803) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.sqlstate == "23505" + assert translated.driver_code == -803 + + +def test_not_null_sqlstate_maps_to_integrity(backend): + error = FirebirdDriverError( + "validation error for column NAME, value *** null ***", + sqlstate="22004", + sqlcode=-625, + ) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.sqlstate == "22004" + assert translated.driver_code == -625 + assert translated.transient is False + + +def test_not_null_code_only_maps_to_integrity(backend): + error = FirebirdDriverError( + "validation error for column NAME, value *** null ***", sqlcode=-625 + ) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code == -625 + + +def test_foreign_key_code_maps_to_integrity(backend): + error = FirebirdDriverError("foreign key constraint violation", sqlcode=-530) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code == -530 + + +def test_update_conflict_sqlstate_maps_to_deadlock(backend): + error = FirebirdDriverError( + "update conflicts with concurrent update", sqlstate="40001", sqlcode=-901 + ) + translated = translate(backend, error) + assert isinstance(translated, DeadlockError) + assert isinstance(translated, LockError) + assert isinstance(translated, ConcurrencyError) + assert translated.transient is True + assert translated.retryable is True + + +def test_lock_conflict_901_maps_to_lock_error(backend): + error = FirebirdDriverError("lock conflict on no wait transaction", sqlcode=-901) + translated = translate(backend, error) + assert isinstance(translated, LockError) + assert not isinstance(translated, DeadlockError) + assert translated.driver_code == -901 + assert translated.transient is True + assert translated.retryable is True + + +def test_lock_conflict_902_maps_to_lock_error(backend): + error = FirebirdDriverError("lock conflict", sqlcode=-902) + translated = translate(backend, error) + assert isinstance(translated, LockError) + assert not isinstance(translated, DeadlockError) + assert translated.driver_code == -902 + assert translated.transient is True + + +def test_deadlock_code_maps_to_deadlock_error(backend): + error = FirebirdDriverError("deadlock detected", sqlcode=-913) + translated = translate(backend, error) + assert isinstance(translated, DeadlockError) + assert translated.driver_code == -913 + assert translated.transient is True + + +def test_message_fallback_lock_conflict(backend): + error = FirebirdDriverError("lock conflict on no wait transaction") + translated = translate(backend, error) + assert isinstance(translated, LockError) + assert translated.transient is True + + +def test_connection_failure_sqlstate_is_transient(backend): + error = FirebirdDriverError("connection failure", sqlstate="08006", sqlcode=-904) + translated = translate(backend, error) + assert isinstance(translated, OperationalError) + assert isinstance(translated, InfrastructureError) + assert translated.transient is True + assert translated.retryable is True + + +def test_connection_failed_sqlstate_is_transient(backend): + error = FirebirdDriverError("unable to establish connection", sqlstate="08001") + translated = translate(backend, error) + assert isinstance(translated, ConnectionError) + assert translated.sqlstate == "08001" + assert translated.transient is True + assert translated.retryable is True + + +def test_connection_code_is_transient(backend): + error = FirebirdDriverError("connection lost", sqlcode=-904) + translated = translate(backend, error) + assert isinstance(translated, ConnectionError) + assert translated.driver_code == -904 + assert translated.transient is True + + +def test_authentication_sqlstate_is_not_transient(backend): + error = FirebirdDriverError( + "Your user name and password are not defined", sqlstate="28000", sqlcode=-804 + ) + translated = translate(backend, error) + assert isinstance(translated, ConnectionError) + assert translated.transient is False + assert translated.retryable is False + + +def test_authentication_message_fallback_is_not_transient(backend): + error = FirebirdDriverError("Your user name and password are not defined") + translated = translate(backend, error) + assert isinstance(translated, ConnectionError) + assert translated.transient is False + assert translated.retryable is False + + +def test_unknown_table_maps_to_query_error(backend): + error = FirebirdDriverError( + "Dynamic SQL Error: Table unknown - USERS", sqlstate="42S02", sqlcode=-204 + ) + translated = translate(backend, error) + assert isinstance(translated, QueryError) + assert translated.sqlstate == "42S02" + assert translated.driver_code == -204 + + +def test_unknown_column_maps_to_query_error(backend): + error = FirebirdDriverError("Column unknown - NAME", sqlstate="42S22", sqlcode=-206) + translated = translate(backend, error) + assert isinstance(translated, QueryError) + assert translated.driver_code == -206 + + +def test_syntax_error_maps_to_query_error(backend): + error = FirebirdDriverError("Token unknown - line 1, column 8", sqlstate="42000") + translated = translate(backend, error) + assert isinstance(translated, QueryError) + assert translated.sqlstate == "42000" + + +def test_syntax_message_fallback_maps_to_query_error(backend): + error = FirebirdDriverError("Token unknown - line 1, column 8") + translated = translate(backend, error) + assert isinstance(translated, QueryError) + + +def test_sqlstate_wins_over_code_and_message(backend): + """A stable SQLSTATE must outrank a misleading code or message.""" + error = FirebirdDriverError("deadlock lock conflict", sqlstate="23000", sqlcode=-901) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert not isinstance(translated, LockError) + + +def test_code_wins_over_message(backend): + error = FirebirdDriverError("lock conflict on no wait transaction", sqlcode=-803) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code == -803 + + +def test_plain_code_attribute_is_recognized(backend): + class FakeDriverError(Exception): + def __init__(self, message, code): + super().__init__(message) + self.sqlstate = None + self.code = code + + error = FakeDriverError("duplicate key value", code=-803) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code == -803 + + +def test_unrecognized_error_falls_back_to_database_error(backend): + error = FirebirdDriverError("some unexpected storage engine failure") + translated = translate(backend, error) + assert isinstance(translated, DatabaseError) + assert not isinstance( + translated, + (IntegrityError, QueryError, LockError, ConnectionError, OperationalError), + ) + assert translated.transient is False From f8fe568447b80e18f28ebac118808b872ae524dd Mon Sep 17 00:00:00 2001 From: Zhao vistart Date: Wed, 11 Nov 2026 13:12:34 +0800 Subject: [PATCH 2/5] fix(execute): align Firebird execute error phases --- .../impl/firebird/backend/async_backend.py | 75 +++-- .../backend/impl/firebird/backend/backend.py | 35 ++- .../backend/test_execute_error_contract.py | 275 ++++++++++++++++++ 3 files changed, 362 insertions(+), 23 deletions(-) create mode 100644 tests/rhosocial/activerecord_firebird_test/feature/backend/test_execute_error_contract.py diff --git a/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py b/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py index de677ad..14a1786 100644 --- a/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py +++ b/src/rhosocial/activerecord/backend/impl/firebird/backend/async_backend.py @@ -308,15 +308,28 @@ async def _handle_error(self, error: Exception) -> None: # A failed statement leaves the cursor with an attached firebird # Statement; closing it here avoids a GC-time abort in a worker thread # once the connection is gone (firebird-driver 2.0.x behaviour). - try: - if self._cursor is not None: - loop = asyncio.get_event_loop() - await loop.run_in_executor(self._executor, self._cursor.close) - except Exception: - pass - self._cursor = None + await self._close_cursor_quietly() FirebirdBackendMixin._handle_error(self, error) + async def _close_cursor_quietly(self) -> None: + """Close the shared cursor without letting a close failure mask a cause. + + The original exception is already propagating when this runs, so a + close failure is logged and discarded rather than raised. + """ + cursor = self._cursor + self._cursor = None + if cursor is None: + return + loop = asyncio.get_event_loop() + try: + await loop.run_in_executor(self._executor, cursor.close) + except Exception as error: + self.log( + logging.DEBUG, + f"Error closing cursor after failed execution, ignored: {error}", + ) + # ------------------------------------------------------------------ # Execution hooks — wrap sync fdb operations in run_in_executor # ------------------------------------------------------------------ @@ -392,12 +405,25 @@ def _char_columns_from_cursor(self, cursor) -> set: return FirebirdBackend._char_columns_from_cursor(cursor) async def _handle_auto_commit(self) -> None: - """Handle auto commit in thread pool.""" + """Commit the connection's active transaction in the executor. + + Mirrors the synchronous contract: a failed commit raises + :class:`TransactionError` from the driver exception, so the caller + never receives a result whose data was not committed. A connection + without an active transaction has nothing to commit. + """ + connection = self._connection + if connection is None: + return + main_transaction = getattr(connection, "main_transaction", None) + is_active = getattr(main_transaction, "is_active", None) + if callable(is_active) and not is_active(): + return loop = asyncio.get_event_loop() try: - await loop.run_in_executor(self._executor, self._connection.commit) - except Exception: - pass + await loop.run_in_executor(self._executor, connection.commit) + except Exception as error: + raise exc.TransactionError("Auto-commit failed") from error async def executescript(self, sql_script: str) -> None: """Execute a multi-statement SQL script asynchronously. @@ -420,19 +446,27 @@ async def executescript(self, sql_script: str) -> None: self.log(logging.INFO, f"Async SQL script executed successfully, duration={duration:.3f}s") await self._handle_auto_commit_if_needed() try: - cursor.close() - except Exception: - pass + await loop.run_in_executor(self._executor, cursor.close) + except Exception as error: + # Closing after a successful script is cleanup: a close failure + # must not turn success into an error. + self.log(logging.DEBUG, f"Error closing script cursor, ignored: {error}") except Exception as e: if cursor is not None: try: - cursor.close() - except Exception: - pass + await loop.run_in_executor(self._executor, cursor.close) + except Exception as error: + # The script failure below is the cause the caller must see. + self.log(logging.DEBUG, f"Error closing failed script cursor, ignored: {error}") + if isinstance(e, exc.DatabaseError): + raise # The exception is re-raised by _handle_error; a second ERROR # record would duplicate the application's own traceback. self.log(logging.DEBUG, f"Error executing async SQL script, will be re-raised: {e}") await self._handle_error(e) + # Defensive contract: a handler that returns must not swallow the + # script failure. + raise exc.DatabaseError(str(e)) from e # ------------------------------------------------------------------ # SQL helpers @@ -493,9 +527,16 @@ async def execute( try: return await super().execute(sql, params, options=options) except Exception as e: + # Mirror the synchronous backend: the errored cursor is closed + # before the failure propagates, and its close failure must not + # replace the original exception. + await self._close_cursor_quietly() if isinstance(e, exc.DatabaseError): raise await self._handle_error(e) + # Defensive contract: a handler that returns must not turn a + # failed execute into a None result. + raise exc.DatabaseError(str(e)) from e # ------------------------------------------------------------------ # High-level operations diff --git a/src/rhosocial/activerecord/backend/impl/firebird/backend/backend.py b/src/rhosocial/activerecord/backend/impl/firebird/backend/backend.py index dd7af89..dc7593e 100644 --- a/src/rhosocial/activerecord/backend/impl/firebird/backend/backend.py +++ b/src/rhosocial/activerecord/backend/impl/firebird/backend/backend.py @@ -207,12 +207,32 @@ def execute( if isinstance(e, exc.DatabaseError): raise self._handle_error(e) + # _handle_error() is required to raise. Keep a defensive fallback so + # a handler that returns cannot turn a failed execute into success. + raise exc.DatabaseError(str(e)) from e def _handle_auto_commit(self) -> None: + """Commit the connection's active transaction. + + The result is built just before this call, so a failed commit must not + leave the caller with a "successful" result: the failure surfaces as + :class:`TransactionError` carrying the driver exception. + + ``Connection.commit()`` goes through firebird-driver's main transaction + manager, which has nothing to commit before the first statement starts + a transaction. That state is not a commit failure, so it is skipped. + """ + connection = self._connection + if connection is None: + return + main_transaction = getattr(connection, "main_transaction", None) + is_active = getattr(main_transaction, "is_active", None) + if callable(is_active) and not is_active(): + return try: - self._connection.commit() - except Exception: - pass + connection.commit() + except Exception as error: + raise exc.TransactionError("Auto-commit failed") from error def execute_many( self, @@ -225,20 +245,22 @@ def execute_many( The caller's ``options`` - including its log context - is forwarded so the core batch logging stays the single owner of SQL/parameter records. + The core pipeline owns auto-commit for the batch; committing here again + would commit the same transaction twice. """ - is_dml = sql.strip().upper().startswith(('INSERT', 'UPDATE', 'DELETE', 'MERGE')) try: result = super().execute_many(sql, params_list, options=options) if result and result.affected_rows < 0 and len(params_list) > 0: result.affected_rows = len(params_list) - if is_dml: - self._handle_auto_commit_if_needed() return result except Exception as e: # Re-raise already-wrapped rhosocial errors unchanged; see execute(). if isinstance(e, exc.DatabaseError): raise self._handle_error(e) + # Defensive contract: a handler that returns must not swallow the + # batch failure. + raise exc.DatabaseError(str(e)) from e def executescript(self, sql: str) -> QueryResult: """Execute a multi-statement SQL script. @@ -262,6 +284,7 @@ def executescript(self, sql: str) -> QueryResult: raise self.log(logging.DEBUG, f"Error executing SQL script, exception will be re-raised: {e}") self._handle_error(e) + raise exc.DatabaseError(str(e)) from e def get_server_version(self) -> Tuple[int, int, int]: if not self._is_connected: diff --git a/tests/rhosocial/activerecord_firebird_test/feature/backend/test_execute_error_contract.py b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_execute_error_contract.py new file mode 100644 index 0000000..27cbe76 --- /dev/null +++ b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_execute_error_contract.py @@ -0,0 +1,275 @@ +# tests/rhosocial/activerecord_firebird_test/feature/backend/test_execute_error_contract.py +"""Firebird execution phases follow the shared execute error contract. + +Every case installs a fake connection/cursor on a backend that never +connects, so the module runs offline. It covers the phases the backend owns: +auto-commit, driver-error mapping inside ``execute_many``, framework errors +passing through untouched, and cleanup that cannot replace the original +failure. + +The sync and async cases are one class each with identical method names; +``pytest.ini``-style asyncio auto mode picks up the coroutines directly. +""" + +from typing import Optional + +import pytest + +pytest.importorskip("firebird.driver") + +from firebird.driver import DatabaseError as FirebirdDriverError # noqa: E402 + +from rhosocial.activerecord.backend.errors import ( # noqa: E402 + ConnectionError, + DatabaseError, + DeadlockError, + QueryError, + TransactionError, +) +from rhosocial.activerecord.backend.impl.firebird.backend.async_backend import ( # noqa: E402 + AsyncFirebirdBackend, +) +from rhosocial.activerecord.backend.impl.firebird.backend.backend import ( # noqa: E402 + FirebirdBackend, +) +from rhosocial.activerecord.backend.options import ( # noqa: E402 + ExecutionOptions, + StatementType, +) + + +class _FakeCursor: + """Cursor double that raises at the requested phase and records cleanup.""" + + def __init__( + self, + *, + execute_error: Optional[Exception] = None, + executemany_error: Optional[Exception] = None, + close_error: Optional[Exception] = None, + ): + self.rowcount = 1 + self.description = None + self.close_calls = 0 + self._execute_error = execute_error + self._executemany_error = executemany_error + self._close_error = close_error + + def execute(self, sql, params=None): + if self._execute_error is not None: + raise self._execute_error + return self + + def executemany(self, sql, params_list): + if self._executemany_error is not None: + raise self._executemany_error + return self + + def fetchall(self): + return [] + + def close(self): + self.close_calls += 1 + if self._close_error is not None: + raise self._close_error + + +class _FakeConnection: + """Connection double that records and optionally fails ``commit``.""" + + def __init__(self, *, commit_error: Optional[Exception] = None): + self.commit_calls = 0 + self._commit_error = commit_error + + def cursor(self): + return _FakeCursor() + + def commit(self): + self.commit_calls += 1 + if self._commit_error is not None: + raise self._commit_error + + +class _RefusingBackend(FirebirdBackend): + """Sync backend whose cursor lookup raises a framework error.""" + + def __init__(self, error: Exception): + super().__init__(database="contract.fdb") + self._raised_error = error + self._connection = _FakeConnection() + self._cursor = _FakeCursor() + self._is_connected = True + + def _get_cursor(self): + raise self._raised_error + + +class _AsyncRefusingBackend(AsyncFirebirdBackend): + """Async backend whose cursor lookup raises a framework error.""" + + def __init__(self, error: Exception): + super().__init__(database="contract.fdb") + self._raised_error = error + self._connection = _FakeConnection() + self._cursor = _FakeCursor() + self._is_connected = True + + async def _get_cursor(self): + raise self._raised_error + + +def _options() -> ExecutionOptions: + return ExecutionOptions(stmt_type=StatementType.DML) + + +def _sync_backend(connection, cursor) -> FirebirdBackend: + backend = FirebirdBackend(database="contract.fdb") + backend._connection = connection + backend._cursor = cursor + backend._is_connected = True + return backend + + +def _async_backend(connection, cursor) -> AsyncFirebirdBackend: + backend = AsyncFirebirdBackend(database="contract.fdb") + backend._connection = connection + backend._cursor = cursor + backend._is_connected = True + return backend + + +class TestExecuteErrorPhases: + """Execution phases: sync Firebird backend.""" + + def test_commit_failure_raises_transaction_error_and_withholds_result(self): + commit_error = FirebirdDriverError( + "connection lost during commit", sqlstate="08006", sqlcode=-904 + ) + connection = _FakeConnection(commit_error=commit_error) + backend = _sync_backend(connection, _FakeCursor()) + + with pytest.raises(TransactionError) as info: + backend.execute("UPDATE users SET name = 'x'", options=_options()) + + assert info.value.__cause__ is commit_error + assert connection.commit_calls == 1 + + def test_execute_many_update_conflict_maps_to_deadlock(self): + conflict = FirebirdDriverError( + "update conflicts with concurrent update", sqlstate="40001", sqlcode=-901 + ) + connection = _FakeConnection() + backend = _sync_backend(connection, _FakeCursor(executemany_error=conflict)) + + with pytest.raises(DeadlockError) as info: + backend.execute_many("UPDATE users SET name = ?", [("a",), ("b",)]) + + assert info.value.__cause__ is conflict + assert info.value.transient is True + # The batch failed before the commit phase, so nothing was committed. + assert connection.commit_calls == 0 + + def test_execute_many_commit_is_attempted_and_failure_surfaces(self): + commit_error = FirebirdDriverError("lock conflict on commit", sqlstate="40001") + connection = _FakeConnection(commit_error=commit_error) + backend = _sync_backend(connection, _FakeCursor()) + + with pytest.raises(TransactionError) as info: + backend.execute_many("UPDATE users SET name = ?", [("a",), ("b",)]) + + assert connection.commit_calls == 1 + assert info.value.__cause__ is commit_error + + def test_framework_connection_error_from_get_cursor_propagates_unchanged(self): + error = ConnectionError("Not connected to Firebird database") + backend = _RefusingBackend(error) + + with pytest.raises(ConnectionError) as info: + backend.execute("SELECT 1 FROM rdb$database", options=_options()) + + assert info.value is error + + def test_cursor_close_failure_does_not_replace_original_error(self): + driver_error = FirebirdDriverError("Token unknown", sqlstate="42000") + cursor = _FakeCursor( + execute_error=driver_error, + close_error=RuntimeError("close failed"), + ) + backend = _sync_backend(_FakeConnection(), cursor) + + with pytest.raises(QueryError) as info: + backend.execute("SELECT FROM", options=_options()) + + assert info.value.__cause__ is driver_error + # The close was attempted, and its failure was discarded instead of + # replacing the QueryError that carries the driver cause. + assert cursor.close_calls == 1 + + def test_executescript_commit_failure_raises_transaction_error(self): + commit_error = FirebirdDriverError("connection lost", sqlstate="08006", sqlcode=-904) + connection = _FakeConnection(commit_error=commit_error) + backend = _sync_backend(connection, _FakeCursor()) + + with pytest.raises(TransactionError) as info: + backend.executescript("UPDATE users SET name = 'x'") + + assert info.value.__cause__ is commit_error + assert connection.commit_calls == 1 + + def test_driver_error_is_mapped_not_returned_none(self): + driver_error = FirebirdDriverError("Token unknown", sqlstate="42000") + backend = _sync_backend(_FakeConnection(), _FakeCursor(execute_error=driver_error)) + + with pytest.raises(DatabaseError) as info: + backend.execute("SELECT FROM", options=_options()) + + assert isinstance(info.value, QueryError) + assert info.value.__cause__ is driver_error + + +class TestAsyncExecuteErrorPhases: + """Execution phases: async Firebird backend (same method names as sync).""" + + async def test_commit_failure_raises_transaction_error_and_withholds_result(self): + commit_error = FirebirdDriverError("connection lost", sqlstate="08006", sqlcode=-904) + connection = _FakeConnection(commit_error=commit_error) + backend = _async_backend(connection, _FakeCursor()) + + with pytest.raises(TransactionError) as info: + await backend.execute("UPDATE users SET name = 'x'", options=_options()) + + assert info.value.__cause__ is commit_error + assert connection.commit_calls == 1 + + async def test_driver_error_is_mapped_not_returned_none(self): + driver_error = FirebirdDriverError("Token unknown", sqlstate="42000") + backend = _async_backend(_FakeConnection(), _FakeCursor(execute_error=driver_error)) + + with pytest.raises(DatabaseError) as info: + await backend.execute("SELECT FROM", options=_options()) + + assert isinstance(info.value, QueryError) + assert info.value.__cause__ is driver_error + + async def test_framework_connection_error_from_get_cursor_propagates_unchanged(self): + error = ConnectionError("Not connected to Firebird database") + backend = _AsyncRefusingBackend(error) + + with pytest.raises(ConnectionError) as info: + await backend.execute("SELECT 1 FROM rdb$database", options=_options()) + + assert info.value is error + + async def test_cursor_close_failure_does_not_replace_original_error(self): + driver_error = FirebirdDriverError("Token unknown", sqlstate="42000") + cursor = _FakeCursor( + execute_error=driver_error, + close_error=RuntimeError("close failed"), + ) + backend = _async_backend(_FakeConnection(), cursor) + + with pytest.raises(QueryError) as info: + await backend.execute("SELECT FROM", options=_options()) + + assert info.value.__cause__ is driver_error + assert cursor.close_calls == 1 From 565b4c92f7653f67a8aea91103faa08a30f6d6e8 Mon Sep 17 00:00:00 2001 From: Zhao vistart Date: Wed, 11 Nov 2026 14:28:08 +0800 Subject: [PATCH 3/5] ci: pin core and testsuite feature/error-taxonomy branches --- .github/workflows/test.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 944fc24..03e8f41 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -68,14 +68,14 @@ jobs: - name: Install activerecord and testsuite run: | - pip install "rhosocial-activerecord @ git+https://github.com/rhosocial/python-activerecord.git@release/v1.0.0.dev30" # NOSONAR + pip install "rhosocial-activerecord @ git+https://github.com/rhosocial/python-activerecord.git@feature/error-taxonomy" # NOSONAR # NOSONAR(githubactions:S8541, S8544): both sibling projects are installed # from a git reference on purpose. The release branches are the contract # between these repositories, the test suite is still on a development # branch until it merges, and neither repository publishes a lock file to # pin against -- a hash-pinned install of a moving branch would be a pin # that goes stale on the next commit instead of a safety property. - pip install "rhosocial-activerecord-testsuite @ git+https://github.com/rhosocial/python-activerecord-testsuite.git@release/v1.0.0.dev17" # NOSONAR + pip install "rhosocial-activerecord-testsuite @ git+https://github.com/rhosocial/python-activerecord-testsuite.git@feature/error-taxonomy" # NOSONAR - name: Install dependencies run: | From a4e8749248dd0e23f52671c13cb77618e6d94474 Mon Sep 17 00:00:00 2001 From: Zhao vistart Date: Wed, 11 Nov 2026 15:47:18 +0800 Subject: [PATCH 4/5] docs(changelog): add fragment for the error taxonomy PR (#14) --- changelog.d/14.added.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/14.added.md diff --git a/changelog.d/14.added.md b/changelog.d/14.added.md new file mode 100644 index 0000000..233a934 --- /dev/null +++ b/changelog.d/14.added.md @@ -0,0 +1 @@ +Classified Firebird driver errors into the semantic error taxonomy (data, record, policy, concurrency, infrastructure) and aligned `execute` / `execute_many` with the execute-phase error contract: framework errors pass through, auto-commit failures raise `TransactionError`, and only transient connection failures are retried. Added contract tests covering the mapping and phases. From 1f5576dccee84142266c115a5421ab95385333bb Mon Sep 17 00:00:00 2001 From: Zhao vistart Date: Wed, 11 Nov 2026 16:24:55 +0800 Subject: [PATCH 5/5] test(firebird): cover taxonomy fallbacks and drop the unused connection helper --- .../impl/firebird/mixins/backend_mixin.py | 13 ---- .../backend/test_driver_error_mapping.py | 78 +++++++++++++++++++ 2 files changed, 78 insertions(+), 13 deletions(-) diff --git a/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py b/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py index 3010998..1fb05aa 100644 --- a/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py +++ b/src/rhosocial/activerecord/backend/impl/firebird/mixins/backend_mixin.py @@ -231,19 +231,6 @@ def _report_driver_code(cls, error: Exception, matched_code): return raw return raw_codes[0] if raw_codes else None - def _is_connection_error(self, error: Exception) -> bool: - """Return ``True`` when the error describes a connection failure.""" - sqlstate = self._error_sqlstate(error) - if sqlstate and sqlstate.startswith( - self.SQLSTATE_CONNECTION_LOST + self.SQLSTATE_CONNECTION_FAILED - ): - return True - connection_codes = {abs(code) for code in self.CONNECTION_ERROR_CODES} - if any(code in connection_codes for code in self._driver_error_codes(error)): - return True - message = str(error).lower() - return any(keyword in message for keyword in self.CONNECTION_KEYWORDS) - def _classify_error(self, error: Exception): """Classify a driver error as ``(error_class, transient, code)``. diff --git a/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py index 07e2748..89ac223 100644 --- a/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py +++ b/tests/rhosocial/activerecord_firebird_test/feature/backend/test_driver_error_mapping.py @@ -256,3 +256,81 @@ def test_unrecognized_error_falls_back_to_database_error(backend): (IntegrityError, QueryError, LockError, ConnectionError, OperationalError), ) assert translated.transient is False + + +class FakeDriverError(Exception): + """Minimal driver error exposing only the attributes the mapper inspects.""" + + def __init__(self, message, *, sqlstate=None, sqlcode=None, code=None, gds_codes=None): + super().__init__(message) + self.sqlstate = sqlstate + if sqlcode is not None: + self.sqlcode = sqlcode + if code is not None: + self.code = code + if gds_codes is not None: + self.gds_codes = gds_codes + + +def test_empty_sqlstate_list_falls_back_to_code(backend): + """An empty SQLSTATE list carries no information; codes still classify.""" + error = FakeDriverError("unique key violation", sqlstate=[], sqlcode=-803) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.sqlstate is None + assert translated.driver_code == -803 + + +def test_sqlstate_list_uses_first_entry(backend): + """Legacy driver builds report SQLSTATE as a sequence.""" + error = FakeDriverError("duplicate key value", sqlstate=["23505"], sqlcode=-803) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.sqlstate == "23505" + + +def test_scalar_gds_code_is_classified(backend): + """A single GDS code is accepted alongside the usual sequence form.""" + error = FakeDriverError("constraint failure", gds_codes=530) + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code == 530 + + +def test_non_numeric_code_is_ignored(backend): + """Codes that are not integers are skipped instead of misfiring.""" + error = FakeDriverError("foreign key constraint violation", code="not-a-number") + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.driver_code is None + + +def test_connection_message_fallback_is_transient(backend): + error = FakeDriverError("network error while talking to the server") + translated = translate(backend, error) + assert isinstance(translated, ConnectionError) + assert translated.transient is True + assert translated.retryable is True + + +def test_deadlock_message_fallback_maps_to_deadlock(backend): + error = FakeDriverError("deadlock detected, transaction aborted") + translated = translate(backend, error) + assert isinstance(translated, DeadlockError) + assert translated.transient is True + + +def test_integrity_message_fallback_maps_to_integrity(backend): + error = FakeDriverError("foreign key constraint violation on insert") + translated = translate(backend, error) + assert isinstance(translated, IntegrityError) + assert translated.transient is False + + +def test_framework_error_is_re_raised_unchanged(backend): + """An already translated framework error keeps its type and identity.""" + error = DatabaseError("already translated") + with pytest.raises(DatabaseError) as info: + backend._handle_error(error) + assert info.value is error +