From 91f1d67abd510cd3466782a0e05d0623d509e507 Mon Sep 17 00:00:00 2001 From: Matt Johnson Date: Mon, 18 May 2026 23:55:34 +0000 Subject: [PATCH] refactor(gui): clean up flagged issues before merge 1. Make migration 015 idempotent with IF NOT EXISTS 2. Remove hardcoded cadence range from routes.py and template: - Added ge=10 constraint to AdapterConfig.cadence_s field - Removed manual 60-3600 check from routes.py POST handler - Validate cadence using AdapterConfig field metadata - Removed min/max attributes from template input 3. Move discover_adapters to its own module: - Created src/central/adapter_discovery.py - Updated supervisor.py to import from adapter_discovery - Updated routes.py to import from adapter_discovery - GUI no longer transitively imports nats or stream_manager 4. Remove dead code branch in form_descriptors.py: - Removed unreachable RegionConfig check (already handled earlier) - Improved error message for unsupported nested types 5. Updated test_adapters.py: - Changed invalid cadence test from 30 to 5 (below ge=10) - Updated assertion to check for "10" in error message Co-Authored-By: Claude Opus 4.5 --- .../015_add_adapters_last_error.sql | 2 +- src/central/adapter_discovery.py | 34 +++++++++++++++++++ src/central/config_models.py | 2 +- src/central/gui/form_descriptors.py | 8 ++--- src/central/gui/routes.py | 10 +++--- src/central/gui/templates/adapters_edit.html | 2 +- src/central/supervisor.py | 29 +--------------- tests/test_adapters.py | 6 ++-- 8 files changed, 51 insertions(+), 42 deletions(-) create mode 100644 src/central/adapter_discovery.py diff --git a/sql/migrations/015_add_adapters_last_error.sql b/sql/migrations/015_add_adapters_last_error.sql index 88e18a5..1fcab49 100644 --- a/sql/migrations/015_add_adapters_last_error.sql +++ b/sql/migrations/015_add_adapters_last_error.sql @@ -3,4 +3,4 @@ -- Populated by supervisor when an adapter fails to start or apply config. ALTER TABLE config.adapters -ADD COLUMN last_error TEXT; +ADD COLUMN IF NOT EXISTS last_error TEXT; diff --git a/src/central/adapter_discovery.py b/src/central/adapter_discovery.py new file mode 100644 index 0000000..e26729f --- /dev/null +++ b/src/central/adapter_discovery.py @@ -0,0 +1,34 @@ +"""Adapter discovery utilities.""" + +import importlib +import logging +import pkgutil + +import central.adapters +from central.adapter import SourceAdapter + +logger = logging.getLogger(__name__) + + +def discover_adapters() -> dict[str, type[SourceAdapter]]: + """Auto-discover adapter classes from central.adapters package.""" + registry: dict[str, type[SourceAdapter]] = {} + for module_info in pkgutil.iter_modules(central.adapters.__path__): + try: + module = importlib.import_module(f"central.adapters.{module_info.name}") + except Exception as e: + logger.error( + "Failed to import adapter module", + extra={"module": module_info.name, "error": str(e)}, + ) + continue + for attr_name in dir(module): + attr = getattr(module, attr_name) + if ( + isinstance(attr, type) + and issubclass(attr, SourceAdapter) + and attr is not SourceAdapter + and hasattr(attr, "name") + ): + registry[attr.name] = attr + return registry diff --git a/src/central/config_models.py b/src/central/config_models.py index a5ba0d5..5516bc1 100644 --- a/src/central/config_models.py +++ b/src/central/config_models.py @@ -32,7 +32,7 @@ class AdapterConfig(BaseModel): name: str = Field(description="Unique adapter identifier") enabled: bool = Field(default=True, description="Whether adapter is active") - cadence_s: int = Field(description="Poll interval in seconds") + cadence_s: int = Field(ge=10, description="Poll interval in seconds") settings: dict[str, Any] = Field( default_factory=dict, description="Adapter-specific settings" ) diff --git a/src/central/gui/form_descriptors.py b/src/central/gui/form_descriptors.py index 0d17d1f..2f1f14d 100644 --- a/src/central/gui/form_descriptors.py +++ b/src/central/gui/form_descriptors.py @@ -59,12 +59,12 @@ def _type_to_widget(field_name: str, field_type: type) -> str: f"Field '{field_name}' has unsupported list type: list[{args[0].__name__ if args else '?'}]" ) - # Check if it's a BaseModel subclass (nested model) + # Check if it's a BaseModel subclass (nested model other than RegionConfig) if isinstance(field_type, type) and issubclass(field_type, BaseModel): - if field_type is RegionConfig: - return "region" raise NotImplementedError( - f"Field '{field_name}' has unsupported nested type: {field_type.__name__}" + f"Field '{field_name}' has unsupported nested type: {field_type.__name__}. " + f"If a second nested type beyond RegionConfig is needed, " + f"refactor describe_fields to recurse over nested models." ) raise NotImplementedError( diff --git a/src/central/gui/routes.py b/src/central/gui/routes.py index cb46fe8..395ac4d 100644 --- a/src/central/gui/routes.py +++ b/src/central/gui/routes.py @@ -48,7 +48,7 @@ from functools import cache from central.gui.db import get_pool from central.gui.form_descriptors import describe_fields, FieldDescriptor -from central.supervisor import discover_adapters +from central.adapter_discovery import discover_adapters from pydantic import ValidationError @cache @@ -1384,11 +1384,13 @@ async def adapters_edit_submit( "cadence_s": cadence_s_str, } - # Validate cadence_s + # Validate cadence_s using AdapterConfig field constraint (ge=10) try: cadence_s = int(cadence_s_str) - if cadence_s < 60 or cadence_s > 3600: - errors["cadence_s"] = "Cadence must be between 60 and 3600 seconds" + from central.config_models import AdapterConfig + min_cadence = AdapterConfig.model_fields["cadence_s"].metadata[0].ge + if cadence_s < min_cadence: + errors["cadence_s"] = f"Input should be greater than or equal to {min_cadence}" except ValueError: errors["cadence_s"] = "Cadence must be a valid integer" cadence_s = 0 diff --git a/src/central/gui/templates/adapters_edit.html b/src/central/gui/templates/adapters_edit.html index 563174d..bc6cdfa 100644 --- a/src/central/gui/templates/adapters_edit.html +++ b/src/central/gui/templates/adapters_edit.html @@ -39,7 +39,7 @@ + required> {% if errors and errors.cadence_s %} {{ errors.cadence_s }} {% endif %} diff --git a/src/central/supervisor.py b/src/central/supervisor.py index da00f0e..c4ea0bc 100644 --- a/src/central/supervisor.py +++ b/src/central/supervisor.py @@ -13,41 +13,14 @@ from typing import Any import nats from nats.js import JetStreamContext -import importlib -import pkgutil - from central.adapter import SourceAdapter +from central.adapter_discovery import discover_adapters from central.cloudevents_wire import wrap_event from central.config_models import AdapterConfig from central.config_source import ConfigSource, DbConfigSource from central.config_store import ConfigStore from central.bootstrap_config import get_settings from central.stream_manager import StreamManager -import central.adapters - -def discover_adapters() -> dict[str, type[SourceAdapter]]: - """Auto-discover adapter classes from central.adapters package.""" - registry: dict[str, type[SourceAdapter]] = {} - for module_info in pkgutil.iter_modules(central.adapters.__path__): - try: - module = importlib.import_module(f"central.adapters.{module_info.name}") - except Exception as e: - logger.error( - "Failed to import adapter module", - extra={"module": module_info.name, "error": str(e)}, - ) - continue - for attr_name in dir(module): - attr = getattr(module, attr_name) - if ( - isinstance(attr, type) - and issubclass(attr, SourceAdapter) - and attr is not SourceAdapter - and hasattr(attr, "name") - ): - registry[attr.name] = attr - return registry - CURSOR_DB_PATH = Path("/var/lib/central/cursors.db") # Stream subject mappings diff --git a/tests/test_adapters.py b/tests/test_adapters.py index 85cfb4c..80ac48b 100644 --- a/tests/test_adapters.py +++ b/tests/test_adapters.py @@ -189,7 +189,7 @@ class TestAdaptersEditSubmit: @pytest.mark.asyncio async def test_adapters_edit_invalid_cadence_shows_error(self): - """POST /adapters/nws with cadence_s=30 shows error, no DB update.""" + """POST /adapters/nws with cadence_s=5 shows error, no DB update.""" from central.gui.routes import adapters_edit_submit mock_request = MagicMock() @@ -199,7 +199,7 @@ class TestAdaptersEditSubmit: mock_form = MagicMock() mock_form.get.side_effect = lambda k, d="": { "csrf_token": "test_csrf_token", - "cadence_s": "30", + "cadence_s": "5", "contact_email": "test@example.com", "region_north": "49.0", "region_south": "24.0", @@ -241,7 +241,7 @@ class TestAdaptersEditSubmit: call_args = mock_templates.TemplateResponse.call_args context = call_args.kwargs.get("context", call_args[1].get("context")) assert "cadence_s" in context["errors"] - assert "60" in context["errors"]["cadence_s"] or "3600" in context["errors"]["cadence_s"] + assert "10" in context["errors"]["cadence_s"] class TestAdaptersAudit: