refactor(api-v3): split the 10,469-line blueprint into a package

test/test_api_v3_font_upload_size.py

@@ -0,0 +1,60 @@
+"""Regression test: POST /fonts/upload must enforce its own stated size limit.
+
+validate_file_upload(filename, max_size_mb=10, allowed_extensions=[...]) reads
+like it checks the upload's size, but it only ever validated the filename
+(traversal characters, extension) -- max_size_mb was accepted and silently
+ignored. Nothing else in the handler checked the actual upload size either,
+so it saved whatever was posted to assets/fonts/<family><ext> regardless of
+size, unlike the sibling .star and plugin-asset upload routes, which check
+`file.tell()` against a stated limit before saving.
+"""
+
+import io
+import sys
+from pathlib import Path
+from unittest.mock import patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+URL = "/api/v3/fonts/upload"
+TEN_MB = 10 * 1024 * 1024
+
+
+@pytest.fixture
+def fonts_root(tmp_path):
+    # PROJECT_ROOT is bound by value in fonts.py, so it is patched there.
+    with patch("web_interface.blueprints.api_v3.fonts.PROJECT_ROOT", tmp_path):
+        yield tmp_path
+
+
+def upload(client, content, filename="myfont.ttf", family="myfont"):
+    data = {
+        "font_file": (io.BytesIO(content), filename),
+        "font_family": family,
+    }
+    return client.post(URL, data=data, content_type="multipart/form-data")
+
+
+class TestFontUploadSizeLimit:
+    def test_oversized_font_is_rejected(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"x" * (TEN_MB + 1))
+        assert response.status_code == 400
+        assert "too large" in response.get_json()["message"].lower()
+        assert not (fonts_root / "assets" / "fonts" / "myfont.ttf").exists(), (
+            "an oversized font was saved to disk before being rejected")
+
+    def test_a_font_right_at_the_limit_is_accepted(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"x" * TEN_MB,
+                          filename="atlimit.ttf", family="atlimit")
+        assert response.status_code == 200, response.get_json()
+        assert (fonts_root / "assets" / "fonts" / "atlimit.ttf").exists()
+
+    def test_an_ordinary_small_font_is_still_accepted(self, api_v3_client, fonts_root):
+        response = upload(api_v3_client, b"fake font bytes",
+                          filename="small.ttf", family="small")
+        assert response.status_code == 200, response.get_json()
+        assert (fonts_root / "assets" / "fonts" / "small.ttf").read_bytes() == b"fake font bytes"

test/test_api_v3_on_demand_restart.py

@@ -0,0 +1,93 @@
+"""Regression test: POST /display/on-demand/start restarting a running
+service must not import a name that does not exist.
+
+display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads
+mutable, test-patched attributes back through it (`_pkg.time.time()`,
+`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per
+the package's own docstring. One spot went further and wrote a genuine
+`import` *statement* against that alias --
+
+    import _pkg.time as time_module
+
+-- but `_pkg` is a local name bound by `import ... as _pkg` in this module,
+not a real top-level package, so `import _pkg.time` is not something Python
+can resolve; it raises ModuleNotFoundError. That line only runs when the
+display service is already running and the caller also asked to (re)start
+it, so this endpoint failed on exactly the restart path -- the one where a
+cache write recording the new on-demand request had already happened.
+
+The route wraps its body in `except Exception`, so the failure reached the
+caller as a handled 500 with a generic message, not an unhandled crash --
+but a 500 all the same on a request that should have restarted the service
+and reported success.
+"""
+
+import sys
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+URL = "/api/v3/display/on-demand/start"
+
+
+@pytest.fixture
+def restart_path(api_v3_module):
+    """Force the `service_was_running and start_service` branch.
+
+    plugin_manager and config_manager are set to None so the route takes
+    the simplest path to that branch rather than tripping over unrelated
+    MagicMock plumbing; _ensure_cache_manager, _get_display_service_status,
+    _stop_display_service and _ensure_display_service_running are bound by
+    value in display.py (see its own docstring), so they are patched on
+    that submodule rather than on the package.
+    """
+    api_v3_module.api_v3.plugin_manager = None
+    api_v3_module.api_v3.config_manager = None
+
+    with patch("web_interface.blueprints.api_v3.display._ensure_cache_manager") as ensure_cache, \
+         patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \
+         patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \
+         patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running:
+        ensure_cache.return_value = MagicMock()
+        # Active before the request: service_was_running becomes True.
+        get_status.return_value = {"active": True}
+        ensure_running.return_value = {"active": True}
+        yield {
+            "ensure_cache": ensure_cache,
+            "get_status": get_status,
+            "stop_service": stop_service,
+            "ensure_running": ensure_running,
+        }
+
+
+class TestRestartingARunningService:
+    def test_it_does_not_500(self, api_v3_client, restart_path):
+        response = api_v3_client.post(
+            URL, json={"plugin_id": "weather", "start_service": True})
+        body = response.get_json()
+        assert response.status_code == 200, body
+        assert body["status"] == "success", body
+
+    def test_the_service_is_actually_stopped_and_restarted(
+            self, api_v3_client, restart_path):
+        api_v3_client.post(
+            URL, json={"plugin_id": "weather", "start_service": True})
+        restart_path["stop_service"].assert_called_once()
+        restart_path["ensure_running"].assert_called_once()
+
+    def test_a_service_that_was_not_running_is_not_stopped_first(
+            self, api_v3_client, restart_path):
+        # The buggy import sits inside `if service_was_running and
+        # start_service`, so it only ever fired on the restart path --
+        # this is the other side of that branch, unaffected either way,
+        # kept here so the branch condition itself stays covered.
+        restart_path["get_status"].return_value = {"active": False}
+        response = api_v3_client.post(
+            URL, json={"plugin_id": "weather", "start_service": True})
+        assert response.status_code == 200, response.get_json()
+        restart_path["stop_service"].assert_not_called()

