diff --git a/CHANGELOG.md b/CHANGELOG.md index 5472360f6..46539f7b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ All notable changes to this project are documented in this file. The format is b ### Fixed +- `alembic check` and SQLite autogenerate no longer report the `file_storage` browse index (`sa.desc(sa.column("created_at"))` instead of `sa.text("created_at DESC")`) or `users_access_token.expires_at` (now `AwareDateTime`) as changed, so the host's `alembic check` on SQLite is clean. `make ci-doctor` now runs `alembic check` against its throwaway SQLite database, and a module index written with `sa.text("col DESC")` is no longer reported as changed on SQLite (#869). - `background_tasks`: the Redis invalidation listener now opens the connection a publish uses (one `PING` after every successful subscribe), so the first invalidation after boot or after a Redis outage no longer spends part of its 1 s budget connecting. Under load that connect was cancelled and the message was lost; other workers kept the stale value until its TTL (#880). ### Changed diff --git a/Makefile b/Makefile index 471478d88..de0f7d6e0 100644 --- a/Makefile +++ b/Makefile @@ -264,6 +264,7 @@ CI_DOCTOR_DB ?= ci-doctor.db ci-doctor: rm -f $(CI_DOCTOR_DB) SM_DATABASE_URL=sqlite+aiosqlite:///./$(CI_DOCTOR_DB) $(MAKE) migrate \ + && SM_DATABASE_URL=sqlite+aiosqlite:///./$(CI_DOCTOR_DB) uv run --project host alembic -c host/alembic.ini check \ && SM_DATABASE_URL=sqlite+aiosqlite:///./$(CI_DOCTOR_DB) $(MAKE) doctor; \ rc=$$?; rm -f $(CI_DOCTOR_DB)*; exit $$rc diff --git a/docs/module-authoring.md b/docs/module-authoring.md index 06e004bb1..ecf43545e 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -368,7 +368,12 @@ generated file against this checklist before treating it as portable (`op.create_index(..., [sa.text("lower(email)")], if_not_exists=True)`) and confirm with the PostgreSQL round trip below. The warning is deliberately not an error: it fires on every autogenerate, including unrelated revisions, and - the Postgres round trip is the actual gate. + the Postgres round trip is the actual gate. Prefer writing an ordered index column as + `sa.desc(sa.column("col"))` over `sa.text("col DESC")` or `sa.desc("col")`: + the latter two are expressions to Alembic, and while the host's include filter + suppresses the false SQLite change when only the ordering differs, the column + form needs no filter and stays out of `SM` expression-index warnings + ([GH #869](https://github.com/antosubash/simple_module_python/issues/869)). 3. **Enum types on downgrade.** `op.drop_table` leaves the PostgreSQL `CREATE TYPE` behind, so `downgrade` then `upgrade` fails with `DuplicateObject`. Autogenerate never emits the drop. Finish `downgrade()` diff --git a/docs/superpowers/plans/2026-11-07-sqlite-alembic-check.md b/docs/superpowers/plans/2026-11-07-sqlite-alembic-check.md new file mode 100644 index 000000000..1077f11f0 --- /dev/null +++ b/docs/superpowers/plans/2026-11-07-sqlite-alembic-check.md @@ -0,0 +1,44 @@ +# Plan: make `alembic check` clean on SQLite (GH #869) + +Design: `docs/superpowers/specs/2026-11-07-sqlite-alembic-check-design.md`. + +## Task 1: SQLite text-ordered index guard (independent, marked hard) + +Files: +- `framework/db/simple_module_db/migrations.py` +- new `framework/db/simple_module_db/_sqlite_index_compare.py` +- new `framework/db/tests/test_migrations_sqlite_index_compare.py` + +1. Write the tests first, against in-memory SQLite through a real `EnvironmentContext` and `compare_metadata`: + - a mixed `text("col DESC")` index produces no diff with the filter; + - the same index without the filter still shows the remove/add pair; + - a changed column, a uniqueness change and a `lower(email)` element are each still reported; + - with no active migration context, the filter neither raises nor skips. +2. Add a Postgres test that patches the migration context's dialect to Postgres and shows the guard never skips there. +3. Implement a private module `simple_module_db/_sqlite_index_compare.py` plus a branch in `include_object`, keeping `make_include_object`'s signature unchanged. + +Done: the tests pass, and they fail without the guard. `migrations.py` stays under 300 lines. + +## Task 2: models, docs and the CI gate (independent of Task 1) + +Files: +- `modules/file_storage/file_storage/models.py` +- `modules/users/users/models/access_token.py` +- a new test in each module's tests +- `Makefile` (`ci-doctor`) +- `docs/module-authoring.md` +- `CHANGELOG.md` + +Steps: +1. Write tests: + - `file_storage`: no `TextClause` among the browse index's elements; + - `users`: an `expires_at` written and read on SQLite comes back aware UTC. +2. Change `sa.text("created_at DESC")` to `sa.desc(sa.column("created_at"))`, and `expires_at` to `sa_type=AwareDateTime()`. Keep the `TIMESTAMPAware` import, because `created_at` still uses it. +3. In `ci-doctor`, run `alembic check` against the same throwaway database after `make migrate`. +4. Add one docs sentence and a CHANGELOG Fixed entry citing #869. + +Done: `make ci-doctor` passes locally, which means the `alembic check` on SQLite is clean. + +## Task 3: integrate and verify (after Tasks 1 and 2) + +Merge the Task 1 diff, then run `make lint`, `make test-py` (with `SM_TEST_REDIS_URL`), `make test-js`, `make ci-doctor` and `make migrations-roundtrip-pg` (dev-services Postgres). diff --git a/docs/superpowers/specs/2026-11-07-sqlite-alembic-check-design.md b/docs/superpowers/specs/2026-11-07-sqlite-alembic-check-design.md new file mode 100644 index 000000000..993ab0957 --- /dev/null +++ b/docs/superpowers/specs/2026-11-07-sqlite-alembic-check-design.md @@ -0,0 +1,111 @@ +# Make `alembic check` clean on SQLite (GH #869) + +## Goal and scope + +On a SQLite database migrated with `upgrade heads`, `alembic check` fails. So does every SQLite +`revision --autogenerate`, which writes the same false operations into the new revision. +Verified on `main` (8f3b9c9b), there are two false diffs: + +1. **#869:** `ix_file_storage_stored_file_tenant_browse` is reported as removed and re-added. + - The model writes `created_at DESC` as `sa.text("created_at DESC")`. + - SQLite reflects it as the plain column `created_at`, and Alembic compares a column with + a text element. +2. **Found while reproducing #869:** `users_access_token.expires_at` is reported as a type + change, `DATETIME` → `TIMESTAMPAware`. + - Its migration (`b4c1e7d9a025`) created `sa.DateTime(timezone=True)`, and the model + declares fastapi-users' `TIMESTAMPAware`, whose impl is `TIMESTAMP`. + - SQLite reflects `DATETIME`, so the type differs. Postgres reflects + `TIMESTAMP WITH TIME ZONE` for both, which is why CI never saw either diff: its only + `alembic check` runs on Postgres. + +In scope: fix both, protect downstream modules from the first, and make CI run `alembic check` +on SQLite. Out of scope: any schema change (no migration), and the expression-index +limitation Alembic already handles (pure expression indexes are skipped on SQLite). + +## Approach + +1. **Fix the model in `file_storage`.** Replace `sa.text("created_at DESC")` with + `sa.desc(sa.column("created_at"))`. (Implementation note: the plain string form + `sa.desc("created_at")` wraps a text label, which Alembic still treats as an + expression, so it did not fix the diff.) + - Both compile to the identical `CREATE INDEX … (tenant_id, is_deleted, created_at DESC, id)` + on SQLite and Postgres, so no migration is needed. + - Measured: `desc()` over a column compares clean on SQLite and on Postgres, and the + `sa.text()` form compares clean only on Postgres. +2. **Guard for other modules in `make_include_object`** (the issue's suggestion). + - On SQLite only, skip comparing an index pair when one side has a text element of the + form ` [ASC|DESC]` and both sides agree on uniqueness and column names, with + that element read as its column. + - Any other difference is still reported: a different column, a real expression such as + `lower(email)`, or a uniqueness change. + - The dialect comes from `alembic.context.get_context()`. When no migration context is + active, the guard does nothing. + - Scaffolded hosts get the guard without editing `env.py`, because they already call + `make_include_object`. +3. **Fix the `expires_at` type in `users`.** Declare `sa_type=AwareDateTime()`, the repo + convention (CLAUDE.md § Database). + - It renders as `sa.DateTime(timezone=True)`, which matches the migration exactly. + - Values still read back aware UTC. + - On write, AwareDateTime converts an aware non-UTC value to UTC, where `TIMESTAMPAware` + stored its wall time as is. Every writer passes UTC (`now_utc`, the strategy's deadline), + so stored values do not change. + - `created_at` stays `TIMESTAMPAware`: the initial migration created it as that type, so it + already compares clean. +4. **CI guard.** `make ci-doctor` already migrates a throwaway SQLite database. It now also runs + `alembic check` against that database, so a new SQLite-only false diff fails the PR instead + of reaching a downstream host. + +Rejected: +- **The guard alone** (no model change): it leaves the framework's own model relying on a + comparison that is skipped. +- **A `compare_type` hook that treats SQLite `TIMESTAMP` and `DATETIME` as equal:** every + existing host would have to edit its `env.py` to get it, and the model fix needs nothing. +- **A migration that retypes `expires_at`:** Postgres already matches, so it would be a + no-op there. On SQLite it would be a batch table rebuild of the auth token table for no + behaviour change. + +## Decisions made + +- The guard matches only a single identifier with an optional `ASC`/`DESC`, plus optional + `NULLS FIRST|LAST`, optionally double-quoted. Anything else in a text element is a real + expression and keeps today's behaviour. +- The guard lives in the private module `simple_module_db._sqlite_index_compare`, called from + the existing foreign-key filter's `include_object` in `simple_module_db.migrations`. + `make_include_object` keeps its signature. It reuses core's `unwrap_ordering` helper. +- `docs/module-authoring.md` (Migrations section) gets one sentence: write an ordered index + column as `sa.desc(sa.column("col"))`, not `sa.text("col DESC")` or `sa.desc("col")`. +- The guard also accepts the `sa.desc("col")` label form, and SM026 counts a column wrapped + in `desc()`/`asc()` as a column, not as an unverifiable expression. +- CHANGELOG `[Unreleased]` → Fixed, citing #869. +- The `alembic check` step in `ci-doctor` runs from the repo root like `make migrate`, using + the same `SM_DATABASE_URL`. + +## Testing + +- **New `framework/db/tests/test_migrations_sqlite_index_compare.py`**, against an in-memory + SQLite database through a real `EnvironmentContext`, so `get_context()` is exercised: + - a mixed `text("col DESC")` index compares clean with the filter; + - the same index without the filter still shows the remove/add pair, which proves the test + reproduces the bug; + - a column change, a uniqueness change, and a `lower(email)` text element are all still + reported; + - called with no active migration context, the filter neither raises nor skips. +- **Postgres:** a test patches the migration context's dialect to Postgres and confirms the + guard never skips there (no live Postgres needed). +- **`file_storage`:** the model's index elements are plain columns or `desc()`, with no text + element. +- **`users`:** an `expires_at` round trip on SQLite reads back aware UTC. +- **Whole branch:** `make ci-doctor` (now including `alembic check`) passes locally, and + `make migrations-roundtrip-pg` passes against the dev-services Postgres, proving Postgres + still reports no diff. +- Full `make lint`, `make test-py` and `make test-js`. No browser QA: nothing user-visible + changes. + +## Risks + +- **`alembic check` on SQLite may surface other false diffs** in modules that CI does not + enable. `ci-doctor` migrates every installed module, and the local run found exactly these + two, so the gate is clean today. +- **A downstream module may rely on `TIMESTAMPAware`'s non-UTC write behaviour for + `expires_at`.** The column belongs to `users`, and nothing outside it writes the column + (checked by grep). diff --git a/framework/core/simple_module_core/diagnostics/_expression_index.py b/framework/core/simple_module_core/diagnostics/_expression_index.py index dbee41b71..07afe434e 100644 --- a/framework/core/simple_module_core/diagnostics/_expression_index.py +++ b/framework/core/simple_module_core/diagnostics/_expression_index.py @@ -25,13 +25,37 @@ logger = logging.getLogger(__name__) -def index_is_expression_based(index: Any) -> bool: - """True when any element of ``index`` is an expression rather than a plain column. +_ORDERING_MODIFIERS = frozenset({"asc_op", "desc_op", "nulls_first_op", "nulls_last_op"}) + + +def unwrap_ordering(expr: Any) -> Any: + """``expr`` without its ``desc()``/``asc()``/``nulls_first()``/``nulls_last()`` wrappers.""" + while getattr(getattr(expr, "modifier", None), "__name__", None) in _ORDERING_MODIFIERS: + expr = expr.element + return expr + - A plain ``Column`` is attached to its table; a ``text()`` or function - element is not. +def _is_column(expr: Any) -> bool: + """A column, possibly wrapped in ``desc()``/``asc()``/``nulls_last()``. + + SQLite reflects ``created_at DESC`` as the column ``created_at`` and Alembic + compares it as one (GH #869), so an ordering keeps an element a column. A + plain ``Column`` is attached to its table; ``sa.column("x")`` is a named, + non-literal column; ``text()``, ``literal_column()``, a function or a bare + string label are expressions. """ - return any(getattr(expr, "table", None) is None for expr in index.expressions) + expr = unwrap_ordering(expr) + if hasattr(expr, "element"): + # Still wraps something once the ordering is gone: a label over an + # arbitrary expression (``lower(email).label("x")``) or ``sa.desc("col")``'s + # bare-string label. A column wraps nothing. + return False + return getattr(expr, "table", None) is not None or getattr(expr, "is_literal", None) is False + + +def index_is_expression_based(index: Any) -> bool: + """True when any element of ``index`` is an expression rather than a plain column.""" + return not all(_is_column(expr) for expr in index.expressions) def find_expression_indexes(tables: Iterable[Any]) -> list[tuple[str, str]]: @@ -103,4 +127,5 @@ def check_expression_indexes_unverifiable( "find_expression_indexes", "index_is_expression_based", "module_all_tables", + "unwrap_ordering", ] diff --git a/framework/core/tests/test_expression_index_diagnostics.py b/framework/core/tests/test_expression_index_diagnostics.py index 2b56ae298..8bfad8f9a 100644 --- a/framework/core/tests/test_expression_index_diagnostics.py +++ b/framework/core/tests/test_expression_index_diagnostics.py @@ -84,3 +84,39 @@ class NoModels(ModuleBase): meta = ModuleMeta(name="NoModels") assert module_all_tables(NoModels()) == [] + + +def test_a_descending_column_is_a_column_not_an_expression(): + """SQLite reflects ``created_at DESC`` as a column and Alembic compares it (GH #869).""" + md = sa.MetaData() + table = sa.Table( + "t", + md, + sa.Column("id", sa.Integer, primary_key=True), + sa.Column("created_at", sa.DateTime), + sa.Index("ix_t_named", "id", sa.desc(sa.column("created_at"))), + ) + sa.Index("ix_t_bound", table.c.created_at.desc().nulls_last()) + assert find_expression_indexes([table]) == [] + + +def test_text_ordering_and_literal_columns_still_count_as_expressions(): + md = sa.MetaData() + table = sa.Table( + "t", + md, + sa.Column("id", sa.Integer, primary_key=True), + sa.Column("created_at", sa.DateTime), + sa.Index("ix_t_text", "id", sa.text("created_at DESC")), + sa.Index("ix_t_label", "id", sa.desc("created_at")), + sa.Index("ix_t_literal", sa.desc(sa.literal_column("created_at"))), + ) + sa.Index("ix_t_labelled_func", sa.func.lower(table.c.id).label("x")) + sa.Index("ix_t_cast", sa.cast(table.c.created_at, sa.String).desc()) + assert find_expression_indexes([table]) == [ + ("t", "ix_t_cast"), + ("t", "ix_t_label"), + ("t", "ix_t_labelled_func"), + ("t", "ix_t_literal"), + ("t", "ix_t_text"), + ] diff --git a/framework/db/simple_module_db/_sqlite_index_compare.py b/framework/db/simple_module_db/_sqlite_index_compare.py new file mode 100644 index 000000000..b7e10d2d9 --- /dev/null +++ b/framework/db/simple_module_db/_sqlite_index_compare.py @@ -0,0 +1,97 @@ +"""Index comparisons SQLite cannot make faithfully (GH #869). + +SQLite reflects ``CREATE INDEX ... (tenant_id, created_at DESC, id)`` as three +plain columns. A model that wrote the ordered column as ``sa.text("created_at +DESC")`` or ``sa.desc("created_at")`` holds an expression there, and Alembic +compares the two element by element: every autogenerate on SQLite then writes a +drop and re-create of a correct index, and ``alembic check`` fails on a correct +schema. Postgres reflects the ordering, so it compares these faithfully. + +:func:`differs_only_in_sqlite_ordering` recognises exactly that pair, so +``make_include_object`` can skip it on SQLite. Anything else still reaches +Alembic: a different column list, a uniqueness change, or a real expression +such as ``lower(email)``. A change of ordering alone (``DESC`` to ``ASC``) is not +reported on SQLite, which cannot reflect ordering at all; the Postgres round +trip (``make migrations-roundtrip-pg``) is what catches it. +""" + +from __future__ import annotations + +import logging +import re + +from alembic import context +from simple_module_core.diagnostics._expression_index import unwrap_ordering +from sqlalchemy import Index +from sqlalchemy.schema import SchemaItem +from sqlalchemy.sql.elements import ColumnClause, TextClause + +logger = logging.getLogger(__name__) + +# One identifier, optionally double-quoted, then an optional ordering. +_ORDERED_COLUMN = re.compile( + r'\s*(?:"([^"]+)"|([A-Za-z_][A-Za-z0-9_]*))' + r"(?:\s+(?:ASC|DESC))?(?:\s+NULLS\s+(?:FIRST|LAST))?\s*", + re.IGNORECASE, +) + + +def _on_sqlite() -> bool: + """Whether the active Alembic migration context targets SQLite. + + Outside a migration (no context configured) the answer is no, so the + caller never suppresses a comparison it cannot place. + """ + try: + return context.get_context().dialect.name == "sqlite" + except Exception as exc: + logger.debug("No Alembic migration context, SQLite index guard off: %s", exc) + return False + + +def _element_name(element: object) -> tuple[str | None, bool]: + """``(column name, written as an expression)`` for one index element. + + The name is ``None`` for a real expression, which never matches. + """ + element = unwrap_ordering(element) + if isinstance(element, TextClause): + match = _ORDERED_COLUMN.fullmatch(element.text) + return (match[1] or match[2], True) if match else (None, True) + if isinstance(element, ColumnClause) and not element.is_literal: + return element.name, False + # ``sa.desc("col")`` wraps a textual label reference holding the bare name. + label = getattr(element, "element", None) + if type(element).__name__ == "_textual_label_reference" and isinstance(label, str): + return label, True + return None, False + + +def _is_partial(index: Index) -> bool: + """A partial index (``WHERE`` predicate), modelled or reflected.""" + return any( + index.dialect_options[dialect].get("where") is not None + for dialect in ("sqlite", "postgresql") + ) + + +def differs_only_in_sqlite_ordering(left: SchemaItem, right: SchemaItem | None) -> bool: + """True when two indexes are the same index as far as SQLite can tell. + + Requires an active SQLite migration context, equal uniqueness, no partial + index (``WHERE`` predicate) on either side, the same + column names in the same order (compared case-insensitively, as SQLite + compares identifiers), and at least one element written as an expression + (otherwise Alembic's own comparison is already right). + """ + if not isinstance(left, Index) or not isinstance(right, Index) or left.unique != right.unique: + return False + if _is_partial(left) or _is_partial(right): + # Only columns are compared here, so a changed predicate would be hidden. + return False + if not _on_sqlite(): + return False + sides = [[_element_name(e) for e in index.expressions] for index in (left, right)] + names = [[name.casefold() if name else None for name, _ in side] for side in sides] + written_as_expression = any(is_expr for side in sides for _, is_expr in side) + return written_as_expression and None not in names[0] and names[0] == names[1] diff --git a/framework/db/simple_module_db/migrations.py b/framework/db/simple_module_db/migrations.py index 8a97d1d6e..711fc1d4c 100644 --- a/framework/db/simple_module_db/migrations.py +++ b/framework/db/simple_module_db/migrations.py @@ -28,6 +28,7 @@ from sqlalchemy.schema import SchemaItem from simple_module_db._migration_render import render_item # noqa: F401 (re-exported) +from simple_module_db._sqlite_index_compare import differs_only_in_sqlite_ordering from simple_module_db.base import all_module_bases from simple_module_db.extensions import ( # noqa: F401 (ensure_db_extensions re-exported) ensure_db_extensions, @@ -147,6 +148,9 @@ def include_object( ) -> bool: if type_ == "table": return name in allowlist + if type_ == "index" and differs_only_in_sqlite_ordering(object, compare_to): + # SQLite reflects ``created_at DESC`` as a plain column (GH #869). + return False if ( ignore_unmodeled_fks and type_ == "foreign_key_constraint" diff --git a/framework/db/tests/test_migrations_sqlite_index_compare.py b/framework/db/tests/test_migrations_sqlite_index_compare.py new file mode 100644 index 000000000..adfa28e41 --- /dev/null +++ b/framework/db/tests/test_migrations_sqlite_index_compare.py @@ -0,0 +1,188 @@ +"""SQLite loses text index ordering on reflection, but real changes must survive (GH #869).""" + +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from alembic import context +from alembic.autogenerate import compare_metadata +from alembic.config import Config +from alembic.runtime.environment import EnvironmentContext +from simple_module_db.migrations import make_include_object +from sqlalchemy.dialects import postgresql + +pytestmark = [ + pytest.mark.filterwarnings("ignore:Skipped unsupported reflection.*:sqlalchemy.exc.SAWarning"), + pytest.mark.filterwarnings("ignore:autogenerate skipping metadata-specified.*:UserWarning"), +] + + +def _metadata(element="created_at DESC", *, first="tenant_id", unique=False): + """``element`` is text for ``sa.text()``, or an already-built SQL element.""" + meta = sa.MetaData() + sa.Table( + "record", + meta, + sa.Column("id", sa.Integer, primary_key=True), + sa.Column("tenant_id", sa.Integer), + sa.Column("created_at", sa.Integer), + sa.Column("email", sa.String), + sa.Index( + "ix", + first, + sa.text(element) if isinstance(element, str) else element, + "id", + unique=unique, + ), + ) + return meta + + +def _index(meta): + return next(iter(meta.tables["record"].indexes)) + + +@pytest.fixture +def connection(): + engine = sa.create_engine("sqlite://") + try: + with engine.connect() as conn: + yield conn + finally: + engine.dispose() + + +def _environment(connection, meta, *, filtered=True): + config = Config() + config.set_main_option("script_location", "host/migrations") + env = EnvironmentContext(config, None) + options = {"include_object": make_include_object(meta)} if filtered else {} + env.configure(connection=connection, target_metadata=meta, **options) + return env + + +@pytest.mark.parametrize( + "element", ["created_at DESC", '"created_at" ASC', "created_at", "Created_At DESC"] +) +def test_sqlite_text_index_compares_clean(connection, element): + meta = _metadata(element) + meta.create_all(connection) + with _environment(connection, meta) as env: + assert compare_metadata(env.get_context(), meta) == [] + + +@pytest.mark.parametrize( + "build", + [lambda: sa.desc("created_at"), lambda: sa.desc(sa.column("created_at"))], + ids=["label", "column"], +) +def test_sqlite_desc_index_compares_clean(connection, build): + """``sa.desc("col")`` is a label expression; ``sa.desc(sa.column("col"))`` a column.""" + meta = _metadata(build()) + meta.create_all(connection) + with _environment(connection, meta) as env: + assert compare_metadata(env.get_context(), meta) == [] + + +def test_sqlite_text_index_without_filter_reproduces_remove_add(connection): + meta = _metadata() + meta.create_all(connection) + with _environment(connection, meta, filtered=False) as env: + diffs = compare_metadata(env.get_context(), meta) + assert [diff[0] for diff in diffs] == ["remove_index", "add_index"] + + +@pytest.mark.parametrize( + "changes", + [{"first": "email"}, {"unique": True}, {"element": "tenant_id DESC", "first": "created_at"}], + ids=["different-column", "unique", "column-order"], +) +def test_sqlite_text_index_real_changes_are_reported(connection, changes): + _metadata().create_all(connection) + target = _metadata(**changes) + with _environment(connection, target) as env: + diffs = compare_metadata(env.get_context(), target) + assert [diff[0] for diff in diffs] == ["remove_index", "add_index"] + + +@pytest.mark.parametrize("reflected", [False, True]) +@pytest.mark.parametrize( + "element", + [ + "created_at", + '"created_at"', + "created_at asc", + '"created_at" DESC NULLS LAST', + "created_at NULLS FIRST", + ], +) +def test_sqlite_text_index_filter_handles_both_comparison_directions( + connection, reflected, element +): + meta = _metadata(element) + modeled = _index(meta) + other = sa.MetaData() + table = meta.tables["record"].to_metadata(other) + table.indexes.clear() + database = sa.Index("ix", table.c.tenant_id, table.c.created_at, table.c.id) + obj, compare_to = (database, modeled) if reflected else (modeled, database) + with _environment(connection, meta): + assert make_include_object(meta)(obj, "ix", "index", reflected, compare_to) is False + + +@pytest.mark.parametrize("reflected", [False, True]) +def test_sqlite_real_expression_is_not_hidden(connection, reflected): + meta = _metadata("lower(email)") + meta.create_all(connection) + # SQLite cannot reflect this index, so exercise the callback pair directly. + modeled = _index(meta) + database = _index(_metadata("email")) + obj, compare_to = (database, modeled) if reflected else (modeled, database) + with _environment(connection, meta): + assert make_include_object(meta)(obj, "ix", "index", reflected, compare_to) is True + + +def test_sqlite_text_index_without_active_context_is_not_hidden(): + meta = _metadata() + assert ( + make_include_object(meta)( + _index(meta), "ix", "index", False, _index(_metadata("created_at")) + ) + is True + ) + + +def test_postgres_text_index_is_not_hidden(monkeypatch): + """Postgres reflects the ordering, so its comparison is left to Alembic.""" + meta = _metadata() + monkeypatch.setattr( + context, "get_context", lambda: SimpleNamespace(dialect=postgresql.dialect()) + ) + assert ( + make_include_object(meta)( + _index(meta), "ix", "index", False, _index(_metadata("created_at")) + ) + is True + ) + + +def _partial(where: str) -> sa.MetaData: + meta = sa.MetaData() + sa.Table( + "record", + meta, + sa.Column("id", sa.Integer, primary_key=True), + sa.Column("active", sa.Integer), + sa.Column("email", sa.String), + sa.Index("ix", sa.text("email DESC"), sqlite_where=sa.text(where)), + ) + return meta + + +def test_a_changed_partial_index_predicate_is_still_reported(connection): + """The guard compares columns only, so it must leave partial indexes alone.""" + _partial("active = 1").create_all(connection) + target = _partial("active = 0") + with _environment(connection, target) as env: + diffs = compare_metadata(env.get_context(), target) + assert [diff[0] for diff in diffs] == ["remove_index", "add_index"] diff --git a/modules/file_storage/file_storage/models.py b/modules/file_storage/file_storage/models.py index bdff62fa8..e43da2f82 100644 --- a/modules/file_storage/file_storage/models.py +++ b/modules/file_storage/file_storage/models.py @@ -66,7 +66,7 @@ class StoredFile(Base, AuditMixin, SoftDeleteMixin, MultiTenantMixin, table=True f"ix_{constants.TABLE_STORED_FILE}_tenant_browse", "tenant_id", "is_deleted", - sa.text("created_at DESC"), + sa.desc(sa.column("created_at")), "id", ), ) diff --git a/modules/file_storage/tests/files/browse/test_file_storage_browse_index_columns.py b/modules/file_storage/tests/files/browse/test_file_storage_browse_index_columns.py new file mode 100644 index 000000000..b99608f39 --- /dev/null +++ b/modules/file_storage/tests/files/browse/test_file_storage_browse_index_columns.py @@ -0,0 +1,40 @@ +"""The browse index is written with column constructs SQLite can reflect (GH #869). + +``sa.text("created_at DESC")`` and ``sa.desc("created_at")`` are both expressions +to Alembic, and SQLite reflects the index as plain columns, so every SQLite +autogenerate reported the index as changed. ``sa.desc(sa.column(...))`` compares +as a column. +""" + +import sqlalchemy as sa +from file_storage.models import StoredFile +from sqlalchemy.sql.elements import ColumnClause, UnaryExpression +from sqlalchemy.sql.operators import desc_op + +_INDEX = "ix_file_storage_stored_file_tenant_browse" + + +def _browse_index() -> sa.Index: + return next(i for i in StoredFile.__table__.indexes if i.name == _INDEX) + + +def test_every_element_is_a_column_or_a_descending_column(): + names = [] + for expr in _browse_index().expressions: + if isinstance(expr, UnaryExpression): + assert expr.modifier is desc_op + expr = expr.element + assert isinstance(expr, ColumnClause), f"{expr!r} is an expression to Alembic" + names.append(expr.name) + assert names == ["tenant_id", "is_deleted", "created_at", "id"] + assert isinstance(_browse_index().expressions[2], UnaryExpression) + + +def test_the_ddl_is_unchanged(): + """Same ``CREATE INDEX`` as revision ``f6c2d4e8b109``, so no migration is needed.""" + from sqlalchemy.dialects import postgresql, sqlite + from sqlalchemy.schema import CreateIndex + + for dialect in (sqlite.dialect(), postgresql.dialect()): + ddl = str(CreateIndex(_browse_index()).compile(dialect=dialect)) + assert ddl.endswith("(tenant_id, is_deleted, created_at DESC, id)"), ddl diff --git a/modules/users/tests/models/tokens/test_users_access_token_expires_at.py b/modules/users/tests/models/tokens/test_users_access_token_expires_at.py new file mode 100644 index 000000000..473f1767c --- /dev/null +++ b/modules/users/tests/models/tokens/test_users_access_token_expires_at.py @@ -0,0 +1,40 @@ +"""``expires_at`` round-trips as aware UTC on SQLite and uses AwareDateTime (GH #869).""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from simple_module_db.types import AwareDateTime +from sqlalchemy import select +from users.models import User, UserAccessToken + + +def test_expires_at_column_type_is_aware_datetime(): + assert isinstance(UserAccessToken.__table__.c.expires_at.type, AwareDateTime) + + +@pytest.mark.anyio +async def test_expires_at_round_trips_aware_utc(db_session): + user_id = uuid.uuid4() + db_session.add( + User( + id=user_id, + email="expires@example.com", + hashed_password="hashed", + is_active=True, + is_superuser=False, + is_verified=True, + ) + ) + await db_session.commit() + expires = datetime.now(UTC).replace(microsecond=0) + timedelta(hours=1) + db_session.add(UserAccessToken(token="b" * 43, user_id=user_id, expires_at=expires)) + await db_session.commit() + db_session.expire_all() + + row = (await db_session.execute(select(UserAccessToken))).scalar_one() + assert row.expires_at.tzinfo is not None + assert row.expires_at.utcoffset() == timedelta(0) + assert row.expires_at == expires diff --git a/modules/users/users/accounts/token_strategy.py b/modules/users/users/accounts/token_strategy.py index 3ef4ce682..ac7ecfefe 100644 --- a/modules/users/users/accounts/token_strategy.py +++ b/modules/users/users/accounts/token_strategy.py @@ -45,8 +45,8 @@ def token_is_live(access_token: UserAccessToken, user: Any, now: datetime | None now = now or datetime.now(UTC) expires_at = access_token.expires_at if expires_at is not None: - # SQLite hands back naive datetimes for a TIMESTAMPAware column; treat - # them as UTC rather than raising on the comparison. + # ``expires_at`` is ``AwareDateTime`` and reads back aware; this guard + # only covers rows or test doubles that still carry a naive value. if expires_at.tzinfo is None: expires_at = expires_at.replace(tzinfo=UTC) if expires_at <= now: diff --git a/modules/users/users/models/access_token.py b/modules/users/users/models/access_token.py index 2d7caa181..73845e10c 100644 --- a/modules/users/users/models/access_token.py +++ b/modules/users/users/models/access_token.py @@ -10,6 +10,7 @@ from typing import ClassVar from fastapi_users_db_sqlalchemy.generics import GUID, TIMESTAMPAware, now_utc +from simple_module_db.types import AwareDateTime from sqlmodel import Field from users.models._base import Base @@ -43,7 +44,7 @@ class UserAccessToken(Base, table=True): # ty: ignore[unsupported-base] ) expires_at: datetime = Field( default_factory=_default_expires_at, - sa_type=TIMESTAMPAware(timezone=True), + sa_type=AwareDateTime(), index=True, ) """When this particular row stops authenticating.