Skip to content

Commit ef2a54e

Browse files
authored
fix(db): preserve expression-based indexes in autogenerate (#144)
Closes #140. SQLAlchemy 2.0 can't reflect functional indexes (e.g. `CREATE INDEX ... ON t (lower(email))`) under the SQLite dialect, so Alembic autogenerate silently skips them — leaving SQLite dev DBs without an index that production Postgres has. - Add `make_process_revision_directives(metadata)` in `simple_module_db.migrations`: walks each generated `MigrationScript` and, for any `CreateTableOp` whose table has expression-based indexes in the target metadata, appends a matching `CreateIndexOp` (and the reverse `DropIndexOp` in the downgrade). Dedups against already-emitted ops so the hook is a no-op on Postgres. - Wire the hook into `host/migrations/env.py` and the scaffold template so every existing and `smpy new`-generated host gets it. - Add catch-up migration `41cf2c53660e` that back-fills `ix_users_user_email_lower` on databases already past `3bf3f9db7f7f`. Uses `if_not_exists` / `if_exists` so it's a no-op on installs that somehow already have the index.
1 parent d18ad9e commit ef2a54e

6 files changed

Lines changed: 271 additions & 5 deletions

File tree

‎framework/cli/simple_module_cli/templates/host/migrations/env.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@
1111
from logging.config import fileConfig
1212

1313
from alembic import context
14-
from simple_module_db import build_module_metadata, make_include_object, render_item
14+
from simple_module_db import (
15+
build_module_metadata,
16+
make_include_object,
17+
make_process_revision_directives,
18+
render_item,
19+
)
1520
from simple_module_hosting.settings import Settings
1621
from sqlalchemy import engine_from_config, pool
1722

@@ -24,6 +29,9 @@
2429

2530
target_metadata = build_module_metadata()
2631
include_object = make_include_object(target_metadata)
32+
# Re-emit expression-based indexes (e.g. ``lower(email)``) that autogenerate
33+
# silently drops under SQLite. See ``make_process_revision_directives`` docstring.
34+
process_revision_directives = make_process_revision_directives(target_metadata)
2735

2836

2937
def _get_url() -> str:
@@ -45,6 +53,7 @@ def run_migrations_offline() -> None:
4553
dialect_opts={"paramstyle": "named"},
4654
include_object=include_object,
4755
render_item=render_item,
56+
process_revision_directives=process_revision_directives,
4857
)
4958

5059
with context.begin_transaction():
@@ -68,6 +77,7 @@ def run_migrations_online() -> None:
6877
target_metadata=target_metadata,
6978
include_object=include_object,
7079
render_item=render_item,
80+
process_revision_directives=process_revision_directives,
7181
)
7282

7383
with context.begin_transaction():

‎framework/db/simple_module_db/__init__.py‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,12 @@
33
from simple_module_db.base import create_module_base
44
from simple_module_db.deps import get_db
55
from simple_module_db.listeners import TenantIsolationError, current_tenant_id
6-
from simple_module_db.migrations import build_module_metadata, make_include_object, render_item
6+
from simple_module_db.migrations import (
7+
build_module_metadata,
8+
make_include_object,
9+
make_process_revision_directives,
10+
render_item,
11+
)
712
from simple_module_db.mixins import AuditMixin, MultiTenantMixin, SoftDeleteMixin, VersionedMixin
813
from simple_module_db.provider import DatabaseProvider, detect_provider
914
from simple_module_db.session import DatabaseState, init_db
@@ -23,5 +28,6 @@
2328
"get_db",
2429
"init_db",
2530
"make_include_object",
31+
"make_process_revision_directives",
2632
"render_item",
2733
]

‎framework/db/simple_module_db/migrations.py‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,10 @@
2121
from typing import Literal
2222

2323
import sqlalchemy as sa
24+
from alembic.operations.ops import CreateIndexOp, CreateTableOp, DropIndexOp, DropTableOp
2425
from simple_module_core import ModuleBase
2526
from simple_module_core.discovery import discover_modules, get_module_package_name
26-
from sqlalchemy import MetaData
27+
from sqlalchemy import Column, Index, MetaData
2728
from sqlalchemy.schema import SchemaItem
2829

