Chris CoutinhoandClaude Opus 4.7
e98903c502
claude-review on #799 flagged: 1. Stale inline comment in ``initialize()`` (line 466) still said "Postgres uses a small bounded pool". Updated to reflect both backends now use NullPool. 2. Stale ``close()`` docstring referenced pool-size starving max_connections — irrelevant with NullPool. Replaced with the NullPool-aware rationale (dispose still tears down in-flight asyncpg connections cleanly). 3. ``docs/configuration.md`` actively directed operators to tune DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW, with worked examples and pool math. Both are now deprecated no-ops; the table entries explain the deprecation and link to PR #799. Operators reading the docs will no longer be confused into tuning settings that don't do anything. 4. ``config.py`` comment for the deprecated fields updated to record the deprecation. Validators are intentionally kept (still reject < 1 / < 0) so misconfigured deploys fail loudly rather than silently — the reviewer flagged this as a minor UX wart but explicitly "not a blocker"; the docs change in (3) keeps operators away from the config altogether. 5. New ``tests/unit/test_storage_engine.py`` with three tests: - ``test_postgres_engine_uses_nullpool`` — pins ``isinstance( engine.pool, NullPool)`` so a refactor back to QueuePool / SingletonThreadPool can't silently re-introduce the cross- event-loop crashes. - ``test_postgres_engine_ignores_pool_sizing_settings`` — setting DATABASE_POOL_SIZE / DATABASE_MAX_OVERFLOW to huge values must not change pool type (proves the deprecated fields are wired-up no-ops). - ``test_postgres_engine_missing_asyncpg_driver_message`` — guards the existing actionable-error branch when the optional ``[postgres]`` extra isn't installed. Verified: - ``uv run pytest tests/unit/`` — 1028 passed - ``uv run ruff check`` clean on the touched python files Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
89 lines
3.7 KiB
Python
89 lines
3.7 KiB
Python
"""Unit tests for ``RefreshTokenStorage._build_postgres_engine``.
|
|
|
|
PR #799 switched the Postgres engine from ``AsyncAdaptedQueuePool``
|
|
to ``NullPool`` to eliminate cross-event-loop crashes under anyio
|
|
TaskGroups. The method is factored out explicitly so a future
|
|
engine-arg unit test has a single seam to mock — these tests pin
|
|
the pool class and the connect-args plumbing so a refactor can't
|
|
silently regress to a sharing pool.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
from sqlalchemy.ext.asyncio import AsyncEngine
|
|
from sqlalchemy.pool import NullPool
|
|
|
|
from nextcloud_mcp_server.auth.storage import RefreshTokenStorage
|
|
|
|
pytestmark = pytest.mark.unit
|
|
|
|
|
|
def _storage(url: str) -> RefreshTokenStorage:
|
|
# ``encryption_key=None`` is fine for engine-shape tests; the
|
|
# cipher is only constructed lazily for cipher-protected ops,
|
|
# and these tests never call those.
|
|
return RefreshTokenStorage(database_url=url, encryption_key=None)
|
|
|
|
|
|
# The unit-test environment may not have the optional ``[postgres]``
|
|
# extra installed (asyncpg is the C-extension dep). Skip the
|
|
# engine-construction tests when asyncpg isn't importable rather
|
|
# than hitting the "DATABASE_URL points at Postgres via asyncpg but
|
|
# the 'asyncpg' driver is not installed" guard — that branch is
|
|
# exercised explicitly by ``test_postgres_engine_missing_asyncpg_driver_message``
|
|
# below.
|
|
asyncpg_required = pytest.importorskip("asyncpg")
|
|
|
|
|
|
def test_postgres_engine_uses_nullpool():
|
|
"""The Postgres engine must use ``NullPool`` to avoid cross-loop
|
|
crashes under anyio TaskGroups (see PR #799)."""
|
|
storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp")
|
|
engine = storage._build_postgres_engine()
|
|
|
|
assert isinstance(engine, AsyncEngine)
|
|
# ``engine.pool`` is the sync proxy pool; the underlying pool
|
|
# class is what we care about for the loop-binding behaviour.
|
|
assert isinstance(engine.pool, NullPool), (
|
|
f"expected NullPool, got {type(engine.pool).__name__} — a regression "
|
|
"to QueuePool/SingletonThreadPool will re-introduce the cross-event-"
|
|
"loop crashes from PR #799"
|
|
)
|
|
|
|
|
|
def test_postgres_engine_ignores_pool_sizing_settings(monkeypatch: pytest.MonkeyPatch):
|
|
"""``DATABASE_POOL_SIZE`` / ``DATABASE_MAX_OVERFLOW`` are kept as
|
|
deprecated no-ops for backward compat. NullPool has no concept of
|
|
these, so changing them must not raise or change pool type."""
|
|
# Stash arbitrarily large values into the settings the engine
|
|
# consults; NullPool is parameterless so the engine should ignore
|
|
# them entirely.
|
|
from nextcloud_mcp_server import config as cfg
|
|
|
|
monkeypatch.setattr(cfg.get_settings(), "database_pool_size", 99, raising=False)
|
|
monkeypatch.setattr(cfg.get_settings(), "database_max_overflow", 99, raising=False)
|
|
|
|
storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp")
|
|
engine = storage._build_postgres_engine()
|
|
assert isinstance(engine.pool, NullPool)
|
|
|
|
|
|
def test_postgres_engine_missing_asyncpg_driver_message(
|
|
monkeypatch: pytest.MonkeyPatch,
|
|
):
|
|
"""When the ``+asyncpg`` dialect is requested but the asyncpg
|
|
optional dep isn't installed, the engine builder must surface an
|
|
actionable error before SQLAlchemy emits its generic
|
|
``ModuleNotFoundError``."""
|
|
import importlib.util
|
|
|
|
def _fake_find_spec(name: str):
|
|
return None if name == "asyncpg" else importlib.util.find_spec(name)
|
|
|
|
monkeypatch.setattr(importlib.util, "find_spec", _fake_find_spec)
|
|
|
|
storage = _storage("postgresql+asyncpg://mcp:placeholder@db.example.com:5432/mcp")
|
|
with pytest.raises(RuntimeError, match="asyncpg.*not installed"):
|
|
storage._build_postgres_engine()
|