From 1aacfa1d441079022e6352e2b1b8c1dd3956f9ea Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 31 Oct 2026 00:01:47 +0100 Subject: [PATCH 1/5] feat: module data-compatibility checks and smpy migrations prepare (#711) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 5 + docs/.vitepress/config.ts | 1 + docs/database/migrations.md | 23 +++ docs/framework/lifecycle.md | 12 ++ docs/guide/upgrading.md | 75 +++++++ docs/reference/diagnostic-codes.md | 1 + framework/cli/simple_module_cli/cli.py | 10 + .../migration_recipes/mail_settings.py | 48 +++++ .../migration_recipes/users_tokens.py | 34 +++ .../cli/simple_module_cli/migrations_cmd.py | 66 ++++++ .../templates/host/README.md.tpl | 18 ++ .../tests/test_cli_prepare_data_migrations.py | 195 ++++++++++++++++++ .../tests/test_cli_prepare_data_postgres.py | 96 +++++++++ framework/core/simple_module_core/__init__.py | 2 + framework/core/simple_module_core/__main__.py | 2 + .../core/simple_module_core/_data_doctor.py | 23 +++ .../simple_module_core/data_compatibility.py | 32 +++ framework/core/simple_module_core/module.py | 12 +- .../test_core_data_compatibility_doctor.py | 45 ++++ .../db/simple_module_db/data_compatibility.py | 52 +++++ .../test_db_data_compatibility_doctor.py | 61 ++++++ .../_deferred_startup.py | 9 +- .../simple_module_hosting/_lifespan.py | 6 + .../data_compatibility.py | 30 +++ .../simple_module_hosting/setup_gate.py | 11 +- .../tests/test_hosting_data_compatibility.py | 130 ++++++++++++ .../tests/test_lifespan_deferred_startup.py | 5 +- modules/mail/mail/constants.py | 11 + modules/mail/mail/data_compatibility.py | 38 ++++ modules/mail/mail/module.py | 8 +- .../tests/test_mail_data_compatibility.py | 38 ++++ .../tests/test_users_data_compatibility.py | 49 +++++ modules/users/users/constants.py | 3 + modules/users/users/data_compatibility.py | 57 +++++ modules/users/users/module.py | 8 +- 35 files changed, 1210 insertions(+), 6 deletions(-) create mode 100644 docs/guide/upgrading.md create mode 100644 framework/cli/simple_module_cli/migration_recipes/mail_settings.py create mode 100644 framework/cli/simple_module_cli/migration_recipes/users_tokens.py create mode 100644 framework/cli/simple_module_cli/migrations_cmd.py create mode 100644 framework/cli/tests/test_cli_prepare_data_migrations.py create mode 100644 framework/cli/tests/test_cli_prepare_data_postgres.py create mode 100644 framework/core/simple_module_core/_data_doctor.py create mode 100644 framework/core/simple_module_core/data_compatibility.py create mode 100644 framework/core/tests/test_core_data_compatibility_doctor.py create mode 100644 framework/db/simple_module_db/data_compatibility.py create mode 100644 framework/db/tests/test_db_data_compatibility_doctor.py create mode 100644 framework/hosting/simple_module_hosting/data_compatibility.py create mode 100644 framework/hosting/tests/test_hosting_data_compatibility.py create mode 100644 modules/mail/mail/data_compatibility.py create mode 100644 modules/mail/tests/test_mail_data_compatibility.py create mode 100644 modules/users/tests/test_users_data_compatibility.py create mode 100644 modules/users/users/data_compatibility.py diff --git a/CHANGELOG.md b/CHANGELOG.md index ac784312d..6b1d5c585 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,11 @@ All notable changes to this project are documented in this file. The format is b ones) above `direct_upload_threshold_bytes` now use bounded multipart, including unknown-length streams, with size/part bounds and abort on failure; the S3 credentials need `s3:AbortMultipartUpload` (#708). +- **Behaviour change:** module data compatibility checks run before startup and in + Doctor (`SM033`); legacy bearer-token storage refuses boot outside development, + while obsolete mail settings warn (#711). `smpy migrations prepare` writes + frozen host-owned recipes for mail namespace changes (`d7b3a9e5c1f2`) and token + invalidation/column changes (`a626b1c8d023`), without applying migrations. - Users auth emails support ordered host template directories through the restart-required DB setting `mail_template_dirs`, with per-file bundled fallback and optional autoescaped HTML alternatives (#712). diff --git a/docs/.vitepress/config.ts b/docs/.vitepress/config.ts index 05db7ba50..74fb48ef7 100644 --- a/docs/.vitepress/config.ts +++ b/docs/.vitepress/config.ts @@ -92,6 +92,7 @@ export default defineConfig({ { text: "Quickstart", link: "/guide/quickstart" }, { text: "Project structure", link: "/guide/project-structure" }, { text: "Configuration", link: "/guide/configuration" }, + { text: "Upgrading a host", link: "/guide/upgrading" }, { text: "Your first module", link: "/guide/first-module" }, { text: "OpenAPI & clients", link: "/guide/openapi-clients" }, ], diff --git a/docs/database/migrations.md b/docs/database/migrations.md index c1f54896e..f8555c8a5 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -8,6 +8,29 @@ All migrations live in `host/migrations/versions/` — **not** in module package - **Autogenerate sees everything.** `host/migrations/env.py` calls `build_module_metadata()` to union every installed module's `MetaData`. Autogenerate diffs the DB against that union and writes one migration covering all changes. - **Operators run one command.** `make migrate` is the only target. No "did you also run `orders/migrate`?" footgun. +## Module data upgrades in consuming hosts (#711) + +Scaffolded hosts own their migration graphs. A schema-only baseline generated +from current models does not include this repository's historical data changes. +Before deploying upgraded packages, run `make doctor` or `uv run smpy doctor`. +Semantic module checks (`SM033`) recognize equivalent/manual ports independently +of upstream revision ids. + +`uv run smpy migrations prepare --head ` +copies a frozen, self-contained recipe into the host's own `migrations/versions`; +it never applies it. An explicit current head is required. The bundled ids are +`mail.settings_namespace_v1` (system settings move, target wins, matching +`d7b3a9e5c1f2`) and `users.hashed_bearer_tokens_v1` (invalidate credentials before +altering both token columns, matching `a626b1c8d023`). The first recipe for an +unlabelled module declares `mail`/`users`; existing labels are not redeclared. +Review the generated revision before applying it with `upgrade heads`. + +Boot checks run before module settings hydration/startup; token incompatibility +refuses boot outside development, while obsolete mail keys warn. Never-migrated +first runs defer checks until the wizard applies migrations. No automatic +credential deletion happens at startup. See the [scaffolded host upgrade +guide](../guide/upgrading.md) for the operator workflow and token sign-out risk. + ## The real graph The history is **not** a single linear chain. It has several roots and several heads, and `make migrate` runs `upgrade heads` for that reason. On `main` today: diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index 4349bad9e..d96d8b3a9 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -27,6 +27,7 @@ register_exception_handlers register_middleware register_routes register_admin_routes (only when meta.admin_view_prefix is set) +-- check_data_compatibility (async, before module settings hydration) -- on_startup (async, after middleware is installed) ``` @@ -350,6 +351,17 @@ def register_resource_types(self, registry: ResourceTypeRegistry) -> None: ) ``` +## `check_data_compatibility(db)` + +This async read-only hook returns a sequence of `DataCompatibilityIssue` SQLModel +DTOs (`upgrade_id`, `message`, `severity="warning"|"error"`), empty by default. +Use stable semantic ids and inspect actual schema/data state so hosts with +independently ported revisions pass. Checks run before module DB-settings +hydration and `on_startup`; errors refuse boot outside development. A never- +migrated first run defers checks until migrations have been applied. Doctor +reports the same issues as `SM033`. Checks must never clear credentials or apply +migrations; see [Migrations](../database/migrations.md). + ## `on_startup()` / `on_shutdown()` Async lifespan hooks that run after all modules are registered. diff --git a/docs/guide/upgrading.md b/docs/guide/upgrading.md new file mode 100644 index 000000000..7ad3f7713 --- /dev/null +++ b/docs/guide/upgrading.md @@ -0,0 +1,75 @@ +# Upgrading a scaffolded host + +A host generated by `smpy new` or `smpy create-host` owns its Alembic history. +Its initial revision reflects the models installed at generation time; it does +not import the framework repository's historical revisions. Updating packages +and running schema autogenerate cannot reproduce historical data transformations. + +## Check and prepare + +Back up the database and review the release notes before upgrading packages. +From the project root, with the host's Python environment active: + +```bash +uv run smpy doctor +uv run alembic -c host/alembic.ini heads +uv run smpy migrations prepare mail.settings_namespace_v1 --head +uv run smpy migrations prepare users.hashed_bearer_tokens_v1 --head users@head +``` + +Replace `` with an actual head in **your** graph. Use `users@head` +only if your graph already declares that label; otherwise pass the relevant host +head id. Preparing the mail revision may advance the same branch, so inspect +`heads` again before preparing the next revision. For flat hosts use +`alembic.ini` instead of `host/alembic.ini`. The preparation command detects both +layouts, or accepts `--config path/to/alembic.ini`. + +Preparation writes one self-contained revision into that host's +`migrations/versions`. It never connects to the database or applies the revision. +The frozen recipes ship in the CLI package, and generated revisions import only +Alembic and SQLAlchemy: future module/package upgrades cannot change their code. +The first prepared recipe for an unlabelled module declares `mail` or `users`; +existing labels are reused without redeclaration. `--head` is required and must +resolve to one current host head. No upstream roots are grafted onto your graph. + +## Review and apply + +Review and keep the generated revision in your application's version control. +Do not generate a token invalidation revision for a baseline that is already +compatible: applying it deliberately signs clients out. + +| Upgrade id | Effect | +|---|---| +| `mail.settings_namespace_v1` | Move system-scope `users.mailer` to `mail.backend` and the six `users.smtp_*` transport keys to `mail.*`. Existing target values win; stale source rows are removed. Tenant/user settings are untouched. Downgrade reverses the mapping with the same target-wins rule. | +| `users.hashed_bearer_tokens_v1` | Delete access and refresh credentials **before** changing both token columns to `String(64)`. Downgrade deletes credentials again and restores the legacy column types; it cannot recover secrets. | + +Stop every old application worker, including background workers, before applying +the token upgrade. Mixed old/new workers must not mint or use plaintext tokens +during this upgrade. Clients using bearer tokens or `sm_auth` must sign in again. +Other session cookies are unaffected. After reviewing and scheduling the upgrade: + +```bash +uv run alembic -c host/alembic.ini upgrade heads +uv run smpy doctor +``` + +Restart the application with the upgraded packages. Role provisioning remains an +idempotent startup step; it needs no prepared data revision. + +## Compatibility checks + +Modules expose read-only `check_data_compatibility(db)` hooks with semantic upgrade +ids. Boot runs them before module settings hydration and module startup. Doctor +(`make doctor` or `smpy doctor`) reports them as `SM033` and exits nonzero on errors. + +Unmoved system mail keys warn. Incompatible token columns or legacy token +representations refuse boot outside development; development logs warnings. +Checks inspect state, so equivalent manual ports are accepted without requiring +upstream revision ids. A current schema-only baseline with empty token tables +passes and does not trigger credential deletion. + +A never-migrated database skips the initial check so the first-run wizard stays +reachable. Checks replay after wizard/out-of-band migrations before deferred +settings hydration/startup; an incompatibility keeps the setup gate closed. +This is detection and explicit preparation for the two bundled upgrades, not an +automatic migration system for arbitrary third-party module data. diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index f9b5ffc71..bc1c7f55f 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -41,6 +41,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM030` | WARNING at a dev boot; ERROR in `make doctor` | A module's `depends_on` names a module that is not loaded (not installed, left out of `SM_MODULES_ENABLED`, or dropped because another auth provider was selected — `keycloak` drops `Users`). Outside development the boot raises `MissingDependencyError` listing every pair. | Install and enable the dependency, or leave the dependent module out. A relationship that is only about order belongs in `optional_depends_on`. See [Discovery](/framework/discovery#modulemeta). | | `SM031` | WARNING | An `/api` route has no permission, `TokenAllowed` or `Authenticated` gate anywhere in its dependency tree (include-level dependencies count) and no public-route match, so the schema cannot say who may call it. Needs the built app: reported at a **dev boot** (and in the Doctor screen), not by `make doctor`. | Add `Depends(RequiresPermission(...))`, or `Depends(Authenticated())` when any signed-in user may call it, or register it in `register_public_routes`. | | `SM032` | ERROR (fails boot in every environment) | A `ModuleBase.spa_mounts()` prefix overlaps a module's `route_prefix`/`view_prefix`/`admin_view_prefix`, a `static_mounts()` prefix, a framework path (`/api`, `/admin`, `/setup`, `/i18n`, `/health`, `/static`, `/modules`) or another SPA mount. The mount would shadow, or be shadowed by, that router. | Pick a prefix nothing else uses. See [Legacy SPA mounts](/framework/spa-mounts). | +| `SM033` | ERROR or WARNING | A module data compatibility check found a missing semantic upgrade (or the database could not be checked). Token schema/legacy credentials are errors; unmoved mail system keys warn. Errors refuse boot outside development. Never-migrated first runs defer checks. | Run the reported `smpy migrations prepare --head `, review the host revision and apply `upgrade heads`; see [Upgrading a host](/guide/upgrading). | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). diff --git a/framework/cli/simple_module_cli/cli.py b/framework/cli/simple_module_cli/cli.py index 30347b8c0..06dbedee4 100644 --- a/framework/cli/simple_module_cli/cli.py +++ b/framework/cli/simple_module_cli/cli.py @@ -23,6 +23,7 @@ from simple_module_cli.add_cmd import add_module, run_add from simple_module_cli.case import to_kebab_case from simple_module_cli.docker_cmd import docker_app +from simple_module_cli.migrations_cmd import migrations_app from simple_module_cli.module_cmd import module_app from simple_module_cli.new import new_project from simple_module_cli.openapi_cmd import openapi_app @@ -49,6 +50,15 @@ app.add_typer(skills_app, name="skills") app.add_typer(module_app, name="module") app.add_typer(openapi_app, name="openapi") +app.add_typer(migrations_app, name="migrations") + + +@app.command("doctor") +def doctor() -> None: + """Check installed modules and their database compatibility.""" + from simple_module_core.__main__ import main as run_doctor + + raise typer.Exit(run_doctor()) @app.command("create-host") diff --git a/framework/cli/simple_module_cli/migration_recipes/mail_settings.py b/framework/cli/simple_module_cli/migration_recipes/mail_settings.py new file mode 100644 index 000000000..2233f4a77 --- /dev/null +++ b/framework/cli/simple_module_cli/migration_recipes/mail_settings.py @@ -0,0 +1,48 @@ +"""Frozen system mail-settings move from d7b3a9e5c1f2 (#711). + +An existing target value wins; the obsolete source row is removed. +""" + +import sqlalchemy as sa +from alembic import op + +_TABLE = "settings_setting" +_SCOPE_SYSTEM = "system" +_SYSTEM_SCOPE_ID = "" + +_KEY_PAIRS = ( + ("users.mailer", "mail.backend"), + *((f"users.smtp_{f}", f"mail.smtp_{f}") for f in ("host", "port", "username", "password")), + ("users.smtp_from", "mail.smtp_from"), + ("users.smtp_tls", "mail.smtp_tls"), +) + + +def _rename(old: str, new: str) -> None: + scope = {"scope": _SCOPE_SYSTEM, "scope_id": _SYSTEM_SCOPE_ID, "old": old, "new": new} + bind = op.get_bind() + bind.execute( + sa.text( + f"DELETE FROM {_TABLE} WHERE scope = :scope AND scope_id = :scope_id AND key = :old" + f" AND EXISTS (SELECT 1 FROM {_TABLE} AS t WHERE t.scope = :scope" + " AND t.scope_id = :scope_id AND t.key = :new)" + ), + scope, + ) + bind.execute( + sa.text( + f"UPDATE {_TABLE} SET key = :new" + " WHERE scope = :scope AND scope_id = :scope_id AND key = :old" + ), + scope, + ) + + +def upgrade() -> None: + for old, new in _KEY_PAIRS: + _rename(old, new) + + +def downgrade() -> None: + for old, new in _KEY_PAIRS: + _rename(new, old) diff --git a/framework/cli/simple_module_cli/migration_recipes/users_tokens.py b/framework/cli/simple_module_cli/migration_recipes/users_tokens.py new file mode 100644 index 000000000..f96671982 --- /dev/null +++ b/framework/cli/simple_module_cli/migration_recipes/users_tokens.py @@ -0,0 +1,34 @@ +"""Frozen token-storage upgrade from a626b1c8d023 (#711). + +Upgrade and downgrade invalidate all access/refresh credentials. Stop all old +application workers before upgrading; clients must sign in again. +""" + +import sqlalchemy as sa +from alembic import op + + +def _clear_tokens() -> None: + for table in ("users_refresh_token", "users_access_token"): + op.execute(sa.table(table).delete()) + + +def upgrade() -> None: + _clear_tokens() + with op.batch_alter_table("users_refresh_token") as batch: + batch.alter_column( + "token", existing_type=sa.Uuid(), type_=sa.String(64), postgresql_using="token::text" + ) + with op.batch_alter_table("users_access_token") as batch: + batch.alter_column("token", existing_type=sa.String(43), type_=sa.String(64)) + + +def downgrade() -> None: + # Digests cannot be restored to UUIDs or bearer secrets. + _clear_tokens() + with op.batch_alter_table("users_refresh_token") as batch: + batch.alter_column( + "token", existing_type=sa.String(64), type_=sa.Uuid(), postgresql_using="token::uuid" + ) + with op.batch_alter_table("users_access_token") as batch: + batch.alter_column("token", existing_type=sa.String(64), type_=sa.String(43)) diff --git a/framework/cli/simple_module_cli/migrations_cmd.py b/framework/cli/simple_module_cli/migrations_cmd.py new file mode 100644 index 000000000..69f501b21 --- /dev/null +++ b/framework/cli/simple_module_cli/migrations_cmd.py @@ -0,0 +1,66 @@ +"""Prepare frozen framework upgrades in a consuming host's own Alembic graph.""" + +from __future__ import annotations + +import os +from importlib.resources import files +from pathlib import Path +from typing import Annotated +from uuid import uuid4 + +import typer + +migrations_app = typer.Typer(help="Prepare host-owned framework upgrade revisions.") +_RECIPES = { + "mail.settings_namespace_v1": ("mail", "mail_settings.py"), + "users.hashed_bearer_tokens_v1": ("users", "users_tokens.py"), +} + + +@migrations_app.command("prepare") +def prepare( + upgrade_id: Annotated[str, typer.Argument(help="Stable framework upgrade id.")], + head: Annotated[str, typer.Option("--head", help="Explicit host head or label@head.")], + config: Annotated[Path | None, typer.Option("--config", help="Host alembic.ini.")] = None, +) -> None: + """Write a self-contained revision; never connect to or migrate the database.""" + if upgrade_id not in _RECIPES: + raise typer.BadParameter(f"Unknown upgrade {upgrade_id!r}; choose {', '.join(_RECIPES)}") + try: + from alembic.config import Config + from alembic.script import ScriptDirectory + from alembic.util.exc import CommandError + except ImportError as exc: + raise typer.BadParameter( + "Run this command in the host environment with Alembic installed" + ) from exc + + root = Path(os.environ.get("SM_PROJECT_ROOT", Path.cwd())) + if config is None: + config = root / "alembic.ini" + if not config.is_file(): + config = root / "host/alembic.ini" + if not config.is_file(): + raise typer.BadParameter(f"Host Alembic config not found: {config}") + try: + script = ScriptDirectory.from_config(Config(str(config))) + parent = script.get_revision(head) + if parent is None or not parent.is_head: + raise typer.BadParameter("--head must resolve to one existing host head") + label, recipe = _RECIPES[upgrade_id] + labels = {label for rev in script.walk_revisions() for label in rev.branch_labels} + except CommandError as exc: + raise typer.BadParameter(str(exc)) from exc + + # Frozen source is copied, so future package changes cannot change this revision. + source = files("simple_module_cli").joinpath("migration_recipes", recipe).read_text() + revision = uuid4().hex[:12] + branch_labels = None if label in labels else (label,) + identifiers = ( + f"\nrevision = {revision!r}\ndown_revision = {parent.revision!r}\n" + f"branch_labels = {branch_labels!r}\ndepends_on = None\n" + ) + path = Path(script.versions) / f"{revision}_{upgrade_id.replace('.', '_')}.py" + with path.open("x") as output: + output.write(source + identifiers) + typer.echo(f"Prepared {upgrade_id}: {path}. Review it, then run alembic upgrade heads.") diff --git a/framework/cli/simple_module_cli/templates/host/README.md.tpl b/framework/cli/simple_module_cli/templates/host/README.md.tpl index 71922d00f..17a3a93b7 100644 --- a/framework/cli/simple_module_cli/templates/host/README.md.tpl +++ b/framework/cli/simple_module_cli/templates/host/README.md.tpl @@ -64,3 +64,21 @@ Modules declare `requires_framework` in their `Meta`. If a module can't work with your installed `simple_module_core` version, the host refuses to boot with `FrameworkVersionError` listing the offending modules. Upgrade or pin as appropriate. + + +Your host owns its Alembic history: package upgrades do not import framework data +migrations. Run `uv run smpy doctor` after upgrading packages. For a reported +upgrade, prepare a revision against an explicit head in this host's graph: + +```bash +uv run smpy migrations prepare mail.settings_namespace_v1 --head +``` + +The CLI detects `alembic.ini` or `host/alembic.ini` (`--config` overrides it). +Preparation only writes a frozen, self-contained revision into this host's +`migrations/versions`; review it before running `alembic upgrade heads`. +`users.hashed_bearer_tokens_v1` deletes access/refresh credentials before changing +column types: stop all old workers first and expect clients to sign in again. +Fresh current baselines need neither recipe. Boot warns about unmoved mail keys +and refuses incompatible token storage outside development. Role seeding stays +an idempotent startup step. See the framework's scaffolded host upgrade guide. diff --git a/framework/cli/tests/test_cli_prepare_data_migrations.py b/framework/cli/tests/test_cli_prepare_data_migrations.py new file mode 100644 index 000000000..8f5c4c2dc --- /dev/null +++ b/framework/cli/tests/test_cli_prepare_data_migrations.py @@ -0,0 +1,195 @@ +"""Prepared revisions belong to the host and execute without module imports (#711).""" + +import importlib.util + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from alembic.script import ScriptDirectory +from simple_module_cli.cli import app +from typer.testing import CliRunner + + +def _host(tmp_path, monkeypatch): + monkeypatch.delenv("SM_PROJECT_ROOT", raising=False) + host = tmp_path / "host" + versions = host / "migrations/versions" + versions.mkdir(parents=True) + (host / "alembic.ini").write_text("[alembic]\nscript_location = %(here)s/migrations\n") + (versions / "local.py").write_text( + "revision = 'local'\ndown_revision = None\nbranch_labels = None\n" + ) + monkeypatch.chdir(tmp_path) + return host, versions + + +def _prepare(tmp_path, monkeypatch, upgrade): + host, versions = _host(tmp_path, monkeypatch) + result = CliRunner().invoke(app, ["migrations", "prepare", upgrade, "--head", "local"]) + assert result.exit_code == 0, result.output + generated = [p for p in versions.glob("*.py") if p.name != "local.py"] + assert len(generated) == 1 + spec = importlib.util.spec_from_file_location("prepared_revision", generated[0]) + revision = importlib.util.module_from_spec(spec) + spec.loader.exec_module(revision) + from alembic.config import Config + + script = ScriptDirectory.from_config(Config(str(host / "alembic.ini"))) + assert script.get_revision(revision.revision).down_revision == "local" + return revision + + +def test_cli_prepared_mail_revision_target_wins_and_roundtrips(tmp_path, monkeypatch): + revision = _prepare(tmp_path, monkeypatch, "mail.settings_namespace_v1") + assert revision.branch_labels == ("mail",) + engine = sa.create_engine("sqlite://") + try: + with engine.begin() as conn: + conn.execute( + sa.text( + "CREATE TABLE settings_setting " + "(scope TEXT, scope_id TEXT, key TEXT, value TEXT)" + ) + ) + rows = [ + ("system", "", "users.mailer", "smtp"), + ("system", "", "users.smtp_host", "old"), + ("system", "", "mail.smtp_host", "new"), + ("tenant", "one", "users.mailer", "untouched"), + ] + for scope, scope_id, key, value in rows: + conn.execute( + sa.text("INSERT INTO settings_setting VALUES (:s, :i, :k, :v)"), + {"s": scope, "i": scope_id, "k": key, "v": value}, + ) + with Operations.context(MigrationContext.configure(conn)): + revision.upgrade() + revision.upgrade() + assert conn.execute( + sa.text("SELECT key, value FROM settings_setting ORDER BY key") + ).all() == [ + ("mail.backend", "smtp"), + ("mail.smtp_host", "new"), + ("users.mailer", "untouched"), + ] + revision.downgrade() + assert ( + conn.execute( + sa.text("SELECT value FROM settings_setting WHERE key='users.smtp_host'") + ).scalar() + == "new" + ) + finally: + engine.dispose() + + +def test_cli_prepared_token_revision_invalidates_before_altering(tmp_path, monkeypatch): + revision = _prepare(tmp_path, monkeypatch, "users.hashed_bearer_tokens_v1") + assert revision.branch_labels == ("users",) + engine = sa.create_engine("sqlite://") + try: + with engine.begin() as conn: + conn.execute(sa.text("CREATE TABLE users_refresh_token (token CHAR(32) PRIMARY KEY)")) + conn.execute(sa.text("CREATE TABLE users_access_token (token VARCHAR(43) PRIMARY KEY)")) + for name in ["refresh", "access"]: + conn.execute(sa.text(f"INSERT INTO users_{name}_token VALUES ('plaintext')")) + with Operations.context(MigrationContext.configure(conn)): + revision.upgrade() + for name in ["refresh", "access"]: + table = f"users_{name}_token" + assert conn.scalar(sa.text(f"SELECT count(*) FROM {table}")) == 0 + assert sa.inspect(conn).get_columns(table)[0]["type"].length == 64 + conn.execute(sa.text(f"INSERT INTO {table} VALUES ('{'a' * 64}')")) + revision.downgrade() + for name, length in [("refresh", 32), ("access", 43)]: + table = f"users_{name}_token" + assert conn.scalar(sa.text(f"SELECT count(*) FROM {table}")) == 0 + assert sa.inspect(conn).get_columns(table)[0]["type"].length == length + finally: + engine.dispose() + + +@pytest.mark.parametrize( + "args", + [ + ["unknown", "--head", "local"], + ["mail.settings_namespace_v1"], + ["mail.settings_namespace_v1", "--head", "missing"], + ], +) +def test_cli_prepare_rejects_invalid_input_without_writing(tmp_path, monkeypatch, args): + _, versions = _host(tmp_path, monkeypatch) + result = CliRunner().invoke(app, ["migrations", "prepare", *args]) + assert result.exit_code != 0 + assert [p.name for p in versions.glob("*.py")] == ["local.py"] + + +def test_cli_prepare_flat_host_reuses_label_and_attaches_explicit_head(tmp_path, monkeypatch): + host, versions = _host(tmp_path, monkeypatch) + (versions / "local.py").write_text( + "revision = 'local'\ndown_revision = None\nbranch_labels = ('mail',)\n" + ) + monkeypatch.chdir(host) + result = CliRunner().invoke( + app, ["migrations", "prepare", "mail.settings_namespace_v1", "--head", "mail@head"] + ) + assert result.exit_code == 0, result.output + from alembic.config import Config + + script = ScriptDirectory.from_config(Config(str(host / "alembic.ini"))) + head = script.get_revision("mail@head") + assert head.down_revision == "local" + assert head.module.branch_labels is None + assert not list(tmp_path.rglob("*.db")) + + +def test_cli_prepare_rejects_ambiguous_heads_and_non_heads(tmp_path, monkeypatch): + _, versions = _host(tmp_path, monkeypatch) + (versions / "second.py").write_text( + "revision = 'second'\ndown_revision = 'local'\nbranch_labels = None\n" + ) + (versions / "third.py").write_text( + "revision = 'third'\ndown_revision = None\nbranch_labels = None\n" + ) + for head in ["heads", "head", "base", "local"]: + result = CliRunner().invoke( + app, ["migrations", "prepare", "mail.settings_namespace_v1", "--head", head] + ) + assert result.exit_code != 0, result.output + assert len(list(versions.glob("*.py"))) == 3 + + +@pytest.mark.parametrize( + "upgrade_id", ["mail.settings_namespace_v1", "users.hashed_bearer_tokens_v1"] +) +def test_cli_prepared_recipe_runs_with_framework_imports_blocked(tmp_path, monkeypatch, upgrade_id): + import subprocess + import sys + + _prepare(tmp_path, monkeypatch, upgrade_id) + generated = next( + (tmp_path / "host/migrations/versions").glob(f"*_{upgrade_id.replace('.', '_')}.py") + ) + code = """ +import importlib.abc +import runpy +import sys +class BlockFramework(importlib.abc.MetaPathFinder): + def find_spec(self, fullname, path=None, target=None): + if fullname.startswith(('simple_module_', 'mail', 'users')): + raise ImportError(fullname) +sys.meta_path.insert(0, BlockFramework()) +namespace = runpy.run_path(sys.argv[1]) +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +with sa.create_engine('sqlite://').begin() as conn: + conn.execute(sa.text('CREATE TABLE settings_setting (scope TEXT, scope_id TEXT, key TEXT)')) + conn.execute(sa.text('CREATE TABLE users_refresh_token (token CHAR(32) PRIMARY KEY)')) + conn.execute(sa.text('CREATE TABLE users_access_token (token VARCHAR(43) PRIMARY KEY)')) + with Operations.context(MigrationContext.configure(conn)): + namespace['upgrade']() + namespace['downgrade']() +""" + subprocess.run([sys.executable, "-I", "-c", code, str(generated)], check=True, timeout=15) diff --git a/framework/cli/tests/test_cli_prepare_data_postgres.py b/framework/cli/tests/test_cli_prepare_data_postgres.py new file mode 100644 index 000000000..9a1a7a245 --- /dev/null +++ b/framework/cli/tests/test_cli_prepare_data_postgres.py @@ -0,0 +1,96 @@ +"""Frozen recipes and semantic recognition against PostgreSQL (#711).""" + +import runpy + +import pytest +import sqlalchemy as sa +from alembic.migration import MigrationContext +from alembic.operations import Operations +from simple_module_cli.cli import app +from typer.testing import CliRunner + +pytestmark = pytest.mark.postgres + + +def _revision(tmp_path, monkeypatch, upgrade): + monkeypatch.delenv("SM_PROJECT_ROOT", raising=False) + versions = tmp_path / "migrations/versions" + versions.mkdir(parents=True) + (tmp_path / "alembic.ini").write_text("[alembic]\nscript_location = %(here)s/migrations\n") + (versions / "local.py").write_text( + "revision = 'local'\ndown_revision = None\nbranch_labels = None\n" + ) + monkeypatch.chdir(tmp_path) + result = CliRunner().invoke(app, ["migrations", "prepare", upgrade, "--head", "local"]) + assert result.exit_code == 0, result.output + return runpy.run_path(str(next(p for p in versions.glob("*.py") if p.name != "local.py"))) + + +async def test_cli_postgres_token_recipe_and_equivalent_port(db_session, tmp_path, monkeypatch): + from users.module import UsersModule + + revision = _revision(tmp_path, monkeypatch, "users.hashed_bearer_tokens_v1") + conn = await db_session.connection() + await conn.execute( + sa.text( + "CREATE TEMP TABLE users_access_token (token VARCHAR(43) PRIMARY KEY) ON COMMIT DROP" + ) + ) + await conn.execute( + sa.text("CREATE TEMP TABLE users_refresh_token (token UUID PRIMARY KEY) ON COMMIT DROP") + ) + await conn.execute(sa.text("INSERT INTO users_access_token VALUES ('plaintext')")) + await conn.execute( + sa.text("INSERT INTO users_refresh_token VALUES ('12345678-1234-1234-1234-123456789abc')") + ) + assert (await UsersModule().check_data_compatibility(db_session))[0].severity == "error" + + def apply(sync, direction): + with Operations.context(MigrationContext.configure(sync)): + revision[direction]() + + await conn.run_sync(apply, "upgrade") + assert await UsersModule().check_data_compatibility(db_session) == [] + for name in ["refresh", "access"]: + assert await conn.scalar(sa.text(f"SELECT count(*) FROM users_{name}_token")) == 0 + await conn.execute( + sa.text(f"INSERT INTO users_{name}_token VALUES (:token)"), {"token": "a" * 64} + ) + assert await UsersModule().check_data_compatibility(db_session) == [] + await conn.run_sync(apply, "downgrade") + for name in ["refresh", "access"]: + assert await conn.scalar(sa.text(f"SELECT count(*) FROM users_{name}_token")) == 0 + assert (await UsersModule().check_data_compatibility(db_session))[ + 0 + ].upgrade_id == "users.hashed_bearer_tokens_v1" + + +async def test_cli_postgres_mail_target_wins(db_session, tmp_path, monkeypatch): + from mail.module import MailModule + + revision = _revision(tmp_path, monkeypatch, "mail.settings_namespace_v1") + conn = await db_session.connection() + await conn.execute( + sa.text( + "CREATE TEMP TABLE settings_setting " + "(scope TEXT, scope_id TEXT, key TEXT, value TEXT) ON COMMIT DROP" + ) + ) + await conn.execute( + sa.text( + "INSERT INTO settings_setting VALUES " + "('system', '', 'users.smtp_host', 'old'), " + "('system', '', 'mail.smtp_host', 'new')" + ) + ) + assert (await MailModule().check_data_compatibility(db_session))[0].severity == "warning" + + def upgrade(sync): + with Operations.context(MigrationContext.configure(sync)): + revision["upgrade"]() + + await conn.run_sync(upgrade) + assert await MailModule().check_data_compatibility(db_session) == [] + assert (await conn.execute(sa.text("SELECT key, value FROM settings_setting"))).all() == [ + ("mail.smtp_host", "new") + ] diff --git a/framework/core/simple_module_core/__init__.py b/framework/core/simple_module_core/__init__.py index adb670e18..e13d0a02e 100644 --- a/framework/core/simple_module_core/__init__.py +++ b/framework/core/simple_module_core/__init__.py @@ -4,6 +4,7 @@ from simple_module_core.body_limits import BodyLimitRegistry from simple_module_core.cors import CorsPolicy, CorsPolicyRegistry from simple_module_core.csp import CspSourceError, CspSourceRegistry +from simple_module_core.data_compatibility import DataCompatibilityIssue from simple_module_core.design_packs import DesignPack, DesignPackRegistry from simple_module_core.diagnostics import ( DiagnosticLevel, @@ -78,6 +79,7 @@ "CorsPolicyRegistry", "CspSourceError", "CspSourceRegistry", + "DataCompatibilityIssue", "DesignPack", "DesignPackRegistry", "DiagnosticLevel", diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 6c4373100..7104a642f 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -24,6 +24,7 @@ import sys from pathlib import Path +from simple_module_core._data_doctor import data_compatibility_diagnostics from simple_module_core.diagnostics import ( Diagnostic, DiagnosticLevel, @@ -182,6 +183,7 @@ def main() -> int: database_dialect=_database_dialect(), ) diagnostics.extend(override_findings) + diagnostics.extend(data_compatibility_diagnostics(modules)) # A missing dependency only warns at a dev boot, but production refuses # to start with it (GH #501), so doctor reports it as what it will be. for finding in diagnostics: diff --git a/framework/core/simple_module_core/_data_doctor.py b/framework/core/simple_module_core/_data_doctor.py new file mode 100644 index 000000000..85a263bbb --- /dev/null +++ b/framework/core/simple_module_core/_data_doctor.py @@ -0,0 +1,23 @@ +"""Optional database diagnostics for Doctor without requiring a host app import.""" + +import asyncio +import importlib.util +import os +from collections.abc import Sequence + +from simple_module_core.diagnostics import Diagnostic +from simple_module_core.module import ModuleBase + + +def data_compatibility_diagnostics(modules: Sequence[ModuleBase]) -> list[Diagnostic]: + # Core-only installations have no database runtime to inspect. + if importlib.util.find_spec("simple_module_db") is None: + return [] + if all( + type(mod).check_data_compatibility is ModuleBase.check_data_compatibility for mod in modules + ): + return [] + from simple_module_db.data_compatibility import database_compatibility_diagnostics + + url = os.environ.get("SM_DATABASE_URL", "sqlite+aiosqlite:///./app.db") + return asyncio.run(database_compatibility_diagnostics(modules, url)) diff --git a/framework/core/simple_module_core/data_compatibility.py b/framework/core/simple_module_core/data_compatibility.py new file mode 100644 index 000000000..c3d58bb2f --- /dev/null +++ b/framework/core/simple_module_core/data_compatibility.py @@ -0,0 +1,32 @@ +"""Semantic, read-only checks for upgrades in host-owned migration histories.""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass +from typing import TYPE_CHECKING, Any, Literal + +if TYPE_CHECKING: + from simple_module_core.module import ModuleBase + + +@dataclass(frozen=True, kw_only=True) +class DataCompatibilityIssue: + """An outstanding module upgrade, independent of Alembic revision ids.""" + + upgrade_id: str + message: str + severity: Literal["warning", "error"] + + def remediation(self) -> str: + return f"smpy migrations prepare {self.upgrade_id} --head " + + +async def collect_data_compatibility( + modules: Sequence[ModuleBase], db: Any +) -> list[DataCompatibilityIssue]: + """Read module state without applying migrations or changing credentials.""" + issues = [] + for module in modules: + issues.extend(await module.check_data_compatibility(db)) + return issues diff --git a/framework/core/simple_module_core/module.py b/framework/core/simple_module_core/module.py index b09a0e2d6..c2b9374cf 100644 --- a/framework/core/simple_module_core/module.py +++ b/framework/core/simple_module_core/module.py @@ -3,10 +3,12 @@ from __future__ import annotations from abc import ABC +from collections.abc import Sequence from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from simple_module_core._request_hooks import RequestHooksMixin +from simple_module_core.data_compatibility import DataCompatibilityIssue from simple_module_core.module_meta import ModuleMeta if TYPE_CHECKING: @@ -226,6 +228,14 @@ def db_extensions(self) -> list[str]: # ── Lifecycle ───────────────────────────────────────────── + async def check_data_compatibility(self, db: Any) -> Sequence[DataCompatibilityIssue]: + """Read-only upgrade checks before settings hydration and module startup. + + Report semantic state, never demand an upstream Alembic revision id: + scaffolded hosts own their histories and may port an equivalent step. + """ + return () + async def on_startup(self, app: FastAPI) -> None: """Called after all modules are registered, during app startup.""" diff --git a/framework/core/tests/test_core_data_compatibility_doctor.py b/framework/core/tests/test_core_data_compatibility_doctor.py new file mode 100644 index 000000000..235ff151f --- /dev/null +++ b/framework/core/tests/test_core_data_compatibility_doctor.py @@ -0,0 +1,45 @@ +"""Both Doctor entry points report upgrade ids and return a failing status (#711).""" + +import pytest +import sqlalchemy as sa +from simple_module_core import ModuleBase, ModuleMeta +from simple_module_core import __main__ as doctor +from simple_module_core.data_compatibility import DataCompatibilityIssue + + +@pytest.mark.parametrize("entrypoint", ["make", "smpy"]) +def test_core_doctor_reports_semantic_data_errors(tmp_path, monkeypatch, capsys, entrypoint): + path = tmp_path / "doctor.db" + engine = sa.create_engine(f"sqlite:///{path}") + with engine.begin() as conn: + conn.execute(sa.text("CREATE TABLE alembic_version (version_num VARCHAR(32))")) + conn.execute(sa.text("INSERT INTO alembic_version VALUES ('local')")) + engine.dispose() + + class Module(ModuleBase): + meta = ModuleMeta(name="Example") + + async def check_data_compatibility(self, db): + assert await db.scalar(sa.text("SELECT version_num FROM alembic_version")) == "local" + return [ + DataCompatibilityIssue( + upgrade_id="example.v1", message="Upgrade required", severity="error" + ) + ] + + monkeypatch.setenv("SM_DATABASE_URL", f"sqlite+aiosqlite:///{path}") + monkeypatch.setattr(doctor, "discover_modules", lambda strict: [Module()]) + monkeypatch.setattr(doctor, "parse_dotenv", dict) + monkeypatch.setattr(doctor, "run_diagnostics", lambda *a, **kw: []) + if entrypoint == "make": + assert doctor.main() == 1 + output = capsys.readouterr().err + else: + from simple_module_cli.cli import app + from typer.testing import CliRunner + + result = CliRunner().invoke(app, ["doctor"]) + assert result.exit_code == 1 + output = result.output + assert "example.v1" in output + assert "smpy migrations prepare example.v1" in output diff --git a/framework/db/simple_module_db/data_compatibility.py b/framework/db/simple_module_db/data_compatibility.py new file mode 100644 index 000000000..7adb20f55 --- /dev/null +++ b/framework/db/simple_module_db/data_compatibility.py @@ -0,0 +1,52 @@ +"""Database-backed Doctor diagnostics, separate from core's static checks.""" + +from collections.abc import Sequence + +from alembic.runtime.migration import MigrationContext +from simple_module_core.data_compatibility import collect_data_compatibility +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_core.module import ModuleBase +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +DATA_COMPATIBILITY_CODE = "SM033" + + +async def database_compatibility_diagnostics( + modules: Sequence[ModuleBase], database_url: str +) -> list[Diagnostic]: + engine = None + try: + engine = create_async_engine(database_url) + async with engine.connect() as conn: + migrated = await conn.run_sync( + lambda sync: bool(MigrationContext.configure(sync).get_current_heads()) + ) + if not migrated: + return [] + async with async_sessionmaker(engine)() as db: + issues = await collect_data_compatibility(modules, db) + return [ + Diagnostic( + level=DiagnosticLevel(issue.severity), + code=DATA_COMPATIBILITY_CODE, + module_name=issue.upgrade_id.split(".", 1)[0], + message=f"{issue.upgrade_id}: {issue.message}", + suggestion=issue.remediation(), + ) + for issue in issues + ] + except Exception: + # Connection exceptions can contain credentials; do not print them. + return [ + Diagnostic( + level=DiagnosticLevel.ERROR, + code=DATA_COMPATIBILITY_CODE, + module_name="", + message="Could not check module data compatibility.", + suggestion="Check database connectivity and apply pending host schema migrations, " + "then rerun doctor.", + ) + ] + finally: + if engine is not None: + await engine.dispose() diff --git a/framework/db/tests/test_db_data_compatibility_doctor.py b/framework/db/tests/test_db_data_compatibility_doctor.py new file mode 100644 index 000000000..804131381 --- /dev/null +++ b/framework/db/tests/test_db_data_compatibility_doctor.py @@ -0,0 +1,61 @@ +"""Doctor checks semantic upgrades without importing host code (#711).""" + +import pytest +from simple_module_core.data_compatibility import DataCompatibilityIssue +from simple_module_core.module import ModuleBase, ModuleMeta +from sqlalchemy import text +from sqlalchemy.ext.asyncio import create_async_engine + + +@pytest.mark.parametrize( + ("migrated", "severity", "expected"), + [ + (True, "error", "error"), + (True, "warning", "warning"), + (False, "error", None), + ], +) +async def test_db_doctor_data_findings(tmp_path, migrated, severity, expected): + from simple_module_db.data_compatibility import database_compatibility_diagnostics + + url = f"sqlite+aiosqlite:///{tmp_path / 'doctor.db'}" + engine = create_async_engine(url) + try: + async with engine.begin() as conn: + if migrated: + await conn.execute(text("CREATE TABLE alembic_version (version_num VARCHAR(32))")) + await conn.execute( + text("INSERT INTO alembic_version VALUES ('host_local_revision')") + ) + finally: + await engine.dispose() + + class Module(ModuleBase): + meta = ModuleMeta(name="Example") + + async def check_data_compatibility(self, db): + return [ + DataCompatibilityIssue( + upgrade_id="example.v1", message="Upgrade required", severity=severity + ) + ] + + findings = await database_compatibility_diagnostics([Module()], url) + if expected: + assert len(findings) == 1 + assert findings[0].level == expected + assert "example.v1" in findings[0].message + assert "smpy migrations prepare example.v1" in findings[0].suggestion + else: + assert findings == [] + + +async def test_db_doctor_reports_unavailable_database(tmp_path): + from simple_module_db.data_compatibility import database_compatibility_diagnostics + + findings = await database_compatibility_diagnostics( + [], f"sqlite+aiosqlite:///{tmp_path}/missing/db" + ) + assert len(findings) == 1 + assert findings[0].level == "error" + assert "could not" in findings[0].message.lower() diff --git a/framework/hosting/simple_module_hosting/_deferred_startup.py b/framework/hosting/simple_module_hosting/_deferred_startup.py index 1d685d793..a6cd14f52 100644 --- a/framework/hosting/simple_module_hosting/_deferred_startup.py +++ b/framework/hosting/simple_module_hosting/_deferred_startup.py @@ -45,8 +45,15 @@ async def run_deferred_startup(app: FastAPI) -> None: operator's out-of-band ``make migrate``) still finishes starting its modules. Serialised per app; a hook that fails again is logged and dropped rather than raised, because this runs after the migrations have already - committed and a 500 there would help nobody. + committed. Compatibility checks run first and still raise: an incompatible + security state must keep the setup gate closed. """ + pending_checks = getattr(app.state, "deferred_data_compatibility", None) + if pending_checks: + from simple_module_hosting.data_compatibility import check_data_compatibility + + await check_data_compatibility(app, pending_checks) + app.state.deferred_data_compatibility = [] deferred = get_deferred_startup(app) if not deferred: return diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index 3fa69119e..2e392afb5 100644 --- a/framework/hosting/simple_module_hosting/_lifespan.py +++ b/framework/hosting/simple_module_hosting/_lifespan.py @@ -22,6 +22,7 @@ from simple_module_hosting._deferred_startup import hydrate_settings_from_db from simple_module_hosting._route_guard import guard_included_routers +from simple_module_hosting.data_compatibility import check_data_compatibility from simple_module_hosting.migrations import migration_status from simple_module_hosting.setup_gate import STEP_MIGRATIONS @@ -114,6 +115,11 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: f"{app.state.migration['head_revision']!r}). Run: make migrate" ) + if app.state.migration.get("current_revision") is not None: + await check_data_compatibility(app, modules) + else: + app.state.deferred_data_compatibility = list(modules) + await hydrate_settings_from_db(app) # Behind head on a first run the tables a module's on_startup reads do diff --git a/framework/hosting/simple_module_hosting/data_compatibility.py b/framework/hosting/simple_module_hosting/data_compatibility.py new file mode 100644 index 000000000..a47e1fbdf --- /dev/null +++ b/framework/hosting/simple_module_hosting/data_compatibility.py @@ -0,0 +1,30 @@ +"""Enforce module data compatibility before hydrated settings and startup.""" + +import logging +from collections.abc import Sequence + +from fastapi import FastAPI +from simple_module_core.data_compatibility import collect_data_compatibility +from simple_module_core.module import ModuleBase + +logger = logging.getLogger(__name__) + + +class DataCompatibilityError(RuntimeError): + """An incompatible security state must also keep the setup gate closed.""" + + +async def check_data_compatibility(app: FastAPI, modules: Sequence[ModuleBase]) -> None: + try: + async with app.state.sm.db.session_factory() as db: + issues = await collect_data_compatibility(modules, db) + except Exception as exc: + raise DataCompatibilityError("Could not check module data compatibility") from exc + errors = [] + for issue in issues: + message = f"{issue.upgrade_id}: {issue.message} Prepare: {issue.remediation()}" + logger.warning(message) + if issue.severity == "error": + errors.append(message) + if errors and app.state.sm.settings.environment != "development": + raise DataCompatibilityError("Data compatibility upgrade required:\n" + "\n".join(errors)) diff --git a/framework/hosting/simple_module_hosting/setup_gate.py b/framework/hosting/simple_module_hosting/setup_gate.py index 2791dcb15..676968a1c 100644 --- a/framework/hosting/simple_module_hosting/setup_gate.py +++ b/framework/hosting/simple_module_hosting/setup_gate.py @@ -23,6 +23,7 @@ from simple_module_hosting import migrations from simple_module_hosting._deferred_startup import run_deferred_startup +from simple_module_hosting.data_compatibility import DataCompatibilityError logger = logging.getLogger(__name__) _migration_failure_warned = False @@ -74,6 +75,11 @@ async def _database_migrated(app) -> bool: if not status: return True if status.get("is_current", True): + if getattr(app.state, "deferred_data_compatibility", None): + try: + await run_deferred_startup(app) + except DataCompatibilityError: + return False return True try: @@ -90,9 +96,12 @@ async def _database_migrated(app) -> bool: if status["is_current"]: # Finish starting the modules whose on_startup needed these tables — # this worker may not be the one that ran the migrations. Never lets - # a failure there change the verdict, which only reports the schema. + # an ordinary startup-hook failure change the schema verdict. A failed + # data compatibility check must keep the gate closed. try: await run_deferred_startup(app) + except DataCompatibilityError: + return False except Exception: logger.exception("Replaying deferred on_startup hooks failed") return bool(status["is_current"]) diff --git a/framework/hosting/tests/test_hosting_data_compatibility.py b/framework/hosting/tests/test_hosting_data_compatibility.py new file mode 100644 index 000000000..5304c1931 --- /dev/null +++ b/framework/hosting/tests/test_hosting_data_compatibility.py @@ -0,0 +1,130 @@ +"""Compatibility errors gate established hosts before settings or startup (#711).""" + +from types import SimpleNamespace + +import pytest +from fastapi import FastAPI +from simple_module_core.module import ModuleBase, ModuleMeta +from simple_module_hosting._lifespan import build_lifespan +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + +@pytest.mark.parametrize( + ("environment", "severity", "never_migrated", "refuses"), + [ + ("production", "error", False, True), + ("testing", "error", False, True), + ("development", "error", False, False), + ("production", "warning", False, False), + ("production", "error", True, False), + ], +) +async def test_hosting_compatibility_before_hydration( + monkeypatch, environment, severity, never_migrated, refuses +): + from simple_module_core.data_compatibility import DataCompatibilityIssue + + events = [] + + class Module(ModuleBase): + meta = ModuleMeta(name="Example") + + async def check_data_compatibility(self, db): + events.append("check") + return [ + DataCompatibilityIssue( + upgrade_id="example.v1", message="Upgrade", severity=severity + ) + ] + + async def on_startup(self, app): + events.append("startup") + + async def status(engine): + return {"is_current": True, "current_revision": None if never_migrated else "local"} + + async def hydrate(app): + events.append("hydrate") + + monkeypatch.setattr("simple_module_hosting._lifespan.migration_status", status) + monkeypatch.setattr("simple_module_hosting._lifespan.hydrate_settings_from_db", hydrate) + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + app = FastAPI() + app.state.sm = SimpleNamespace( + db=SimpleNamespace(engine=engine, session_factory=async_sessionmaker(engine)), + settings=SimpleNamespace(environment=environment), + ) + if refuses: + with pytest.raises(RuntimeError, match=r"example\.v1"): + async with build_lifespan([Module()])(app): + pass + assert events == ["check"] + else: + async with build_lifespan([Module()])(app): + assert events == ( + ["hydrate", "startup"] if never_migrated else ["check", "hydrate", "startup"] + ) + + +async def test_hosting_deferred_checks_gate_replay_before_hydration(monkeypatch): + from simple_module_core.data_compatibility import DataCompatibilityIssue + from simple_module_hosting._deferred_startup import run_deferred_startup + + class Module(ModuleBase): + meta = ModuleMeta(name="Example") + + async def check_data_compatibility(self, db): + return [ + DataCompatibilityIssue(upgrade_id="example.v1", message="Upgrade", severity="error") + ] + + app = FastAPI() + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + app.state.sm = SimpleNamespace( + db=SimpleNamespace(engine=engine, session_factory=async_sessionmaker(engine)), + settings=SimpleNamespace(environment="production"), + ) + app.state.deferred_data_compatibility = [Module()] + app.state.deferred_startup = [] + try: + with pytest.raises(RuntimeError, match=r"example\.v1"): + await run_deferred_startup(app) + assert app.state.deferred_data_compatibility + finally: + await engine.dispose() + + +@pytest.mark.parametrize("check_raises", [False, True]) +async def test_hosting_setup_gate_keeps_incompatible_replay_closed(monkeypatch, check_raises): + from simple_module_core.data_compatibility import DataCompatibilityIssue + from simple_module_hosting.setup_gate import _database_migrated + + class Module(ModuleBase): + meta = ModuleMeta(name="Example") + + async def check_data_compatibility(self, db): + if check_raises: + raise RuntimeError("database unavailable") + return [ + DataCompatibilityIssue(upgrade_id="example.v1", message="Upgrade", severity="error") + ] + + app = FastAPI() + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + app.state.sm = SimpleNamespace( + db=SimpleNamespace(engine=engine, session_factory=async_sessionmaker(engine)), + settings=SimpleNamespace(environment="production"), + ) + app.state.migration = {"is_current": False, "current_revision": None} + app.state.deferred_data_compatibility = [Module()] + app.state.deferred_startup = [] + + async def status(engine): + return {"is_current": True, "current_revision": "local"} + + monkeypatch.setattr("simple_module_hosting.setup_gate.migrations.migration_status", status) + try: + assert await _database_migrated(app) is False + assert await _database_migrated(app) is False + finally: + await engine.dispose() diff --git a/framework/hosting/tests/test_lifespan_deferred_startup.py b/framework/hosting/tests/test_lifespan_deferred_startup.py index 1e03d7f9d..c7cd87beb 100644 --- a/framework/hosting/tests/test_lifespan_deferred_startup.py +++ b/framework/hosting/tests/test_lifespan_deferred_startup.py @@ -53,7 +53,10 @@ async def test_failing_hook_is_deferred_then_replayed_on_unmigrated_first_run(): async def capture(app_): seen["deferred"] = [m.meta.name for m in app_.state.deferred_startup] - with patch("simple_module_hosting._deferred_startup.hydrate_settings_from_db", AsyncMock()): + with ( + patch("simple_module_hosting._deferred_startup.hydrate_settings_from_db", AsyncMock()), + patch("simple_module_hosting.data_compatibility.check_data_compatibility", AsyncMock()), + ): status = {"is_current": False, "pending_count": 1} with ( patch( diff --git a/modules/mail/mail/constants.py b/modules/mail/mail/constants.py index aba3f76c5..c6448183e 100644 --- a/modules/mail/mail/constants.py +++ b/modules/mail/mail/constants.py @@ -22,3 +22,14 @@ #: Seconds per SMTP operation (aiosmtplib defaults to 60). A send can run in a #: request's after-commit hook, so a hung server must not hold it for a minute. SMTP_TIMEOUT_SECONDS = 15.0 + +UPGRADE_SETTINGS_NAMESPACE = "mail.settings_namespace_v1" +LEGACY_MAIL_KEYS = ( + "users.mailer", + "users.smtp_host", + "users.smtp_port", + "users.smtp_username", + "users.smtp_password", + "users.smtp_from", + "users.smtp_tls", +) diff --git a/modules/mail/mail/data_compatibility.py b/modules/mail/mail/data_compatibility.py new file mode 100644 index 000000000..3217c912c --- /dev/null +++ b/modules/mail/mail/data_compatibility.py @@ -0,0 +1,38 @@ +"""Detect system settings that still use the pre-mail-module namespace.""" + +from simple_module_core.data_compatibility import DataCompatibilityIssue +from simple_module_db.stored_settings import ( + SCOPE_SYSTEM, + SYSTEM_SCOPE_ID, + settings_table_clause, + settings_table_exists, +) +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from mail.constants import LEGACY_MAIL_KEYS, UPGRADE_SETTINGS_NAMESPACE + + +async def check_data_compatibility(db: AsyncSession) -> list[DataCompatibilityIssue]: + if not await settings_table_exists(db): + return [] + settings = settings_table_clause() + found = await db.scalar( + select(settings.c.key) + .where( + settings.c.scope == SCOPE_SYSTEM, + settings.c.scope_id == SYSTEM_SCOPE_ID, + settings.c.key.in_(LEGACY_MAIL_KEYS), + ) + .limit(1) + ) + if found is None: + return [] + return [ + DataCompatibilityIssue( + upgrade_id=UPGRADE_SETTINGS_NAMESPACE, + message="Obsolete system users.mailer/SMTP settings must move to mail.*; " + "target values win.", + severity="warning", + ) + ] diff --git a/modules/mail/mail/module.py b/modules/mail/mail/module.py index 4e707beeb..ce0f0976c 100644 --- a/modules/mail/mail/module.py +++ b/modules/mail/mail/module.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from simple_module_core.health import HealthCheck, HealthRegistry from simple_module_core.mail import install_mail_resolver, install_mail_sender @@ -12,6 +12,7 @@ if TYPE_CHECKING: from fastapi import FastAPI + from simple_module_core.data_compatibility import DataCompatibilityIssue from simple_module_core.events import EventBus @@ -65,6 +66,11 @@ async def _reinstall(event: SettingsReloaded) -> None: bus.subscribe(SettingsReloaded, _reinstall) + async def check_data_compatibility(self, db: Any) -> list[DataCompatibilityIssue]: + from mail.data_compatibility import check_data_compatibility + + return await check_data_compatibility(db) + async def on_startup(self, app: FastAPI) -> None: from mail.runtime import install_app_sender diff --git a/modules/mail/tests/test_mail_data_compatibility.py b/modules/mail/tests/test_mail_data_compatibility.py new file mode 100644 index 000000000..64df79312 --- /dev/null +++ b/modules/mail/tests/test_mail_data_compatibility.py @@ -0,0 +1,38 @@ +"""Only obsolete system mail keys require a host revision (#711).""" + +import pytest +from mail.module import MailModule +from sqlalchemy import text +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + + +@pytest.mark.parametrize( + ("scope", "scope_id", "key", "expected"), + [ + ("system", "", "users.smtp_password", True), + ("system", "", "users.mailer", True), + ("tenant", "one", "users.smtp_host", False), + ("system", "other", "users.mailer", False), + ("system", "", "mail.smtp_host", False), + ("system", "", "users.unrelated", False), + ], +) +async def test_mail_obsolete_system_keys(scope, scope_id, key, expected): + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + async with engine.begin() as conn: + await conn.execute( + text("CREATE TABLE settings_setting (scope TEXT, scope_id TEXT, key TEXT)") + ) + await conn.execute( + text("INSERT INTO settings_setting VALUES (:scope, :scope_id, :key)"), + {"scope": scope, "scope_id": scope_id, "key": key}, + ) + async with async_sessionmaker(engine)() as db: + issues = await MailModule().check_data_compatibility(db) + assert bool(issues) is expected + if expected: + assert issues[0].upgrade_id == "mail.settings_namespace_v1" + assert issues[0].severity == "warning" + finally: + await engine.dispose() diff --git a/modules/users/tests/test_users_data_compatibility.py b/modules/users/tests/test_users_data_compatibility.py new file mode 100644 index 000000000..ad482d952 --- /dev/null +++ b/modules/users/tests/test_users_data_compatibility.py @@ -0,0 +1,49 @@ +"""Semantic token checks accept current baselines and equivalent ports (#711).""" + +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine +from users.module import UsersModule + + +@pytest.mark.parametrize( + ("access_type", "refresh_type", "token", "incompatible"), + [ + ("VARCHAR(64)", "VARCHAR(64)", None, False), + ("VARCHAR(64)", "VARCHAR(64)", "a" * 64, False), + ("VARCHAR(43)", "CHAR(32)", None, True), + ("VARCHAR(64)", "VARCHAR(64)", "plaintext", True), + ("VARCHAR(64)", "VARCHAR(64)", "z" * 64, True), + ], +) +async def test_users_semantic_token_compatibility(access_type, refresh_type, token, incompatible): + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + async with engine.begin() as conn: + for name, kind in [("access", access_type), ("refresh", refresh_type)]: + await conn.execute(text(f"CREATE TABLE users_{name}_token (token {kind})")) + if token: + await conn.execute( + text(f"INSERT INTO users_{name}_token VALUES (:token)"), {"token": token} + ) + async with async_sessionmaker(engine)() as db: + issues = await UsersModule().check_data_compatibility(db) + assert bool(issues) is incompatible + if incompatible: + assert issues[0].upgrade_id == "users.hashed_bearer_tokens_v1" + assert issues[0].severity == "error" + if token: + assert await db.scalar(text("SELECT token FROM users_access_token")) == token + finally: + await engine.dispose() + + +async def test_missing_token_tables_are_not_a_finding(): + # An unmigrated database is the migration check's problem; this must not + # refuse a production boot on its own. + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + try: + async with async_sessionmaker(engine)() as db: + assert await UsersModule().check_data_compatibility(db) == [] + finally: + await engine.dispose() diff --git a/modules/users/users/constants.py b/modules/users/users/constants.py index e24a21a9e..162b8d823 100644 --- a/modules/users/users/constants.py +++ b/modules/users/users/constants.py @@ -87,3 +87,6 @@ def sanitize_list_filters( sort if sort in ALLOWED_SORT else "email", order if order in ALLOWED_ORDER else "asc", ) + + +UPGRADE_HASHED_BEARER_TOKENS = "users.hashed_bearer_tokens_v1" diff --git a/modules/users/users/data_compatibility.py b/modules/users/users/data_compatibility.py new file mode 100644 index 000000000..b2baea339 --- /dev/null +++ b/modules/users/users/data_compatibility.py @@ -0,0 +1,57 @@ +"""Recognize legacy bearer-token columns and credentials without reading secrets out.""" + +from simple_module_core.data_compatibility import DataCompatibilityIssue +from sqlalchemy import String, column, func, inspect, or_, select, table +from sqlalchemy.ext.asyncio import AsyncSession + +from users.constants import UPGRADE_HASHED_BEARER_TOKENS +from users.models import RefreshToken, UserAccessToken + +# A SHA-256 hex digest; tokens hashed at rest (#626) are always this long. +_DIGEST_LENGTH = 64 + + +async def check_data_compatibility(db: AsyncSession) -> list[DataCompatibilityIssue]: + connection = await db.connection() + for model in (RefreshToken, UserAccessToken): + name = model.__tablename__ + + def token_type(sync, name=name): + inspector = inspect(sync) + if not inspector.has_table(name): + return None + return next( + (c["type"] for c in inspector.get_columns(name) if c["name"] == "token"), None + ) + + kind = await connection.run_sync(token_type) + if kind is None: + # Not migrated yet: the migration check owns that, not this one. + continue + if not isinstance(kind, String) or kind.length != _DIGEST_LENGTH: + return _incompatible() + # One probe, not a scan into Python: boot runs this every time. Stripping + # the hex digits with nested replace() is portable (SQLite has no regex); + # anything left over, or the wrong length, is a pre-hashing token. + token = table(name, column("token")).c.token + leftover = token + for digit in "0123456789abcdef": + leftover = func.replace(leftover, digit, "") + legacy = await db.scalar( + select(1).where(or_(func.length(token) != _DIGEST_LENGTH, leftover != "")).limit(1) + ) + if legacy is not None: + return _incompatible() + return [] + + +def _incompatible() -> list[DataCompatibilityIssue]: + return [ + DataCompatibilityIssue( + upgrade_id=UPGRADE_HASHED_BEARER_TOKENS, + message="Bearer-token schema or credentials predate hashing at rest. " + "Stop all old workers and apply the token invalidation/column upgrade; " + "clients must sign in again.", + severity="error", + ) + ] diff --git a/modules/users/users/module.py b/modules/users/users/module.py index f209e6056..c44765938 100644 --- a/modules/users/users/module.py +++ b/modules/users/users/module.py @@ -4,7 +4,7 @@ import importlib.resources from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any from fastapi import APIRouter, Depends from simple_module_core.audit_links import AuditLinkRegistry @@ -25,6 +25,7 @@ if TYPE_CHECKING: from fastapi import FastAPI + from simple_module_core.data_compatibility import DataCompatibilityIssue from simple_module_core.events import EventBus from simple_module_core.invalidation import InvalidationBus @@ -254,6 +255,11 @@ def register_admin_routes(self, admin_router: APIRouter) -> None: admin_router.include_router(admin_views) + async def check_data_compatibility(self, db: Any) -> list[DataCompatibilityIssue]: + from users.data_compatibility import check_data_compatibility + + return await check_data_compatibility(db) + async def on_startup(self, app: FastAPI) -> None: """Build the mailer, limiters, OAuth clients and the seeded admin. From bdc0cd128b40d8536a41f59a36cd040eb80ef483 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Sat, 31 Oct 2026 00:10:04 +0100 Subject: [PATCH 2/5] fix: address code review findings (round 1, pass 1) Setup wizard answers 409 on a required data upgrade; token check samples rows and accepts any column wide enough for a digest; doctor skips a SQLite file that doesn't exist and warns when the DB is unreachable; docs and CLI skill. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CLAUDE.md | 6 ++++-- docs/framework/lifecycle.md | 4 ++-- docs/reference/diagnostic-codes.md | 2 +- .../db/simple_module_db/data_compatibility.py | 18 +++++++++++++++++- .../tests/test_db_data_compatibility_doctor.py | 18 +++++++++++++----- .../data_compatibility.py | 4 ++++ .../setup_wizard/migrate.py | 10 +++++++++- modules/users/users/data_compatibility.py | 10 ++++++++-- skills/simple-module-cli/SKILL.md | 16 ++++++++++++++++ 9 files changed, 74 insertions(+), 14 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3a8147407..2c082d2ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -173,8 +173,10 @@ every Tailwind utility, so keep tokens in `theme.css` and rules in `styles.css` `register_design_packs`, `register_audit_links`, `register_resource_types`, `register_exception_handlers`, `register_middleware`, `register_routes(api_router, view_router)`, `register_admin_routes(admin_router)` -(only with `ModuleMeta.admin_view_prefix`), then async `on_startup` / -`on_shutdown` (reverse order). Non-hook overrides: `locale_dirs()`, +(only with `ModuleMeta.admin_view_prefix`), then async `check_data_compatibility(db)` +(read-only upgrade checks before settings hydration; an `error` refuses boot +outside development, `smpy migrations prepare ` writes the fix into +the host), then async `on_startup` / `on_shutdown` (reverse order). Non-hook overrides: `locale_dirs()`, `db_extensions()`, `template_dirs()`, `static_mounts()`, `spa_mounts()`. - A module gets one view router per prefix. All-admin modules point diff --git a/docs/framework/lifecycle.md b/docs/framework/lifecycle.md index d96d8b3a9..68e9a9727 100644 --- a/docs/framework/lifecycle.md +++ b/docs/framework/lifecycle.md @@ -353,8 +353,8 @@ def register_resource_types(self, registry: ResourceTypeRegistry) -> None: ## `check_data_compatibility(db)` -This async read-only hook returns a sequence of `DataCompatibilityIssue` SQLModel -DTOs (`upgrade_id`, `message`, `severity="warning"|"error"`), empty by default. +This async read-only hook returns a sequence of `DataCompatibilityIssue` frozen +dataclasses (`upgrade_id`, `message`, `severity="warning"|"error"`), empty by default. Use stable semantic ids and inspect actual schema/data state so hosts with independently ported revisions pass. Checks run before module DB-settings hydration and `on_startup`; errors refuse boot outside development. A never- diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index bc1c7f55f..f7a77e73a 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -41,7 +41,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM030` | WARNING at a dev boot; ERROR in `make doctor` | A module's `depends_on` names a module that is not loaded (not installed, left out of `SM_MODULES_ENABLED`, or dropped because another auth provider was selected — `keycloak` drops `Users`). Outside development the boot raises `MissingDependencyError` listing every pair. | Install and enable the dependency, or leave the dependent module out. A relationship that is only about order belongs in `optional_depends_on`. See [Discovery](/framework/discovery#modulemeta). | | `SM031` | WARNING | An `/api` route has no permission, `TokenAllowed` or `Authenticated` gate anywhere in its dependency tree (include-level dependencies count) and no public-route match, so the schema cannot say who may call it. Needs the built app: reported at a **dev boot** (and in the Doctor screen), not by `make doctor`. | Add `Depends(RequiresPermission(...))`, or `Depends(Authenticated())` when any signed-in user may call it, or register it in `register_public_routes`. | | `SM032` | ERROR (fails boot in every environment) | A `ModuleBase.spa_mounts()` prefix overlaps a module's `route_prefix`/`view_prefix`/`admin_view_prefix`, a `static_mounts()` prefix, a framework path (`/api`, `/admin`, `/setup`, `/i18n`, `/health`, `/static`, `/modules`) or another SPA mount. The mount would shadow, or be shadowed by, that router. | Pick a prefix nothing else uses. See [Legacy SPA mounts](/framework/spa-mounts). | -| `SM033` | ERROR or WARNING | A module data compatibility check found a missing semantic upgrade (or the database could not be checked). Token schema/legacy credentials are errors; unmoved mail system keys warn. Errors refuse boot outside development. Never-migrated first runs defer checks. | Run the reported `smpy migrations prepare --head `, review the host revision and apply `upgrade heads`; see [Upgrading a host](/guide/upgrading). | +| `SM033` | ERROR or WARNING | A module data compatibility check found a missing semantic upgrade (a WARNING when the database could not be checked; a SQLite file that does not exist yet is skipped). Token schema/legacy credentials are errors; unmoved mail system keys warn. Errors refuse boot outside development. Never-migrated first runs defer checks. | Run the reported `smpy migrations prepare --head `, review the host revision and apply `upgrade heads`; see [Upgrading a host](/guide/upgrading). | `SM022`/`SM023` are the two halves of the same invariant: a module's optional [`theme.css` is imported unlayered and `styles.css` into `layer(components)`](/module-authoring#styling), and CSS put in the wrong one silently does nothing (or silently wins everything). diff --git a/framework/db/simple_module_db/data_compatibility.py b/framework/db/simple_module_db/data_compatibility.py index 7adb20f55..0a405a4ed 100644 --- a/framework/db/simple_module_db/data_compatibility.py +++ b/framework/db/simple_module_db/data_compatibility.py @@ -1,6 +1,7 @@ """Database-backed Doctor diagnostics, separate from core's static checks.""" from collections.abc import Sequence +from pathlib import Path from alembic.runtime.migration import MigrationContext from simple_module_core.data_compatibility import collect_data_compatibility @@ -14,6 +15,8 @@ async def database_compatibility_diagnostics( modules: Sequence[ModuleBase], database_url: str ) -> list[Diagnostic]: + if not _database_exists(database_url): + return [] # Nothing migrated yet; opening it would create an empty SQLite file. engine = None try: engine = create_async_engine(database_url) @@ -38,8 +41,10 @@ async def database_compatibility_diagnostics( except Exception: # Connection exceptions can contain credentials; do not print them. return [ + # A warning: doctor also runs where no database is reachable (CI, a + # laptop without the stack); boot is where a real error is refused. Diagnostic( - level=DiagnosticLevel.ERROR, + level=DiagnosticLevel.WARNING, code=DATA_COMPATIBILITY_CODE, module_name="", message="Could not check module data compatibility.", @@ -50,3 +55,14 @@ async def database_compatibility_diagnostics( finally: if engine is not None: await engine.dispose() + + +def _database_exists(database_url: str) -> bool: + """False for a SQLite file that isn't there yet; every other URL is tried.""" + from sqlalchemy.engine import make_url + + url = make_url(database_url) + if not url.get_backend_name().startswith("sqlite"): + return True + database = url.database or "" + return database in ("", ":memory:") or Path(database).exists() diff --git a/framework/db/tests/test_db_data_compatibility_doctor.py b/framework/db/tests/test_db_data_compatibility_doctor.py index 804131381..61c21a134 100644 --- a/framework/db/tests/test_db_data_compatibility_doctor.py +++ b/framework/db/tests/test_db_data_compatibility_doctor.py @@ -50,12 +50,20 @@ async def check_data_compatibility(self, db): assert findings == [] -async def test_db_doctor_reports_unavailable_database(tmp_path): +async def test_db_doctor_skips_a_sqlite_file_that_does_not_exist(tmp_path): from simple_module_db.data_compatibility import database_compatibility_diagnostics - findings = await database_compatibility_diagnostics( - [], f"sqlite+aiosqlite:///{tmp_path}/missing/db" - ) + path = tmp_path / "app.db" + assert await database_compatibility_diagnostics([], f"sqlite+aiosqlite:///{path}") == [] + # Doctor must not leave an empty database behind. + assert not path.exists() + + +async def test_db_doctor_warns_on_unreachable_database(tmp_path): + from simple_module_db.data_compatibility import database_compatibility_diagnostics + + # A directory exists but cannot be opened as a SQLite database. + findings = await database_compatibility_diagnostics([], f"sqlite+aiosqlite:///{tmp_path}") assert len(findings) == 1 - assert findings[0].level == "error" + assert findings[0].level == "warning" assert "could not" in findings[0].message.lower() diff --git a/framework/hosting/simple_module_hosting/data_compatibility.py b/framework/hosting/simple_module_hosting/data_compatibility.py index a47e1fbdf..1bb7ec4b9 100644 --- a/framework/hosting/simple_module_hosting/data_compatibility.py +++ b/framework/hosting/simple_module_hosting/data_compatibility.py @@ -19,6 +19,10 @@ async def check_data_compatibility(app: FastAPI, modules: Sequence[ModuleBase]) async with app.state.sm.db.session_factory() as db: issues = await collect_data_compatibility(modules, db) except Exception as exc: + # Log the cause here: callers such as the setup gate only see the verdict. + logger.exception("Module data compatibility check failed") + if app.state.sm.settings.environment == "development": + return raise DataCompatibilityError("Could not check module data compatibility") from exc errors = [] for issue in issues: diff --git a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py index 4584699c4..dfba155b8 100644 --- a/framework/hosting/simple_module_hosting/setup_wizard/migrate.py +++ b/framework/hosting/simple_module_hosting/setup_wizard/migrate.py @@ -16,6 +16,7 @@ from fastapi import HTTPException, Request from simple_module_hosting._deferred_startup import run_deferred_startup +from simple_module_hosting.data_compatibility import DataCompatibilityError from simple_module_hosting.setup_wizard._migration_lock import migration_lock logger = logging.getLogger(__name__) @@ -87,6 +88,13 @@ def _upgrade() -> None: request.app.state.migration = await migration_status(request.app.state.sm.db.engine) # Modules whose on_startup could not run before the tables existed. - await run_deferred_startup(request.app) + try: + await run_deferred_startup(request.app) + except DataCompatibilityError as exc: + # The detail names upgrade ids and the remediation command, never data. + logger.error("Setup: %s", exc) + raise HTTPException( + status_code=409, detail="A module data upgrade is required; see the server log." + ) from exc logger.info("Setup: migrations applied") return {"migration": request.app.state.migration} diff --git a/modules/users/users/data_compatibility.py b/modules/users/users/data_compatibility.py index b2baea339..190279930 100644 --- a/modules/users/users/data_compatibility.py +++ b/modules/users/users/data_compatibility.py @@ -9,6 +9,7 @@ # A SHA-256 hex digest; tokens hashed at rest (#626) are always this long. _DIGEST_LENGTH = 64 +_SAMPLE = 100 async def check_data_compatibility(db: AsyncSession) -> list[DataCompatibilityIssue]: @@ -28,12 +29,17 @@ def token_type(sync, name=name): if kind is None: # Not migrated yet: the migration check owns that, not this one. continue - if not isinstance(kind, String) or kind.length != _DIGEST_LENGTH: + # Only a column too short for a digest is wrong; TEXT or a wider VARCHAR + # from a hand-ported revision holds hashes just as well. + length = getattr(kind, "length", None) + if not isinstance(kind, String) or (length is not None and length < _DIGEST_LENGTH): return _incompatible() # One probe, not a scan into Python: boot runs this every time. Stripping # the hex digits with nested replace() is portable (SQLite has no regex); # anything left over, or the wrong length, is a pre-hashing token. - token = table(name, column("token")).c.token + # A sample, not the table: un-hashed tokens come from a skipped upgrade, + # so every old row is plaintext and the first few show it. + token = select(table(name, column("token")).c.token).limit(_SAMPLE).subquery().c.token leftover = token for digit in "0123456789abcdef": leftover = func.replace(leftover, digit, "") diff --git a/skills/simple-module-cli/SKILL.md b/skills/simple-module-cli/SKILL.md index 7280e0c46..fabfb5433 100644 --- a/skills/simple-module-cli/SKILL.md +++ b/skills/simple-module-cli/SKILL.md @@ -17,6 +17,7 @@ The `smpy` command is provided by `simple_module_cli` (installed as a dep of `si | `smpy create-host ` | You want just a bare host project; you'll add modules later by `pip install`-ing them | | `smpy create-module ` | You're authoring a publishable module package (separate repo, distributed via PyPI) | | `smpy package-update` | Bump every `simple_module_*` / `simple-module-*` dependency, optional extra and dependency group to its latest non-yanked PyPI release, and `@simple-module-py/*` npm pins to the same core version (workspace/path/git/URL sources are left untouched) | +| `smpy migrations prepare --head