2930
from simple_module_db.base import all_module_bases
@@ -36,6 +37,7 @@
3637
"schema", "table", "column", "index", "unique_constraint", "foreign_key_constraint"
3738
]
3839
IncludeObjectFn = Callable[[SchemaItem, str | None, _SchemaItemType, bool, SchemaItem | None], bool]
40+
ProcessRevisionDirectivesFn = Callable[[object, object, list], None]
3941

4042

4143
def build_module_metadata(modules: Sequence[ModuleBase] | None = None) -> MetaData:
@@ -110,6 +112,97 @@ def include_object(
110112
return include_object
111113

112114

115+
def make_process_revision_directives(
116+
metadata: MetaData,
117+
) -> ProcessRevisionDirectivesFn:
118+
"""Return an Alembic ``process_revision_directives`` hook that re-adds
119+
expression-based indexes silently dropped by autogenerate.
120+
121+
SQLAlchemy 2.0 can't reflect expression-based indexes (functional indexes
122+
like ``CREATE INDEX ... ON t (lower(email))``) under the SQLite dialect.
123+
Autogenerate guards against false-positive diffs there by *skipping* the
124+
index entirely — which is correct for an "existing table, can't tell if
125+
the index already exists" diff, but disastrous on initial CREATE TABLE:
126+
SQLite dev DBs end up without an index that production Postgres has.
127+
128+
This hook walks each generated ``MigrationScript`` and, for every
129+
``CreateTableOp`` whose target table has expression-based indexes in the
130+
metadata, appends a matching ``CreateIndexOp``. The reverse ``DropIndexOp``
131+
is inserted into ``downgrade_ops`` before the table drop for symmetry —
132+
not strictly required (dropping the table drops the index) but it keeps
133+
autogen output readable.
134+
135+
Call as::
136+
137+
context.configure(
138+
...,
139+
process_revision_directives=make_process_revision_directives(target_metadata),
140+
)
141+
"""
142+
expression_indexes: dict[str, list[Index]] = {}
143+
for table in metadata.tables.values():
144+
for index in table.indexes:
145+
if _index_is_expression_based(index):
146+
expression_indexes.setdefault(table.name, []).append(index)
147+
148+
def process_revision_directives(context, revision, directives):
149+
if not expression_indexes:
150+
return
151+
for script in directives:
152+
upgrade_ops = getattr(script, "upgrade_ops", None)
153+
if upgrade_ops is not None:
154+
_inject_create_index_after_create_table(upgrade_ops, expression_indexes)
155+
downgrade_ops = getattr(script, "downgrade_ops", None)
156+
if downgrade_ops is not None:
157+
_inject_drop_index_before_drop_table(downgrade_ops, expression_indexes)
158+
159+
return process_revision_directives
160+
161+
162+
def _index_is_expression_based(index: Index) -> bool:
163+
"""An index is expression-based when any of its expressions is not a plain ``Column``."""
164+
return any(not isinstance(expr, Column) for expr in index.expressions)
165+
166+
167+
def _inject_create_index_after_create_table(upgrade_ops, expression_indexes) -> None:
168+
existing_index_names = {
169+
getattr(op, "index_name", None) for op in upgrade_ops.ops if isinstance(op, CreateIndexOp)
170+
}
171+
new_ops: list = []
172+
for op in upgrade_ops.ops:
173+
new_ops.append(op)
174+
if not isinstance(op, CreateTableOp):
175+
continue
176+
for index in expression_indexes.get(op.table_name, []):
177+
if index.name in existing_index_names:
178+
continue
179+
new_ops.append(CreateIndexOp.from_index(index))
180+
existing_index_names.add(index.name)
181+
logger.info(
182+
"Re-emitting expression-based index %r on %r — autogenerate "
183+
"skipped it (dialect can't reflect functional indexes).",
184+
index.name,
185+
op.table_name,
186+
)
187+
upgrade_ops.ops = new_ops
188+
189+
190+
def _inject_drop_index_before_drop_table(downgrade_ops, expression_indexes) -> None:
191+
existing_drop_names = {
192+
getattr(op, "index_name", None) for op in downgrade_ops.ops if isinstance(op, DropIndexOp)
193+
}
194+
new_ops: list = []
195+
for op in downgrade_ops.ops:
196+
if isinstance(op, DropTableOp):
197+
for index in expression_indexes.get(op.table_name, []):
198+
if index.name in existing_drop_names:
199+
continue
200+
new_ops.append(DropIndexOp(index.name, table_name=op.table_name))
201+
existing_drop_names.add(index.name)
202+
new_ops.append(op)
203+
downgrade_ops.ops = new_ops
204+
205+
113206
def render_item(type_, obj, autogen_context):
114207
"""Alembic ``render_item`` callback for SQLModel + extension types.
115208

‎framework/db/tests/test_migrations.py‎

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
"""Tests for build_module_metadata and make_include_object (Gap 1)."""
1+
"""Tests for the simple_module_db.migrations helpers."""
22

33
from __future__ import annotations
44

@@ -93,3 +93,110 @@ async def test_include_object_skips_unmodeled_cross_module_fks_by_default(self):
9393
include(stranger_fk, "fk_stranger", "foreign_key_constraint", False, stranger_fk)
9494
is False
9595
)
96+
97+
98+
class TestProcessRevisionDirectives:
99+
"""Autogenerate silently drops expression-based indexes (e.g. ``lower(email)``)
100+
on SQLite. ``make_process_revision_directives`` re-injects them when the
101+
target table is being newly created in the same revision."""
102+
103+
def _build_meta(self, *, expression_index: bool = True):
104+
from sqlalchemy import Column, Index, Integer, MetaData, String, Table, text
105+
106+
meta = MetaData()
107+
t = Table(
108+
"things",
109+
meta,
110+
Column("id", Integer, primary_key=True),
111+
Column("email", String(320)),
112+
)
113+
if expression_index:
114+
Index("ix_things_email_lower", text("lower(email)"), _table=t)
115+
else:
116+
Index("ix_things_email", t.c.email)
117+
return meta
118+
119+
def _build_directives(self, table_name: str, *extra_ops, empty: bool = False):
120+
from alembic.operations.ops import (
121+
CreateTableOp,
122+
DowngradeOps,
123+
DropTableOp,
124+
MigrationScript,
125+
UpgradeOps,
126+
)
127+
from sqlalchemy import Column, Integer, String
128+
129+
if empty:
130+
upgrade_ops_list, downgrade_ops_list = [], []
131+
else:
132+
create_table = CreateTableOp(
133+
table_name,
134+
[Column("id", Integer, primary_key=True), Column("email", String(320))],
135+
)
136+
upgrade_ops_list = [create_table, *extra_ops]
137+
downgrade_ops_list = [DropTableOp(table_name)]
138+
return [
139+
MigrationScript(
140+
rev_id="abc123",
141+
upgrade_ops=UpgradeOps(ops=upgrade_ops_list),
142+
downgrade_ops=DowngradeOps(ops=downgrade_ops_list),
143+
message="test",
144+
)
145+
]
146+
147+
def test_injects_expression_index_after_create_table(self):
148+
"""A functional index in metadata is appended as ``CreateIndexOp`` after
149+
the matching ``CreateTableOp``, and the reverse drop is inserted before
150+
the ``DropTableOp`` in the downgrade."""
151+
from alembic.operations.ops import CreateIndexOp, DropIndexOp
152+
from simple_module_db.migrations import make_process_revision_directives
153+
154+
directives = self._build_directives("things")
155+
make_process_revision_directives(self._build_meta())(None, None, directives)
156+
157+
index_ops = [op for op in directives[0].upgrade_ops.ops if isinstance(op, CreateIndexOp)]
158+
assert len(index_ops) == 1
159+
assert index_ops[0].index_name == "ix_things_email_lower"
160+
assert index_ops[0].table_name == "things"
161+
162+
drop_ops = [op for op in directives[0].downgrade_ops.ops if isinstance(op, DropIndexOp)]
163+
assert len(drop_ops) == 1
164+
assert drop_ops[0].index_name == "ix_things_email_lower"
165+
166+
def test_does_not_inject_when_no_create_table(self):
167+
"""If the revision is not creating the table (e.g. a pure data migration
168+
or unrelated change), the hook should not append a CreateIndexOp."""
169+
from alembic.operations.ops import CreateIndexOp
170+
from simple_module_db.migrations import make_process_revision_directives
171+
172+
directives = self._build_directives("things", empty=True)
173+
make_process_revision_directives(self._build_meta())(None, None, directives)
174+
175+
assert not any(isinstance(op, CreateIndexOp) for op in directives[0].upgrade_ops.ops)
176+
177+
def test_does_not_double_inject_when_already_present(self):
178+
"""On Postgres, autogenerate emits the expression index normally. The
179+
hook must not duplicate it."""
180+
from alembic.operations.ops import CreateIndexOp
181+
from simple_module_db.migrations import make_process_revision_directives
182+
183+
meta = self._build_meta()
184+
idx = next(iter(meta.tables["things"].indexes))
185+
directives = self._build_directives("things", CreateIndexOp.from_index(idx))
186+
make_process_revision_directives(meta)(None, None, directives)
187+
188+
index_ops = [op for op in directives[0].upgrade_ops.ops if isinstance(op, CreateIndexOp)]
189+
assert len(index_ops) == 1
190+
191+
def test_ignores_column_based_indexes(self):
192+
"""Plain column indexes are already handled correctly by autogenerate;
193+
the hook must not touch them."""
194+
from alembic.operations.ops import CreateIndexOp
195+
from simple_module_db.migrations import make_process_revision_directives
196+
197+
directives = self._build_directives("things")
198+
make_process_revision_directives(self._build_meta(expression_index=False))(
199+
None, None, directives
200+
)
201+
202+
assert not any(isinstance(op, CreateIndexOp) for op in directives[0].upgrade_ops.ops)

‎host/migrations/env.py‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@
1111
from logging.config import fileConfig
1212

1313
from alembic import context
14-
from simple_module_db import build_module_metadata, make_include_object, render_item
14+
from simple_module_db import (
15+
build_module_metadata,
16+
make_include_object,
17+
make_process_revision_directives,
18+
render_item,
19+
)
1520
from simple_module_hosting.settings import Settings
1621
from sqlalchemy import engine_from_config, pool
1722

@@ -31,6 +36,10 @@
3136
# host's user-added tables or framework internals.
3237
include_object = make_include_object(target_metadata)
3338

39+
# Re-emit expression-based indexes (e.g. ``lower(email)``) that autogenerate
40+
# silently drops under SQLite — see make_process_revision_directives docstring.
41+
process_revision_directives = make_process_revision_directives(target_metadata)
42+
3443

3544
def _get_url() -> str:
3645
"""Read database URL from settings, convert async to sync driver."""
@@ -48,6 +57,7 @@ def run_migrations_offline() -> None:
4857
dialect_opts={"paramstyle": "named"},
4958
include_object=include_object,
5059
render_item=render_item,
60+
process_revision_directives=process_revision_directives,
5161
)
5262

5363
with context.begin_transaction():
@@ -71,6 +81,7 @@ def run_migrations_online() -> None:
7181
target_metadata=target_metadata,
7282
include_object=include_object,
7383
render_item=render_item,
84+
process_revision_directives=process_revision_directives,
7485
)
7586

7687
with context.begin_transaction():
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
"""users_user lower(email) functional index
2+
3+
Revision ID: 41cf2c53660e
4+
Revises: 3bf3f9db7f7f
5+
Create Date: 2026-05-12 00:00:00.000000
6+
7+
The functional index ``ix_users_user_email_lower`` on ``lower(users_user.email)``
8+
backs the case-insensitive lookup used by ``UserDatabaseWithRoles.get_by_email``
9+
and ``users.bootstrap``. Autogenerate silently dropped it from the original
10+
initial-schema revision (``77162e7b184b``) because SQLAlchemy 2.0 can't reflect
11+
expression-based indexes under the SQLite dialect, so dev DBs are missing it.
12+
This revision back-fills the index unconditionally.
13+
"""
14+
15+
from collections.abc import Sequence
16+
17+
import sqlalchemy as sa
18+
from alembic import op
19+
20+
revision: str = "41cf2c53660e"
21+
down_revision: str | None = "3bf3f9db7f7f"
22+
branch_labels: str | Sequence[str] | None = None
23+
depends_on: str | Sequence[str] | None = None
24+
25+
26+
def upgrade() -> None:
27+
# ``if_not_exists`` covers the case where a Postgres developer's original
28+
# autogen run already emitted this index (only SQLite skips it).
29+
op.create_index(
30+
"ix_users_user_email_lower",
31+
"users_user",
32+
[sa.text("lower(email)")],
33+
unique=False,
34+
if_not_exists=True,
35+
)
36+
37+
38+
def downgrade() -> None:
39+
op.drop_index("ix_users_user_email_lower", table_name="users_user", if_exists=True)

0 commit comments

Comments
 (0)