test: address PR #707 reviewer feedback on config path helpers
- _resolve_settings_files() now raises FileNotFoundError when NEXTCLOUD_MCP_SETTINGS_FILE points to a missing file, instead of silently falling back to defaults (footgun on typos). - .secrets.toml is now looked for alongside the explicit settings file when NEXTCLOUD_MCP_SETTINGS_FILE is set, matching user expectation for /etc-style deployments. Unset behaviour (cwd lookup) is unchanged. - get_token_db_path() drops the redundant os.environ.get() short-circuit; TOKEN_STORAGE_DB is already bound through dynaconf because the key is declared in _DEFAULTS. - is_ephemeral_token_db() docstring documents the "must call get_token_db_path() first" precondition. - alembic.ini comment clarifies the ./tokens.db placeholder is cwd-relative by design and points readers at the -x database_url escape hatch. - New tests/unit/test_config_paths.py (12 tests) covering the ephemeral tempfile lifecycle, the TOKEN_STORAGE_DB override path, and all six _resolve_settings_files() cases including the two new behaviours. Full unit suite now at 476 passed (464 + 12 new). Ruff + ty clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
146b622ebf
commit
512de1f6b0
+5
-1
@@ -44,7 +44,11 @@ path_separator = os
|
|||||||
# Runtime code (migrations.get_alembic_config / alembic/env.get_database_url)
|
# Runtime code (migrations.get_alembic_config / alembic/env.get_database_url)
|
||||||
# overrides this with config.get_token_db_path() which defaults to an
|
# overrides this with config.get_token_db_path() which defaults to an
|
||||||
# ephemeral per-process tempfile unless TOKEN_STORAGE_DB is set.
|
# ephemeral per-process tempfile unless TOKEN_STORAGE_DB is set.
|
||||||
# This value only matters for manual `alembic` invocations from the repo root.
|
# This value only matters for manual `alembic` invocations (e.g.
|
||||||
|
# `uv run alembic current`). The `./tokens.db` path is intentionally
|
||||||
|
# resolved relative to the shell's current working directory at the time
|
||||||
|
# of invocation — run alembic from the repo root for predictable behaviour,
|
||||||
|
# or pass `-x database_url=sqlite+aiosqlite:///<abs-path>`.
|
||||||
sqlalchemy.url = sqlite+aiosqlite:///./tokens.db
|
sqlalchemy.url = sqlite+aiosqlite:///./tokens.db
|
||||||
|
|
||||||
[post_write_hooks]
|
[post_write_hooks]
|
||||||
|
|||||||
@@ -113,26 +113,34 @@ def _resolve_settings_files() -> list[str]:
|
|||||||
"""Find optional external settings files.
|
"""Find optional external settings files.
|
||||||
|
|
||||||
Priority:
|
Priority:
|
||||||
1. NEXTCLOUD_MCP_SETTINGS_FILE env var (absolute or relative path)
|
1. NEXTCLOUD_MCP_SETTINGS_FILE env var (absolute or relative path).
|
||||||
2. ./settings.toml in cwd (for docker / dev workflows)
|
If set but the file does not exist, raise FileNotFoundError —
|
||||||
3. .secrets.toml alongside whichever settings.toml was found
|
silently falling back to defaults on a typo would be a footgun.
|
||||||
|
.secrets.toml is looked for alongside the explicit file.
|
||||||
|
2. Otherwise ./settings.toml in cwd (for docker / dev workflows),
|
||||||
|
with .secrets.toml also looked for in cwd.
|
||||||
|
|
||||||
Returns an empty list if no files are present — that's fine, defaults and
|
Returns an empty list if nothing is configured — that's fine, defaults
|
||||||
env vars still apply.
|
and env vars still apply.
|
||||||
"""
|
"""
|
||||||
files: list[str] = []
|
files: list[str] = []
|
||||||
explicit = os.environ.get("NEXTCLOUD_MCP_SETTINGS_FILE")
|
explicit = os.environ.get("NEXTCLOUD_MCP_SETTINGS_FILE")
|
||||||
if explicit:
|
if explicit:
|
||||||
p = Path(explicit)
|
p = Path(explicit)
|
||||||
if p.exists():
|
if not p.exists():
|
||||||
files.append(str(p))
|
raise FileNotFoundError(
|
||||||
|
f"NEXTCLOUD_MCP_SETTINGS_FILE points to a file that does "
|
||||||
|
f"not exist: {explicit}"
|
||||||
|
)
|
||||||
|
files.append(str(p))
|
||||||
|
secrets = p.parent / ".secrets.toml"
|
||||||
else:
|
else:
|
||||||
cwd_settings = Path.cwd() / "settings.toml"
|
cwd_settings = Path.cwd() / "settings.toml"
|
||||||
if cwd_settings.exists():
|
if cwd_settings.exists():
|
||||||
files.append(str(cwd_settings))
|
files.append(str(cwd_settings))
|
||||||
cwd_secrets = Path.cwd() / ".secrets.toml"
|
secrets = Path.cwd() / ".secrets.toml"
|
||||||
if cwd_secrets.exists():
|
if secrets.exists():
|
||||||
files.append(str(cwd_secrets))
|
files.append(str(secrets))
|
||||||
return files
|
return files
|
||||||
|
|
||||||
|
|
||||||
@@ -198,14 +206,15 @@ def get_token_db_path() -> str:
|
|||||||
"""Resolve the token SQLite database path.
|
"""Resolve the token SQLite database path.
|
||||||
|
|
||||||
Priority:
|
Priority:
|
||||||
1. TOKEN_STORAGE_DB env var / dynaconf setting if explicitly set —
|
1. TOKEN_STORAGE_DB if explicitly set — docker-compose pins
|
||||||
docker-compose pins /app/data/tokens.db this way.
|
/app/data/tokens.db this way. Read via dynaconf, which picks up
|
||||||
|
the env var because TOKEN_STORAGE_DB is declared in _DEFAULTS.
|
||||||
2. Otherwise a per-process tempfile under tempfile.gettempdir(),
|
2. Otherwise a per-process tempfile under tempfile.gettempdir(),
|
||||||
allocated lazily and deleted at interpreter exit via atexit.
|
allocated lazily and deleted at interpreter exit via atexit.
|
||||||
Ephemeral: tokens are wiped on restart, matching the Qdrant
|
Ephemeral: tokens are wiped on restart, matching the Qdrant
|
||||||
":memory:" default pattern used elsewhere in this project.
|
":memory:" default pattern used elsewhere in this project.
|
||||||
"""
|
"""
|
||||||
explicit = os.environ.get("TOKEN_STORAGE_DB") or _dynaconf.get("TOKEN_STORAGE_DB")
|
explicit = _dynaconf.get("TOKEN_STORAGE_DB")
|
||||||
if explicit:
|
if explicit:
|
||||||
return str(explicit)
|
return str(explicit)
|
||||||
global _ephemeral_db_path
|
global _ephemeral_db_path
|
||||||
@@ -228,7 +237,14 @@ def get_token_db_path() -> str:
|
|||||||
|
|
||||||
|
|
||||||
def is_ephemeral_token_db(path: str) -> bool:
|
def is_ephemeral_token_db(path: str) -> bool:
|
||||||
"""Return True if the given path is the process-local ephemeral tempfile."""
|
"""Return True if the given path is the process-local ephemeral tempfile.
|
||||||
|
|
||||||
|
Precondition: `get_token_db_path()` must have been called at least once
|
||||||
|
in this process to allocate the tempfile. If called before allocation,
|
||||||
|
this returns False for any input (including the eventual tempfile path),
|
||||||
|
because there is nothing to compare against yet. In practice every call
|
||||||
|
site in this repo resolves the path via `get_token_db_path()` first.
|
||||||
|
"""
|
||||||
return path == _ephemeral_db_path
|
return path == _ephemeral_db_path
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,158 @@
|
|||||||
|
"""Tests for config.py path resolution helpers added in PR #707.
|
||||||
|
|
||||||
|
Covers:
|
||||||
|
- get_token_db_path() / is_ephemeral_token_db() — ephemeral tempfile default
|
||||||
|
with TOKEN_STORAGE_DB override.
|
||||||
|
- _resolve_settings_files() — optional external settings file discovery,
|
||||||
|
including the NEXTCLOUD_MCP_SETTINGS_FILE env var and its colocation
|
||||||
|
semantics for .secrets.toml.
|
||||||
|
"""
|
||||||
|
|
||||||
|
import os
|
||||||
|
import tempfile
|
||||||
|
from pathlib import Path
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
import nextcloud_mcp_server.config as cfg
|
||||||
|
from nextcloud_mcp_server.config import (
|
||||||
|
_reload_config,
|
||||||
|
_resolve_settings_files,
|
||||||
|
get_token_db_path,
|
||||||
|
is_ephemeral_token_db,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture(autouse=True)
|
||||||
|
def _reset_ephemeral_state(monkeypatch):
|
||||||
|
"""Reset module-global ephemeral tempfile state between tests.
|
||||||
|
|
||||||
|
get_token_db_path() memoizes its result in a module-level global and
|
||||||
|
registers an atexit hook for cleanup. Tests need an isolated slate so
|
||||||
|
assertions about "already allocated" vs "not yet" are meaningful.
|
||||||
|
"""
|
||||||
|
old = cfg._ephemeral_db_path
|
||||||
|
cfg._ephemeral_db_path = None
|
||||||
|
monkeypatch.delenv("TOKEN_STORAGE_DB", raising=False)
|
||||||
|
monkeypatch.delenv("NEXTCLOUD_MCP_SETTINGS_FILE", raising=False)
|
||||||
|
_reload_config()
|
||||||
|
yield
|
||||||
|
if cfg._ephemeral_db_path and os.path.exists(cfg._ephemeral_db_path):
|
||||||
|
os.unlink(cfg._ephemeral_db_path)
|
||||||
|
cfg._ephemeral_db_path = old
|
||||||
|
_reload_config()
|
||||||
|
|
||||||
|
|
||||||
|
class TestGetTokenDbPath:
|
||||||
|
def test_explicit_env_var_returned(self, monkeypatch, tmp_path):
|
||||||
|
target = tmp_path / "explicit.db"
|
||||||
|
monkeypatch.setenv("TOKEN_STORAGE_DB", str(target))
|
||||||
|
_reload_config()
|
||||||
|
|
||||||
|
assert get_token_db_path() == str(target)
|
||||||
|
# No tempfile should have been allocated since we took the explicit
|
||||||
|
# branch.
|
||||||
|
assert cfg._ephemeral_db_path is None
|
||||||
|
|
||||||
|
def test_ephemeral_tempfile_when_unset(self):
|
||||||
|
path = get_token_db_path()
|
||||||
|
|
||||||
|
assert path.startswith(tempfile.gettempdir())
|
||||||
|
assert f"nextcloud-mcp-tokens-{os.getpid()}-" in os.path.basename(path)
|
||||||
|
assert path.endswith(".db")
|
||||||
|
assert os.path.exists(path)
|
||||||
|
|
||||||
|
def test_ephemeral_tempfile_is_memoized(self):
|
||||||
|
first = get_token_db_path()
|
||||||
|
second = get_token_db_path()
|
||||||
|
|
||||||
|
assert first == second
|
||||||
|
# Only the memoized file should exist — no stray siblings.
|
||||||
|
parent = Path(first).parent
|
||||||
|
matches = list(parent.glob(f"nextcloud-mcp-tokens-{os.getpid()}-*.db"))
|
||||||
|
assert matches == [Path(first)]
|
||||||
|
|
||||||
|
def test_is_ephemeral_token_db_detects_allocated_path(self):
|
||||||
|
path = get_token_db_path()
|
||||||
|
|
||||||
|
assert is_ephemeral_token_db(path) is True
|
||||||
|
assert is_ephemeral_token_db("/some/other/path") is False
|
||||||
|
|
||||||
|
def test_is_ephemeral_token_db_before_allocation(self):
|
||||||
|
# The autouse fixture reset _ephemeral_db_path to None; do not call
|
||||||
|
# get_token_db_path() first. Nothing is allocated yet.
|
||||||
|
assert cfg._ephemeral_db_path is None
|
||||||
|
assert is_ephemeral_token_db("/any/path") is False
|
||||||
|
assert is_ephemeral_token_db("") is False
|
||||||
|
|
||||||
|
def test_explicit_path_does_not_trigger_tempfile(self, monkeypatch, tmp_path):
|
||||||
|
"""Regression guard: the explicit branch must short-circuit cleanly."""
|
||||||
|
monkeypatch.setenv("TOKEN_STORAGE_DB", str(tmp_path / "pinned.db"))
|
||||||
|
_reload_config()
|
||||||
|
|
||||||
|
get_token_db_path()
|
||||||
|
# Nothing under the tempfile prefix should have been created.
|
||||||
|
matches = list(
|
||||||
|
Path(tempfile.gettempdir()).glob(f"nextcloud-mcp-tokens-{os.getpid()}-*.db")
|
||||||
|
)
|
||||||
|
assert matches == []
|
||||||
|
|
||||||
|
|
||||||
|
class TestResolveSettingsFiles:
|
||||||
|
def test_empty_list_when_nothing_present(self, monkeypatch, tmp_path):
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
assert _resolve_settings_files() == []
|
||||||
|
|
||||||
|
def test_picks_up_cwd_settings(self, monkeypatch, tmp_path):
|
||||||
|
(tmp_path / "settings.toml").write_text("[default]\n")
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
|
||||||
|
result = _resolve_settings_files()
|
||||||
|
assert str(tmp_path / "settings.toml") in result
|
||||||
|
|
||||||
|
def test_picks_up_cwd_secrets(self, monkeypatch, tmp_path):
|
||||||
|
(tmp_path / ".secrets.toml").write_text("[default]\n")
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
|
||||||
|
result = _resolve_settings_files()
|
||||||
|
assert str(tmp_path / ".secrets.toml") in result
|
||||||
|
|
||||||
|
def test_explicit_settings_file_included(self, monkeypatch, tmp_path):
|
||||||
|
explicit = tmp_path / "nested" / "my-settings.toml"
|
||||||
|
explicit.parent.mkdir()
|
||||||
|
explicit.write_text("[default]\n")
|
||||||
|
monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(explicit))
|
||||||
|
# cwd has no settings.toml / .secrets.toml
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
|
||||||
|
result = _resolve_settings_files()
|
||||||
|
assert result == [str(explicit)]
|
||||||
|
|
||||||
|
def test_explicit_settings_file_secrets_colocated(self, monkeypatch, tmp_path):
|
||||||
|
"""PR #707 reviewer feedback: .secrets.toml should live beside
|
||||||
|
the explicit settings file, not always in cwd."""
|
||||||
|
config_dir = tmp_path / "etc"
|
||||||
|
config_dir.mkdir()
|
||||||
|
explicit = config_dir / "settings.toml"
|
||||||
|
explicit.write_text("[default]\n")
|
||||||
|
secrets = config_dir / ".secrets.toml"
|
||||||
|
secrets.write_text("[default]\n")
|
||||||
|
|
||||||
|
monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(explicit))
|
||||||
|
# cwd is elsewhere and deliberately contains *no* secrets file
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
|
||||||
|
result = _resolve_settings_files()
|
||||||
|
assert str(explicit) in result
|
||||||
|
assert str(secrets) in result
|
||||||
|
|
||||||
|
def test_explicit_missing_raises(self, monkeypatch, tmp_path):
|
||||||
|
"""PR #707 reviewer feedback: missing explicit path must not be
|
||||||
|
silently ignored — users will think their config is applied when
|
||||||
|
it isn't."""
|
||||||
|
missing = tmp_path / "does-not-exist.toml"
|
||||||
|
monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(missing))
|
||||||
|
monkeypatch.chdir(tmp_path)
|
||||||
|
|
||||||
|
with pytest.raises(FileNotFoundError, match="does-not-exist.toml"):
|
||||||
|
_resolve_settings_files()
|
||||||
Reference in New Issue
Block a user