diff --git a/CHANGELOG.md b/CHANGELOG.md index c4c5631d3..59414d470 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 +- `make doctor` (and `smpy doctor`) now runs the migration checks it documents. It reads `alembic_version` and the table list from `SM_DATABASE_URL` and the heads from the project's `alembic.ini`: `SM010` is an error when the database lacks any head and a warning when it was never migrated or could not be read, and `SM011` reports a module table that a database at every head does not have. A development boot reports `SM011` too once its database is at head (or once the setup wizard has migrated it), and the Doctor screen keeps it on re-run. Doctor now resolves a relative SQLite `SM_DATABASE_URL` against the project root as the app does, so run from `host/` it reads the app's database for `SM033` too. Before, neither code was ever reported. **Behaviour change:** `make doctor` now exits 1 against a dev database that is behind head; run `make migrate` (#895). - `background_tasks`: `SM_BG_TASKS_BROKER_URL` and `SM_BG_TASKS_RESULT_BACKEND` are reported as deprecated only when they hold a Redis URL (`redis`, `rediss`), the only values `SM_REDIS_URL` can replace. A production host that runs tasks in-process with `memory://` + `cache+memory://`, or uses an AMQP, SQS or `rpc://` transport, no longer logs a warning on every boot that it cannot act on, and the production localhost-broker error no longer calls those vars deprecated for non-Redis values (#868). - `SecurityHeadersMiddleware` no longer overwrites a `Content-Security-Policy` that a response sets. The app policy is sent beside it as a second header and browsers enforce both, so a page can tighten the policy (for example `frame-ancestors 'none'` on a public page) but never relax it. Before, only a policy containing `sandbox` survived and any other was dropped silently. **Behaviour change:** a response that set a looser policy, relying on it being replaced, now has both policies enforced: remove that header, or make it a policy the page can live with alongside the app policy (#870). - `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). diff --git a/docs/database/migrations.md b/docs/database/migrations.md index d08a1d651..2410b90a0 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -206,20 +206,20 @@ context.configure( On startup, the framework compares `alembic_version` to **every** head (`resolve_head_revisions` in `simple_module_hosting.migrations`; the history has several, so it never asks for a single head): -- **Development:** prints a warning if the DB is behind. Result stored on `app.state.migration`. -- **Production:** fails boot with `SM010`. Don't ship a web process that's pointed at an unmigrated DB. +- **Every environment:** a database behind head fails the boot (`Database is N revision(s) behind … Run: make migrate`), except a never-migrated first run, which boots far enough to serve the `/setup` wizard. The result is stored on `app.state.migration`. +- `make doctor` reports the same state as `SM010` before you boot. Don't ship a web process that's pointed at an unmigrated DB. Fix locally with `make migrate`. In production, run migrations before rolling over the web tier. ## Diagnostic: `SM011` -Fires as a warning when a module's model declares a table that doesn't appear in any Alembic migration. Typical causes: +Fires as a warning when a database at every head lacks a table a module's `models` declare, so no migration creates it. `make doctor` checks the database `SM_DATABASE_URL` points at; a development boot checks its own database once it is open. Typical causes: - You added a model but haven't generated a migration for it yet. - You renamed a table but the old migration still references the old name. - You used `__abstract__ = True` somewhere it shouldn't be. -The dev-mode boot log prints the offending table names. Resolution: run `make migration msg="..."`, review, `make migrate`. +`make doctor` and the dev-mode boot log print the offending table names; while the database is behind head the check is skipped, since the table may be in a pending revision. Resolution: run `make migration msg="..."`, review, `make migrate`. ## Cross-module foreign keys diff --git a/docs/database/per-module-base.md b/docs/database/per-module-base.md index 6fd7c052f..b153af8de 100644 --- a/docs/database/per-module-base.md +++ b/docs/database/per-module-base.md @@ -86,4 +86,4 @@ for t in meta.sorted_tables: print(t.name) ``` -This is also what the boot-time `SM011` check uses — it compares this set against the Alembic history to detect tables that exist in code but not in any migration. +This is also what the boot-time `SM011` check uses — it compares this set against the migrated database's tables to detect tables that exist in code but that no migration creates. diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 1a22e9890..943b3ef00 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -626,8 +626,8 @@ Dispatch keys on the **exact** event class (`module.qualname`), not its MRO — | SM007 | INFO | Module defines no `register_*` hooks | | SM008 | ERROR | Duplicate module name / schema prefix conflict | | SM009 | ERROR | Framework package directly imports from a plugin module | -| SM010 | ERROR | DB revision behind migration head | -| SM011 | WARNING | Module table not in migration history | +| SM010 | ERROR or WARNING | DB lacks a migration head (WARNING: never migrated) | +| SM011 | WARNING | Module table missing from a database at head | | SM012 | WARNING | `register_settings` overridden but nothing added to `app.state` | | SM013 | WARNING | Locale file missing for a supported locale | | SM014 | WARNING | Non-default locale missing keys present in the default | diff --git a/docs/reference/diagnostic-codes.md b/docs/reference/diagnostic-codes.md index 8d176223e..c32e9b22c 100644 --- a/docs/reference/diagnostic-codes.md +++ b/docs/reference/diagnostic-codes.md @@ -18,8 +18,8 @@ The framework runs a set of static checks over installed modules at app boot. Th | `SM007` | INFO | Module overrides zero `register_*` hooks and has no `on_startup`/`on_shutdown`. | Delete the module if it's vestigial; otherwise ignore. | | `SM008` | ERROR | Two modules declare the same `ModuleMeta.name`. Table-prefix collision. | Rename one. Remember: the name is the table-name prefix (all modules share one DB schema) and the Inertia namespace. | | `SM009` | ERROR | A file under `framework/*` imports from any package under `modules/*`, including dynamic imports with literal names or fixed f-string package prefixes. | Invert the dependency: have the module register a callback into the framework (see `principal_serializer` pattern). | -| `SM010` | ERROR | DB `alembic_version` is behind the migration head. | Run `make migrate`. The lifespan migration check raises and aborts boot in every environment before serving. | -| `SM011` | WARNING | A module's model declares a table that doesn't appear in any Alembic migration. | Run `uv run alembic revision --autogenerate -m "..."`, review, `make migrate`. | +| `SM010` | ERROR or WARNING | `make doctor` reads `alembic_version` from `SM_DATABASE_URL` and the heads from the project's `alembic.ini` (else `host/alembic.ini`). ERROR when the database lacks any head; WARNING when it was never migrated (a production boot sends that to `/setup`) or could not be read, in which case SM011 is skipped too. A SQLite file that does not exist yet counts as never migrated and is not created. | Run `make migrate`. The lifespan migration check also refuses a behind-head database at boot in every environment. | +| `SM011` | WARNING | A table in a module's `models` is missing from a database at every head, so no migration creates it. Reported by `make doctor` and, once the database is open, a development boot (also on the Doctor screen). Not checked while the database is behind head. | Generate a revision on the module's branch (`make migration LABEL= msg="..."` in this repo, `make migration msg="..."` in a scaffolded app), review it, `make migrate`. | | `SM012` | WARNING | Module overrides `register_settings` but does not assign to `app.state.`. Dev-only. | Either remove the override or move your state assignment into it. | | `SM013` | WARNING | Supported locale has no corresponding file in some module's `locales/`. | Add `modules///locales/.json`, or drop the locale from `SM_I18N_SUPPORTED_LOCALES`. | | `SM014` | WARNING | Non-default locale is missing keys present in the default (untranslated). | Translate the missing keys. | @@ -50,7 +50,7 @@ The framework runs a set of static checks over installed modules at app boot. Th | Context | What runs | |---|---| -| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. After the routes are wired, SM031 (unguarded `/api` routes) is added. | +| App boot in development | Full structural/page/i18n suite, results logged to stderr. ERRORS abort boot. After the routes are wired, SM031 (unguarded `/api` routes) is added; once the database is open and at head, SM011. | | App boot in non-development | Strict module discovery (raises on SM001-class failures) + the migration check (SM010) + SM025 (logged as a warning). The page/locale static suite is dev-only. | Sample dev-mode output: @@ -60,7 +60,7 @@ running diagnostics … WARNING SM003 modules/orders/orders/pages/Unused.tsx: no inertia.render("Orders/Unused") call found WARNING SM013 locale "es" missing file for module "orders" (expected modules/orders/orders/locales/es.json) -ERROR SM010 DB revision 3a1b2c3d4e5f is behind migration head 9f8e7d6c5b4a — run `make migrate` +ERROR SM010 Database is behind: missing heads '9f8e7d6c5b4a' — run `make migrate` 1 error, 2 warnings ``` diff --git a/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md b/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md new file mode 100644 index 000000000..193be092c --- /dev/null +++ b/docs/superpowers/plans/2026-11-10-doctor-migration-checks.md @@ -0,0 +1,46 @@ +# Doctor migration checks (#895) — implementation plan + +Design: `docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md`. + +Tasks run in order; 1 and 2 are independent of each other. + +1. **`MigrationDiagnostics.check_heads` + `module_table_names`** (core) + - Files: `framework/core/simple_module_core/diagnostics/_migration.py`, + `framework/core/tests/test_diagnostics.py`. + - Test first: at heads → `[]`; no current → SM010 WARNING; one of several + heads missing → SM010 ERROR naming it; an extra unknown current revision + with every head present → `[]`. + - Done: targeted tests pass. + +2. **`simple_module_db.migration_state.read_migration_state`** (db) + - Files: `framework/db/simple_module_db/migration_state.py` (new; also + takes over `_database_exists` from `data_compatibility.py`), + `framework/db/tests/test_db_migration_state.py` (new). + - Test first: no ini → `None`; missing SQLite file → empty current and + tables, file not created; unstamped file → empty current, its tables; + stamped file → current revisions. + - Done: targeted tests pass. + +3. **Doctor wiring** (hard: cross-package) + - Files: `framework/core/simple_module_core/_migration_doctor.py` (new), + `framework/core/simple_module_core/__main__.py`, + `framework/core/tests/test_core_migration_doctor.py` (new). + - Test first, through `main()` with a temp project root holding an + `alembic.ini` and two revisions: DB at heads with a module table missing + → SM011, exit 0; DB behind → SM010, exit 1; missing DB → SM010 warning, + exit 0; unreachable DB → one warning. + - Done: targeted tests + existing doctor tests pass. + +4. **Dev boot SM011** + - Files: `framework/hosting/simple_module_hosting/_table_coverage.py` (new), + `framework/hosting/simple_module_hosting/_lifespan.py`, + `framework/hosting/tests/test_hosting_table_coverage.py` (new). + - Test first: with a runner set and a table missing, results gain SM011 and + `rerun()` keeps it; with no runner nothing runs. + - Done: targeted tests pass. + +5. **Docs + CHANGELOG**: `docs/reference/diagnostic-codes.md` (SM010/SM011 + rows, "When diagnostics fire"), `CHANGELOG.md` Fixed. + +6. **Full checks**: `make lint`, `make test-py` (PYTEST_WORKERS=8), `make + ci-doctor`. diff --git a/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md b/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md new file mode 100644 index 000000000..f511b5e04 --- /dev/null +++ b/docs/superpowers/specs/2026-11-10-doctor-migration-checks-design.md @@ -0,0 +1,104 @@ +# Doctor runs the migration checks SM010 and SM011 (#895) + +## Goal & scope + +`smpy doctor` / `make doctor` never report `SM010` (database behind head) or +`SM011` (a module table no migration creates), although `CLAUDE.md` and +`docs/reference/diagnostic-codes.md` promise both. `run_diagnostics()` runs them +only when it is handed a `migration_state`, and neither caller (doctor in +`simple_module_core/__main__.py`, the dev boot in +`simple_module_hosting/_dev_boot.py`) passes one. A new module's table can go +unmigrated with no warning. + +In scope: doctor reports both codes; the development boot (and so the in-app +Doctor screen) reports `SM011`; tests; docs; CHANGELOG. +Out of scope: changing what a production boot refuses; `alembic check`-style +column drift (CI already runs `alembic check`, #869). + +## Approach + +Chosen: **read the state from the database doctor already points at.** +Doctor reads `alembic_version` and the table list from `SM_DATABASE_URL`, and +the heads from the host's `alembic.ini`. When the database is at every head, +any table a selected module's models declare that the database does not have +is a table no migration creates: `SM011`. Behind head is `SM010`. + +Rejected: +- *Scan the revision files for `op.create_table(...)`* (what the issue + sketched). It misses `rename_table`, batch operations, tables built in + helpers and raw SQL, so it would raise false `SM011`s on this repo's own + history; the database is the ground truth for "what the migrations create". +- *Migrate a throwaway SQLite database inside doctor.* Accurate, but slow on + every run, and `make ci-doctor` already does exactly this before calling + doctor, so the database approach gets the same answer in CI for free. + +## Design + +**`simple_module_db.migration_state`** (new; db owns Alembic, core may not +import hosting): +`async def read_migration_state(database_url, alembic_ini) -> MigrationState | None` +returns `heads`, `current` (plural `get_current_heads()`, like +`migration_status`) and `tables` (`inspect(...).get_table_names()`), or `None` +when there is no `alembic.ini` / no revisions. It never creates a database: a +SQLite file URL whose file does not exist yields an empty state instead of +connecting (connecting would create `app.db`). + +**`MigrationDiagnostics.check_heads(current, heads)`** (core, new beside the +existing `check_revision_mismatch`, which stays for `run_diagnostics` +callers): the same rule the boot uses (`heads - current`): +- at every head → nothing; +- never migrated (no `alembic_version` rows) → `SM010` **WARNING**, "run + `make migrate`" (a production boot sends this database to `/setup` rather + than refusing it, so it is not an error); +- behind on some branch → `SM010` **ERROR** naming the missing heads (a + production boot refuses this). +`check_table_coverage` (existing) gives `SM011`, run only when at every head. + +**Doctor** (`simple_module_core/__main__.py` → a `_migration_doctor.py` helper +like `_data_doctor.py`): skips silently when `simple_module_db` is not +installed; resolves `alembic.ini` as the CLI does (project root `alembic.ini`, +else `host/alembic.ini`); module tables come from `module_all_tables()` over the +selected modules (the set SM026 already uses). A database it cannot reach (a +stopped Postgres) gives one `SM010` WARNING saying the checks were skipped, +not a crash. + +**Dev boot**: the diagnostics runner runs at build time, before the database +is open, so `SM011` is computed in the lifespan instead: in development, once +`migration_status` reports the database at head, the lifespan reads the +table list from the app's own engine, prints any `SM011`, and wraps +`diagnostics_state.runner` so the in-app Doctor screen (which already maps +`SM010`/`SM011` to its migrations check) shows them on every re-run. `SM010` at +boot is already enforced by the lifespan's own refusal. + +## Decisions made + +- Never-migrated is a WARNING, behind is an ERROR (mirrors production boot). +- Unreachable database: WARNING, migration checks skipped; doctor still exits + on the other findings. +- `SM011` only when at every head; behind head the missing table may simply be + in a pending revision. +- Module tables = tables in each selected module's `models` (as SM026). Tables + declared outside `models` are not checked (missed, never falsely reported). +- No new setting, no new code; docs: `docs/reference/diagnostic-codes.md` + (when each fires, that doctor reads `SM_DATABASE_URL`), CHANGELOG Fixed. + +## Testing + +- `check_heads`: at heads, never migrated, behind (multi-head), extra unknown + revision is not "behind". +- `read_migration_state`: a SQLite file stamped at heads (the test plugin's + create-all + stamp), an unstamped one, a missing file is not created. +- Doctor `main()` end to end: a database at heads with one module table + dropped reports `SM011` and exits 0 (warning); a database stamped one + revision behind reports `SM010` and exits 1; a missing DB file warns. +- Dev boot: the lifespan helper adds `SM011` to `diagnostics_state` and + `rerun()` keeps it. +No browser QA (`--skip-browser`): CLI and boot diagnostics only. + +## Risks + +- `make doctor` on a developer checkout whose dev database is behind will now + exit 1. That is the point of SM010, and the message says `make migrate`. +- `make ci-doctor` migrates first, so CI should stay green; if this repo's own + modules have a table no migration creates, doctor will now say so and the + fix is a migration (that would be a real bug the check exists to find). diff --git a/framework/core/simple_module_core/__main__.py b/framework/core/simple_module_core/__main__.py index 335f31bab..94e008b5a 100644 --- a/framework/core/simple_module_core/__main__.py +++ b/framework/core/simple_module_core/__main__.py @@ -25,6 +25,7 @@ from pathlib import Path from simple_module_core._data_doctor import data_compatibility_diagnostics +from simple_module_core._migration_doctor import migration_diagnostics from simple_module_core.diagnostics import ( Diagnostic, DiagnosticLevel, @@ -39,7 +40,7 @@ select_auth_provider, topological_sort, ) -from simple_module_core.dotenv import find_env_file, parse_dotenv +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file, parse_dotenv from simple_module_core.exceptions import InvalidModuleError @@ -77,8 +78,13 @@ def _load_i18n_settings_from_env() -> tuple[list[str] | None, str]: def _database_url() -> str: - """``SM_DATABASE_URL``, defaulting like ``Settings.database_url``.""" - return os.environ.get("SM_DATABASE_URL", "sqlite+aiosqlite:///./app.db") + """``SM_DATABASE_URL``, defaulting and anchored like ``Settings.database_url``. + + A relative SQLite path is resolved against the project root, as the app + resolves it, so doctor run from ``host/`` reads the app's database. + """ + url = os.environ.get("SM_DATABASE_URL", "sqlite+aiosqlite:///./app.db") + return absolutize_sqlite_url(url, anchor=find_env_file().parent) def _database_dialect() -> str: @@ -188,6 +194,7 @@ def main() -> int: database_dialect=_database_dialect(), ) diagnostics.extend(override_findings) + diagnostics.extend(migration_diagnostics(modules, _database_url(), find_env_file().parent)) diagnostics.extend(data_compatibility_diagnostics(modules, _database_url())) # 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. diff --git a/framework/core/simple_module_core/_migration_doctor.py b/framework/core/simple_module_core/_migration_doctor.py new file mode 100644 index 000000000..fe37e16cd --- /dev/null +++ b/framework/core/simple_module_core/_migration_doctor.py @@ -0,0 +1,56 @@ +"""Doctor's migration checks (SM010, SM011) against ``SM_DATABASE_URL`` (#895).""" + +import asyncio +import importlib.util +from collections.abc import Sequence +from pathlib import Path + +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_core.diagnostics._migration import MigrationDiagnostics, module_table_names +from simple_module_core.module import ModuleBase + + +def _alembic_ini(project_root: Path) -> Path | None: + """``alembic.ini`` at the project root, else ``host/alembic.ini``, as the CLI resolves it.""" + for candidate in (project_root / "alembic.ini", project_root / "host" / "alembic.ini"): + if candidate.is_file(): + return candidate + return None + + +def migration_diagnostics( + modules: Sequence[ModuleBase], database_url: str, project_root: Path +) -> list[Diagnostic]: + # Core-only installations have neither Alembic nor a database runtime. + if importlib.util.find_spec("simple_module_db") is None: + return [] + ini = _alembic_ini(project_root) + if ini is None: + return [] + from simple_module_db.migration_state import read_migration_state + + try: + state = asyncio.run(read_migration_state(database_url, ini)) + except Exception: + # Unreachable is a warning, as for SM033: doctor also runs where no + # database is up. A revision file that fails to import lands here too. + # The error text can carry credentials, so it is not shown. + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code="SM010", + message="Could not read the migration state (database or Alembic revisions); " + "SM010 and SM011 were not checked", + module_name="migrations", + suggestion="Check SM_DATABASE_URL and that the database is running, and that " + "the revisions load (`make migration-history`).", + ) + ] + if state is None: + return [] + checks = MigrationDiagnostics() + behind = checks.check_heads(state.current, state.heads) + if behind: + # Behind head, a missing table may simply be in a pending revision. + return behind + return checks.check_table_coverage(module_table_names(modules), set(state.tables)) diff --git a/framework/core/simple_module_core/diagnostics/_migration.py b/framework/core/simple_module_core/diagnostics/_migration.py index 5c135b0db..9e470bdf5 100644 --- a/framework/core/simple_module_core/diagnostics/_migration.py +++ b/framework/core/simple_module_core/diagnostics/_migration.py @@ -2,12 +2,60 @@ from __future__ import annotations +from collections.abc import Iterable, Set +from typing import TYPE_CHECKING + +from simple_module_core.diagnostics._expression_index import module_all_tables from simple_module_core.diagnostics._types import Diagnostic, DiagnosticLevel +if TYPE_CHECKING: + from simple_module_core.module import ModuleBase + +_CODE_BEHIND = "SM010" +_MODULE_NAME = "migrations" +_SUGGEST_MIGRATE = "Run: make migrate" + + +def module_table_names(modules: Iterable[ModuleBase]) -> set[str]: + """Names of the tables declared in each module's ``models`` (the set SM026 checks).""" + return {table.name for mod in modules for table in module_all_tables(mod)} + class MigrationDiagnostics: """Validates database migration state.""" + def check_heads(self, current: Set[str], heads: Set[str]) -> list[Diagnostic]: + """SM010 for a database missing any head; the boot check's ``heads - current`` rule. + + A never-migrated database is a WARNING: a production boot sends it to + ``/setup`` rather than refusing it. One behind on any branch is an ERROR, + because a production boot refuses it. + """ + missing = heads - current + if not missing: + return [] + if not current: + return [ + Diagnostic( + level=DiagnosticLevel.WARNING, + code=_CODE_BEHIND, + message="Database has never been migrated (no alembic_version)", + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, + ) + ] + return [ + Diagnostic( + level=DiagnosticLevel.ERROR, + code=_CODE_BEHIND, + message=( + f"Database is behind: missing heads {', '.join(map(repr, sorted(missing)))}" + ), + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, + ) + ] + def check_revision_mismatch( self, current_revision: str | None, @@ -19,10 +67,10 @@ def check_revision_mismatch( return [ Diagnostic( level=DiagnosticLevel.ERROR, - code="SM010", + code=_CODE_BEHIND, message=(f"Database at revision {current_revision!r}, expected {head_revision!r}"), - module_name="migrations", - suggestion="Run: make migrate", + module_name=_MODULE_NAME, + suggestion=_SUGGEST_MIGRATE, ) ] @@ -31,14 +79,17 @@ def check_table_coverage( module_tables: set[str], migrated_tables: set[str], ) -> list[Diagnostic]: - """SM011: Warning if module tables are missing from migration history.""" + """SM011: Warning for module tables a database at every head does not have.""" missing = module_tables - migrated_tables return [ Diagnostic( level=DiagnosticLevel.WARNING, code="SM011", - message=f"Table '{table}' declared in models but not found in migration history", - module_name="migrations", + message=( + f"Table '{table}' is declared in models but missing from the migrated " + "database, so no migration creates it" + ), + module_name=_MODULE_NAME, suggestion=f'Run: make migration msg="add {table}"', ) for table in sorted(missing) diff --git a/framework/core/simple_module_core/dotenv.py b/framework/core/simple_module_core/dotenv.py index 3f228c1f4..6da511f1a 100644 --- a/framework/core/simple_module_core/dotenv.py +++ b/framework/core/simple_module_core/dotenv.py @@ -1,5 +1,8 @@ """Minimal ``.env`` parser + env-var helpers — dependency-free. +Also holds ``absolutize_sqlite_url``, which anchors a relative SQLite +``SM_DATABASE_URL`` at the project root for both the app and the doctor CLI. + Used in places that can't or shouldn't pull in ``pydantic-settings`` (the diagnostics CLI runs before the host package is imported; the users-module bootstrap runs after settings are constructed and needs to read values that @@ -139,3 +142,23 @@ def env_bool(name: str, default: bool = False) -> bool: if raw in BOOL_LITERALS_FALSE: return False return default + + +def absolutize_sqlite_url(url: str, *, anchor: Path) -> str: + """Rewrite a relative sqlite path to an absolute one under ``anchor``. + + ``sqlite+aiosqlite:///./host/app.db`` means "relative to the project + root" by convention, but SQLAlchemy resolves it against the process cwd + — correct in the web process (which chdirs) and silently wrong in every + CLI run from a subdirectory. Absolute paths (``:////...``), ``:memory:``, + SQLite URI-mode paths (``file:...?uri=true``), and non-sqlite URLs pass + through untouched. + """ + if not url.startswith("sqlite"): + return url + scheme, sep, rest = url.partition(":///") + if not sep or not rest or rest.startswith(("/", ":memory:", "file:")): + return url + path_part, query_sep, query = rest.partition("?") + resolved = (anchor / path_part).resolve() + return f"{scheme}:///{resolved}{query_sep}{query}" diff --git a/framework/core/tests/test_diagnostics.py b/framework/core/tests/test_diagnostics.py index 5a329a9d3..a6e6f6c1a 100644 --- a/framework/core/tests/test_diagnostics.py +++ b/framework/core/tests/test_diagnostics.py @@ -52,6 +52,25 @@ async def test_sm011_no_warning_when_covered(self): ) assert len(results) == 0 + def test_check_heads_at_every_head(self): + assert MigrationDiagnostics().check_heads({"a", "b"}, {"a", "b"}) == [] + + def test_check_heads_never_migrated_warns(self): + (finding,) = MigrationDiagnostics().check_heads(set(), {"a", "b"}) + assert finding.code == "SM010" + assert finding.level == DiagnosticLevel.WARNING + assert "make migrate" in finding.suggestion + + def test_check_heads_behind_names_the_missing_heads(self): + (finding,) = MigrationDiagnostics().check_heads({"a", "old"}, {"a", "b", "c"}) + assert finding.code == "SM010" + assert finding.level == DiagnosticLevel.ERROR + assert "missing heads 'b', 'c'" in finding.message + + def test_check_heads_extra_revision_is_not_behind(self): + # Same rule as the boot check: only heads the database lacks count. + assert MigrationDiagnostics().check_heads({"a", "b", "extra"}, {"a", "b"}) == [] + class TestPrintDiagnostics: async def test_writes_to_stderr(self, capsys): diff --git a/framework/db/simple_module_db/data_compatibility.py b/framework/db/simple_module_db/data_compatibility.py index 170837ac0..d5f029f52 100644 --- a/framework/db/simple_module_db/data_compatibility.py +++ b/framework/db/simple_module_db/data_compatibility.py @@ -1,7 +1,6 @@ """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 @@ -9,6 +8,7 @@ from simple_module_core.module import ModuleBase from simple_module_db.listeners import register_listeners +from simple_module_db.migration_state import database_exists from simple_module_db.session import init_db DATA_COMPATIBILITY_CODE = "SM033" @@ -19,7 +19,7 @@ async def database_compatibility_diagnostics( ) -> list[Diagnostic]: state = None try: - if not _database_exists(database_url): + if not database_exists(database_url): return [] # Nothing migrated yet; opening it would create an empty SQLite file. # Read-only diagnostics: never switch a legacy SQLite file to WAL. state = init_db(database_url, sqlite_wal=False) @@ -80,14 +80,3 @@ def _problem(level: DiagnosticLevel, message: str) -> Diagnostic: suggestion="Check database connectivity and apply pending host schema migrations, " "then rerun doctor.", ) - - -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/simple_module_db/migration_state.py b/framework/db/simple_module_db/migration_state.py new file mode 100644 index 000000000..53f600712 --- /dev/null +++ b/framework/db/simple_module_db/migration_state.py @@ -0,0 +1,78 @@ +"""Read a database's Alembic state without a host app (Doctor's SM010/SM011, #895).""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +from alembic.config import Config +from alembic.runtime.migration import MigrationContext +from alembic.script import ScriptDirectory +from sqlalchemy import inspect +from sqlalchemy.engine import make_url +from sqlalchemy.ext.asyncio import create_async_engine +from sqlalchemy.pool import NullPool + + +@dataclass(frozen=True, slots=True) +class MigrationState: + """What the migrations define (``heads``) and what the database holds.""" + + heads: frozenset[str] + #: ``alembic_version`` rows; empty for a database that was never migrated. + current: frozenset[str] + tables: frozenset[str] + + +def database_exists(database_url: str) -> bool: + """False for a SQLite file that isn't there yet; every other URL is tried. + + A URI-mode path (``file:...?uri=true``) is checked by the file it names, + since opening a missing one in the default ``rwc`` mode would create it. + """ + url = make_url(database_url) + if not url.get_backend_name().startswith("sqlite"): + return True + database = url.database or "" + if database.startswith("file:"): + if url.query.get("mode") == "memory": + return True + database = database.removeprefix("file:").split("?", 1)[0] + return database in ("", ":memory:") or Path(database).exists() + + +def _heads(alembic_ini: Path) -> frozenset[str]: + # An ini that exists but cannot be loaded (bad script_location, a broken + # revision) raises: the caller reports it rather than skipping silently. + if not alembic_ini.is_file(): + return frozenset() + return frozenset(ScriptDirectory.from_config(Config(str(alembic_ini))).get_heads()) + + +async def read_migration_state(database_url: str, alembic_ini: str | Path) -> MigrationState | None: + """The heads in *alembic_ini* beside the current revisions and tables of the database. + + ``None`` when there is no ``alembic.ini`` or it defines no revisions: there + is nothing to compare against. An ini Alembic cannot load raises. A SQLite + file that does not exist reads as empty rather than being opened, which + would create it. Connection errors propagate; their text can carry + credentials, so callers must not echo it. + """ + heads = _heads(Path(alembic_ini)) + if not heads: + return None + if not database_exists(database_url): + return MigrationState(heads=heads, current=frozenset(), tables=frozenset()) + + def _read(sync_conn) -> tuple[frozenset[str], frozenset[str]]: + # Plural, like the boot check: a branching history keeps one row per branch. + current = MigrationContext.configure(sync_conn).get_current_heads() + return frozenset(current), frozenset(inspect(sync_conn).get_table_names()) + + engine = create_async_engine(database_url, poolclass=NullPool) + try: + async with engine.connect() as conn: + current, tables = await conn.run_sync(_read) + finally: + await engine.dispose() + return MigrationState(heads=heads, current=current, tables=tables) diff --git a/framework/db/tests/_alembic_project.py b/framework/db/tests/_alembic_project.py new file mode 100644 index 000000000..caae27b3a --- /dev/null +++ b/framework/db/tests/_alembic_project.py @@ -0,0 +1,46 @@ +"""A throwaway Alembic project and SQLite database for the migration-state tests.""" + +from pathlib import Path + +import sqlalchemy as sa + +_REVISION = """revision = "{rev}" +down_revision = {down} +branch_labels = None +depends_on = None + + +def upgrade(): + pass + + +def downgrade(): + pass +""" + + +def write_alembic_project(root: Path, chain: list[str]) -> Path: + """An ``alembic.ini`` over the ``"rev:down_revision"`` entries in *chain*. + + ``down_revision`` is empty for a root, so a test can build several heads + without a real ``env.py``. + """ + versions = root / "migrations" / "versions" + versions.mkdir(parents=True) + for entry in chain: + rev, _, down = entry.partition(":") + (versions / f"{rev}.py").write_text( + _REVISION.format(rev=rev, down=repr(down) if down else "None") + ) + ini = root / "alembic.ini" + ini.write_text("[alembic]\nscript_location = %(here)s/migrations\n") + return ini + + +def sqlite_database(path: Path, *statements: str) -> str: + engine = sa.create_engine(f"sqlite:///{path}") + with engine.begin() as conn: + for statement in statements: + conn.execute(sa.text(statement)) + engine.dispose() + return f"sqlite+aiosqlite:///{path}" diff --git a/framework/db/tests/test_db_migration_doctor.py b/framework/db/tests/test_db_migration_doctor.py new file mode 100644 index 000000000..14bdefb86 --- /dev/null +++ b/framework/db/tests/test_db_migration_doctor.py @@ -0,0 +1,123 @@ +"""``make doctor`` reports SM010 and SM011 from the database it points at (#895).""" + +from __future__ import annotations + +import pytest +from _alembic_project import sqlite_database, write_alembic_project +from simple_module_core import ModuleBase, ModuleMeta, _migration_doctor +from simple_module_core import __main__ as doctor + +_STAMP = "CREATE TABLE alembic_version (version_num VARCHAR(32))" + + +class _Module(ModuleBase): + meta = ModuleMeta(name="Orders") + + +@pytest.fixture +def run_doctor(tmp_path, monkeypatch, capsys): + """Run ``main()`` over one module declaring ``orders_order`` and ``orders_line``.""" + monkeypatch.setattr(doctor, "discover_modules", lambda strict: [_Module()]) + monkeypatch.setattr(doctor, "find_env_file", lambda: tmp_path / ".env") + monkeypatch.setattr(doctor, "parse_dotenv", dict) + monkeypatch.setattr(doctor, "run_diagnostics", lambda *a, **kw: []) + monkeypatch.setattr( + _migration_doctor, "module_table_names", lambda modules: {"orders_order", "orders_line"} + ) + + def run(url: str) -> tuple[int, str]: + monkeypatch.setenv("SM_DATABASE_URL", url) + code = doctor.main() + return code, capsys.readouterr().err + + return run + + +def test_db_doctor_reports_a_table_no_migration_creates(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:", "b:"]) + url = sqlite_database( + tmp_path / "app.db", + _STAMP, + "INSERT INTO alembic_version VALUES ('a'), ('b')", + "CREATE TABLE orders_order (id INTEGER)", + ) + + code, err = run_doctor(url) + + assert code == 0 + assert "SM011" in err + assert "orders_line" in err + assert "orders_order" not in err + assert "SM010" not in err + + +def test_db_doctor_fails_on_a_database_behind_head(tmp_path, run_doctor): + # The CLI's fallback location: host/alembic.ini under the project root. + write_alembic_project(tmp_path / "host", ["a:", "a2:a", "b:"]) + url = sqlite_database( + tmp_path / "app.db", _STAMP, "INSERT INTO alembic_version VALUES ('a'), ('b')" + ) + + code, err = run_doctor(url) + + assert code == 1 + assert "SM010" in err + assert "missing heads 'a2'" in err + # Behind head the table may only be in a pending revision. + assert "SM011" not in err + + +def test_db_doctor_warns_on_a_missing_database(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + + code, err = run_doctor(f"sqlite+aiosqlite:///{path}") + + assert code == 0 + assert "SM010" in err + assert "never been migrated" in err + assert not path.exists() + + +def test_db_doctor_warns_on_an_unreadable_database(tmp_path, run_doctor): + write_alembic_project(tmp_path, ["a:"]) + + # A directory exists but cannot be opened as a SQLite database. + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path}") + + assert code == 0 + assert "SM010" in err + assert "could not read the migration state" in err.lower() + + +def test_db_doctor_skips_without_an_alembic_project(tmp_path, run_doctor): + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path / 'app.db'}") + + assert code == 0 + assert "SM010" not in err + assert "SM011" not in err + + +def test_db_doctor_anchors_a_relative_sqlite_url_at_the_project_root( + tmp_path, monkeypatch, run_doctor +): + # The app resolves ./app.db against the project root, not the cwd; doctor must too. + write_alembic_project(tmp_path, ["a:"]) + sqlite_database(tmp_path / "app.db", _STAMP, "INSERT INTO alembic_version VALUES ('a')") + (tmp_path / "host").mkdir() + monkeypatch.chdir(tmp_path / "host") + + code, err = run_doctor("sqlite+aiosqlite:///./app.db") + + assert code == 0 + assert "never been migrated" not in err + assert not (tmp_path / "host" / "app.db").exists() + + +def test_db_doctor_warns_on_a_broken_alembic_ini(tmp_path, run_doctor): + (tmp_path / "alembic.ini").write_text("[alembic]\nscript_location = %(here)s/missing\n") + + code, err = run_doctor(f"sqlite+aiosqlite:///{tmp_path / 'app.db'}") + + assert code == 0 + assert "could not read the migration state" in err.lower() diff --git a/framework/db/tests/test_db_migration_state.py b/framework/db/tests/test_db_migration_state.py new file mode 100644 index 000000000..c45a7ea0c --- /dev/null +++ b/framework/db/tests/test_db_migration_state.py @@ -0,0 +1,74 @@ +"""Doctor reads the migration state from the database it points at (#895).""" + +from _alembic_project import sqlite_database, write_alembic_project +from simple_module_db.migration_state import read_migration_state + + +async def test_db_migration_state_none_without_an_ini(tmp_path): + assert await read_migration_state("sqlite+aiosqlite://", tmp_path / "alembic.ini") is None + + +async def test_db_migration_state_missing_file_is_not_created(tmp_path): + ini = write_alembic_project(tmp_path, ["a:", "b:"]) + path = tmp_path / "app.db" + + state = await read_migration_state(f"sqlite+aiosqlite:///{path}", ini) + + assert state is not None + assert state.heads == {"a", "b"} + assert state.current == set() + assert state.tables == set() + assert not path.exists() + + +async def test_db_migration_state_reads_an_unstamped_database(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + url = sqlite_database(tmp_path / "app.db", "CREATE TABLE orders_order (id INTEGER)") + + state = await read_migration_state(url, ini) + + assert state is not None + assert state.current == set() + assert state.tables == {"orders_order"} + + +async def test_db_migration_state_reads_current_heads(tmp_path): + ini = write_alembic_project(tmp_path, ["a:", "a2:a", "b:"]) + url = sqlite_database( + tmp_path / "app.db", + "CREATE TABLE alembic_version (version_num VARCHAR(32))", + "INSERT INTO alembic_version VALUES ('a'), ('b')", + ) + + state = await read_migration_state(url, ini) + + assert state is not None + assert state.heads == {"a2", "b"} + assert state.current == {"a", "b"} + assert "alembic_version" in state.tables + + +async def test_db_migration_state_reads_a_uri_mode_database(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + sqlite_database( + path, + "CREATE TABLE alembic_version (version_num VARCHAR(32))", + "INSERT INTO alembic_version VALUES ('a')", + ) + + state = await read_migration_state(f"sqlite+aiosqlite:///file:{path}?mode=ro&uri=true", ini) + + assert state is not None + assert state.current == {"a"} + + +async def test_db_migration_state_missing_uri_mode_file_is_not_created(tmp_path): + ini = write_alembic_project(tmp_path, ["a:"]) + path = tmp_path / "app.db" + + state = await read_migration_state(f"sqlite+aiosqlite:///file:{path}?uri=true", ini) + + assert state is not None + assert state.current == set() + assert not path.exists() diff --git a/framework/hosting/simple_module_hosting/_deferred_startup.py b/framework/hosting/simple_module_hosting/_deferred_startup.py index d2b67e2bd..55022be3f 100644 --- a/framework/hosting/simple_module_hosting/_deferred_startup.py +++ b/framework/hosting/simple_module_hosting/_deferred_startup.py @@ -11,6 +11,7 @@ from simple_module_db.stored_settings import SessionOverrideSource, settings_table_exists from simple_module_hosting._hydrate_step import hydrate_all +from simple_module_hosting._table_coverage import add_table_coverage from simple_module_hosting.data_compatibility import check_data_compatibility logger = logging.getLogger(__name__) @@ -52,6 +53,7 @@ async def run_deferred_startup(app: FastAPI) -> None: pending_checks = getattr(app.state, "deferred_data_compatibility", None) if pending_checks: await check_data_compatibility(app, pending_checks) + await add_table_coverage(app, pending_checks) app.state.deferred_data_compatibility = [] deferred = get_deferred_startup(app) if not deferred: diff --git a/framework/hosting/simple_module_hosting/_lifespan.py b/framework/hosting/simple_module_hosting/_lifespan.py index ef3c0ca5c..8bbf49840 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._table_coverage import add_table_coverage 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 @@ -89,6 +90,7 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: migration = app.state.migration if migration["is_current"] and migration.get("current_revision") is not None: await check_data_compatibility(app, modules) + await add_table_coverage(app, modules) else: app.state.deferred_data_compatibility = list(modules) diff --git a/framework/hosting/simple_module_hosting/_table_coverage.py b/framework/hosting/simple_module_hosting/_table_coverage.py new file mode 100644 index 000000000..de9201303 --- /dev/null +++ b/framework/hosting/simple_module_hosting/_table_coverage.py @@ -0,0 +1,44 @@ +"""SM011 at a development boot, once the database is open and at head (#895).""" + +from __future__ import annotations + +import logging +from collections.abc import Sequence + +from fastapi import FastAPI +from simple_module_core.diagnostics import Diagnostic, print_diagnostics +from simple_module_core.diagnostics._migration import MigrationDiagnostics, module_table_names +from simple_module_core.module import ModuleBase +from sqlalchemy import inspect + +logger = logging.getLogger(__name__) + + +async def add_table_coverage(app: FastAPI, modules: Sequence[ModuleBase]) -> None: + """Report module tables the migrated database lacks, and keep them for the Doctor screen. + + The boot diagnostics run while the app is built, before the database is + open, so this half runs from the lifespan. Only development keeps a runner; + wrapping it lets the Doctor screen's ``rerun()`` show the findings again. + Call it only for a database at every head: behind head, a missing table may + simply be in a pending revision. + """ + state = getattr(app.state.sm, "diagnostics", None) + runner = state.runner if state is not None else None + if state is None or runner is None: + return + try: + async with app.state.sm.db.engine.connect() as conn: + tables = set(await conn.run_sync(lambda sync: inspect(sync).get_table_names())) + except Exception: + # A diagnostics-only read must never fail the boot or the wizard's migrations action. + logger.warning("SM011 table coverage could not be read; skipped", exc_info=True) + return + found: list[Diagnostic] = MigrationDiagnostics().check_table_coverage( + module_table_names(modules), tables + ) + if not found: + return + print_diagnostics(found) + state.runner = lambda: [*runner(), *found] + state.results = [*state.results, *found] diff --git a/framework/hosting/simple_module_hosting/bootstrap_settings.py b/framework/hosting/simple_module_hosting/bootstrap_settings.py index 8fc68a23b..7b0e32426 100644 --- a/framework/hosting/simple_module_hosting/bootstrap_settings.py +++ b/framework/hosting/simple_module_hosting/bootstrap_settings.py @@ -22,32 +22,12 @@ from pydantic import model_validator from pydantic_settings import BaseSettings, SettingsConfigDict -from simple_module_core.dotenv import find_env_file +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file from simple_module_core.environments import NON_PROD_ENVIRONMENTS PLACEHOLDER_SECRET_KEY = "change-me-in-production" -def _absolutize_sqlite_url(url: str, *, anchor: Path) -> str: - """Rewrite a relative sqlite path to an absolute one under ``anchor``. - - ``sqlite+aiosqlite:///./host/app.db`` means "relative to the project - root" by convention, but SQLAlchemy resolves it against the process cwd - — correct in the web process (which chdirs) and silently wrong in every - CLI run from a subdirectory. Absolute paths (``:////...``), ``:memory:``, - SQLite URI-mode paths (``file:...?uri=true``), and non-sqlite URLs pass - through untouched. - """ - if not url.startswith("sqlite"): - return url - scheme, sep, rest = url.partition(":///") - if not sep or not rest or rest.startswith(("/", ":memory:", "file:")): - return url - path_part, query_sep, query = rest.partition("?") - resolved = (anchor / path_part).resolve() - return f"{scheme}:///{resolved}{query_sep}{query}" - - class BootstrapSettings(BaseSettings): """Pre-DB bootstrap environment knobs.""" @@ -77,7 +57,7 @@ def __init__(self, **values: Any) -> None: anchor = ( Path(env_file).parent if isinstance(env_file, (str, Path)) else find_env_file().parent ) - self.database_url = _absolutize_sqlite_url(self.database_url, anchor=anchor) + self.database_url = absolutize_sqlite_url(self.database_url, anchor=anchor) database_url: str = "sqlite+aiosqlite:///./app.db" diff --git a/framework/hosting/tests/test_hosting_table_coverage.py b/framework/hosting/tests/test_hosting_table_coverage.py new file mode 100644 index 000000000..95913f266 --- /dev/null +++ b/framework/hosting/tests/test_hosting_table_coverage.py @@ -0,0 +1,108 @@ +"""A development boot reports SM011 once the database is open (#895).""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +import sqlalchemy as sa +from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel +from simple_module_core.services import DiagnosticsState +from simple_module_hosting import _table_coverage +from simple_module_hosting._table_coverage import add_table_coverage +from sqlalchemy.ext.asyncio import create_async_engine + +_BOOT = Diagnostic( + level=DiagnosticLevel.WARNING, code="SM019", message="boot finding", module_name="Demo" +) + + +@pytest.fixture +async def engine(): + engine = create_async_engine("sqlite+aiosqlite://") + async with engine.begin() as conn: + await conn.execute(sa.text("CREATE TABLE orders_order (id INTEGER)")) + yield engine + await engine.dispose() + + +def _app(engine, state: DiagnosticsState) -> SimpleNamespace: + return SimpleNamespace( + state=SimpleNamespace( + sm=SimpleNamespace(diagnostics=state, db=SimpleNamespace(engine=engine)) + ) + ) + + +@pytest.fixture(autouse=True) +def _module_tables(monkeypatch): + monkeypatch.setattr( + _table_coverage, "module_table_names", lambda modules: {"orders_order", "orders_line"} + ) + + +async def test_hosting_table_coverage_survives_rerun(engine, capsys): + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(engine, state), []) + + assert [d.code for d in state.results] == ["SM019", "SM011"] + assert "orders_line" in state.results[1].message + assert "SM011" in capsys.readouterr().err + assert [d.code for d in state.rerun()] == ["SM019", "SM011"] + + +async def test_hosting_table_coverage_is_quiet_when_covered(engine, monkeypatch): + monkeypatch.setattr(_table_coverage, "module_table_names", lambda modules: {"orders_order"}) + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(engine, state), []) + + assert [d.code for d in state.rerun()] == ["SM019"] + + +async def test_hosting_table_coverage_skips_when_the_database_cannot_be_read(caplog): + # A diagnostics-only read must not fail the boot or the wizard's migrations action. + class _BrokenEngine: + def connect(self): + raise RuntimeError("database is gone") + + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + + await add_table_coverage(_app(_BrokenEngine(), state), []) + + assert [d.code for d in state.rerun()] == ["SM019"] + assert "SM011 table coverage could not be read" in caplog.text + + +async def test_hosting_table_coverage_skips_without_a_runner(engine): + # Outside development no diagnostics run, so the database is not inspected. + state = DiagnosticsState() + + await add_table_coverage(_app(engine, state), []) + + assert state.results == [] + assert state.supported is False + + +async def test_hosting_table_coverage_runs_after_the_wizard_migrates(engine, monkeypatch): + # A first run defers the check; replaying deferred startup reports it once at head. + from simple_module_hosting import _deferred_startup + + async def no_compat_issues(app, modules): + return None + + monkeypatch.setattr(_deferred_startup, "check_data_compatibility", no_compat_issues) + state = DiagnosticsState(runner=lambda: [_BOOT]) + state.rerun() + app = _app(engine, state) + app.state.deferred_data_compatibility = ["module"] + app.state.deferred_startup = [] + + await _deferred_startup.run_deferred_startup(app) + + assert [d.code for d in state.rerun()] == ["SM019", "SM011"] + assert app.state.deferred_data_compatibility == [] diff --git a/framework/hosting/tests/test_sqlite_path_anchor.py b/framework/hosting/tests/test_sqlite_path_anchor.py index 2f763a3cc..266259983 100644 --- a/framework/hosting/tests/test_sqlite_path_anchor.py +++ b/framework/hosting/tests/test_sqlite_path_anchor.py @@ -19,42 +19,41 @@ from pathlib import Path import pytest -from simple_module_core.dotenv import find_env_file -from simple_module_hosting.bootstrap_settings import _absolutize_sqlite_url +from simple_module_core.dotenv import absolutize_sqlite_url, find_env_file class TestAbsolutize: def test_relative_path_resolves_against_anchor(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///./host/app.db", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///./host/app.db", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'host' / 'app.db'}" def test_bare_relative_path_resolves(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///app.db", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///app.db", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'app.db'}" def test_absolute_path_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite:////var/data/app.db" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_memory_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite:///:memory:" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_uri_mode_untouched(self, tmp_path: Path) -> None: """SQLite URI-mode paths (shared in-memory DBs) are not file paths.""" url = "sqlite+aiosqlite:///file:shared?mode=memory&cache=shared&uri=true" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_bare_scheme_untouched(self, tmp_path: Path) -> None: url = "sqlite+aiosqlite://" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_postgres_untouched(self, tmp_path: Path) -> None: url = "postgresql+asyncpg://u:p@localhost/db" - assert _absolutize_sqlite_url(url, anchor=tmp_path) == url + assert absolutize_sqlite_url(url, anchor=tmp_path) == url def test_query_string_survives(self, tmp_path: Path) -> None: - url = _absolutize_sqlite_url("sqlite+aiosqlite:///app.db?mode=ro", anchor=tmp_path) + url = absolutize_sqlite_url("sqlite+aiosqlite:///app.db?mode=ro", anchor=tmp_path) assert url == f"sqlite+aiosqlite:///{tmp_path / 'app.db'}?mode=ro" diff --git a/skills/simple-module-doctor/SKILL.md b/skills/simple-module-doctor/SKILL.md index 8a3dfe446..3a9cf6e0d 100644 --- a/skills/simple-module-doctor/SKILL.md +++ b/skills/simple-module-doctor/SKILL.md @@ -9,7 +9,7 @@ description: Use when interpreting a simple_module_python diagnostic code (SM001 The same diagnostics also run at host boot, but **only in development** (`SM_ENVIRONMENT == development`) — they print to the boot logs and `raise SystemExit` if there's any ERROR. **In production the diagnostics pass is skipped**; what protects prod is that module *discovery* runs strictly (any bad entry point — broken import, missing `meta`, non-`ModuleBase` class — or a missing `depends_on` target raises and fails the boot). So "errors fail prod boot" is true for discovery-level breakage (surfaced as **SM001**), not for the per-finding checks below, which you'll catch via `make doctor` / dev boot before you ever deploy. -You can also call `run_diagnostics(...)` from `simple_module_core` programmatically (e.g. in a CI job) to surface issues without booting the full app — pass `migration_state=` / `module_tables=` / `migrated_tables=` to additionally get **SM010/SM011** (the migration checks below), and `i18n_supported_locales=` / `i18n_default_locale=` to get the locale checks. +You can also call `run_diagnostics(...)` from `simple_module_core` programmatically (e.g. in a CI job) to surface issues without booting the full app — pass `migration_state=` / `module_tables=` / `migrated_tables=` to additionally get **SM010/SM011** (`make doctor` reads those from the database itself), and `i18n_supported_locales=` / `i18n_default_locale=` to get the locale checks. ## Code reference @@ -21,8 +21,8 @@ You can also call `run_diagnostics(...)` from `simple_module_core` programmatica | **SM007** | INFO | Module overrides no `register_*` hooks at all — likely scaffolded but empty | Either implement at least one hook or delete the module if unused | | **SM008** | ERROR | Two modules declare the same `ModuleMeta.name`, or two names collide once lowercased as table prefixes in the shared schema (one finding per collision, from either the duplicate-name or the schema-conflict check) | Rename one module's `meta.name` | | **SM009** | ERROR | A `framework/*` package directly imports from a plugin module (`modules/*`) | Move the symbol *up* into `simple_module_core` / `simple_module_hosting`, or invert the dependency via the event bus / a registry | -| **SM010** | ERROR | Live DB revision is behind the migration head | Run `make migrate` before booting; in CI, ensure migrations are part of the deploy step. *Note:* only emitted when a caller passes `migration_state=` to `run_diagnostics` — `make doctor` and dev boot do **not** check this. The live boot enforces it separately: `check_migrations()` raises a plain `RuntimeError` (not an SM code) in dev and prod if the DB is behind head | -| **SM011** | WARNING | A module declares a SQLModel table that has no entry in migration history | Run `make migration msg="..."` and apply. *Note:* only emitted when `module_tables=`/`migrated_tables=` are passed to `run_diagnostics`; not surfaced by `make doctor` or boot | +| **SM010** | ERROR / WARNING | `make doctor` compares `alembic_version` in `SM_DATABASE_URL` with the heads in `alembic.ini` (else `host/alembic.ini`): ERROR when any head is missing, WARNING when the DB was never migrated or could not be read | Run `make migrate` before booting; in CI, ensure migrations are part of the deploy step. The live boot enforces it separately: the lifespan raises a plain `RuntimeError` in dev and prod if the DB is behind head (a never-migrated first run goes to `/setup`) | +| **SM011** | WARNING | A table in a module's `models` is missing from a database at every head, so no migration creates it. Reported by `make doctor` and by a development boot once the database is open; skipped while the DB is behind head | Run `make migration msg="..."`, review the revision, apply | | **SM012** | WARNING (dev only) | `register_settings` is overridden but nothing was added to `app.state.` | Either store the module's settings dataclass on `app.state.` or remove the empty override | | **SM013** | WARNING | A locale file declared in `locale_dirs()` is missing for one of the supported locales | Create the file (even if empty) or trim `SM_I18N_SUPPORTED_LOCALES` | | **SM014** | WARNING | A non-default locale is missing keys present in the default locale | Add the missing keys, or accept that a fallback to default applies |