fix: address Codex routes-split follow-up review (1.0.0-39)
Three low-severity findings from Codex on the 1.0.0-37 split:
1. Trim dead package-level imports in routes/__init__.py — only
`poller` was actually used; auth/burnin/mailer/settings_store
were the exact shadowing footgun the absolute sub-router
imports work around. Reword the comment block to match.
2. Thread `operator` through smart_start + smart_cancel.
Previously the JS client sent it but the server ignored it;
add audit_events rows (smart_test_start / smart_test_cancel)
so the field is actually meaningful.
3. New tests/test_routes_resolution.py — guards two historical
regressions: /api/v1/burnin/export.csv must register before
/{job_id} (FastAPI int-coerce 422 trap) and the mailer
back-compat shim `from app.routes import _fetch_drives_for_template`
must keep importing. Plus a sub-router enumeration test that
catches missed include_router calls in future splits.
This commit is contained in:
parent
a8a7d99621
commit
8033161efb
4 changed files with 110 additions and 6 deletions
|
|
@ -83,7 +83,7 @@ class Settings(BaseSettings):
|
||||||
ssh_key: str = "" # PEM private key content (paste full key including headers)
|
ssh_key: str = "" # PEM private key content (paste full key including headers)
|
||||||
|
|
||||||
# Application version — used by the /api/v1/updates/check endpoint
|
# Application version — used by the /api/v1/updates/check endpoint
|
||||||
app_version: str = "1.0.0-38"
|
app_version: str = "1.0.0-39"
|
||||||
|
|
||||||
# ---- Authentication (1.0.0-22) ----
|
# ---- Authentication (1.0.0-22) ----
|
||||||
# session_secret: HMAC key for signing session cookies. Empty = generate
|
# session_secret: HMAC key for signing session cookies. Empty = generate
|
||||||
|
|
|
||||||
|
|
@ -9,7 +9,7 @@ from fastapi import APIRouter, Depends, HTTPException, Query, Request
|
||||||
from fastapi.responses import HTMLResponse, StreamingResponse
|
from fastapi.responses import HTMLResponse, StreamingResponse
|
||||||
from sse_starlette.sse import EventSourceResponse
|
from sse_starlette.sse import EventSourceResponse
|
||||||
|
|
||||||
from app import auth, burnin, mailer, poller, settings_store
|
from app import poller
|
||||||
from app.config import settings
|
from app.config import settings
|
||||||
from app.database import get_db
|
from app.database import get_db
|
||||||
from app.models import (
|
from app.models import (
|
||||||
|
|
@ -39,10 +39,12 @@ router = APIRouter()
|
||||||
# working unchanged. Future slices can extract more — drives, burnin,
|
# working unchanged. Future slices can extract more — drives, burnin,
|
||||||
# settings, history — using the same pattern.
|
# settings, history — using the same pattern.
|
||||||
#
|
#
|
||||||
# Absolute imports (vs `from . import auth`) because the line-12
|
# Absolute imports (`import app.routes.X as _Y`) instead of relative
|
||||||
# `from app import auth` binds `auth` as an attribute on this package's
|
# (`from . import X as _Y`) so we stay safe even if a future top-level
|
||||||
# namespace, which would shadow the relative-submodule lookup and yield
|
# `from app import X` is reintroduced here — `from app import auth`
|
||||||
# `app.auth` instead of `app.routes.auth`.
|
# would bind `auth` on the `app.routes` package namespace and shadow
|
||||||
|
# any relative-submodule lookup. Absolute imports always resolve to
|
||||||
|
# `app.routes.X` regardless of what's already bound on the package.
|
||||||
import app.routes.auth as _auth_routes # noqa: E402
|
import app.routes.auth as _auth_routes # noqa: E402
|
||||||
import app.routes.system as _system_routes # noqa: E402
|
import app.routes.system as _system_routes # noqa: E402
|
||||||
import app.routes.history as _history_routes # noqa: E402
|
import app.routes.history as _history_routes # noqa: E402
|
||||||
|
|
|
||||||
|
|
@ -131,6 +131,7 @@ async def get_drive(drive_id: int, db: aiosqlite.Connection = Depends(get_db)):
|
||||||
@router.post("/api/v1/drives/{drive_id}/smart/start")
|
@router.post("/api/v1/drives/{drive_id}/smart/start")
|
||||||
async def smart_start(
|
async def smart_start(
|
||||||
drive_id: int,
|
drive_id: int,
|
||||||
|
request: Request,
|
||||||
body: dict,
|
body: dict,
|
||||||
db: aiosqlite.Connection = Depends(get_db),
|
db: aiosqlite.Connection = Depends(get_db),
|
||||||
):
|
):
|
||||||
|
|
@ -152,6 +153,7 @@ async def smart_start(
|
||||||
raise HTTPException(status_code=404, detail="Drive not found")
|
raise HTTPException(status_code=404, detail="Drive not found")
|
||||||
devname = row[0]
|
devname = row[0]
|
||||||
|
|
||||||
|
operator = operator_for(request, body.get("operator"))
|
||||||
now = datetime.now(timezone.utc).isoformat()
|
now = datetime.now(timezone.utc).isoformat()
|
||||||
ttype_lower = test_type.lower()
|
ttype_lower = test_type.lower()
|
||||||
|
|
||||||
|
|
@ -173,6 +175,12 @@ async def smart_start(
|
||||||
raw_output=excluded.raw_output""",
|
raw_output=excluded.raw_output""",
|
||||||
(drive_id, ttype_lower, "running", 0, now, output),
|
(drive_id, ttype_lower, "running", 0, now, output),
|
||||||
)
|
)
|
||||||
|
await db.execute(
|
||||||
|
"""INSERT INTO audit_events (event_type, drive_id, operator, message)
|
||||||
|
VALUES (?,?,?,?)""",
|
||||||
|
("smart_test_start", drive_id, operator,
|
||||||
|
f"{test_type} SMART test started on {devname}"),
|
||||||
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
poller._notify_subscribers()
|
poller._notify_subscribers()
|
||||||
return {"devname": devname, "type": test_type, "message": output[:200]}
|
return {"devname": devname, "type": test_type, "message": output[:200]}
|
||||||
|
|
@ -186,12 +194,20 @@ async def smart_start(
|
||||||
tn_job_id = await client.start_smart_test([devname], test_type)
|
tn_job_id = await client.start_smart_test([devname], test_type)
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
raise HTTPException(status_code=502, detail=f"TrueNAS error: {exc}")
|
raise HTTPException(status_code=502, detail=f"TrueNAS error: {exc}")
|
||||||
|
await db.execute(
|
||||||
|
"""INSERT INTO audit_events (event_type, drive_id, operator, message)
|
||||||
|
VALUES (?,?,?,?)""",
|
||||||
|
("smart_test_start", drive_id, operator,
|
||||||
|
f"{test_type} SMART test started on {devname}"),
|
||||||
|
)
|
||||||
|
await db.commit()
|
||||||
return {"job_id": tn_job_id, "devname": devname, "type": test_type}
|
return {"job_id": tn_job_id, "devname": devname, "type": test_type}
|
||||||
|
|
||||||
|
|
||||||
@router.post("/api/v1/drives/{drive_id}/smart/cancel")
|
@router.post("/api/v1/drives/{drive_id}/smart/cancel")
|
||||||
async def smart_cancel(
|
async def smart_cancel(
|
||||||
drive_id: int,
|
drive_id: int,
|
||||||
|
request: Request,
|
||||||
body: dict,
|
body: dict,
|
||||||
db: aiosqlite.Connection = Depends(get_db),
|
db: aiosqlite.Connection = Depends(get_db),
|
||||||
):
|
):
|
||||||
|
|
@ -205,6 +221,7 @@ async def smart_cancel(
|
||||||
if not row:
|
if not row:
|
||||||
raise HTTPException(status_code=404, detail="Drive not found")
|
raise HTTPException(status_code=404, detail="Drive not found")
|
||||||
devname = row[0]
|
devname = row[0]
|
||||||
|
operator = operator_for(request, body.get("operator"))
|
||||||
|
|
||||||
client = burnin._client
|
client = burnin._client
|
||||||
if client is None:
|
if client is None:
|
||||||
|
|
@ -248,6 +265,12 @@ async def smart_cancel(
|
||||||
"UPDATE smart_tests SET state='aborted', finished_at=? WHERE drive_id=? AND test_type=? AND state='running'",
|
"UPDATE smart_tests SET state='aborted', finished_at=? WHERE drive_id=? AND test_type=? AND state='running'",
|
||||||
(now, drive_id, test_type),
|
(now, drive_id, test_type),
|
||||||
)
|
)
|
||||||
|
await db.execute(
|
||||||
|
"""INSERT INTO audit_events (event_type, drive_id, operator, message)
|
||||||
|
VALUES (?,?,?,?)""",
|
||||||
|
("smart_test_cancel", drive_id, operator,
|
||||||
|
f"{test_type.upper()} SMART test cancelled on {devname}"),
|
||||||
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
|
|
||||||
return {"cancelled": True, "devname": devname, "type": test_type}
|
return {"cancelled": True, "devname": devname, "type": test_type}
|
||||||
|
|
|
||||||
79
tests/test_routes_resolution.py
Normal file
79
tests/test_routes_resolution.py
Normal file
|
|
@ -0,0 +1,79 @@
|
||||||
|
"""Route-resolution invariants for the routes/ package.
|
||||||
|
|
||||||
|
Guards two historical regressions Codex flagged were untested:
|
||||||
|
|
||||||
|
1. /api/v1/burnin/export.csv must resolve to the CSV export, not to
|
||||||
|
/api/v1/burnin/{job_id} with int("export.csv") → 422. FastAPI's
|
||||||
|
path matching tries declarations in registration order, so the
|
||||||
|
literal must be declared before the parameterized route.
|
||||||
|
|
||||||
|
2. app.mailer reaches into app.routes for _fetch_drives_for_template
|
||||||
|
(back-compat from before the routes/ split). The shim re-export
|
||||||
|
in app/routes/__init__.py must remain importable.
|
||||||
|
|
||||||
|
Run inside the container image so app deps are present.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import unittest
|
||||||
|
|
||||||
|
|
||||||
|
class TestRouteResolution(unittest.TestCase):
|
||||||
|
|
||||||
|
def test_export_csv_declared_before_job_id(self):
|
||||||
|
"""Route order in burnin.py: /export.csv must come before
|
||||||
|
/{job_id} or FastAPI will int-coerce 'export.csv' and 422.
|
||||||
|
"""
|
||||||
|
from app.routes import burnin as burnin_routes
|
||||||
|
|
||||||
|
paths = [r.path for r in burnin_routes.router.routes]
|
||||||
|
self.assertIn("/api/v1/burnin/export.csv", paths)
|
||||||
|
self.assertIn("/api/v1/burnin/{job_id}", paths)
|
||||||
|
self.assertLess(
|
||||||
|
paths.index("/api/v1/burnin/export.csv"),
|
||||||
|
paths.index("/api/v1/burnin/{job_id}"),
|
||||||
|
"/export.csv must be registered before /{job_id} or FastAPI "
|
||||||
|
"will try to int-coerce 'export.csv' and return 422",
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_mailer_backcompat_shim(self):
|
||||||
|
"""app.mailer imports _fetch_drives_for_template from app.routes
|
||||||
|
(NOT app.routes._drives_helpers) — the shim re-export in
|
||||||
|
routes/__init__.py keeps that working post-split.
|
||||||
|
"""
|
||||||
|
from app.routes import _fetch_drives_for_template
|
||||||
|
self.assertTrue(callable(_fetch_drives_for_template))
|
||||||
|
|
||||||
|
def test_all_subrouters_included(self):
|
||||||
|
"""Sanity check: every sub-router in app.routes.* is wired into
|
||||||
|
the package-level router.include_router calls. If a future split
|
||||||
|
adds a new file but forgets the include, this catches it.
|
||||||
|
"""
|
||||||
|
import importlib
|
||||||
|
import pkgutil
|
||||||
|
import app.routes as routes_pkg
|
||||||
|
|
||||||
|
sub_modules = [
|
||||||
|
name for _, name, _ in pkgutil.iter_modules(routes_pkg.__path__)
|
||||||
|
if not name.startswith("_") # skip _helpers, _drives_helpers
|
||||||
|
]
|
||||||
|
|
||||||
|
registered_paths = {r.path for r in routes_pkg.router.routes}
|
||||||
|
for mod_name in sub_modules:
|
||||||
|
mod = importlib.import_module(f"app.routes.{mod_name}")
|
||||||
|
sub_router = getattr(mod, "router", None)
|
||||||
|
self.assertIsNotNone(
|
||||||
|
sub_router,
|
||||||
|
f"app.routes.{mod_name} has no `router` attribute",
|
||||||
|
)
|
||||||
|
for r in sub_router.routes:
|
||||||
|
self.assertIn(
|
||||||
|
r.path, registered_paths,
|
||||||
|
f"{mod_name}.router has {r.path} but the package "
|
||||||
|
"router didn't include it",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
if __name__ == "__main__":
|
||||||
|
unittest.main()
|
||||||
Loading…
Add table
Reference in a new issue