Repository navigation
fix(sqlalchemy-spanner): resolve compliance and migration failures with Alembic 1.20 and SQLAlchemy 2.1 - #18570
chalmerlowe wants to merge 7 commits into
Conversation
…th Alembic 1.20 and SQLAlchemy 2.1
…_run_migration_test helper
There was a problem hiding this comment.
Code Review
This pull request makes the Spanner dialect for SQLAlchemy more robust by guarding Alembic imports, allowing the dialect to be imported cleanly even if Alembic is not installed or if there is a version mismatch. It wraps Alembic-specific DDL compilation overrides under a conditional check, updates the Nox test suite to run migration tests across both SQLAlchemy 1.4 and 2.0 environments, guards deprecated test imports for SQLAlchemy 2.1+ compatibility, and adds a unit test to verify import behavior when Alembic is missing. There are no review comments, so no additional feedback is provided.
…test failures in compliance suite
…LAlchemy 2.0 compliance suite
…mbic fallback branch
| if server_default is None | ||
| else f" DEFAULT {format_server_default(compiler, server_default)}" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Do you think we need to add any version guards to setup.py? Right now, there are no upper-bound limits, and alembic doesn't have any version pin at all. It seems like that could lead to more versioning issues in the future, if a major update comes out and breaks things overnight
There was a problem hiding this comment.
RESOLVED
We added version bounds.
|
|
||
| HAS_ALEMBIC_INSTALLED = True | ||
| except ImportError: | ||
| HAS_ALEMBIC_INSTALLED = False |
There was a problem hiding this comment.
This feels misleading, since alembic is a required dependency, so we should be able to trust that it was installed. It seems like the ImportError is actually thrown here when the environment has a version compatibility mismatch?
I'm a little confused about how that would happen in practice, and how we should best guard against it. Because we should be able to trust the package managers to sort this out for us
In the compliance_test_14 Nox session, .[tracing] was installed first (pulling in SQLAlchemy 2.1 and Alembic 1.20) before force-reinstalling sqlalchemy>=1.4,<2.0. This left alembic 1.20.0 paired with sqlalchemy 1.4.54, causing an ImportError (cannot import name '_NoneName' from 'sqlalchemy.sql.base') when importing the Spanner dialect.
Is this really a problem with the package, or do we just have a broken test environment? Maybe we just need to solve the contradictions in our nox installations?
Or, should we change alembic to an optional dependency, and keep this fall-back code?
There was a problem hiding this comment.
You are correct that package managers prevent this mismatch in normal customer installs and our nox script had some long standing inefficiencies that broke due to updates in upstream dependencies. However, I would like to keep the guard as defensive decoupling:
- so the dialect doesn't hard-crash if Alembic is ever omitted (e.g. via
--no-deps) - AND more importantly, in anticipation of making Alembic an optional extra in the future, much as it is in
sqlalchemy-bigquery.
See: sqlalchemy-spanner Make alembic an optional extra dependency
In the short term we will add version upper bounds (sqlalchemy<3.0.0, alembic<2.0.0) in setup.py.
Note
For context, it appears that alembic was originally installed as a hard dependency because someone put alembic into dependencies purely because sqlalchemy_spanner.py had unconditional @compiles decorators that imported from alembic.ddl.base at the top level. That code forced the packaging requirement, rather than the packaging requirement dictating the code.
…defensive Alembic decoupling
Fixes #18580
Problem
Recent upstream releases of Alembic (
1.20.0) and SQLAlchemy (2.1.3) caused failures in thesqlalchemy-spannercompliance and migration test suites:alembic 1.20.0requiresSQLAlchemy>=2.0. In thecompliance_test_14Nox session,.[tracing]was installed first (pulling in SQLAlchemy 2.1 and Alembic 1.20) before force-reinstallingsqlalchemy>=1.4,<2.0. This leftalembic 1.20.0paired withsqlalchemy 1.4.54, causing anImportError(cannot import name '_NoneName' from 'sqlalchemy.sql.base') when importing the Spanner dialect. Additionally,migration_testpreviously installedsqlalchemy>=1.4,<2.0before calling_migration_test, which subsequently ransession.install("pytest", "alembic")and upgraded SQLAlchemy back to 2.x.sqlalchemy.testing.suite.test_deprecations:tests/test_suite_20.pyimportedsqlalchemy.testing.suite.test_deprecationsunconditionally at the top of the file, raising aModuleNotFoundErrorwhen runningcompliance_test_20against SQLAlchemy 2.1+.types.FloatMRO change: Upstream decoupledtypes.Floatfromtypes.Numeric(it now inherits fromNumericCommon), causingComponentReflectionTest.test_get_columnsintersection checks to fail.HasTableTest.test_has_multi_table_schemaandJSONTest.test_index_cross_casts. Cloud Spanner GoogleSQL does not support user-defined schema namespaces (TABLE_SCHEMA = '') or non-string return types fromJSON_VALUE, requiring skips matching existing tests in those classes.Solution
sqlalchemy_spanner.py: Wrap thefrom alembic.ddl.base import ...block in atry ... except ImportError:statement and gate the@compileshooks behindHAS_ALEMBIC_INSTALLED. This ensures customers who only use SQLAlchemy Core or ORM (without running Alembic database migrations) can still import the Spanner dialect even if Alembic is missing or mismatched in their environment.alembic<1.20.0for SQLAlchemy 1.4 test sessions and clean upmigration_testinnoxfile.py:alembic<1.20.0toSQLALCHEMY_14_DEPENDENCIESand install.[tracing]and*SQLALCHEMY_14_DEPENDENCIESin a singlesession.installstep incompliance_test_14.migration_testwith paired("python", "extra_dependencies")values for SQLAlchemy 1.4 and 2.0+, and extract the shared migration workflow into a plain_run_migration_testhelper called by bothmigration_testandsystem.tests/test_suite_20.pyfor SQLAlchemy 2.1:from sqlalchemy.testing.suite.test_deprecations import *in atry ... except ModuleNotFoundError:block.types.FloatinComponentReflectionTest.test_get_columnsgeneric type intersection check.HasTableTest.test_has_multi_table_schemaandJSONTest.test_index_cross_castsas skipped on Spanner with explanatory reasons matching sibling tests.tests/unit/test_alembic.py: Verify thatsqlalchemy_spannerimports cleanly and setsHAS_ALEMBIC_INSTALLED = Falsewhenalembic.ddl.baseis unavailable.Notes for Reviewers
tests/unit/test_alembic.py,test_dialect_import_without_alembicexecutes the module spec into an isolated module object (importlib.util.module_from_spec) insidemock.patch.dict(sys.modules, {"alembic.ddl.base": None})rather than callingimportlib.reload(sqlalchemy_spanner). Reloading the module in-place would recreateSpannerIdentifierPreparerwith a new class identity insqlalchemy_spanner.__dict__, breaking other tests that importedSpannerDialectprior to the reload.