Skip to content

Commit 873ef99

Browse files
authored
fix(sqlalchemy-spanner): resolve compliance and migration failures with Alembic 1.20 and SQLAlchemy 2.1 (#18570)
1 parent 734cebe commit 873ef99

5 files changed

Lines changed: 163 additions & 88 deletions

File tree

‎packages/sqlalchemy-spanner/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py‎

Lines changed: 75 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,6 @@
1515
import re
1616

1717
import sqlalchemy
18-
from alembic.ddl.base import (
19-
ColumnNullable,
20-
ColumnType,
21-
alter_column,
22-
alter_table,
23-
format_server_default,
24-
format_type,
25-
)
2618
from google.api_core.client_options import ClientOptions
2719
from google.auth.credentials import AnonymousCredentials
2820
from google.cloud.spanner_v1 import Client, TransactionOptions
@@ -51,6 +43,26 @@
5143
from google.cloud.sqlalchemy_spanner import version as sqlalchemy_spanner_version
5244
from google.cloud.sqlalchemy_spanner._opentelemetry_tracing import trace_call
5345

46+
# Defensively decouple the Alembic import so the Spanner dialect does not
47+
# hard-depend on Alembic at runtime. A database dialect does not inherently
48+
# require a schema migration tool, and consumers using only SQLAlchemy Core
49+
# or ORM (or managing DDL outside of Alembic) can still import and use the
50+
# dialect even if Alembic is omitted or unavailable in the environment
51+
# (see #18584).
52+
try:
53+
from alembic.ddl.base import (
54+
ColumnNullable,
55+
ColumnType,
56+
alter_column,
57+
alter_table,
58+
format_server_default,
59+
format_type,
60+
)
61+
62+
HAS_ALEMBIC_INSTALLED = True
63+
except ImportError:
64+
HAS_ALEMBIC_INSTALLED = False
65+
5466
USING_SQLACLCHEMY_20 = False
5567
if sqlalchemy.__version__.split(".")[0] == "2":
5668
USING_SQLACLCHEMY_20 = True
@@ -1882,55 +1894,60 @@ def do_execute_no_params(self, cursor, statement, context=None):
18821894
cursor.execute(statement)
18831895

18841896

1885-
# Alembic ALTER operation override
1886-
@compiles(ColumnNullable, "spanner+spanner")
1887-
def visit_column_nullable(
1888-
element: "ColumnNullable", compiler: "SpannerDDLCompiler", **kw
1889-
) -> str:
1890-
return _format_alter_column(
1891-
compiler,
1892-
element.table_name,
1893-
element.schema,
1894-
element.column_name,
1895-
element.existing_type,
1896-
element.nullable,
1897-
element.existing_server_default,
1898-
)
1899-
1900-
1901-
# Alembic ALTER operation override
1902-
@compiles(ColumnType, "spanner+spanner")
1903-
def visit_column_type(
1904-
element: "ColumnType", compiler: "SpannerDDLCompiler", **kw
1905-
) -> str:
1906-
return _format_alter_column(
1907-
compiler,
1908-
element.table_name,
1909-
element.schema,
1910-
element.column_name,
1911-
element.type_,
1912-
element.existing_nullable,
1913-
element.existing_server_default,
1914-
)
1897+
# Cloud Spanner requires ALTER TABLE ... ALTER COLUMN statements to specify the
1898+
# complete column definition (type, nullability, and default expression), whereas
1899+
# Alembic's default DDL compiler emits partial clauses (e.g., only SET NOT NULL
1900+
# or TYPE). Because the @compiles decorators reference Alembic's ColumnNullable
1901+
# and ColumnType classes at module import time, we only register these overrides
1902+
# when Alembic is available in the environment.
1903+
if HAS_ALEMBIC_INSTALLED:
1904+
# Alembic ALTER operation override
1905+
@compiles(ColumnNullable, "spanner+spanner")
1906+
def visit_column_nullable(
1907+
element: "ColumnNullable", compiler: "SpannerDDLCompiler", **kw
1908+
) -> str:
1909+
return _format_alter_column(
1910+
compiler,
1911+
element.table_name,
1912+
element.schema,
1913+
element.column_name,
1914+
element.existing_type,
1915+
element.nullable,
1916+
element.existing_server_default,
1917+
)
19151918

1919+
# Alembic ALTER operation override
1920+
@compiles(ColumnType, "spanner+spanner")
1921+
def visit_column_type(
1922+
element: "ColumnType", compiler: "SpannerDDLCompiler", **kw
1923+
) -> str:
1924+
return _format_alter_column(
1925+
compiler,
1926+
element.table_name,
1927+
element.schema,
1928+
element.column_name,
1929+
element.type_,
1930+
element.existing_nullable,
1931+
element.existing_server_default,
1932+
)
19161933

1917-
def _format_alter_column(
1918-
compiler, table_name, schema, column_name, type_, nullable, server_default
1919-
):
1920-
# Older versions of SQLAlchemy pass in a boolean to indicate whether there
1921-
# is an existing DEFAULT constraint, instead of the actual DEFAULT constraint
1922-
# expression. In those cases, we do not want to explicitly include the DEFAULT
1923-
# constraint in the expression that is generated here.
1924-
if isinstance(server_default, bool):
1925-
server_default = None
1926-
return "%s %s %s%s%s" % (
1927-
alter_table(compiler, table_name, schema),
1928-
alter_column(compiler, column_name),
1929-
format_type(compiler, type_),
1930-
"" if nullable else " NOT NULL",
1931-
(
1932-
""
1933-
if server_default is None
1934-
else f" DEFAULT {format_server_default(compiler, server_default)}"
1935-
),
1936-
)
1934+
def _format_alter_column(
1935+
compiler, table_name, schema, column_name, type_, nullable, server_default
1936+
):
1937+
# Older versions of SQLAlchemy pass in a boolean to indicate whether there
1938+
# is an existing DEFAULT constraint, instead of the actual DEFAULT constraint
1939+
# expression. In those cases, we do not want to explicitly include the DEFAULT
1940+
# constraint in the expression that is generated here.
1941+
if isinstance(server_default, bool):
1942+
server_default = None
1943+
return "%s %s %s%s%s" % (
1944+
alter_table(compiler, table_name, schema),
1945+
alter_column(compiler, column_name),
1946+
format_type(compiler, type_),
1947+
"" if nullable else " NOT NULL",
1948+
(
1949+
""
1950+
if server_default is None
1951+
else f" DEFAULT {format_server_default(compiler, server_default)}"
1952+
),
1953+
)

‎packages/sqlalchemy-spanner/noxfile.py‎

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ class = StreamHandler
120120

121121
SQLALCHEMY_14_DEPENDENCIES = [
122122
"sqlalchemy>=1.4,<2.0",
123+
"alembic<1.20.0",
123124
]
124125

125126
SQLALCHEMY_20_DEPENDENCIES = [
@@ -139,7 +140,6 @@ class = StreamHandler
139140
"compliance_test_14",
140141
"compliance_test_20",
141142
"migration_test",
142-
"_migration_test",
143143
"mockserver",
144144
]
145145

@@ -206,13 +206,7 @@ def compliance_test_14(session):
206206

207207
try:
208208
session.install(*SYSTEM_TEST_STANDARD_DEPENDENCIES)
209-
session.install(".[tracing]")
210-
session.run(
211-
"pip",
212-
"install",
213-
*SQLALCHEMY_14_DEPENDENCIES,
214-
"--force-reinstall",
215-
)
209+
session.install(".[tracing]", *SQLALCHEMY_14_DEPENDENCIES)
216210
session.run(
217211
"python",
218212
"create_test_database.py",
@@ -335,29 +329,35 @@ def mockserver(session):
335329
)
336330

337331

338-
@nox.session(python=SYSTEM_COMPLIANCE_MIGRATION_TEST_PYTHON_VERSIONS[0])
339-
def migration_test(session):
340-
"""Test migrations with SQLAlchemy v1.4 and Alembic"""
341-
session.run(
342-
"pip",
343-
"install",
344-
*SQLALCHEMY_14_DEPENDENCIES,
345-
"--force-reinstall",
346-
)
347-
_migration_test(session)
332+
@nox.session
333+
@nox.parametrize(
334+
("python", "extra_dependencies"),
335+
[
336+
(
337+
SYSTEM_COMPLIANCE_MIGRATION_TEST_PYTHON_VERSIONS[0],
338+
SQLALCHEMY_14_DEPENDENCIES,
339+
),
340+
(
341+
DEFAULT_PYTHON_VERSION_FOR_SQLALCHEMY_20,
342+
SQLALCHEMY_20_DEPENDENCIES,
343+
),
344+
],
345+
ids=["14", "20"],
346+
)
347+
def migration_test(session, extra_dependencies):
348+
"""Test migrations with SQLAlchemy and Alembic."""
349+
_run_migration_test(session, extra_dependencies=extra_dependencies)
348350

349351

350-
@nox.session(python=SYSTEM_COMPLIANCE_MIGRATION_TEST_PYTHON_VERSIONS[-1])
351-
def _migration_test(session):
352+
def _run_migration_test(session, extra_dependencies=()):
352353
"""Migrate with SQLAlchemy and Alembic and check the result."""
353354
import glob
354355
import os
355356
import shutil
356357

357358
config_file = f"test_migration_{session.python}_{uuid.uuid4().hex[:6]}.cfg"
358359

359-
session.install(*MIGRATION_TEST_DEPENDENCIES)
360-
session.install(".")
360+
session.install(*MIGRATION_TEST_DEPENDENCIES, *extra_dependencies, ".")
361361

362362
try:
363363
session.run(
@@ -482,9 +482,13 @@ def system(session, test_type):
482482
elif test_type == "compliance_20":
483483
return compliance_test_20(session)
484484
elif test_type == "migration_14":
485-
return migration_test(session)
485+
return _run_migration_test(
486+
session, extra_dependencies=SQLALCHEMY_14_DEPENDENCIES
487+
)
486488
elif test_type == "migration_20":
487-
return _migration_test(session)
489+
return _run_migration_test(
490+
session, extra_dependencies=SQLALCHEMY_20_DEPENDENCIES
491+
)
488492

489493
config_file = f"test_{test_type}_{session.python}_{uuid.uuid4().hex[:6]}.cfg"
490494

‎packages/sqlalchemy-spanner/setup.py‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,9 @@
2222
name = "sqlalchemy-spanner"
2323
description = "SQLAlchemy dialect integrated into Cloud Spanner database"
2424
dependencies = [
25-
"sqlalchemy>=1.1.13",
25+
"sqlalchemy>=1.1.13,<3.0.0",
2626
"google-cloud-spanner>=3.55.0",
27-
"alembic",
27+
"alembic>=1.0.0,<2.0.0",
2828
]
2929
extras = {
3030
"tracing": [

‎packages/sqlalchemy-spanner/tests/test_suite_20.py‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,6 @@
7878
LongNameBlowoutTest as _LongNameBlowoutTest,
7979
)
8080
from sqlalchemy.testing.suite.test_ddl import TableDDLTest as _TableDDLTest
81-
from sqlalchemy.testing.suite.test_deprecations import * # noqa: F401, F403
8281
from sqlalchemy.testing.suite.test_dialect import * # noqa: F401, F403
8382
from sqlalchemy.testing.suite.test_dialect import (
8483
DifficultParametersTest as _DifficultParametersTest,
@@ -225,6 +224,15 @@
225224
get_project,
226225
)
227226

227+
try:
228+
# SQLAlchemy 2.1+ removed test_deprecations from sqlalchemy.testing.suite.
229+
# Guard this import so the test suite remains compatible with both 2.0.x and 2.1+.
230+
# Tell flake8 to ignore F401 (unused import) and F403 (wildcard import) since
231+
# pytest discovers the imported SQLAlchemy compliance test classes at module scope.
232+
from sqlalchemy.testing.suite.test_deprecations import * # noqa: F401, F403
233+
except ModuleNotFoundError:
234+
pass
235+
228236
config.test_schema = ""
229237

230238

@@ -721,6 +729,7 @@ def test_get_columns(self, connection, use_views, use_schema):
721729
[
722730
types.Integer,
723731
types.Numeric,
732+
types.Float,
724733
types.DateTime,
725734
types.Date,
726735
types.Time,
@@ -2916,6 +2925,10 @@ def test_has_table_nonexistent_schema(self):
29162925
def test_has_table_schema(self):
29172926
pass
29182927

2928+
@pytest.mark.skip("Not supported by Cloud Spanner")
2929+
def test_has_multi_table_schema(self):
2930+
pass
2931+
29192932
@pytest.mark.skip("Not supported by Cloud Spanner")
29202933
def test_has_table_cache(self):
29212934
pass
@@ -3066,19 +3079,26 @@ def test_eval_none_flag_orm(self):
30663079
pass
30673080

30683081
@pytest.mark.skip(
3069-
"Spanner JSON_VALUE() always returns STRING,"
3082+
"Spanner JSON_VALUE() always returns STRING, "
30703083
"thus, this test case can't be executed."
30713084
)
30723085
def test_index_typed_comparison(self):
30733086
pass
30743087

30753088
@pytest.mark.skip(
3076-
"Spanner JSON_VALUE() always returns STRING,"
3089+
"Spanner JSON_VALUE() always returns STRING, "
30773090
"thus, this test case can't be executed."
30783091
)
30793092
def test_path_typed_comparison(self):
30803093
pass
30813094

3095+
@pytest.mark.skip(
3096+
"Spanner JSON_VALUE() always returns STRING, "
3097+
"thus, this test case can't be executed."
3098+
)
3099+
def test_index_cross_casts(self):
3100+
pass
3101+
30823102
@pytest.mark.skip("Custom JSON de-/serializers are not supported.")
30833103
def test_round_trip_custom_json(self):
30843104
pass

‎packages/sqlalchemy-spanner/tests/unit/test_alembic.py‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,12 @@
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
1414

15+
import importlib.util
16+
import sys
17+
from unittest import mock
18+
1519
from alembic.ddl import base as ddl_base
16-
from sqlalchemy import String, TextClause
20+
from sqlalchemy import String, TextClause, event
1721
from sqlalchemy.testing import eq_
1822
from sqlalchemy.testing.plugin.plugin_base import fixtures
1923

@@ -96,3 +100,33 @@ def test_visit_column_type_with_default(self):
96100
"ALTER COLUMN col "
97101
"STRING(256) NOT NULL DEFAULT (GENERATE_UUID())",
98102
)
103+
104+
def test_dialect_import_without_alembic(self):
105+
"""Verify that sqlalchemy_spanner imports cleanly when Alembic is unavailable.
106+
107+
Why we test this way instead of calling importlib.reload(sqlalchemy_spanner):
108+
1. Setting sys.modules["alembic.ddl.base"] = None via mock.patch.dict causes
109+
Python to raise ModuleNotFoundError (a subclass of ImportError) when
110+
sqlalchemy_spanner attempts to import from alembic.ddl.base.
111+
2. Calling importlib.reload(sqlalchemy_spanner) would re-execute the module
112+
in-place inside the existing sqlalchemy_spanner.__dict__. That has two
113+
undesirable side effects:
114+
- Functions defined during the initial import (like visit_column_nullable)
115+
remain in sqlalchemy_spanner.__dict__ even if skipped on reload.
116+
- Classes (like SpannerIdentifierPreparer) are recreated with new class
117+
identities in sqlalchemy_spanner.__dict__, which breaks other test
118+
modules (such as test_dialect.py) that already imported SpannerDialect
119+
before the reload and rely on super(SpannerIdentifierPreparer, self).
120+
3. Creating a fresh module object via importlib.util.module_from_spec and
121+
executing the loader into that isolated namespace tests a clean import
122+
without mutating the shared sqlalchemy_spanner module in sys.modules.
123+
"""
124+
with mock.patch.dict(sys.modules, {"alembic.ddl.base": None}):
125+
module = importlib.util.module_from_spec(sqlalchemy_spanner.__spec__)
126+
sqlalchemy_spanner.__spec__.loader.exec_module(module)
127+
# Executing the module registers module.reset_connection on the shared
128+
# SQLAlchemy Pool class via @listens_for(Pool, "reset"); remove that
129+
# temporary listener so global Pool state stays clean for other tests.
130+
event.remove(sqlalchemy_spanner.Pool, "reset", module.reset_connection)
131+
assert not module.HAS_ALEMBIC_INSTALLED
132+
assert not hasattr(module, "visit_column_nullable")

0 commit comments

Comments
 (0)