test/test_api_v3_optional_body.py

@@ -106,7 +106,12 @@ def test_malformed_json_gets_a_400_not_a_500(self, api_v3_client, api_v3_module,
 
 
 class TestNoBodyReadContradictsItsOwnGuard:
-    SOURCE = Path(__file__).parent.parent / "web_interface/blueprints/api_v3.py"
+    PKG = Path(__file__).parent.parent / "web_interface/blueprints/api_v3"
+
+    @property
+    def _source(self) -> str:
+        """api_v3 is a package; read every module of it."""
+        return "\n".join(p.read_text() for p in sorted(self.PKG.glob("*.py")))
 
     def test_no_or_default_read_is_unguarded(self):
         """`get_json() or <default>` is a contradiction without silent=True.
@@ -115,7 +120,7 @@ def test_no_or_default_read_is_unguarded(self):
         means the call raises before the default can apply.
         """
         offenders = [
-            line.strip() for line in self.SOURCE.read_text().splitlines()
+            line.strip() for line in self._source.splitlines()
             if "request.get_json()" in line and " or " in line
         ]
         assert offenders == [], (
@@ -124,7 +129,7 @@ def test_no_or_default_read_is_unguarded(self):
 
     def test_no_not_data_guard_is_unreachable(self):
         """A `if not data:` guard needs a read that can actually return None."""
-        lines = self.SOURCE.read_text().splitlines()
+        lines = self._source.splitlines()
         offenders = []
         for i, line in enumerate(lines):
             if re.search(r"=\s*request\.get_json\(\)\s*$", line):

test/test_api_v3_schedule_error_messages.py

@@ -0,0 +1,54 @@
+"""Regression test: per-day schedule validation errors must say "time", not
+"_pkg.time".
+
+The split into a package rewrote every bare `time` reference that needed to
+read through the shared module as `_pkg.time` (see the package's own
+docstring on why -- tests patch it, so it has to be read back live rather
+than bound by value). That rewrite was mechanical and matched the substring
+"time" inside string literals and comments too, so the user-facing message
+
+    "Invalid start time for {day}: ..."
+
+came out as
+
+    "Invalid start _pkg.time for {day}: ..."
+
+in both POST /config/schedule and POST /config/dim-schedule, for both the
+start and end time of a per-day entry.
+"""
+
+import sys
+from pathlib import Path
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).parent.parent))
+
+from test._api_v3_test_helpers import api_v3_client, api_v3_module  # noqa: F401,E402
+
+
+@pytest.mark.parametrize("url", [
+    "/api/v3/config/schedule",
+    "/api/v3/config/dim-schedule",
+])
+class TestPerDayTimeErrorsAreNotCorrupted:
+    def test_invalid_start_time_message(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url, json={
+            "mode": "per_day",
+            "monday_start": "not-a-time",
+        })
+        assert response.status_code == 400
+        message = response.get_json()["message"]
+        assert "_pkg" not in message, message
+        assert message.startswith("Invalid start time for monday:"), message
+
+    def test_invalid_end_time_message(self, api_v3_client, api_v3_module, url):
+        response = api_v3_client.post(url, json={
+            "mode": "per_day",
+            "monday_start": "07:00",
+            "monday_end": "not-a-time",
+        })
+        assert response.status_code == 400
+        message = response.get_json()["message"]
+        assert "_pkg" not in message, message
+        assert message.startswith("Invalid end time for monday:"), message

test/test_api_v3_url_map.py

