fix: incorrect failover error rewrapping

aws_advanced_python_wrapper/errors.py

@@ -51,11 +51,13 @@ class FailoverFailedError(FailoverError):
 
 
 class FailoverSuccessError(FailoverError):
-    # SA classification is handled at the dialect boundary by
-    # ``sqlalchemy_dialects._exception_handling._FailoverSuccessRewrapMixin``,
-    # which catches FailoverSuccessError in ``do_execute`` /
-    # ``do_executemany`` and re-raises as the dialect's native
-    # OperationalError. Do NOT add driver-native OperationalError classes
+    # SA classification needs no help at the dialect boundary: ``FailoverError``
+    # derives from the wrapper's own ``pep249.OperationalError``, and SA's
+    # ``DBAPIError.instance`` matches ``orig.__class__.__mro__`` by class *name*,
+    # so this already maps to ``sqlalchemy.exc.OperationalError`` with
+    # ``DBAPIError.orig`` left as this exception.
+    #
+    # Do NOT add driver-native OperationalError classes
     # (psycopg / mysql.connector / aiomysql) as bases here: Django's
     # ``wrap_database_errors`` walks ``issubclass`` against the driver's
     # own error module and would swallow FailoverSuccessError before any

aws_advanced_python_wrapper/pep249.py

@@ -47,6 +47,8 @@ class Warning(Exception):
 
 class Error(Exception):
     __module__ = "aws_advanced_python_wrapper"
+    errno = None
+    sqlstate = None
 
 
 class InterfaceError(Error):

aws_advanced_python_wrapper/sqlalchemy/__init__.py

@@ -0,0 +1,28 @@
+#  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+#  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
+#
+#  http://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.
+
+"""Deprecated location of the SQLAlchemy dialects.
+
+The dialects moved to :mod:`aws_advanced_python_wrapper.sqlalchemy_dialects` in
+3.1.0. This package exists only so that ``import`` statements written against
+3.0.0 keep working; it will be removed in the next major version.
+
+Only the MySQL name is aliased. 3.0.0's ``entry_points.txt`` also declared
+``postgresql.aws_wrapper_psycopg =
+aws_advanced_python_wrapper.sqlalchemy.pg_orm_dialect:SqlAlchemyOrmPgDialect``,
+but that module was never present in the 3.0.0 distribution — the entry point
+was dangling and ``create_engine("postgresql+aws_wrapper_psycopg://")`` raised
+``ModuleNotFoundError`` (aws/aws-advanced-python-wrapper#1260, #1273). There is
+therefore no working 3.0.0 PostgreSQL name to preserve.
+"""

aws_advanced_python_wrapper/sqlalchemy/mysql_orm_dialect.py

@@ -0,0 +1,54 @@
+#  Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+#
+#  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
+#
+#  http://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.
+
+"""Deprecated alias for the pre-3.1.0 MySQL SQLAlchemy dialect module path.
+
+``SqlAlchemyOrmMysqlDialect`` was renamed to
+:class:`~aws_advanced_python_wrapper.sqlalchemy_dialects.mysql.AwsWrapperMySQLConnectorDialect`
+and moved to :mod:`aws_advanced_python_wrapper.sqlalchemy_dialects` in 3.1.0.
+
+URL-based configuration was never affected: the registered driver name
+``aws_wrapper_mysqlconnector`` is unchanged, so
+``mysql+aws_wrapper_mysqlconnector://`` resolves in both releases. This shim
+exists for the narrower case of code that imported the dialect class directly —
+to subclass it, or to register it by hand — which would otherwise break with
+``ModuleNotFoundError`` on upgrade.
+
+Note that starting v3.1.0, the plugin chain is no longer hardcoded to `aurora_connection_tracker,failover_v2`
+if the connection property `wrapper_plugins` is not set, and will use the default plugin chain instead.
+"""
+
+import warnings
+
+from aws_advanced_python_wrapper.sqlalchemy_dialects.mysql import \
+    AwsWrapperMySQLConnectorDialect
+
+warnings.warn(
+    "aws_advanced_python_wrapper.sqlalchemy.mysql_orm_dialect."
+    "SqlAlchemyOrmMysqlDialect is deprecated and will be removed in the next "
+    "major version. Use aws_advanced_python_wrapper.sqlalchemy_dialects.mysql."
+    "AwsWrapperMySQLConnectorDialect instead. URL-based configuration needs no "
+    "change: mysql+aws_wrapper_mysqlconnector:// is unchanged. Note that the "
+    "replacement no longer injects plugins=aurora_connection_tracker,"
+    "failover_v2; the wrapper's default chain applies instead, which for "
+    "mysql-connector is a superset of it.",
+    DeprecationWarning,
+    stacklevel=2,
+)
+
+#: Deprecated alias. Kept so 3.0.0 imports resolve; prefer
+#: ``AwsWrapperMySQLConnectorDialect``.
+SqlAlchemyOrmMysqlDialect = AwsWrapperMySQLConnectorDialect
+
+__all__ = ["SqlAlchemyOrmMysqlDialect"]

