modbus: add explicit cascade delete (device + readings + toggles + HA cleanup)
A disabled device with historical readings still cannot be deleted by default
(409 guard stays, to prevent accidental data loss). A new explicit opt-in lets
the user remove it for real:
- DELETE /api/modbus/devices/{uuid}?cascade=true removes the device's readings
and exposed_entity_toggle rows, then the device, in one transaction (FK order:
readings before device). Best-effort clears the HA discovery configs first
(MQTT not connected -> no-op, never blocks the delete). Default (no cascade)
unchanged. Returns 200 with deletion counts.
- Frontend: a 'Force Delete (all data)' button appears only after the 409, with
a red irreversible warning; cascade is sent only on that explicit action.
- Also corrected a stale comment: FK RESTRICT IS enforced at runtime now.
This commit is contained in:
@@ -4,14 +4,14 @@ All endpoints are under /api/modbus, require an authenticated session, and
|
||||
write endpoints (POST/PATCH/DELETE + test-read) additionally require a
|
||||
non-empty X-CSRF-Token header.
|
||||
|
||||
Deletion safety (red-line):
|
||||
DELETE /api/modbus/devices/{uuid} explicitly queries the application
|
||||
layer for existing modbus_reading rows before deleting. If any exist
|
||||
the endpoint returns 409 and suggests disabling the device instead.
|
||||
We do NOT rely on the SQLite FK RESTRICT constraint because SQLite does
|
||||
not enforce foreign-key constraints at runtime unless
|
||||
PRAGMA foreign_keys=ON is set per connection (and it is not in this
|
||||
project — see M5-T02 review note OBS-1).
|
||||
Deletion safety:
|
||||
DELETE /api/modbus/devices/{uuid} queries the application layer for
|
||||
existing modbus_reading rows before deleting. Without ``cascade`` it
|
||||
returns 409 and suggests disabling the device instead — a friendly guard
|
||||
rather than letting the DB raise. With ``cascade=true`` it removes the
|
||||
readings (and expose toggles) first, then the device. SQLite FK RESTRICT
|
||||
*is* enforced at runtime here (the app sets PRAGMA foreign_keys=ON; see
|
||||
app/db.py), so the cascade delete order matters.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
@@ -21,7 +21,7 @@ from datetime import UTC, datetime
|
||||
from typing import Any
|
||||
|
||||
from fastapi import APIRouter, Depends, HTTPException, Query, status
|
||||
from sqlalchemy import func, select
|
||||
from sqlalchemy import delete as sa_delete, func, select
|
||||
from sqlalchemy.orm import Session
|
||||
|
||||
from app.api.routes.api.deps import require_csrf, require_session
|
||||
@@ -34,9 +34,11 @@ from app.integrations.modbus.profiles import (
|
||||
list_profiles,
|
||||
load_profile,
|
||||
)
|
||||
from app.models.expose import ExposedEntityToggle
|
||||
from app.models.modbus import ModbusDevice, ModbusReading
|
||||
from app.schemas.modbus import (
|
||||
MetricInfo,
|
||||
ModbusDeleteResponse,
|
||||
ModbusDeviceCreate,
|
||||
ModbusDeviceListResponse,
|
||||
ModbusDeviceResponse,
|
||||
@@ -220,29 +222,41 @@ def patch_device(
|
||||
)
|
||||
def delete_device(
|
||||
uuid: str,
|
||||
cascade: bool = Query(default=False),
|
||||
db: Session = Depends(get_db),
|
||||
_auth: AuthenticatedSession = Depends(require_session),
|
||||
_csrf: None = Depends(require_csrf),
|
||||
) -> None:
|
||||
) -> None | ModbusDeleteResponse:
|
||||
"""Delete a Modbus device.
|
||||
|
||||
**Default behaviour (cascade=false)**:
|
||||
Returns 409 Conflict if the device has any associated readings; use
|
||||
``enabled=false`` to disable it instead.
|
||||
|
||||
**Application-layer safety**: the check is performed via an explicit
|
||||
SELECT COUNT query — not by relying on SQLite's FK RESTRICT constraint,
|
||||
which is not enforced at runtime in this project (no PRAGMA foreign_keys=ON).
|
||||
**Cascade delete (cascade=true)**:
|
||||
Permanently deletes the device together with all its readings and any
|
||||
``ExposedEntityToggle`` rows whose key matches ``modbus.<uuid>.*``.
|
||||
Also makes a best-effort attempt to clear the device's HA Discovery
|
||||
config topics from MQTT (empty retained payload) before the DB rows
|
||||
are removed. MQTT failures are swallowed — the DB deletion proceeds
|
||||
regardless.
|
||||
Returns HTTP 200 with a ``ModbusDeleteResponse`` JSON body on success.
|
||||
|
||||
**Application-layer safety**: the 409 guard uses an explicit SELECT COUNT
|
||||
query to return a friendly message. FK RESTRICT is enforced at runtime
|
||||
(the app sets ``PRAGMA foreign_keys=ON``), so the cascade path deletes
|
||||
readings before the device.
|
||||
"""
|
||||
from app.services.ha_discovery import clear_device_discovery
|
||||
|
||||
device = _get_device_or_404(db, uuid)
|
||||
|
||||
# Application-layer guard: refuse deletion if any readings exist.
|
||||
# We do NOT rely on SQLite FK RESTRICT — it is not enforced at runtime
|
||||
# without PRAGMA foreign_keys=ON, which is not set in this project.
|
||||
reading_count: int = db.execute(
|
||||
select(func.count(ModbusReading.id)).where(ModbusReading.device_id == device.id)
|
||||
).scalar_one()
|
||||
|
||||
if reading_count > 0:
|
||||
if reading_count > 0 and not cascade:
|
||||
raise HTTPException(
|
||||
status_code=status.HTTP_409_CONFLICT,
|
||||
detail=(
|
||||
@@ -252,6 +266,60 @@ def delete_device(
|
||||
),
|
||||
)
|
||||
|
||||
if cascade:
|
||||
# --- Cascade deletion path ---
|
||||
logger.info(
|
||||
"Cascade-deleting Modbus device %r (uuid=%s): %d reading(s)",
|
||||
device.friendly_name,
|
||||
uuid,
|
||||
reading_count,
|
||||
)
|
||||
|
||||
# 1. Best-effort HA Discovery cleanup (before DB rows are gone).
|
||||
try:
|
||||
clear_device_discovery(db, uuid)
|
||||
except Exception: # noqa: BLE001
|
||||
logger.exception(
|
||||
"delete_device(cascade): HA discovery cleanup raised for uuid=%s; continuing",
|
||||
uuid,
|
||||
)
|
||||
|
||||
# 2. Delete all readings for this device (must happen before device row).
|
||||
deleted_readings_result = db.execute(
|
||||
sa_delete(ModbusReading).where(ModbusReading.device_id == device.id)
|
||||
)
|
||||
readings_deleted: int = deleted_readings_result.rowcount
|
||||
|
||||
# 3. Delete all ExposedEntityToggle rows for this device.
|
||||
# Keys follow the pattern "modbus.<uuid>.<metric>".
|
||||
toggle_prefix = f"modbus.{uuid}.%"
|
||||
deleted_toggles_result = db.execute(
|
||||
sa_delete(ExposedEntityToggle).where(ExposedEntityToggle.key.like(toggle_prefix))
|
||||
)
|
||||
toggles_deleted: int = deleted_toggles_result.rowcount
|
||||
|
||||
# 4. Delete the device itself.
|
||||
db.delete(device)
|
||||
db.commit()
|
||||
|
||||
logger.info(
|
||||
"Cascade-delete complete for device uuid=%s: %d reading(s), %d toggle(s) removed",
|
||||
uuid,
|
||||
readings_deleted,
|
||||
toggles_deleted,
|
||||
)
|
||||
|
||||
from fastapi.responses import JSONResponse
|
||||
return JSONResponse(
|
||||
status_code=status.HTTP_200_OK,
|
||||
content={
|
||||
"deleted": True,
|
||||
"readings_deleted": readings_deleted,
|
||||
"toggles_deleted": toggles_deleted,
|
||||
},
|
||||
)
|
||||
|
||||
# --- Non-cascade path (no readings exist at this point) ---
|
||||
logger.info("Deleting Modbus device %r (uuid=%s)", device.friendly_name, uuid)
|
||||
db.delete(device)
|
||||
db.commit()
|
||||
|
||||
@@ -152,3 +152,20 @@ class ModbusTestReadResponse(BaseModel):
|
||||
ok: bool
|
||||
payload: dict[str, Any] | None = None
|
||||
error: str | None = None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Cascade-delete schema
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class ModbusDeleteResponse(BaseModel):
|
||||
"""Response for DELETE /api/modbus/devices/{uuid}?cascade=true.
|
||||
|
||||
Returned only when cascade deletion succeeds (HTTP 200). Non-cascade
|
||||
successful deletes continue to return HTTP 204 (no body).
|
||||
"""
|
||||
|
||||
deleted: bool
|
||||
readings_deleted: int
|
||||
toggles_deleted: int
|
||||
|
||||
@@ -407,6 +407,74 @@ def publish_device_state(session: Session, device: Any) -> None:
|
||||
)
|
||||
|
||||
|
||||
def clear_device_discovery(session: Session, device_uuid: str) -> None:
|
||||
"""Best-effort: send empty retained payloads to all HA Discovery config topics
|
||||
for the given device, effectively removing the device's entities from HA.
|
||||
|
||||
This must be called **before** the device rows are deleted from the DB, so
|
||||
that ``build_catalog`` can still enumerate the device's entities.
|
||||
|
||||
Behaviour
|
||||
---------
|
||||
- No-op if MQTT / HA Discovery is not enabled or the MQTT client is not
|
||||
connected.
|
||||
- All exceptions are caught internally; this function never raises.
|
||||
The caller proceeds with the DB deletion regardless of MQTT outcome.
|
||||
|
||||
Parameters
|
||||
----------
|
||||
session:
|
||||
Active SQLAlchemy session (device must still exist in DB at call time).
|
||||
device_uuid:
|
||||
UUID string of the device being deleted.
|
||||
"""
|
||||
from app.config import get_settings
|
||||
settings = build_runtime_settings(session, get_settings())
|
||||
if not _should_publish(settings):
|
||||
logger.debug(
|
||||
"clear_device_discovery: skipped (MQTT/Discovery not enabled or not connected)"
|
||||
)
|
||||
return
|
||||
|
||||
discovery_prefix = settings.ha_discovery_prefix
|
||||
state_prefix = settings.ha_state_topic_prefix
|
||||
|
||||
try:
|
||||
catalog = build_catalog(session)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"clear_device_discovery: failed to build catalog for device uuid=%s; skipping HA cleanup",
|
||||
device_uuid,
|
||||
)
|
||||
return
|
||||
|
||||
cleared = 0
|
||||
for entry in catalog:
|
||||
entity = entry.entity
|
||||
# Only clear entities belonging to this device.
|
||||
if entity.device.identifiers[1] != device_uuid:
|
||||
continue
|
||||
try:
|
||||
topic, _config = build_discovery_payload(entity, discovery_prefix, state_prefix)
|
||||
mqtt_manager.publish(topic, b"", retain=True)
|
||||
cleared += 1
|
||||
logger.debug(
|
||||
"clear_device_discovery: cleared config topic for entity %r → %s",
|
||||
entity.key,
|
||||
topic,
|
||||
)
|
||||
except Exception:
|
||||
logger.exception(
|
||||
"clear_device_discovery: error clearing entity %r; continuing", entity.key
|
||||
)
|
||||
|
||||
logger.info(
|
||||
"clear_device_discovery: cleared %d HA discovery topic(s) for device uuid=%s",
|
||||
cleared,
|
||||
device_uuid,
|
||||
)
|
||||
|
||||
|
||||
def publish_device_offline(session: Session, device_uuid: str) -> None:
|
||||
"""Publish an "offline" availability payload for *device_uuid*.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user