Addresses all 8 items in the round-4 bot review plus 4 remaining SonarQube OPEN issues that were silently broken by round 3's malformed NOSONAR markers. NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues) ---------------------------------------------------------- Round 3 used ``# NOSONAR S<rule_key>`` form. SonarQube Python doesn't recognize the rule-key suffix — it treats the whole thing as a malformed suppression directive (S7632) AND lets the underlying rule keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``). Switch every marker to bare ``# NOSONAR``, with the rationale moved into a preceding comment block. Affected sites: - storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__`` - config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()`` - test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant - test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False` -------------------------------------------------------------- Bot predicted S4830 fires on the operator-opt-out path. SQ output shows it doesn't currently fire, but bare NOSONAR added defensively with rationale comment. Bot 🔴#2 — defensive NOSONAR on f-string SQL -------------------------------------------- ``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``; ``get_audit_logs`` builds its WHERE clause via string concatenation. Both are safe (the fragments only come from this function's own branches, no user input), but the patterns trip taint analysers. Annotated both with bare NOSONAR + safety comment explaining the hardcoded-fragments invariant. Note: S2077 doesn't currently fire on these; defensive. Bot 🟡#3 — pg_advisory_lock for concurrent migrations ----------------------------------------------------- Without coordination, two pods rolling-updating simultaneously can both observe ``has_alembic=False`` and both try to apply migrations from scratch — the second crashes with "relation already exists". New ``_migration_lock()`` async context manager: - On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh connection (separate from the engine pool so it survives the ``to_thread.run_sync`` worker), held across BOTH the schema-inspect AND the migration call. Without that span, two pods could each observe "no alembic_version" before either started migrating, defeating the lock. - On SQLite: yields immediately (file-level locking serializes writes natively). Lock ID derived from ``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed int64 so we can't collide with other apps sharing the same Postgres. Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring -------------------------------------------------------- New idempotent ``close()`` method calls ``await engine.dispose()``, nulls the engine, resets ``_initialized``. Wired into both ``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown, each wrapped in ``try/except Exception`` with ``logger.warning`` so a buggy dispose can't block SIGTERM. Without this, pooled asyncpg connections leak server-side slots until ``idle_in_transaction_session_timeout`` reaps them — with small pool defaults and frequent k8s rolling restarts this can starve ``max_connections``. Bot 🟢#5 — is_sqlite_url docstring on :memory: ---------------------------------------------- Updated docstring to note both file-backed and in-memory forms are recognized; caller is responsible for ``:memory:`` magic. Bot 🟢#6 — db_path via make_url(...).database --------------------------------------------- Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's own URL parsing. Naturally handles in-memory (``.database is None`` → falls back to ``""``). Same lazy-import pattern as the existing ``mask_db_password`` to avoid module-import-time cost. Bot 🟢#7 — _to_sync_url unrecognized-driver guard ------------------------------------------------- Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a module constant. When an unrecognized ``+<driver>`` token survives the strip, emits ``logger.warning`` with the known-supported list. Behavior unchanged for valid URLs. Bot 🟢#8 — get_audit_logs SELECT * → explicit columns ----------------------------------------------------- Replaced ``SELECT *`` with explicit column list. Future schema additions stay out of the dict return. New tests --------- - ``test_close_disposes_engine``: pins the public contract — engine nulled, state reset, second call is a no-op. - ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns 3 concurrent inits against a fresh schema; asserts no "relation already exists" and exactly one ``alembic_version`` row at the end. Without the lock, this reliably fails on the second concurrent task. Docs ---- - ADR-026: new "Concurrent migrations across pods" subsection documents the advisory-lock approach + lock-ID derivation. Verification ------------ - ``uv run pytest tests/unit/`` — 1025 passed. - ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7). - ``ruff check && ruff format --check && ty check`` — clean. Expected post-push: SQ scan reports 0 OPEN issues (was 4). 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>
203 lines
7.1 KiB
Python
203 lines
7.1 KiB
Python
"""Database migration utilities for nextcloud-mcp-server.
|
|
|
|
This module provides helper functions for managing Alembic database migrations
|
|
programmatically. It enables automatic migration on application startup and
|
|
provides CLI integration.
|
|
|
|
All helpers accept a SQLAlchemy URL (``sqlite+aiosqlite:///...`` or
|
|
``postgresql+asyncpg://...``). When called without an explicit URL they fall
|
|
back to :func:`nextcloud_mcp_server.config.get_database_url`.
|
|
"""
|
|
|
|
import logging
|
|
from pathlib import Path
|
|
|
|
from alembic.config import Config
|
|
from sqlalchemy import create_engine, inspect, text
|
|
|
|
import nextcloud_mcp_server.alembic as alembic_package
|
|
from alembic import command
|
|
from nextcloud_mcp_server.config import get_database_url, mask_db_password
|
|
|
|
logger = logging.getLogger(__name__)
|
|
|
|
|
|
def _coerce_url(database_url: str | Path | None) -> str:
|
|
"""Accept either a URL string, a Path (legacy SQLite path), or None.
|
|
|
|
A bare ``Path`` is interpreted as a SQLite database file for backward
|
|
compatibility with the prior path-based API.
|
|
"""
|
|
if database_url is None:
|
|
return get_database_url()
|
|
if isinstance(database_url, Path):
|
|
return f"sqlite+aiosqlite:///{database_url.resolve()}"
|
|
return database_url
|
|
|
|
|
|
_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")
|
|
|
|
|
|
def _to_sync_url(database_url: str) -> str:
|
|
"""Map an async driver URL to its sync equivalent for blocking inspection.
|
|
|
|
SQLAlchemy's :func:`inspect` and :func:`create_engine` used below are
|
|
synchronous APIs. The runtime uses async drivers (``aiosqlite``,
|
|
``asyncpg``) but Alembic and these utility queries don't need them.
|
|
|
|
Emits a one-shot warning when the URL carries an async-driver
|
|
suffix we don't recognize — the sync engine creation downstream
|
|
will still fail, but with a clearer hint than SQLAlchemy's generic
|
|
"Can't load plugin" error.
|
|
"""
|
|
out = database_url
|
|
for driver in _KNOWN_ASYNC_DRIVERS:
|
|
out = out.replace(f"+{driver}", "")
|
|
# Detect a leftover ``+<driver>`` token (we know the URL is
|
|
# ``scheme[+driver]://...``, so a remaining ``+`` before ``://``
|
|
# means an unrecognized async driver). Log once and pass through.
|
|
head = out.split("://", 1)[0]
|
|
if "+" in head:
|
|
unknown = head.split("+", 1)[1]
|
|
logger.warning(
|
|
"_to_sync_url: unrecognized driver %r in DATABASE_URL; "
|
|
"passing through unchanged. Supported async drivers: %s",
|
|
unknown,
|
|
", ".join(_KNOWN_ASYNC_DRIVERS),
|
|
)
|
|
return out
|
|
|
|
|
|
def get_alembic_config(database_url: str | Path | None = None) -> Config:
|
|
"""
|
|
Get Alembic configuration for programmatic use.
|
|
|
|
Works in both development and installed (Docker) modes by using
|
|
package location instead of alembic.ini file.
|
|
|
|
Args:
|
|
database_url: SQLAlchemy URL. If None, resolves via
|
|
:func:`get_database_url` (DATABASE_URL env var, falling back
|
|
to the ephemeral SQLite tempfile under ``TOKEN_STORAGE_DB``).
|
|
For backward compatibility a ``Path`` is treated as a SQLite
|
|
file path.
|
|
|
|
Returns:
|
|
Alembic Config object configured for the resolved URL.
|
|
"""
|
|
if alembic_package.__file__ is None:
|
|
raise RuntimeError("alembic package __file__ is None")
|
|
script_location = Path(alembic_package.__file__).parent
|
|
|
|
config = Config()
|
|
config.set_main_option("script_location", str(script_location))
|
|
config.set_main_option("path_separator", "os")
|
|
|
|
url = _coerce_url(database_url)
|
|
config.set_main_option("sqlalchemy.url", url)
|
|
|
|
logger.debug("Alembic script location: %s", script_location)
|
|
logger.debug("Database URL: %s", mask_db_password(url))
|
|
|
|
return config
|
|
|
|
|
|
def upgrade_database(
|
|
database_url: str | Path | None = None, revision: str = "head"
|
|
) -> None:
|
|
"""Upgrade database to a specific revision (default: latest)."""
|
|
config = get_alembic_config(database_url)
|
|
logger.info("Upgrading database to revision: %s", revision)
|
|
command.upgrade(config, revision)
|
|
logger.info("Database upgrade completed successfully")
|
|
|
|
|
|
def downgrade_database(
|
|
database_url: str | Path | None = None, revision: str = "-1"
|
|
) -> None:
|
|
"""Downgrade database to a specific revision (default: previous)."""
|
|
config = get_alembic_config(database_url)
|
|
logger.warning("Downgrading database to revision: %s", revision)
|
|
command.downgrade(config, revision)
|
|
logger.info("Database downgrade completed successfully")
|
|
|
|
|
|
def get_current_revision(database_url: str | Path | None = None) -> str | None:
|
|
"""
|
|
Get the current database revision by reading the ``alembic_version`` table.
|
|
|
|
Returns ``None`` when the database does not exist or has no
|
|
``alembic_version`` table (i.e. has never been migrated).
|
|
"""
|
|
url = _to_sync_url(_coerce_url(database_url))
|
|
|
|
if url.startswith("sqlite:///"):
|
|
path = url[len("sqlite:///") :]
|
|
if path and not Path(path).exists():
|
|
logger.debug("Database does not exist: %s", path)
|
|
return None
|
|
|
|
try:
|
|
engine = create_engine(url, future=True)
|
|
try:
|
|
inspector = inspect(engine)
|
|
if not inspector.has_table("alembic_version"):
|
|
return None
|
|
with engine.connect() as conn:
|
|
row = conn.execute(
|
|
text("SELECT version_num FROM alembic_version")
|
|
).fetchone()
|
|
return row[0] if row else None
|
|
finally:
|
|
engine.dispose()
|
|
except Exception as e:
|
|
logger.error("Failed to get current revision: %s", e)
|
|
return None
|
|
|
|
|
|
def stamp_database(
|
|
database_url: str | Path | None = None, revision: str = "head"
|
|
) -> None:
|
|
"""
|
|
Stamp database with a specific revision without running migrations.
|
|
|
|
Useful for marking pre-Alembic databases as already at a known revision.
|
|
"""
|
|
config = get_alembic_config(database_url)
|
|
logger.info("Stamping database with revision: %s", revision)
|
|
command.stamp(config, revision)
|
|
logger.info("Database stamped successfully")
|
|
|
|
|
|
def show_migration_history(database_url: str | Path | None = None) -> None:
|
|
"""Display migration history."""
|
|
config = get_alembic_config(database_url)
|
|
command.history(config, verbose=True)
|
|
|
|
|
|
def create_migration(message: str, autogenerate: bool = False) -> None:
|
|
"""
|
|
Create a new migration script.
|
|
|
|
Args:
|
|
message: Description of the migration
|
|
autogenerate: Whether to attempt auto-generation (requires SQLAlchemy models)
|
|
|
|
Note:
|
|
Since we don't use SQLAlchemy models, autogenerate will be disabled
|
|
and migrations must be written manually using portable Alembic
|
|
operations (``op.create_table``, ``op.add_column`` …) rather than
|
|
raw SQL so they work on both SQLite and Postgres.
|
|
"""
|
|
config = get_alembic_config()
|
|
logger.info("Creating new migration: %s", message)
|
|
|
|
if autogenerate:
|
|
logger.warning(
|
|
"Auto-generation is not supported (no SQLAlchemy models). "
|
|
"Migration will be created with empty upgrade/downgrade functions."
|
|
)
|
|
|
|
command.revision(config, message=message, autogenerate=False)
|
|
logger.info("Migration created successfully. Edit the file to add SQL statements.")
|