aws_advanced_python_wrapper/sqlalchemy_dialects/_exception_handling.py

@@ -14,40 +14,58 @@
 
 """Shared exception-handling helpers for the wrapper's SA dialects.
 
-SQLAlchemy classifies DBAPI exceptions in ``Connection._handle_dbapi_exception``
-by walking ``dialect.loaded_dbapi.<ErrorClass>`` and wrapping into
-``sqlalchemy.exc.<MappedClass>``. SA's classifier needs the raised exception
-to be an instance of the **driver-native** ``OperationalError`` class
-(e.g. ``psycopg.OperationalError``), not the wrapper's PEP-249
-``OperationalError``. Wrapper-internal exceptions like
-``FailoverSuccessError`` are single-inherit from ``FailoverError`` (the
-driver-native multi-inheritance was reverted in commit ``d994d02`` because
-it caused Django's ``wrap_database_errors`` to swallow the failover signal
-on MySQL), so SA's classifier lets them escape raw and any user-written
-``except sqlalchemy.exc.OperationalError:`` retry loop never fires.
-
-The mixin below sidesteps that by intercepting ``FailoverSuccessError`` at
-the ``do_execute`` / ``do_executemany`` boundary and re-raising it as the
-driver-native ``OperationalError`` class — which SA's classifier DOES
-reclassify reliably to ``sqlalchemy.exc.OperationalError``. The original
-wrapper exception is preserved via ``__cause__`` so callers that need the
-exact wrapper type can ``isinstance(exc.__cause__, FailoverSuccessError)``.
-
-Each concrete dialect declares its target class via
-``_failover_success_target_cls``; the mixin handles both sync and async
-``do_execute`` shapes.
-
-Scope: only ``do_execute`` and ``do_executemany`` are wrapped. If
-``FailoverSuccessError`` ever surfaces from ``do_commit`` / ``do_rollback``
-/ ``do_begin_twophase`` / etc., it will escape raw — extend the mixin then.
+How SQLAlchemy classifies the wrapper's errors
+----------------------------------------------
+``sqlalchemy.exc.DBAPIError.instance`` walks ``orig.__class__.__mro__`` and
+matches each base **by class name** against the names exported from
+``sqlalchemy.exc``, gated on ``isinstance(orig, dialect.loaded_dbapi.Error)``.
+Our ``loaded_dbapi`` is the wrapper itself, so that gate is the wrapper's
+PEP-249 ``Error`` — and because ``FailoverError`` derives from the wrapper's
+``pep249.OperationalError``, every failover error already carries a base *named*
+``OperationalError`` in its MRO. SA therefore maps the whole family to
+``sqlalchemy.exc.OperationalError`` on its own. The class does NOT need to be
+driver-native, and nothing at the dialect boundary needs to re-raise anything
+for ``except sqlalchemy.exc.OperationalError:`` to fire.
+
+The wrapper's own errors are therefore passed through ``do_execute`` UNCHANGED.
+Two things depend on that:
+
+* SA puts the original exception in ``DBAPIError.orig``, so a consumer can write
+  ``isinstance(err.orig, FailoverSuccessError)`` — the documented idiom — as
+  well as ``except sqlalchemy.exc.OperationalError:``.
+* Each dialect's ``is_disconnect`` override sees the real ``FailoverError`` and
+  decides pool invalidation from it: ``FailoverFailedError`` -> invalidate,
+  ``FailoverSuccessError`` / ``TransactionResolutionUnknownError`` -> keep the
+  pooled connection, which the wrapper has already rebound to the new writer.
+
+**Do not reintroduce a rewrap here.** An earlier version of this module caught
+``FailoverSuccessError`` and re-raised ``pep249.OperationalError``. It gained
+nothing — SA produced the same ``sqlalchemy.exc.OperationalError`` either way —
+and it cost two things:
+
+1. ``DBAPIError.orig`` became the substitute, silently breaking
+   ``isinstance(err.orig, FailoverSuccessError)``.
+2. The substitute is not a ``FailoverError``, so ``is_disconnect`` below no
+   longer recognised it and fell through to
+   ``MySQLDialect_mysqlconnector.is_disconnect``, which probes ``e.errno``. The
+   wrapper's PEP-249 errors carry no ``errno``, and SA calls ``is_disconnect``
+   at the top of ``Connection._handle_dbapi_exception`` with no enclosing
+   ``try:`` — so an ``AttributeError`` escaped and the consumer's
+   ``except DBAPIError:`` never ran at all. Confirmed on a real Aurora MySQL
+   failover: a successful failover surfaced to the application as
+   ``AttributeError: 'OperationalError' object has no attribute 'errno'``.
+
+What this module DOES do
+------------------------
+Normalize *raw driver-native* DBAPI errors (``mysql.connector.errors.*`` /
+``psycopg.*`` / ``pymysql.*``) into the wrapper's PEP-249 equivalents, for plugin
+chains that do not already re-wrap them (``iam`` / ``aws_secrets_manager`` / no
+plugins). Without that SA cannot classify them at all, so e.g. ``has_table``
+never sees MySQL's 1146 and ``create_all`` fails.
 """
 
 from __future__ import annotations
 