@@ -0,0 +1,136 @@
+"""The /api/v3 URL map is a contract, and the split must not have moved it.
+
+api_v3.py was one 10,469-line module and is now a package. Every route module
+in it decorates the *same* Blueprint object, so this refactor was supposed to
+be invisible from outside: same URLs, same endpoint names, same methods.
+
+"Supposed to be" is the problem. A route function silently dropped during a
+move -- a module that never gets imported, a decorator left behind -- costs
+nothing at import time and fails only when someone hits the URL. So the map is
+pinned here.
+
+The snapshot is intentionally the *whole* map rather than a count. A count
+passes when one route is deleted and another added, which is exactly the shape
+a careless move produces.
+
+If you are adding a route, this test is meant to fail: add the entry to
+EXPECTED. If you are moving one between modules, it is meant to pass unchanged
+-- endpoint names are `api_v3.<function>` regardless of which module the
+function lives in, and that is the property that makes the package safe.
+"""
+import json
+import os
+
+import pytest
+from flask import Flask
+
+from web_interface.blueprints.api_v3 import api_v3
+
+SNAPSHOT = os.path.join(os.path.dirname(os.path.abspath(__file__)),
+                        "fixtures", "api_v3_url_map.json")
+
+
+def _current_map():
+    app = Flask(__name__)
+    app.register_blueprint(api_v3, url_prefix="/api/v3")
+    return sorted(
+        [r.rule, r.endpoint, sorted(r.methods)]
+        for r in app.url_map.iter_rules()
+        if r.endpoint != "static"
+    )
+
+
+def test_the_url_map_matches_the_snapshot():
+    current = _current_map()
+    with open(SNAPSHOT, encoding="utf-8") as fh:
+        expected = json.load(fh)
+
+    cur = {(r, e) for r, e, _ in current}
+    exp = {(r, e) for r, e, _ in expected}
+
+    lost = sorted(exp - cur)
+    added = sorted(cur - exp)
+    assert not lost, (
+        f"{len(lost)} route(s) disappeared from /api/v3: {lost[:5]}. "
+        "A route lost in a module move costs nothing at import time and fails "
+        "only when someone hits the URL.")
+    assert not added, (
+        f"{len(added)} new route(s): {added[:5]}. If that is intended, "
+        f"regenerate {os.path.relpath(SNAPSHOT)}.")
+
+    # Methods too: a route that quietly loses POST is still a broken route.
+    cur_methods = {(r, e): m for r, e, m in current}
+    for rule, endpoint, methods in expected:
+        assert cur_methods[(rule, endpoint)] == methods, (
+            f"{rule} ({endpoint}) methods changed: "
+            f"{methods} -> {cur_methods[(rule, endpoint)]}")
+
+
+def test_every_endpoint_is_on_the_one_blueprint():
+    """The package must not fragment into several blueprints.
+
+    Splitting into per-domain *blueprints* would rename every endpoint from
+    `api_v3.foo` to `api_v3_plugins.foo` and break any url_for() that names
+    one. Keeping a single Blueprint object across the modules is what makes
+    the split a pure code move, so assert it rather than trusting it.
+    """
+    for rule, endpoint, _ in _current_map():
+        assert endpoint.startswith("api_v3."), (
+            f"{rule} is registered as {endpoint}, not on the api_v3 blueprint")
+
+
+def test_the_snapshot_is_not_empty():
+    """Guards the failure mode this file exists to prevent.
+
+    An empty or truncated snapshot would make every assertion above pass
+    vacuously -- the same trap as a route map that imports no modules.
+    """
+    with open(SNAPSHOT, encoding="utf-8") as fh:
+        expected = json.load(fh)
+    assert len(expected) > 100, (
+        f"snapshot has only {len(expected)} routes; it should have the whole "
+        "/api/v3 surface")
+
+
+@pytest.mark.parametrize("module", [
+    "backup", "config", "display", "fonts", "misc",
+    "plugins", "starlark", "system", "wifi",
+])
+def test_every_route_module_contributes(module):
+    """Each module must actually register something.
+
+    A module that fails to import, or that is left out of __init__, takes its
+    routes with it silently -- the package still imports and the app still
+    starts.
+    """
+    import importlib
+    mod = importlib.import_module(f"web_interface.blueprints.api_v3.{module}")
+    routes = [n for n in dir(mod)
+              if callable(getattr(mod, n, None))
+              and getattr(getattr(mod, n), "__module__", "") == mod.__name__]
+    assert routes, f"{module}.py defines no view functions"
+
+
+def test_project_root_points_at_the_project():
+    """PROJECT_ROOT is derived from __file__, so moving the file breaks it.
+
+    The split moved this code from web_interface/blueprints/api_v3.py to
+    web_interface/blueprints/api_v3/_common.py -- one directory deeper -- and
+    `Path(__file__).parent.parent.parent` quietly began resolving to
+    web_interface/ instead of the project root. Nothing failed at import. It
+    surfaced as routes returning 404 and "installation script not found",
+    because every path built from it pointed one level too shallow.
+
+    A URL-map check cannot catch that: the routes were all registered, they
+    just could not find anything.
+    """
+    from pathlib import Path
+
+    from web_interface.blueprints.api_v3 import PROJECT_ROOT
+
+    # The project root is the directory holding run.py and web_interface/.
+    assert (PROJECT_ROOT / "run.py").is_file(), (
+        f"PROJECT_ROOT is {PROJECT_ROOT}, which has no run.py; it is not the "
+        "project root")
+    assert (PROJECT_ROOT / "web_interface").is_dir()
+    assert PROJECT_ROOT == Path(__file__).resolve().parents[1]