Address review findings on PR #147 (P2+P3)

P2-1 (containment): api/nestquest_core.py no longer mutates sys.path.
Load core by file location with importlib.util.spec_from_file_location,
registering it under the distinct top-level name 'nestquest_core' with
submodule_search_locations pointing at the bundled core directory. No
generic top-level names (db, const, schema, ...) are published that
could shadow a future dependency, and the integration's HA-coupled
__init__ is never executed. Use importlib.import_module for submodules;
drop the redundant _BOOTSTRAPPED flag. Document that two core copies
(nestquest_core.* and custom_components.nestquest.core.*) coexist in
the pytest process so later tasks must not compare class/exception
identity across them.

P2-2 (fail fast): ApiConfig.from_env now RAISES ConfigError when
NESTQUEST_DB_PATH is unset or empty — no silent fallback to a
predictable world-writable temp path that loses data at reboot. Remove
the module-level 'app = create_app()' (it read env at import time and
would break fail-fast); document serving with 'uvicorn --factory
api.app:create_app'.

P3 cleanups:
- /health drops db_path from the body (no filesystem path leak on an
  unauthenticated route) and runs a cheap SELECT 1 to reflect database
  reachability.
- Remove dead code: resolve_db_path in api/config.py; unused LOGGER in
  api/app.py.
- tests/test_api_health.py: add ApiConfig.from_env tests (set value,
  empty raises, missing raises); add startup-failure tests (unwritable
  dir, non-SQLite file); assert shutdown closes the connection
  (connected is False); make _AppRunner.__aexit__ use try/finally so a
  client error still shuts down the lifespan; reuse api.database._executor
  instead of a duplicated local copy; in the no-HA subprocess test
  assert core.__file__ points at the bundled directory and stop passing
  the unused ':memory:'.
- pyproject.toml: comment that fastapi/uvicorn are dev-group only so the
  monorepo test venv can exercise the app; the API deployable's runtime
  dependencies must be declared when it is packaged (Feature 17).