-from typing import ClassVar, Optional, Type
-
-from aws_advanced_python_wrapper.errors import FailoverSuccessError
-
 
 def _normalize_driver_error(e, driver_error_module):
     """Translate a raw driver-native DBAPI error into the wrapper's PEP-249
@@ -98,27 +116,15 @@ def _normalize_driver_error(e, driver_error_module):
     return wrapped
 
 
-class _FailoverSuccessRewrapMixin:
-    """Re-raise ``FailoverSuccessError`` as the driver-native OperationalError.
-
-    Concrete dialect subclasses set ``_failover_success_target_cls`` to the
-    driver's own ``OperationalError`` class (e.g. ``psycopg.OperationalError``,
-    ``mysql.connector.errors.OperationalError``, ``aiomysql.OperationalError``).
-    The mixin's ``do_execute`` wraps the parent's call: on
-    ``FailoverSuccessError``, it raises the target class with the same message,
-    chaining the original via ``__cause__``. SA's classifier reliably maps
-    driver-native ``OperationalError`` -> ``sqlalchemy.exc.OperationalError``,
-    so user retry loops (``except sqlalchemy.exc.OperationalError:``) fire.
+class _DriverErrorNormalizeMixin:
+    """Normalize raw driver-native DBAPI errors into the wrapper's PEP-249 types.
 
-    Other ``FailoverError`` subclasses (``FailoverFailedError``,
-    ``TransactionResolutionUnknownError``) are NOT rewrapped: failed failover
-    is a hard error the user should see, and transaction-resolution-unknown
-    has its own semantics distinct from a generic OperationalError.
+    The wrapper's own errors — including the whole ``FailoverError`` family — are
+    passed through UNTOUCHED, so ``DBAPIError.orig`` stays the exception the
+    wrapper raised and each dialect's ``is_disconnect`` can classify it. See the
+    module docstring for why re-raising a substitute class here is wrong.
     """
 
-    # Subclasses MUST set this to the driver-native OperationalError class.
-    _failover_success_target_cls: ClassVar[Optional[Type[BaseException]]] = None
-
     def _driver_error_module(self):
         """Driver-native DBAPI exception namespace (module exposing PEP-249
         error classes: ``Error``, ``OperationalError``, ``ProgrammingError``,
@@ -134,11 +140,6 @@ def do_execute(  # type: ignore[no-untyped-def]
         try:
             super().do_execute(  # type: ignore[misc]
                 cursor, statement, parameters, context)
-        except FailoverSuccessError as e:
-            target = self._failover_success_target_cls
-            if target is None:
-                raise  # mis-configured dialect; surface the raw error
-            raise target(str(e)) from e
         except Exception as e:
             normalized = _normalize_driver_error(e, self._driver_error_module())
             if normalized is not None:
@@ -150,20 +151,15 @@ def do_executemany(  # type: ignore[no-untyped-def]
         try:
             super().do_executemany(  # type: ignore[misc]
                 cursor, statement, parameters, context)
-        except FailoverSuccessError as e:
-            target = self._failover_success_target_cls
-            if target is None:
-                raise
-            raise target(str(e)) from e
         except Exception as e:
             normalized = _normalize_driver_error(e, self._driver_error_module())
             if normalized is not None:
                 raise normalized from e
             raise
 
 
-class _AsyncFailoverSuccessRewrapMixin:
-    """Async counterpart of :class:`_FailoverSuccessRewrapMixin`.
+class _AsyncDriverErrorNormalizeMixin:
+    """Async counterpart of :class:`_DriverErrorNormalizeMixin`.
 
     IMPORTANT: ``do_execute`` / ``do_executemany`` MUST be SYNCHRONOUS even for
     async dialects. SQLAlchemy's execution context calls
@@ -176,25 +172,18 @@ class _AsyncFailoverSuccessRewrapMixin:
     ``dialect.initialize``'s ``SELECT version()`` (the ``sqlalchemy_creator_*``
     integration tests). So this mixin is functionally identical to the sync
     one; it exists as a distinct class only so async dialects can be wired to a
-    different ``_failover_success_target_cls`` / ``_driver_error_module``.
+    different ``_driver_error_module``.
     """
 
