feat(storage): pluggable database backend via DATABASE_URL (ADR-026)

Adds a `DATABASE_URL` setting that lets `RefreshTokenStorage` run against any SQLAlchemy async backend, primarily `postgresql+asyncpg://...` for HA k8s deployments. Default behavior is unchanged: when `DATABASE_URL` is unset the server falls back to the existing `TOKEN_STORAGE_DB` path / ephemeral SQLite tempfile. Why --- Today every MCP pod needs its own PVC to hold the SQLite file, which pins the Deployment to one replica and blocks horizontal scaling. With this change, operators can point all replicas at a shared Postgres (CNPG, RDS, etc.) and the pods become stateless. Encryption stays in Python (Fernet); the database only sees ciphertext. What changed ------------ - `config.get_database_url()` resolves DATABASE_URL → TOKEN_STORAGE_DB → ephemeral tempfile in that priority order. - `RefreshTokenStorage` builds a process-shared `AsyncEngine` in `initialize()`. SQLite gets NullPool; Postgres gets pool_size=10, max_overflow=20, pool_pre_ping=True. 30 aiosqlite call sites adapted via a thin `_DBConn` / `_Cursor` / `_Row` / `_ExecuteCtx` shim so existing method bodies need no churn beyond the connection context-manager swap. - 7 `INSERT OR REPLACE` statements rewritten as portable `INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5). - `sqlite_master` legacy-detection lookup replaced with SQLAlchemy inspector so the path works against either backend. - File-permission hardening + parent-dir creation gated on `is_sqlite_url(...)` — centralized backends manage their own filesystem. - Alembic migrations 001/002/003/005 converted from raw `op.execute(SQL)` to portable `op.create_table()` / `op.create_index()` with SQLAlchemy types. All timestamp columns are `sa.BigInteger` so Postgres allocates BIGINT (unix epochs don't fit in INT4). SQLite treats BIGINT as INTEGER, so existing deployments at revision 006 see no schema drift. - `migrations.py` + CLI take URLs; `db {upgrade,downgrade,current,history}` gain `--database-url / -u` alongside the legacy `--database-path / -d`. `get_current_revision()` uses SQLAlchemy inspector instead of raw sqlite3, so the CLI works against Postgres too. - `docker-compose.yml` adds a `postgres-test` service under the `postgres` profile (pinned `postgres:16-alpine` digest) for integration testing. - Unit storage tests parametrized over backends via shared `tests/fixtures/storage_backend.py` — every test in `test_app_password_storage.py` and `test_webhook_storage.py` runs once per backend that is available. Postgres is opted in by `TEST_DATABASE_URL`. - New `tests/integration/test_storage_postgres.py` (5 tests, marked `postgres` + `integration`) covers refresh-token, app-password, OAuth-session, webhook, and audit-log paths end-to-end on Postgres. - New `docs/ADR-026-pluggable-database-backend.md` records the decision; `docs/configuration.md` documents `DATABASE_URL` with examples. Out of scope ------------ - No SQLite → Postgres data migration tool (clean cutover; tokens reissue on next login, webhooks re-register on next sync tick). - This repo does not provision Postgres. The matching helm chart change lives in cbcoutinho/helm-charts (database.url / existingSecret values). Verification ------------ - `uv run pytest tests/unit/` — 1012 passed, SQLite path unchanged. - `docker compose --profile postgres up -d postgres-test` - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres -v` — 5 passed. - `TEST_DATABASE_URL=... uv run pytest tests/unit/test_app_password_storage.py tests/unit/test_webhook_storage.py` — 50 passed (25 per backend). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-16 18:06:42 +02:00
parent 15a7e680a6
commit 292cbb3292
19 changed files with 1402 additions and 588 deletions
@@ -5,8 +5,8 @@ import click
 import uvicorn

 from nextcloud_mcp_server.config import (
+    get_database_url,
    get_settings,
-    get_token_db_path,
    is_ephemeral_token_db,
 )
 from nextcloud_mcp_server.migrations import (
@@ -287,27 +287,67 @@ def db():
    pass


-def _warn_if_ephemeral(database_path: str) -> None:
-    if is_ephemeral_token_db(database_path):
+def _resolve_db_url(database_url: str | None, database_path: str | None) -> str:
+    """Pick the database URL for a CLI subcommand.
+
+    Priority: explicit ``--database-url`` > legacy ``--database-path``
+    (treated as a SQLite file) > :func:`get_database_url` (honors
+    ``DATABASE_URL`` env or falls back to the ephemeral SQLite tempfile).
+    """
+    if database_url:
+        return database_url
+    if database_path:
+        return f"sqlite+aiosqlite:///{database_path}"
+    return get_database_url()
+
+
+def _warn_if_ephemeral(database_url: str) -> None:
+    """Warn when the resolved URL is the per-process SQLite tempfile."""
+    if not database_url.startswith(
+        "sqlite+aiosqlite:///"
+    ) and not database_url.startswith("sqlite:///"):
+        return
+    path = database_url.split("///", 1)[1]
+    if is_ephemeral_token_db(path):
        click.echo(
            click.style(
-                f"⚠ Using ephemeral tempfile {database_path}; changes "
-                "will be lost on exit. Pass --database-path or set "
-                "TOKEN_STORAGE_DB to operate on a persistent database.",
+                f"⚠ Using ephemeral tempfile {path}; changes "
+                "will be lost on exit. Pass --database-url / --database-path "
+                "or set DATABASE_URL / TOKEN_STORAGE_DB to operate on a "
+                "persistent database.",
                fg="yellow",
            ),
            err=True,
        )


+def _db_target_options(fn):
+    """Attach the shared ``--database-url`` / ``--database-path`` options.
+
+    Using a decorator factory rather than ``**kwargs`` dict-expansion so
+    static type checkers (ty) see ``click.option`` called with literal
+    keyword arguments, which is the only form it's typed to accept.
+    """
+    fn = click.option(
+        "--database-path",
+        "-d",
+        envvar="TOKEN_STORAGE_DB",
+        default=None,
+        help="SQLite database file path. Equivalent to "
+        "--database-url sqlite+aiosqlite:///<path>.",
+    )(fn)
+    fn = click.option(
+        "--database-url",
+        "-u",
+        envvar="DATABASE_URL",
+        default=None,
+        help="SQLAlchemy URL (e.g. postgresql+asyncpg://...). Wins over --database-path.",
+    )(fn)
+    return fn
+
+
@db.command()
-@click.option(
-    "--database-path",
-    "-d",
-    envvar="TOKEN_STORAGE_DB",
-    default=None,
-    help="Path to token storage database (can also use TOKEN_STORAGE_DB env var)",
-)
+@_db_target_options
@click.option(
    "--revision",
    "-r",
@@ -315,7 +355,7 @@ def _warn_if_ephemeral(database_path: str) -> None:
    show_default=True,
    help="Target revision (default: head for latest)",
 )
-def upgrade(database_path: str | None, revision: str):
+def upgrade(database_url: str | None, database_path: str | None, revision: str):
    """Upgrade database to a specific revision.

    \b
@@ -323,17 +363,17 @@ def upgrade(database_path: str | None, revision: str):
      # Upgrade to latest version
      $ nextcloud-mcp-server db upgrade

-      # Upgrade to specific revision
-      $ nextcloud-mcp-server db upgrade --revision 001
+      # Upgrade a Postgres backend
+      $ nextcloud-mcp-server db upgrade -u postgresql+asyncpg://mcp:mcp@db/mcp

-      # Use custom database path
+      # Use custom SQLite path
      $ nextcloud-mcp-server db upgrade -d /path/to/tokens.db
    """
-    database_path = database_path or get_token_db_path()
-    _warn_if_ephemeral(database_path)
+    url = _resolve_db_url(database_url, database_path)
+    _warn_if_ephemeral(url)
    try:
        click.echo(f"Upgrading database to revision: {revision}")
-        upgrade_database(database_path, revision)
+        upgrade_database(url, revision)
        click.echo(click.style("✓ Database upgraded successfully", fg="green"))
    except Exception as e:
        click.echo(click.style(f"✗ Upgrade failed: {e}", fg="red"), err=True)
@@ -341,13 +381,7 @@ def upgrade(database_path: str | None, revision: str):


@db.command()
-@click.option(
-    "--database-path",
-    "-d",
-    envvar="TOKEN_STORAGE_DB",
-    default=None,
-    help="Path to token storage database",
-)
+@_db_target_options
@click.option(
    "--revision",
    "-r",
@@ -358,27 +392,16 @@ def upgrade(database_path: str | None, revision: str):
@click.confirmation_option(
    prompt="Are you sure you want to downgrade the database? This may result in data loss."
 )
-def downgrade(database_path: str | None, revision: str):
+def downgrade(database_url: str | None, database_path: str | None, revision: str):
    """Downgrade database to a specific revision.

    WARNING: This may result in data loss! Use with caution.
-
-    \b
-    Examples:
-      # Downgrade by one version
-      $ nextcloud-mcp-server db downgrade
-
-      # Downgrade to specific revision
-      $ nextcloud-mcp-server db downgrade --revision 001
-
-      # Downgrade to base (empty database)
-      $ nextcloud-mcp-server db downgrade --revision base
    """
-    database_path = database_path or get_token_db_path()
-    _warn_if_ephemeral(database_path)
+    url = _resolve_db_url(database_url, database_path)
+    _warn_if_ephemeral(url)
    try:
        click.echo(f"Downgrading database to revision: {revision}")
-        downgrade_database(database_path, revision)
+        downgrade_database(url, revision)
        click.echo(click.style("✓ Database downgraded successfully", fg="green"))
    except Exception as e:
        click.echo(click.style(f"✗ Downgrade failed: {e}", fg="red"), err=True)
@@ -386,24 +409,13 @@ def downgrade(database_path: str | None, revision: str):


@db.command()
-@click.option(
-    "--database-path",
-    "-d",
-    envvar="TOKEN_STORAGE_DB",
-    default=None,
-    help="Path to token storage database",
-)
-def current(database_path: str | None):
-    """Show current database revision.
-
-    \b
-    Example:
-      $ nextcloud-mcp-server db current
-    """
-    database_path = database_path or get_token_db_path()
-    _warn_if_ephemeral(database_path)
+@_db_target_options
+def current(database_url: str | None, database_path: str | None):
+    """Show current database revision."""
+    url = _resolve_db_url(database_url, database_path)
+    _warn_if_ephemeral(url)
    try:
-        revision = get_current_revision(database_path)
+        revision = get_current_revision(url)
        if revision:
            click.echo(f"Current revision: {click.style(revision, fg='cyan')}")
        else:
@@ -420,25 +432,14 @@ def current(database_path: str | None):


@db.command()
-@click.option(
-    "--database-path",
-    "-d",
-    envvar="TOKEN_STORAGE_DB",
-    default=None,
-    help="Path to token storage database",
-)
-def history(database_path: str | None):
-    """Show migration history.
-
-    \b
-    Example:
-      $ nextcloud-mcp-server db history
-    """
-    database_path = database_path or get_token_db_path()
-    _warn_if_ephemeral(database_path)
+@_db_target_options
+def history(database_url: str | None, database_path: str | None):
+    """Show migration history."""
+    url = _resolve_db_url(database_url, database_path)
+    _warn_if_ephemeral(url)
    try:
        click.echo("Migration history:")
-        show_migration_history(database_path)
+        show_migration_history(url)
    except Exception as e:
        click.echo(click.style(f"✗ Failed to show history: {e}", fg="red"), err=True)
        raise click.ClickException(str(e))