This commit is contained in:
2026-09-21 16:11:55 -04:00
parent 70dce8a536
commit 48552ac62e
6 changed files with 278 additions and 121 deletions
+6 -4
View File
@@ -9,9 +9,11 @@ This package imports the NestQuest domain ``core`` WITHOUT executing
``custom_components/nestquest/__init__.py`` — that parent package's
``__init__.py`` imports ``homeassistant``, which is absent from the
API's runtime. The coupling is encapsulated in
:mod:`api.nestquest_core` (a ``sys.path`` bootstrap that makes ``core``
importable as a top-level package). It is a TRANSITIONAL coupling that
Feature 18 will revisit: once the integration becomes a thin API client
and ``core`` moves out of the integration bundle, this helper goes away.
:mod:`api.nestquest_core`, which loads ``core`` by file location with
:mod:`importlib.util` and registers it under the distinct top-level name
``nestquest_core`` (no ``sys.path`` mutation, no generic top-level
names published). It is a TRANSITIONAL coupling that Feature 18 will
revisit: once the integration becomes a thin API client and ``core``
moves out of the integration bundle, this helper goes away.
"""
from __future__ import annotations
+27 -20
View File
@@ -4,9 +4,10 @@ This module builds the ASGI app for the NestQuest API service (Feature
16). The skeleton here wires the bundled ``core`` (Feature 15) and
exposes the two endpoints every later task depends on:
- ``GET /health`` — returns ``200`` with a JSON status body carrying
``status: "ok"`` and the resolved database path, proving the app
imported core, opened the database, and applied migrations.
- ``GET /health`` — returns ``200`` with a JSON status body once the
app has imported core, opened the database, and applied migrations;
a cheap ``SELECT 1`` confirms the connection is live without leaking
the filesystem path on an unauthenticated route.
- ``GET /docs`` — FastAPI's auto-served OpenAPI document (Swagger UI);
``/openapi.json`` is the raw schema. No extra wiring is needed;
FastAPI serves both by default.
@@ -14,10 +15,19 @@ exposes the two endpoints every later task depends on:
The database is opened and migrated in a FastAPI lifespan context so a
startup failure (e.g. an unwritable path) surfaces as a startup error
rather than the first request. The connection is closed on shutdown.
This module exports ONLY the :func:`create_app` factory — there is no
module-level app instance, because building one at import time would
read ``NESTQUEST_DB_PATH`` at import (and fail-fast on a missing value)
before any worker is ready to report it. Serve with::
uvicorn --factory api.app:create_app
so the factory runs on each worker startup and reads the environment
then.
"""
from __future__ import annotations
import logging
from contextlib import asynccontextmanager
from collections.abc import AsyncIterator
@@ -26,8 +36,6 @@ from fastapi import FastAPI
from api.config import ApiConfig
from api.database import DatabaseState, make_database
LOGGER = logging.getLogger(__name__)
def _build_lifespan(db_path: str):
"""Return a lifespan that opens+migrates then closes one database.
@@ -53,9 +61,10 @@ def create_app(config: ApiConfig | None = None) -> FastAPI:
"""Build the NestQuest API FastAPI app.
``config`` defaults to one built from the process environment, so
``uvicorn api.app:app`` picks up ``NESTQUEST_DB_PATH``. Tests pass an
explicit config (or set the env var) so each test gets a hermetic
temp SQLite file.
``uvicorn --factory api.app:create_app`` picks up
``NESTQUEST_DB_PATH`` (and fails fast on startup if it is missing).
Tests pass an explicit config so each test gets a hermetic temp
SQLite file.
"""
cfg = config if config is not None else ApiConfig.from_env()
@@ -72,21 +81,19 @@ def create_app(config: ApiConfig | None = None) -> FastAPI:
@app.get("/health", summary="Service health")
async def health() -> dict[str, str]:
"""Return service health and the resolved database path.
"""Return service health.
The body carries ``status: "ok"`` and the ``db_path`` the app
opened against — so a probe can confirm the app wired core and
reached the configured SQLite file. Returns 200 once the
lifespan has opened and migrated the database.
Returns ``200`` with ``{"status": "ok"}`` once the lifespan has
opened and migrated the database. A cheap ``SELECT 1`` against
the live connection confirms the database is reachable WITHOUT
leaking the filesystem path on an unauthenticated route; if the
connection is dead the query raises and the probe fails.
"""
state: DatabaseState = app.state.db
return {"status": "ok", "db_path": state.db_path}
await state.database.fetch_one("SELECT 1")
return {"status": "ok"}
return app
#: The module-level app instance, for ``uvicorn api.app:app``.
app: FastAPI = create_app()
__all__ = ["app", "create_app"]
__all__ = ["create_app"]
+27 -20
View File
@@ -1,33 +1,35 @@
"""Configuration for the NestQuest API service.
The API reads its single configuration knob — the SQLite database path —
from the ``NESTQUEST_DB_PATH`` environment variable, with a documented
default inside the OS temp directory. This mirrors the integration's
from the ``NESTQUEST_DB_PATH`` environment variable. Unlike a dev
convenience default, a missing or empty value is a configuration ERROR,
not a silent fallback to a predictable world-writable temp path: the API
owns the database (it is the writer of record per Feature 16's
contract), and silently dropping data into ``$TMPDIR/nestquest.db``
would lose it at reboot. ``ApiConfig.from_env`` therefore RAISES when
the variable is unset or empty. This mirrors the integration's
"explicit settings object" discipline (Feature 15's
``core.settings``): the API never reads HA config; callers pass the
resolved path into the database wiring.
Serving: the app is built with the factory ``api.app:create_app`` — run
it with ``uvicorn --factory api.app:create_app`` so the factory reads
the environment on each worker startup rather than at import time.
"""
from __future__ import annotations
import os
import tempfile
from dataclasses import dataclass
from pathlib import Path
#: Environment variable holding the SQLite file path. The API opens
#: this file on startup (creating it if absent) and applies migrations.
#: REQUIRED — unset or empty raises :class:`ConfigError` from
#: :meth:`ApiConfig.from_env`.
DB_PATH_ENV = "NESTQUEST_DB_PATH"
#: The default database location when the env var is unset. Lives in
#: the OS temp directory so a dev run without configuration does not
#: litter the repo, and so tests that forget to set the env var still
#: get a hermetic-ish path. Production deployments ALWAYS set
#: ``NESTQUEST_DB_PATH`` to a persistent volume path.
_DEFAULT_DB_NAME = "nestquest.db"
def _default_db_path() -> str:
return str(Path(tempfile.gettempdir()) / _DEFAULT_DB_NAME)
class ConfigError(RuntimeError):
"""Raised when required API configuration is missing or invalid."""
@dataclass(frozen=True)
@@ -47,15 +49,20 @@ class ApiConfig:
``env`` defaults to ``os.environ``; tests pass an explicit dict
so they are hermetic (no monkeypatch of the global environ).
Raises :class:`ConfigError` when ``NESTQUEST_DB_PATH`` is unset
or empty/whitespace-only — there is no silent default, because a
default in the temp directory would silently lose data at reboot.
"""
source = env if env is not None else os.environ
raw = source.get(DB_PATH_ENV, "").strip()
return cls(db_path=raw if raw else _default_db_path())
if not raw:
raise ConfigError(
f"{DB_PATH_ENV} must be set to a writable SQLite file path "
f"(it is unset or empty — the API will not fall back to a "
f"temp directory and silently lose data)"
)
return cls(db_path=raw)
def resolve_db_path(env: dict[str, str] | None = None) -> str:
"""Return the resolved SQLite path (env override or documented default)."""
return ApiConfig.from_env(env).db_path
__all__ = ["ApiConfig", "DB_PATH_ENV", "resolve_db_path"]
__all__ = ["ApiConfig", "ConfigError", "DB_PATH_ENV"]
+68 -45
View File
@@ -10,11 +10,18 @@ API cannot start.
``core`` itself is a clean, Home-Assistant-free package: its
``__init__.py`` is a bare docstring and every internal import is
relative (``from .db import ...``). It can therefore be imported as a
top-level package if the integration directory is placed on
``sys.path``. This module does exactly that, in ONE documented place,
so the coupling between the API and the bundled core is explicit and
testable rather than scattered across route handlers.
relative (``from .db import ...``). It can therefore be imported by
file location as a standalone package.
This module loads ``core`` with :func:`importlib.util.spec_from_file_location`
— registering it under the distinct top-level name ``nestquest_core``
with ``submodule_search_locations`` set to the bundled ``core``
directory — so its relative imports resolve against that directory
without touching ``sys.path``. No generic top-level name (``db``,
``const``, ``schema``, ...) is published, so nothing here can silently
shadow a future dependency, and the integration's HA-coupled
``__init__.py`` is never executed: ``custom_components.nestquest`` is
never imported.
Usage::
@@ -25,9 +32,22 @@ This is a TRANSITIONAL coupling. Feature 18 (task 430f7f98) converts
the integration into a thin API client and moves ``core`` out of the
integration bundle; once that lands, this bootstrap helper goes away and
the API imports ``core`` as an ordinary installed dependency.
NOTE on the pytest process: the suite's ``conftest.py`` stubs the
parent integration package and imports ``custom_components.nestquest.core.*``
BEFORE any API test runs, so two copies of the core package coexist in
the pytest process — the one registered here as ``nestquest_core.*``
(loaded by file location) and the conftest-stubbed
``custom_components.nestquest.core.*``. Later tasks must NOT compare
class or exception identity across these two copies: a
``nestquest_core.db.NestQuestDatabase`` is a DIFFERENT class object from
``custom_components.nestquest.core.db.NestQuestDatabase`` even though
they share source. Operate on a single copy within any one code path.
"""
from __future__ import annotations
import importlib
import importlib.util
import sys
from pathlib import Path
from types import ModuleType
@@ -35,53 +55,56 @@ from types import ModuleType
#: The repository root (this file is ``<root>/api/nestquest_core.py``).
_REPO_ROOT: Path = Path(__file__).resolve().parent.parent
#: The integration directory that contains the bundled ``core`` package.
#: Adding this to ``sys.path`` makes ``import core`` resolve to
#: ``custom_components/nestquest/core/`` WITHOUT executing the
#: integration's ``__init__.py`` (which would import ``homeassistant``).
_INTEGRATION_DIR: Path = _REPO_ROOT / "custom_components" / "nestquest"
#: The bundled ``core`` package directory, inside the HA integration.
_CORE_DIR: Path = _REPO_ROOT / "custom_components" / "nestquest" / "core"
_BOOTSTRAPPED = False
#: The ``core`` package's ``__init__.py`` — a bare docstring, so loading
#: it by file location executes no imports of its own.
_CORE_INIT: Path = _CORE_DIR / "__init__.py"
#: The distinct top-level name under which the bundled ``core`` package
#: is registered in ``sys.modules``. A distinctive name (rather than the
#: generic ``core``) avoids publishing shadow-able top-level names and
#: keeps the bundled copy separate from the conftest-stubbed
#: ``custom_components.nestquest.core`` present in the pytest process.
_CORE_MODULE_NAME = "nestquest_core"
def bootstrap_core() -> None:
"""Make the bundled ``core`` package importable as a top-level package.
def _load_core() -> ModuleType:
"""Load the bundled ``core`` package by file location and return it.
Inserts the integration directory at the front of ``sys.path`` (once,
idempotently). After this call, ``import core`` resolves to the
bundled ``custom_components/nestquest/core`` package, and its
relative internal imports (``from .db import ...``) resolve against
that same directory. The integration's HA-coupled ``__init__.py`` is
never executed: ``core`` is imported directly as a top-level package,
so ``custom_components.nestquest`` is never touched.
Registers the package under :data:`_CORE_MODULE_NAME` in
``sys.modules`` with ``submodule_search_locations`` pointing at the
bundled directory, so its relative imports (``from .db import ...``)
resolve to ``nestquest_core.<submodule>`` against that directory
without mutating ``sys.path``. Idempotent: a second call returns
the already-registered module.
"""
global _BOOTSTRAPPED
if _BOOTSTRAPPED:
return
path = str(_INTEGRATION_DIR)
if path not in sys.path:
sys.path.insert(0, path)
_BOOTSTRAPPED = True
existing = sys.modules.get(_CORE_MODULE_NAME)
if existing is not None:
return existing
spec = importlib.util.spec_from_file_location(
_CORE_MODULE_NAME,
_CORE_INIT,
submodule_search_locations=[str(_CORE_DIR)],
)
if spec is None or spec.loader is None:
raise ImportError(f"could not load core from {_CORE_INIT}")
module = importlib.util.module_from_spec(spec)
sys.modules[_CORE_MODULE_NAME] = module
spec.loader.exec_module(module)
return module
def _ensure_core() -> ModuleType:
"""Import ``core`` (bootstrapping first) and return the module object."""
bootstrap_core()
import core # type: ignore[import-not-found] # noqa: F401
return core
#: The bundled core package, imported via the sys.path bootstrap. Hold a
#: reference here so callers import the submodules through it
#: (``core.db``, ``core.migrations``) the same way the integration does.
core: ModuleType = _ensure_core()
#: The bundled core package, loaded by file location and registered as
#: ``nestquest_core`` in ``sys.modules``. Held here so callers import the
#: submodules through it (``nestquest_core.db``, ``nestquest_core.migrations``)
#: the same relative way the integration does.
core: ModuleType = _load_core()
#: Re-exported for convenience; route handlers import these names from
#: here rather than reaching into ``core`` directly.
core_db = __import__("core.db", fromlist=["db"]) # type: ignore[import-not-found]
core_migrations = __import__( # type: ignore[import-not-found]
"core.migrations", fromlist=["migrations"]
)
#: here rather than reaching into the core package directly.
core_db = importlib.import_module(f"{_CORE_MODULE_NAME}.db")
core_migrations = importlib.import_module(f"{_CORE_MODULE_NAME}.migrations")
__all__ = ["bootstrap_core", "core", "core_db", "core_migrations"]
__all__ = ["core", "core_db", "core_migrations"]