-    _failover_success_target_cls: ClassVar[Optional[Type[BaseException]]] = None
-
     def _driver_error_module(self):
-        """See :meth:`_FailoverSuccessRewrapMixin._driver_error_module`."""
+        """See :meth:`_DriverErrorNormalizeMixin._driver_error_module`."""
         return None
 
     def do_execute(  # type: ignore[no-untyped-def]
             self, cursor, statement, parameters, context=None):
         try:
             super().do_execute(  # type: ignore[misc]
                 cursor, statement, parameters, context)
-        except FailoverSuccessError as e:
-            target = self._failover_success_target_cls
-            if target is None:
-                raise
-            raise target(str(e)) from e
         except Exception as e:
             normalized = _normalize_driver_error(e, self._driver_error_module())
             if normalized is not None:
@@ -206,16 +195,11 @@ def do_executemany(  # type: ignore[no-untyped-def]
         try:
             super().do_executemany(  # type: ignore[misc]
                 cursor, statement, parameters, context)
-        except FailoverSuccessError as e:
-            target = self._failover_success_target_cls
-            if target is None:
-                raise
-            raise target(str(e)) from e
         except Exception as e:
             normalized = _normalize_driver_error(e, self._driver_error_module())
             if normalized is not None:
                 raise normalized from e
             raise
 
 
-__all__ = ["_FailoverSuccessRewrapMixin", "_AsyncFailoverSuccessRewrapMixin"]
+__all__ = ["_DriverErrorNormalizeMixin", "_AsyncDriverErrorNormalizeMixin"]

aws_advanced_python_wrapper/sqlalchemy_dialects/mysql.py

@@ -26,26 +26,17 @@
 from sqlalchemy.dialects.mysql.mysqlconnector import \
     MySQLDialect_mysqlconnector
 
-from aws_advanced_python_wrapper.pep249 import \
-    OperationalError as _PEP249OperationalError
 from aws_advanced_python_wrapper.sqlalchemy_dialects._exception_handling import \
-    _FailoverSuccessRewrapMixin
+    _DriverErrorNormalizeMixin
 
 
 class AwsWrapperMySQLConnectorDialect(
-        _FailoverSuccessRewrapMixin, MySQLDialect_mysqlconnector):
+        _DriverErrorNormalizeMixin, MySQLDialect_mysqlconnector):
     """SQLAlchemy dialect that uses the AWS Advanced Python Wrapper as its DBAPI."""
 
     driver = "aws_wrapper_mysqlconnector"
     supports_statement_cache = True
 
-    # See _FailoverSuccessRewrapMixin / sqlalchemy_dialects/pg.py for the
-    # full rationale. The shim's ``dialect.dbapi.OperationalError`` resolves
-    # to the wrapper's PEP-249 ``OperationalError`` via ``_dbapi.install``,
-    # so the rewrap target must be that class for SA's classifier to wrap
-    # us to ``sqlalchemy.exc.OperationalError``.
-    _failover_success_target_cls = _PEP249OperationalError
-
     @classmethod
     def import_dbapi(cls):
         import aws_advanced_python_wrapper.mysql_connector as dbapi
@@ -113,9 +104,13 @@ def is_disconnect(self, e, connection, cursor):
         #      psycopg's upstream is_disconnect returns False for this.)
         #    - FailoverFailedError: the wrapper has no working connection;
         #      pool slot really is dead. Return True so SA invalidates.
-        # _FailoverSuccessRewrapMixin still handles the do_execute path;
-        # this method handles the cursor-creation path which runs before
-        # do_execute reaches the mixin.
+        # This is the ONLY handler for the whole FailoverError family, on both
+        # paths: cursor creation and do_execute. Nothing at the do_execute
+        # boundary substitutes the exception any more -- an earlier rewrap there
+        # made this method unreachable for FailoverSuccessError, which then fell
+        # through to upstream's e.errno probe and raised AttributeError out of
+        # SA's unguarded is_disconnect call. See
+        # sqlalchemy_dialects/_exception_handling.py.
         from aws_advanced_python_wrapper.errors import (FailoverError,
                                                         FailoverFailedError)