From b694fc0c9d136041b4fc7fa67629eb8038002d97 Mon Sep 17 00:00:00 2001 From: Matt Johnson Date: Wed, 20 May 2026 23:10:10 +0000 Subject: [PATCH] fix(3-L.5): per-backend settings schemas (fixes build_enrichers TypeError) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaced during the 2026-05-20 NaviBackend activation: toggling config.enrichment.backend_class to NoOpBackend while backend_settings still held {"base_url": ...} crashed _rebuild_enrichers with `TypeError: NoOpBackend() takes no arguments`, BEFORE invalidate() ran. Fixed by mirroring the SourceAdapter.settings_schema pattern: each backend declares a Pydantic settings_schema; validation happens at write-time (GUI POST) and read-time (supervisor). A mismatch is now a clean ValidationError, never a constructor TypeError. Backends — each gets a `BackendSettings(BaseModel, extra="forbid")` + `settings_schema` class attr, mirroring __init__ defaults EXACTLY (note: timeout_s stays 10.0 — the brief's "5.0" was a transcription slip; preserve the production default): NoOpBackend -> NoOpBackendSettings (no fields) NaviBackend -> NaviBackendSettings (base_url, timeout_s, headers, warmup) PhotonBackend -> PhotonBackendSettings (base_url, timeout_s, headers) NominatimBackend-> NominatimBackendSettings (base_url, user_agent, rate_limit_per_sec, timeout_s) GeocoderBackend Protocol (in geocoder.py, where the base actually lives — not base.py, which only has Enricher) gains `settings_schema: type[BaseModel]`. supervisor: - build_enrichers validates backend_cls.settings_schema.model_validate( backend_settings) before instantiating, and constructs from the validated .model_dump(). ValidationError (not TypeError) on mismatch. - _rebuild_enrichers builds into locals and commits to instance state only on success — a ValidationError leaves the previously-active enrichers/config/ cache untouched. - _handle_enrichment_change wraps the rebuild in try/except ValidationError: logs and returns, keeping the previous backend running (supervisor stays up; operator fixes the row; next NOTIFY applies cleanly). No cache invalidation on a failed change. GUI /enrichment: - GET skips the outer EnrichmentConfig.backend_settings field and renders a separate
from describe_fields(backend_cls.settings_schema, ...) for the row's current backend_class. Backend fields namespaced bs_. - POST reassembles bs_ inputs into a backend_settings dict, validates it against the SUBMITTED backend_class's schema (so errors attach to the right fields when an operator is mid-switch), then validates the outer EnrichmentConfig. DB row written only if both pass; otherwise re-renders with field-level errors against the submitted backend. - backend_class stays a plain text field (no {{ errors[field.name] }} {% endif %} - {% elif field.widget == "number" %} - {% if field.description %}{{ field.description }}{% endif %} {% if errors and errors[field.name] %} {{ errors[field.name] }} {% endif %} + {% endif %} + {% endfor %} +
+
+ Backend settings — {{ backend_class }} + {% if not backend_fields %} + This backend takes no settings. + {% endif %} + {% for field in backend_fields %} + {% set fk = "bs_" ~ field.name %} + {% if field.widget == "text" %} + + + {% if field.description %}{{ field.description }}{% endif %} + {% if errors and errors[fk] %} + {{ errors[fk] }} + {% endif %} + {% elif field.widget == "number" %} + + + {% if field.description %}{{ field.description }}{% endif %} + {% if errors and errors[fk] %} + {{ errors[fk] }} + {% endif %} + {% elif field.widget == "checkbox" %} + + {% if field.description %}{{ field.description }}{% endif %} + {% if errors and errors[fk] %} + {{ errors[fk] }} + {% endif %} {% elif field.widget == "json" %} - - + + JSON object{% if field.description %} — {{ field.description }}{% endif %} - {% if errors and errors[field.name] %} - {{ errors[field.name] }} + {% if errors and errors[fk] %} + {{ errors[fk] }} {% endif %} {% endif %} {% endfor %} diff --git a/src/central/supervisor.py b/src/central/supervisor.py index 1ef7cdf..90913a7 100644 --- a/src/central/supervisor.py +++ b/src/central/supervisor.py @@ -12,6 +12,7 @@ from typing import Any import nats from nats.js import JetStreamContext +from pydantic import ValidationError from central.adapter import SourceAdapter from central.adapter_discovery import discover_adapters @@ -56,7 +57,12 @@ def build_enrichers( startup and hot-reloaded via LISTEN/NOTIFY (see Supervisor._on_config_change). """ backend_cls = _BACKEND_REGISTRY[enrichment_config.backend_class] - backend = backend_cls(**enrichment_config.backend_settings) + # Validate backend_settings against the backend's settings_schema BEFORE + # constructing. A mismatch (e.g. a stale base_url left in the row after + # switching to NoOpBackend) raises a clean pydantic ValidationError here + # instead of a TypeError inside the backend constructor. + validated = backend_cls.settings_schema.model_validate(enrichment_config.backend_settings) + backend = backend_cls(**validated.model_dump()) enricher_cls = _ENRICHER_REGISTRY[enrichment_config.enricher_class] return [enricher_cls(backend, cache=cache)] @@ -680,17 +686,27 @@ class Supervisor: ) def _rebuild_enrichers(self, config: EnrichmentConfig) -> None: - """Rebuild the active enricher set + cache from an EnrichmentConfig.""" + """Rebuild the active enricher set + cache from an EnrichmentConfig. + + Builds into locals first and commits to instance state only on success, + so a ValidationError (bad backend_settings) leaves the previously-active + enrichers/config/cache untouched and propagates to the caller. + """ + cache = EnrichmentCache(ENRICHMENT_CACHE_DB_PATH, ttl_s=config.cache_ttl_s) + enrichers = build_enrichers(config, cache) # may raise ValidationError self._active_enrichment_config = config - self._enrichment_cache = EnrichmentCache( - ENRICHMENT_CACHE_DB_PATH, ttl_s=config.cache_ttl_s - ) - self._enrichers = build_enrichers(config, self._enrichment_cache) + self._enrichment_cache = cache + self._enrichers = enrichers async def _handle_enrichment_change(self) -> None: """Re-read config.enrichment and rebuild enrichers. Invalidate the cache when the backend changed, so stale results from the previous backend - don't survive until TTL expiry.""" + don't survive until TTL expiry. + + Invalid backend_settings (ValidationError) leave the previous backend + running — the supervisor stays up; the operator fixes the row and the + next NOTIFY brings it in cleanly. + """ new_config = await self._config_source.get_enrichment_config() old_config = self._active_enrichment_config backend_changed = ( @@ -698,7 +714,19 @@ class Supervisor: or new_config.backend_settings != old_config.backend_settings or new_config.enricher_class != old_config.enricher_class ) - self._rebuild_enrichers(new_config) + try: + self._rebuild_enrichers(new_config) + except ValidationError as e: + logger.error( + "Enrichment config invalid; keeping previous backend", + extra={ + "enricher_class": new_config.enricher_class, + "backend_class": new_config.backend_class, + "backend_settings": new_config.backend_settings, + "errors": e.errors(), + }, + ) + return if backend_changed: deleted = await self._enrichment_cache.invalidate() logger.info( diff --git a/tests/test_backend_settings_schema.py b/tests/test_backend_settings_schema.py new file mode 100644 index 0000000..1d02fcc --- /dev/null +++ b/tests/test_backend_settings_schema.py @@ -0,0 +1,235 @@ +"""Tests for per-backend settings schemas (PR L.5). + +Each GeocoderBackend declares a Pydantic settings_schema (extra='forbid'). +build_enrichers validates backend_settings against it BEFORE constructing, so +a config/settings mismatch is a clean ValidationError, not a TypeError. The +supervisor's hot-reload keeps the previous backend running on ValidationError; +the GUI POST re-renders with field errors and does not write the DB row. + +Regression guard for the 2026-05-20 incident: switching backend_class to +NoOpBackend while backend_settings still held {"base_url": ...} crashed +_rebuild_enrichers with `TypeError: NoOpBackend() takes no arguments`. +""" + +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from pydantic import BaseModel, ValidationError + +from central.config_models import EnrichmentConfig +from central.enrichment.cache import EnrichmentCache +from central.supervisor import _BACKEND_REGISTRY, build_enrichers + + +# --- schemas exist + extra='forbid' ----------------------------------------- + +def test_every_backend_declares_a_settings_schema(): + for name, cls in _BACKEND_REGISTRY.items(): + schema = getattr(cls, "settings_schema", None) + assert schema is not None, f"{name} has no settings_schema" + assert isinstance(schema, type) and issubclass(schema, BaseModel), name + + +def test_every_settings_schema_forbids_extra(): + for name, cls in _BACKEND_REGISTRY.items(): + with pytest.raises(ValidationError): + cls.settings_schema.model_validate({"definitely_not_a_field": 1}) + + +def test_navi_schema_accepts_valid_kwargs_and_preserves_defaults(): + from central.enrichment.backends.navi import NaviBackendSettings + + m = NaviBackendSettings() + assert m.base_url == "http://localhost:8440" + assert m.timeout_s == 10.0 # preserved from __init__, NOT 5.0 + assert m.headers is None + assert m.warmup is True + m2 = NaviBackendSettings(base_url="http://navi:8440", timeout_s=3.0, warmup=False) + assert m2.base_url == "http://navi:8440" and m2.timeout_s == 3.0 + + +def test_noop_schema_has_no_fields(): + from central.enrichment.backends.no_op import NoOpBackendSettings + + assert list(NoOpBackendSettings.model_fields) == [] + + +# --- build_enrichers validation ---------------------------------------------- + +def _cache(tmp_path) -> EnrichmentCache: + return EnrichmentCache(tmp_path / "c.db") + + +def test_build_enrichers_raises_validation_error_not_typeerror(tmp_path): + """The exact 2026-05-20 bug: NoOpBackend + stale base_url. Must be a clean + ValidationError, never a TypeError from the constructor.""" + cfg = EnrichmentConfig(backend_class="NoOpBackend", backend_settings={"base_url": "http://x"}) + with pytest.raises(ValidationError): + build_enrichers(cfg, _cache(tmp_path)) + + +def test_build_enrichers_navi_valid_settings(tmp_path): + from central.enrichment.backends.navi import NaviBackend + + cfg = EnrichmentConfig( + backend_class="NaviBackend", + backend_settings={"base_url": "http://navi:8440", "warmup": False}, + ) + enrichers = build_enrichers(cfg, _cache(tmp_path)) + assert isinstance(enrichers[0]._backend, NaviBackend) + + +def test_build_enrichers_navi_unknown_setting_rejected(tmp_path): + cfg = EnrichmentConfig( + backend_class="NaviBackend", + backend_settings={"base_url": "http://navi:8440", "bogus": 1}, + ) + with pytest.raises(ValidationError): + build_enrichers(cfg, _cache(tmp_path)) + + +# --- supervisor hot-reload keeps previous backend on ValidationError --------- + +def _supervisor(): + from central import supervisor as sup_mod + + config_source = MagicMock() + config_source.get_enrichment_config = AsyncMock(return_value=EnrichmentConfig()) + return sup_mod.Supervisor( + config_source=config_source, + config_store=MagicMock(), + nats_url="nats://localhost:4222", + ) + + +@pytest.mark.asyncio +async def test_handle_enrichment_change_keeps_previous_on_validation_error(): + """Navi active, then a bad NOTIFY (NoOp + leftover base_url) arrives. The + supervisor must keep NaviBackend running and not crash.""" + from central.enrichment.backends.navi import NaviBackend + from central.enrichment.backends.no_op import NoOpBackend + + with patch("central.supervisor.EnrichmentCache") as cache_cls: + cache_cls.return_value = MagicMock(invalidate=AsyncMock(return_value=0)) + sup = _supervisor() + # Establish a valid NaviBackend active state. + sup._rebuild_enrichers(EnrichmentConfig( + backend_class="NaviBackend", + backend_settings={"base_url": "http://navi:8440", "warmup": False}, + )) + assert isinstance(sup._enrichers[0]._backend, NaviBackend) + active_before = sup._active_enrichment_config + + # Bad config arrives via NOTIFY: NoOp + stale base_url -> ValidationError. + sup._config_source.get_enrichment_config = AsyncMock( + return_value=EnrichmentConfig( + backend_class="NoOpBackend", + backend_settings={"base_url": "http://navi:8440"}, + ) + ) + await sup._handle_enrichment_change() # must NOT raise + + # Previous backend still active; config unchanged; cache NOT invalidated. + assert isinstance(sup._enrichers[0]._backend, NaviBackend) + assert sup._active_enrichment_config is active_before + cache_cls.return_value.invalidate.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_handle_enrichment_change_applies_valid_noop_with_empty_settings(): + """Switching to NoOp with empty backend_settings (the correct way) applies + cleanly and invalidates the cache.""" + from central.enrichment.backends.no_op import NoOpBackend + + with patch("central.supervisor.EnrichmentCache") as cache_cls: + cache_cls.return_value = MagicMock(invalidate=AsyncMock(return_value=3)) + sup = _supervisor() + sup._rebuild_enrichers(EnrichmentConfig( + backend_class="NaviBackend", + backend_settings={"base_url": "http://navi:8440", "warmup": False}, + )) + sup._config_source.get_enrichment_config = AsyncMock( + return_value=EnrichmentConfig(backend_class="NoOpBackend", backend_settings={}) + ) + await sup._handle_enrichment_change() + assert isinstance(sup._enrichers[0]._backend, NoOpBackend) + cache_cls.return_value.invalidate.assert_awaited() + + +# --- GUI POST validates backend_settings + does not write on error ---------- + +def _mock_pool(conn): + pool = MagicMock() + cm = MagicMock() + cm.__aenter__ = AsyncMock(return_value=conn) + cm.__aexit__ = AsyncMock(return_value=None) + pool.acquire = MagicMock(return_value=cm) + return pool + + +@pytest.mark.asyncio +async def test_gui_post_rejects_bad_backend_settings_without_writing(): + """POST with a bad backend setting (timeout_s non-numeric) re-renders with a + field error keyed bs_timeout_s and does NOT execute the DB upsert.""" + from central.gui.routes import enrichment_update + + request = MagicMock() + request.state.operator = MagicMock(username="op") + request.state.csrf_token = "tok" + + form = { + "csrf_token": "tok", + "enricher_class": "GeocoderEnricher", + "backend_class": "NaviBackend", + "cache_ttl_s": "86400", + "bs_base_url": "http://navi:8440", + "bs_timeout_s": "not-a-number", + } + request.form = AsyncMock(return_value=form) + + conn = MagicMock() + conn.execute = AsyncMock() + conn.fetchrow = AsyncMock(return_value={}) + templates = MagicMock() + templates.TemplateResponse.return_value = MagicMock() + + with patch("central.gui.routes._get_templates", return_value=templates), \ + patch("central.gui.routes.get_pool", return_value=_mock_pool(conn)): + await enrichment_update(request) + + conn.execute.assert_not_awaited() # no DB write on validation failure + ctx = templates.TemplateResponse.call_args.kwargs["context"] + assert "bs_timeout_s" in ctx["errors"] + assert ctx["backend_class"] == "NaviBackend" # re-rendered against SUBMITTED backend + + +@pytest.mark.asyncio +async def test_gui_post_writes_on_valid_settings(): + from central.gui.routes import enrichment_update + + request = MagicMock() + request.state.operator = MagicMock(username="op") + request.state.csrf_token = "tok" + form = { + "csrf_token": "tok", + "enricher_class": "GeocoderEnricher", + "backend_class": "NaviBackend", + "cache_ttl_s": "86400", + "bs_base_url": "http://navi:8440", + "bs_timeout_s": "10", + } + request.form = AsyncMock(return_value=form) + conn = MagicMock() + conn.execute = AsyncMock() + templates = MagicMock() + + with patch("central.gui.routes._get_templates", return_value=templates), \ + patch("central.gui.routes.get_pool", return_value=_mock_pool(conn)): + resp = await enrichment_update(request) + + conn.execute.assert_awaited() # DB upsert ran + # the backend_settings written are the validated/dumped dict + args = conn.execute.call_args.args + assert "INSERT INTO config.enrichment" in args[0] + assert resp.status_code == 302 diff --git a/tests/test_enrichment_config_plumbing.py b/tests/test_enrichment_config_plumbing.py index 5f3c0f1..1ac5e36 100644 --- a/tests/test_enrichment_config_plumbing.py +++ b/tests/test_enrichment_config_plumbing.py @@ -203,6 +203,11 @@ async def test_enrichment_form_renders(): await enrichment_form(request) ctx = templates.TemplateResponse.call_args.kwargs["context"] - field_widgets = {f.name: f.widget for f in ctx["fields"]} - assert field_widgets["backend_settings"] == "json" + # PR L.5: outer fields exclude backend_settings (now a per-backend fieldset); + # NoOpBackend's fieldset has zero fields. + outer = {f.name for f in ctx["outer_fields"]} + assert "backend_settings" not in outer + assert "backend_class" in outer + assert ctx["backend_class"] == "NoOpBackend" + assert ctx["backend_fields"] == [] assert ctx["csrf_token"] == "tok"