From 123b5abdfc90bb9684834d373cda67fcfad68e5b Mon Sep 17 00:00:00 2001 From: ayurishchev Date: Sun, 27 Sep 2026 21:23:34 +0300 Subject: [PATCH] =?UTF-8?q?=D0=A0=D0=B5=D0=B2=D1=8C=D1=8E=20=D0=BA=D0=BE?= =?UTF-8?q?=D0=B4=D0=BE=D0=B2=D0=BE=D0=B9=20=D0=B1=D0=B0=D0=B7=D1=8B=20?= =?UTF-8?q?=D0=B8=20=D0=B8=D1=81=D0=BF=D1=80=D0=B0=D0=B2=D0=BB=D0=B5=D0=BD?= =?UTF-8?q?=D0=B8=D1=8F=20=D0=BA=D0=BE=D1=80=D1=80=D0=B5=D0=BA=D1=82=D0=BD?= =?UTF-8?q?=D0=BE=D1=81=D1=82=D0=B8=20=D0=BF=D0=BE=20=D0=B5=D0=B3=D0=BE=20?= =?UTF-8?q?=D0=B8=D1=82=D0=BE=D0=B3=D0=B0=D0=BC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ревью кодовой базы: docs/reviews/2026-09-27-codebase-review.md. Корректность и согласованность, пункты 5–7 ревью (docs/changes/018): - одиночное удаление бэкапа в UI идёт через общий delete_many: пометка deleted_at и событие backup.deleted, как у группового удаления и API; - единая система миграций: ручные ALTER из db._migrate перенесены в migrations.run (при user_version < 1, до замены ID); - групповая смена канала выполняется фоновыми задачами set_channel; PUT /api/v1/batch/channel → 202 {"job_ids": [...]} (ломающее изменение API), меню «Канал» в UI выводит задачи в панель «Задачи». Тесты: 22 из 22. Стенд проверен на порту 8001 (8000 занят посторонним процессом), боевые данные не изменены. Ручная проверка UI пользователем на момент коммита не подтверждена. Co-Authored-By: Claude Opus 5.5 --- README.md | 8 +- app/api/v1.py | 10 +-- app/db.py | 13 --- app/migrations.py | 18 ++++- app/services/jobs.py | 15 ++-- app/services/ops.py | 6 ++ app/ui/routes.py | 16 ++-- app/ui/templates/_jobs.html | 2 +- app/ui/templates/dashboard.html | 2 +- .../018-correctness-consistency/plan.md | 80 +++++++++++++++++++ .../018-correctness-consistency/summary.md | 29 +++++++ docs/reviews/2026-09-27-codebase-review.md | 78 ++++++++++++++++++ tests/test_app.py | 67 ++++++++++++++++ 13 files changed, 303 insertions(+), 41 deletions(-) create mode 100644 docs/changes/018-correctness-consistency/plan.md create mode 100644 docs/changes/018-correctness-consistency/summary.md create mode 100644 docs/reviews/2026-09-27-codebase-review.md diff --git a/README.md b/README.md index 93b0c62..8f93c3d 100644 --- a/README.md +++ b/README.md @@ -89,7 +89,7 @@ docker compose up -d --build # UI: http://localhost:8000, OpenAPI: /docs python3 -m venv venv && ./venv/bin/pip install -r requirements.txt set -a; . ./.env; set +a ./venv/bin/uvicorn app.main:app --reload -./venv/bin/python -m pytest # 20 тестов, фоновый опрос в тестах выключен +./venv/bin/python -m pytest # 22 теста, фоновый опрос в тестах выключен ``` ## API v1 @@ -113,8 +113,8 @@ curl -s -H "Authorization: Bearer $API_TOKEN" http://localhost:8000/api/v1/devic | POST | `/api/v1/devices/{id}/firmware/upgrade` | обновление FW (задача) | | GET/POST | `/api/v1/groups` | группы (с числом устройств) / создать | | PATCH/DELETE | `/api/v1/groups/{id}` | переименовать / удалить (устройства остаются без группы) | -| POST | `/api/v1/batch/{backup\|ros_update\|fw_update}` | `{"device_ids": [...]}` и/или `{"group_id": N}` — групповая операция | -| PUT | `/api/v1/batch/channel` | `{"device_ids": [...] или "group_id": N, "channel": "..."}` | +| POST | `/api/v1/batch/{backup\|ros_update\|fw_update}` | `{"device_ids": [...]}` и/или `{"group_id": N}` — групповая операция (задача) | +| PUT | `/api/v1/batch/channel` | `{"device_ids": [...] или "group_id": N, "channel": "..."}` — групповая смена канала (задача `set_channel`) → 202 `{"job_ids": [...]}` | | GET/DELETE | `/api/v1/backups`, `/backups/download?key=` | бэкапы в бакете (фильтры `device_id`, `group`, `kind`, `date_from`, `date_to`, `q`) | | POST | `/api/v1/backups/delete` | групповое удаление файлов: `{"keys": [...]}` → `{"deleted": N, "failed": M}` | | GET | `/api/v1/jobs`, `/jobs/{id}` | состояние задач | @@ -131,6 +131,7 @@ curl -s -H "Authorization: Bearer $API_TOKEN" http://localhost:8000/api/v1/devic - `app/services` — устройства и фильтры, группы, бэкапы, операции, задачи, фоновый опрос (`poller.py`) - `app/api` — JSON API; `app/ui` — WEB UI (Jinja2 + HTMX, `static/` со стилями, скриптом и шрифтами) - `tests` — минимальный набор; `docs/changes` — планы и итоги каждого изменения +- `docs/reviews` — ревью кодовой базы (замечания и приоритеты исправлений) ## История изменений @@ -152,3 +153,4 @@ curl -s -H "Authorization: Bearer $API_TOKEN" http://localhost:8000/api/v1/devic - `015-immutable-device-name` — имя устройства задаётся только при создании. - `016-unique-ids-and-event-log` — глобально уникальные ID (префикс + UUIDv7) для всех сущностей, журнал событий в БД, миграция числовых ID. - `017-events-ui-rotation` — страница «Журнал» в UI, настройки ротации, очистка журнала с подтверждением паролем. +- `018-correctness-consistency` — одиночное удаление бэкапа в UI через общий сервис `delete_many`, единая система миграций (колонки старой схемы — в `migrations.run` до миграции ID), групповая смена канала фоновыми задачами (`set_channel`). diff --git a/app/api/v1.py b/app/api/v1.py index 4af97c2..078a7c0 100644 --- a/app/api/v1.py +++ b/app/api/v1.py @@ -208,14 +208,10 @@ async def batch(action: Literal["backup", "ros_update", "fw_update"], body: Batc return {"job_ids": jobs.start_jobs(action, body.resolve())} -@router.put("/batch/channel", response_model=list[DeviceOut]) +@router.put("/batch/channel", status_code=202) async def batch_channel(body: BatchChannelIn): - targets = body.resolve() - for i in targets: - devices.get_device(i) - for i in targets: - await ops.set_channel(i, body.channel) - return [DeviceOut.of(devices.get_device(i)) for i in targets] + """Групповая смена канала — фоновыми задачами (как /batch/{backup|ros_update|fw_update}).""" + return {"job_ids": jobs.start_jobs("set_channel", body.resolve(), {"channel": body.channel})} # --- резервные копии в S3 --- diff --git a/app/db.py b/app/db.py index 9fe6f4d..d34873d 100644 --- a/app/db.py +++ b/app/db.py @@ -26,7 +26,6 @@ def init_db(url: str | None = None) -> None: from app import models # noqa: F401 (регистрация моделей) Base.metadata.create_all(_engine) # у новой БД — все таблицы, у старой — только недостающие (events) - _migrate() from app import migrations @@ -34,18 +33,6 @@ def init_db(url: str | None = None) -> None: migrations.run(_engine, db_file) -def _migrate() -> None: - """create_all не добавляет колонки в существующие таблицы — докидываем вручную.""" - with _engine.begin() as conn: - cols = {r[1] for r in conn.exec_driver_sql("PRAGMA table_info(devices)")} - if "use_tls" not in cols: - conn.exec_driver_sql("ALTER TABLE devices ADD COLUMN use_tls BOOLEAN NOT NULL DEFAULT 1") - if "group_id" not in cols: - conn.exec_driver_sql("ALTER TABLE devices ADD COLUMN group_id VARCHAR(40)") - if "note" not in cols: - conn.exec_driver_sql("ALTER TABLE devices ADD COLUMN note TEXT") - - @contextmanager def session_scope(): """Короткая транзакция: commit при успехе, rollback при ошибке.""" diff --git a/app/migrations.py b/app/migrations.py index c9be774..34d872a 100644 --- a/app/migrations.py +++ b/app/migrations.py @@ -41,6 +41,18 @@ def _is_legacy(con: sqlite3.Connection) -> bool: return bool(cols) and cols.get("id", "").startswith("INT") +def _add_legacy_columns(con: sqlite3.Connection) -> None: + """Колонки devices, которых не было в самой старой схеме (create_all их не добавляет в существующую + таблицу) — докидываем идемпотентно, до чтения старой таблицы в _to_v1.""" + cols = {r[1] for r in con.execute("PRAGMA table_info(devices)")} + if "use_tls" not in cols: + con.execute("ALTER TABLE devices ADD COLUMN use_tls BOOLEAN NOT NULL DEFAULT 1") + if "group_id" not in cols: + con.execute("ALTER TABLE devices ADD COLUMN group_id VARCHAR(40)") + if "note" not in cols: + con.execute("ALTER TABLE devices ADD COLUMN note TEXT") + + def run(engine: Engine, db_path: Path | None) -> None: """Приводит БД к текущей версии схемы. Идемпотентна.""" if db_path is None: # не файловая БД (:memory:) — старых данных быть не может @@ -53,8 +65,10 @@ def run(engine: Engine, db_path: Path | None) -> None: version = con.execute("PRAGMA user_version").fetchone()[0] if version >= SCHEMA_VERSION: return - if version < 1 and _is_legacy(con): - _to_v1(con, db_path) + if version < 1: + _add_legacy_columns(con) # _to_v1 читает use_tls/group_id/note из старой таблицы + if _is_legacy(con): + _to_v1(con, db_path) con.execute(f"PRAGMA user_version = {SCHEMA_VERSION}") finally: con.close() diff --git a/app/services/jobs.py b/app/services/jobs.py index 2bf0fc6..1f65c9f 100644 --- a/app/services/jobs.py +++ b/app/services/jobs.py @@ -11,6 +11,7 @@ JOB_TYPES = { "backup": ops.run_backup, "ros_update": ops.run_ros_update, "fw_update": ops.run_fw_update, + "set_channel": ops.run_set_channel, } _tasks: set[asyncio.Task] = set() @@ -35,20 +36,22 @@ def _finish(job_id: str, status: str, message: str) -> None: job_id=job_id, data={"type": j.type, "status": status}, s=s) -async def _run(job_id: str, job_type: str, device_id: str) -> None: +async def _run(job_id: str, job_type: str, device_id: str, params: dict) -> None: events.set_job(job_id) # события и бэкап, созданные внутри задачи, ссылаются на её ID async with _semaphore(): _finish(job_id, "running", "") try: - _finish(job_id, "done", await JOB_TYPES[job_type](device_id)) + _finish(job_id, "done", await JOB_TYPES[job_type](device_id, **params)) except Exception as e: # noqa: BLE001 — любой сбой фиксируем в задаче _finish(job_id, "failed", str(e)) -def start_jobs(job_type: str, device_ids: list[str]) -> list[str]: - """Создаёт задачи для списка устройств и запускает их в фоне. Возвращает ID задач.""" +def start_jobs(job_type: str, device_ids: list[str], params: dict | None = None) -> list[str]: + """Создаёт задачи для списка устройств и запускает их в фоне. params передаются раннеру именованными + аргументами (device_id, **params) и попадают в data события job.created. Возвращает ID задач.""" if job_type not in JOB_TYPES: raise ValueError(f"Неизвестный тип задачи: {job_type}") + params = params or {} for did in device_ids: ids.check(did, "dev") started = [] @@ -61,10 +64,10 @@ def start_jobs(job_type: str, device_ids: list[str]) -> list[str]: s.add(j) s.flush() events.record("job.created", "job", j.id, f"Задача {job_type} для {d.name} создана", device_id=did, - job_id=j.id, data={"type": job_type}, s=s) + job_id=j.id, data={"type": job_type, **params}, s=s) started.append((j.id, did)) for jid, did in started: - task = asyncio.create_task(_run(jid, job_type, did)) # наследует контекст (актор) + task = asyncio.create_task(_run(jid, job_type, did, params)) # наследует контекст (актор) _tasks.add(task) task.add_done_callback(_tasks.discard) return [jid for jid, _ in started] diff --git a/app/services/ops.py b/app/services/ops.py index f174742..dc4f902 100644 --- a/app/services/ops.py +++ b/app/services/ops.py @@ -127,6 +127,12 @@ async def set_channel(device_id: str, channel: str) -> None: await refresh_status(device_id) +async def run_set_channel(device_id: str, channel: str) -> str: + """Раннер задачи set_channel: та же смена канала, что и для одного устройства.""" + await set_channel(device_id, channel) + return f"Канал {channel} установлен" + + async def run_ros_update(device_id: str) -> str: async with devices.open_client(devices.get_conn(device_id)) as c: return await ros.install_ros_update(c) diff --git a/app/ui/routes.py b/app/ui/routes.py index dd490a3..de4ff7f 100644 --- a/app/ui/routes.py +++ b/app/ui/routes.py @@ -191,10 +191,11 @@ async def batch(request: Request, action: str, device_ids: list[str] = Form(defa if not device_ids: return _render(request, "_jobs.html", jobs=jobs.list_jobs(15), flash="Выберите устройства") if action == "channel": - for i in device_ids: - await ops.set_channel(i, channel) - return _render(request, "_devices.html", oob=True, **_devices_ctx(_flt(await request.form()))) - jobs.start_jobs(action, device_ids) + if channel not in CHANNELS: + raise ValueError(f"Неизвестный канал: {channel}") + jobs.start_jobs("set_channel", device_ids, {"channel": channel}) + else: + jobs.start_jobs(action, device_ids) return _render(request, "_jobs.html", jobs=jobs.list_jobs(15)) @@ -411,10 +412,9 @@ async def backup_download(key: str): @router.post("/backups/delete", dependencies=[Depends(require_login)]) async def backup_delete(key: str = Form()): - if not s3.key_allowed(key): - raise ValueError("Недопустимый ключ") - await s3.delete_object(key) - return RedirectResponse("/backups", status_code=303) + """Одиночное удаление — тот же сервис, что и групповое: метаданные и событие backup.deleted не теряются.""" + deleted, failed = await backups.delete_many([key]) + return RedirectResponse(f"/backups?deleted={deleted}&failed={failed}", status_code=303) # --- журнал событий --- diff --git a/app/ui/templates/_jobs.html b/app/ui/templates/_jobs.html index 344eb71..67c176b 100644 --- a/app/ui/templates/_jobs.html +++ b/app/ui/templates/_jobs.html @@ -18,7 +18,7 @@ {{ j.id|short_id }} {{ j.device_name }} - {{ {"backup": "Бэкап", "ros_update": "Обновление ROS", "fw_update": "Обновление FW"}.get(j.type, j.type) }} + {{ {"backup": "Бэкап", "ros_update": "Обновление ROS", "fw_update": "Обновление FW", "set_channel": "Смена канала"}.get(j.type, j.type) }} {{ {"pending": "В очереди", "running": "Выполняется", "done": "Готово", "failed": "Ошибка"}.get(j.status, j.status) }} {{ j.message }} {{ j.created_at|dt }} diff --git a/app/ui/templates/dashboard.html b/app/ui/templates/dashboard.html index d4a6e28..8a1c043 100644 --- a/app/ui/templates/dashboard.html +++ b/app/ui/templates/dashboard.html @@ -63,7 +63,7 @@
Канал {{ ui.icon("chev", 14) }}
- {% for c in channels %}{% endfor %} + {% for c in channels %}{% endfor %}
diff --git a/docs/changes/018-correctness-consistency/plan.md b/docs/changes/018-correctness-consistency/plan.md new file mode 100644 index 0000000..cc8a07f --- /dev/null +++ b/docs/changes/018-correctness-consistency/plan.md @@ -0,0 +1,80 @@ +# План: 018 — корректность и согласованность (по ревью 2026-09-27) + +## Context + +Ревью `docs/reviews/2026-09-27-codebase-review.md`, раздел «Корректность и согласованность», пункты 5–7: + +- **п. 5** — одиночное удаление бэкапа в UI (`POST /backups/delete`, `app/ui/routes.py::backup_delete`) вызывает + `s3.delete_object` напрямую. Метаданные (`backups.deleted_at`) не обновляются, событие `backup.deleted` не пишется — + в отличие от группового удаления и API, которые идут через `backups.delete_many`. +- **п. 6** — две системы миграций: `app/db.py::_migrate` (ручные `ALTER TABLE devices ADD COLUMN …` без версии) + и `app/migrations.py` (`PRAGMA user_version`). Нужна одна точка с версией схемы. +- **п. 7** — групповая смена канала (`PUT /api/v1/batch/channel`, `POST /ui/batch/channel`) выполняется последовательно + внутри HTTP-запроса: на большом парке упирается в таймаут, сбой одного устройства прерывает остальные. + +Решения пользователя: +- п. 7 — **фоновые задачи**: новый тип задачи `set_channel`; `PUT /api/v1/batch/channel` меняет контракт на + `202 {"job_ids": [...]}` (как `/batch/backup`). +- Стенд — существующий контейнер `ros_control-ros_control-1` (compose-проект в корне репозитория). **В томе боевые данные: + удалять существующие устройства, группы, бэкапы, задачи и записи журнала нельзя.** Для проверок можно создавать новые + объекты; созданное проверкой убирается только штатными средствами, существующее не трогается. + +## Изменения + +### п. 5 — одиночное удаление через общий сервис (`app/ui/routes.py`) +- `backup_delete`: вместо `s3.key_allowed` + `s3.delete_object` вызвать `backups.delete_many([key])` + (проверка ключа, удаление, `sync_rows` с событием уже внутри). Недопустимый ключ — прежний `ValueError` → 400. +- Ответ — редирект на `/backups?deleted=N&failed=M`, как у `backups_delete_many` (страница уже показывает итог по этим параметрам). +- API `DELETE /api/v1/backups` уже использует `delete_many` — не меняется. + +### п. 6 — единая система миграций (`app/db.py`, `app/migrations.py`) +- Удалить `db._migrate` и его вызов из `init_db`. +- В `migrations.py` добавить `_add_legacy_columns(con)`: те же три `ALTER` (`use_tls BOOLEAN NOT NULL DEFAULT 1`, + `group_id VARCHAR(40)`, `note TEXT`), идемпотентно по `PRAGMA table_info(devices)`. +- Вызов в `migrations.run` **при `version < 1` до `_is_legacy`/`_to_v1`** — `_to_v1` читает `use_tls`, `group_id`, `note` + из старой таблицы, поэтому колонки должны появиться раньше. `SCHEMA_VERSION` остаётся 2 (схема не меняется). +- Ветка `:memory:` (тесты) не меняется: `create_all` создаёт полную схему. +- Боевая БД уже на `user_version = 2` — для неё изменение не выполняет никаких действий. + +### п. 7 — групповая смена канала через задачи (`app/services/jobs.py`, `app/services/ops.py`, `app/api/v1.py`, `app/ui/routes.py`, `app/ui/templates/dashboard.html`) +- `ops.run_set_channel(device_id, channel) -> str`: **переиспользовать** `ops.set_channel` (установка канала + `refresh_status`); + возвращает сообщение «Канал установлен». +- `jobs.JOB_TYPES["set_channel"] = ops.run_set_channel`. `jobs.start_jobs(job_type, device_ids, params: dict | None = None)`: + `params` передаются раннеру как именованные аргументы (`await JOB_TYPES[t](device_id, **params)`) и пишутся в `data` + события `job.created`. Колонку в `jobs` не добавляем: незавершённые задачи после перезапуска всё равно помечаются + проваленными (`fail_stale_jobs`), повторно параметры не нужны. +- Проверка канала — до создания задач: `channel not in CHANNELS` → `ValueError` (400); в API и так `Literal[CHANNELS]`. +- API: `PUT /api/v1/batch/channel` → `status_code=202`, ответ `{"job_ids": jobs.start_jobs("set_channel", body.resolve(), {"channel": body.channel})}`. + `PUT /api/v1/devices/{id}/update/channel` (одно устройство) — **без изменений**, синхронный. +- UI: `POST /ui/batch/channel` — создаёт задачи и возвращает `_jobs.html`, как остальные групповые действия; + в `dashboard.html` у кнопок меню «Канал» `hx-target="#jobs"`, `hx-include="[name=device_ids]:checked"` (как у «Обновление»). + Смена канала у одного устройства из меню строки (`/ui/devices/{id}/channel`) — без изменений. +- Таблица устройств обновится автоопросом (`/ui/devices?poll=1`) после завершения задач — `refresh_status` уже записывает статус. +- Отображение типа задачи `set_channel` в панели «Задачи» и журнале — проверить подписи типов (если есть словарь подписей — добавить «Смена канала»). + +## Тесты (минимально) +- В существующий тест группы/устройств или новый короткий: `PUT /api/v1/batch/channel` → 202 и `job_ids` по числу устройств; + раннер подменён (`monkeypatch` на `ops.set_channel`), задача завершается `done`, в `job.created` есть `{"channel": …}`. +- `test_migration_replaces_numeric_ids`: убедиться, что тест по-прежнему покрывает старую схему без колонок `use_tls/group_id/note` + (если его фикстура создаёт их сама — добавить вариант без них). +- `test_bulk_delete_backups`: одна проверка `POST /backups/delete` → редирект с `deleted=1`, вызов идёт через `delete_many`. + +## Документация +- README: таблица API (`PUT /api/v1/batch/channel` → 202 `{"job_ids"}`), типы задач, строка 018 в «История изменений»; + число тестов в разделе «Разработка». +- `summary.md` — оркестратор после проверки. + +## Исполнение +- Код, тесты, пересборка стенда — исполнитель (Sonnet). Тесты не запускает, не коммитит, `summary.md` не создаёт. +- Оркестратор: ревью диффа, полный прогон тестов, проверки на стенде на **новых** тестовых объектах без удаления существующих данных. +- Пользователь: ручная проверка UI. + +## Проверка +- `./venv/bin/python -m pytest -q` — все зелёные. +- Стенд: `docker compose up -d --build --force-recreate`, контейнер `ros_control-ros_control-1` запущен, + `docker exec … grep -c set_channel /srv/app/services/jobs.py` ≥ 1, `PRAGMA user_version` = 2, число строк в таблицах до и после совпадает. +- Сценарии на стенде (новое тестовое устройство с недоступным адресом, например `192.0.2.1` из TEST-NET): + `PUT /api/v1/batch/channel` → 202, задача `set_channel` завершается `failed` с ошибкой соединения, остальные задачи не затронуты; + неверный канал → 422; тестовое устройство затем удаляется через API (только оно). +- Миграция старой схемы — тестом на временной БД (боевую не трогаем). +- Ручная проверка UI — пользователь: меню «Канал» при выбранных устройствах создаёт задачи в панели «Задачи»; удаление одного файла на «Бэкапах» показывает итог. diff --git a/docs/changes/018-correctness-consistency/summary.md b/docs/changes/018-correctness-consistency/summary.md new file mode 100644 index 0000000..9751132 --- /dev/null +++ b/docs/changes/018-correctness-consistency/summary.md @@ -0,0 +1,29 @@ +# Итоги: 018 — корректность и согласованность (по ревью 2026-09-27) + +Источник — ревью `docs/reviews/2026-09-27-codebase-review.md`, раздел «Корректность и согласованность» (пункты 5–7). + +## Сделано +- **п. 5 — одиночное удаление бэкапа** (`app/ui/routes.py::backup_delete`): идёт через `backups.delete_many([key])`, как групповое и API. + Проверка ключа, удаление, `sync_rows` (пометка `deleted_at`, событие `backup.deleted`) теперь общие; ответ — редирект `/backups?deleted=N&failed=M`. +- **п. 6 — единая система миграций**: `db._migrate` удалён; колонки старой схемы (`use_tls`, `group_id`, `note`) добавляет + `migrations._add_legacy_columns` при `user_version < 1` **до** `_to_v1` (та читает эти колонки). `SCHEMA_VERSION` = 2, схема не менялась. +- **п. 7 — групповая смена канала фоновыми задачами**: тип задачи `set_channel` (`ops.run_set_channel` поверх `ops.set_channel`); + `jobs.start_jobs(job_type, device_ids, params)` передаёт `params` раннеру и в `data` события `job.created`. + `PUT /api/v1/batch/channel` → **202 `{"job_ids": [...]}`** (было: синхронный список устройств). UI: меню «Канал» создаёт задачи и обновляет панель «Задачи»; + подпись «Смена канала» в таблице задач. Смена канала одного устройства (API и меню строки) не менялась. +- README: таблица API, число тестов, строка 018 в истории изменений, каталог `docs/reviews`. + +## Проверено +- `pytest`: 22 из 22 (новые: `test_batch_channel_runs_as_jobs`, `test_migration_adds_legacy_columns_before_id_migration`; `test_bulk_delete_backups` дополнен одиночным удалением). +- Стенд `ros_control-ros_control-1` пересобран, отдаёт новый код (`grep set_channel` в контейнере), `/login` → 200, старт без ошибок. +- Сценарий на стенде с временным устройством `192.0.2.1`: `PUT /batch/channel` → 202; задача `set_channel` → `failed` (`ConnectTimeout`, ожидаемо); + в `job.created` записан `{"channel": "stable"}`; неверный канал → 422; временное устройство удалено (204). +- Боевые данные: группы 4/4, устройства 12/12, бэкапы 41/41 — совпадают по ID до и после; добавились только 1 задача и 7 событий проверки; `user_version` = 2. + +## Оговорки +- **Ломающее изменение API**: `PUT /api/v1/batch/channel` возвращает 202 и ID задач вместо списка устройств — клиентам нужно опрашивать `/api/v1/jobs/{id}`. +- Порт 8000 на хосте занят посторонним процессом (`telemetry_web`), поэтому стенд временно запущен на 8001 через override-файл вне репозитория; + `docker-compose.yml` не менялся. После освобождения порта — `docker compose up -d --force-recreate`. +- Одиночное удаление на стенде не выполнялось (удалило бы реальный файл из бакета) — покрыто тестом. +- Ручная проверка UI пользователем на момент коммита не подтверждена. +- Записи проверки (тестовое устройство, задача, события) остались в журнале как обычные события. diff --git a/docs/reviews/2026-09-27-codebase-review.md b/docs/reviews/2026-09-27-codebase-review.md new file mode 100644 index 0000000..ca43f1a --- /dev/null +++ b/docs/reviews/2026-09-27-codebase-review.md @@ -0,0 +1,78 @@ +# Ревью кодовой базы ros_control — 2026-09-27 + +Состояние на коммит `26dd1f8`. Тесты: 20/20 проходят. + +## Назначение + +Централизованное управление парком MikroTik RouterOS: опрос статуса, бэкапы в S3 (Yandex Object Storage), +обновление ROS и прошивки, группы устройств, журнал событий. Объём — около 2,5 тыс. строк (Python и шаблоны). + +## Архитектура + +| Слой | Модули | Назначение | +|---|---|---| +| Вход | `app/main.py` | FastAPI; при старте запускает опрос устройств, сверку бэкапов с бакетом и ротацию журнала; ошибки переводятся в HTTP-коды 404, 400 и 502 | +| API | `app/api/v1.py` | REST под Bearer-токеном | +| UI | `app/ui/routes.py`, шаблоны | Jinja2 + HTMX, сессия в cookie, один администратор | +| Сервисы | `app/services/*` | `devices`, `groups`, `jobs` (фоновые задачи), `ops` (связка устройство + S3 + БД), `poller`, `backups`, `events`, `settings` | +| Интеграции | `app/ros/client.py`, `app/ros/operations.py`, `app/s3.py` | REST-клиент RouterOS на httpx; boto3 вызывается через `to_thread` | +| Данные | `app/models.py`, `app/db.py`, `app/migrations.py`, `app/ids.py` | SQLite; ID вида `<префикс>_`; версия схемы в `PRAGMA user_version` | + +API и UI используют один и тот же сервисный слой, поэтому логика не дублируется. Операции с устройствами +(`app/ros/operations.py`) не зависят от БД и S3, поэтому их легко тестировать. Паролей устройств нет в ответах +API, в БД они зашифрованы (Fernet). + +## Сильные стороны + +- **Бэкап:** файлы скачиваются с устройства по REST блоками в base64 со сверкой размера, затем уходят в S3. + В `finally` файлы удаляются с устройства и при успехе, и при сбое. +- **Опрос:** быстрый режим — один запрос `system/resource`. Полная проверка обновлений идёт по отдельному + интервалу. Короткий таймаут соединения позволяет быстро замечать недоступные устройства. +- **Журнал событий:** автор действия передаётся через ContextVar. Запись события идёт в одной транзакции + с изменением. Есть ротация и постраничный вывод по ID. +- **Миграция на новые ID:** перед ней делается копия файла БД, всё выполняется одной транзакцией + со сверкой числа строк. + +## Замечания + +### Безопасность + +| № | Замечание | Где | Приоритет | +|---|---|---|---| +| 1 | Небезопасные значения по умолчанию не блокируются: `API_TOKEN`, `ADMIN_PASSWORD`, `SESSION_SECRET` = `change-me`. Если `.env` не заполнен, API открыт с известным токеном. Стоит отказываться запускаться при таких значениях | `app/config.py` | Высокий | +| 2 | Вход в UI без защиты от перебора паролей. Блокировка после неверных попыток (`security.register_failure`) есть только при очистке журнала | `app/ui/routes.py`, `login` | Высокий | +| 3 | Сессионная cookie без флага Secure (`https_only=False`). Нужен TLS-терминатор перед сервисом и флаг, включаемый через настройки | `app/main.py` | Средний | +| 4 | Открытый редирект в `/ui/move`: значение `next=/\evil.com` проходит проверку, а браузер воспринимает его как адрес другого сайта. Риск низкий: запрос только POST, cookie с `SameSite=strict` | `app/ui/routes.py`, `move` | Низкий | + +### Корректность и согласованность + +| № | Замечание | Где | Приоритет | +|---|---|---|---| +| 5 | Одиночное удаление бэкапа в UI вызывает `s3.delete_object` напрямую, в обход `backups.delete_many`. В итоге не обновляются метаданные (`deleted_at`) и не пишется событие, в отличие от API и группового удаления | `app/ui/routes.py`, `backup_delete` | Средний | +| 6 | Две системы миграций: `db._migrate` (ручные `ALTER`) и `migrations.py` (`user_version`). Лучше свести их в одну | `app/db.py`, `app/migrations.py` | Низкий | +| 7 | Смена канала обновлений у группы устройств идёт последовательно и синхронно внутри HTTP-запроса. На большом парке запрос упрётся в таймаут, а ошибка на одном устройстве прерывает остальные | `/api/v1/batch/channel`, `/ui/batch/channel` | Средний | + +### Производительность и масштабирование + +| № | Замечание | Где | Приоритет | +|---|---|---|---| +| 8 | Каждое открытие страницы бэкапов перечитывает весь бакет и запускает `sync_rows` по всей таблице `backups`. Нагрузка растёт линейно с числом копий и оплачивается запросами к S3 | `app/services/backups.py`, `search` | Средний | +| 9 | Синхронные запросы к БД внутри async-обработчиков. На SQLite и небольших объёмах это терпимо, но под нагрузкой блокирует event loop | сервисный слой | Низкий | +| 10 | Состояние хранится в памяти процесса: семафор задач, `_sync_lock`, счётчики неудачных попыток пароля. Приложение рассчитано на один процесс, `--workers > 1` сломает эти гарантии. Стоит явно указать это в README | `jobs`, `backups`, `security` | Низкий | + +### Эксплуатация + +| № | Замечание | Где | Приоритет | +|---|---|---|---| +| 11 | Нет healthcheck, логирование не настроено (используется стандартный `logging`) | `Dockerfile`, `docker-compose.yml` | Низкий | +| 12 | Тесты собраны в одном файле (570 строк). Это соответствует требованию минимума тестов, но при росте проекта файл станет трудно поддерживать | `tests/test_app.py` | Низкий | + +## Рекомендуемый порядок исправлений + +1. Пункты 1 и 2: проверка небезопасных значений при старте и защита входа от перебора. Это небольшие изменения, + которые сильнее всего снижают риск. +2. Пункт 5: одиночное удаление через `delete_many`. +3. Пункт 7: групповая смена канала через фоновые задачи (`jobs`). +4. Пункт 8: кэшировать список объектов бакета или вызывать `sync_rows` только после изменений. + +Каждое исправление оформляется отдельным изменением в `docs/changes/` (план → доработка → итоги → README). diff --git a/tests/test_app.py b/tests/test_app.py index 7e22b49..e182240 100644 --- a/tests/test_app.py +++ b/tests/test_app.py @@ -269,6 +269,11 @@ def test_bulk_delete_backups(monkeypatch): h = {"Authorization": "Bearer test-token"} assert c.post("/api/v1/backups/delete", headers=h, json={"keys": ok}).json() == {"deleted": 2, "failed": 0} assert c.post("/api/v1/backups/delete", headers=h, json={"keys": ["x/../y"]}).status_code == 400 + # UI: одиночное удаление — тот же сервис delete_many (метаданные и событие backup.deleted не теряются) + removed.clear() + r = c.post("/backups/delete", data={"key": ok[0]}, follow_redirects=False) + assert r.status_code == 303 and r.headers["location"] == "/backups?deleted=1&failed=0" + assert removed == [ok[0]] @pytest.mark.asyncio @@ -383,6 +388,68 @@ def test_migration_replaces_numeric_ids(tmp_path): con.close() +LEGACY_DDL_NO_EXTRA_COLUMNS = """ +CREATE TABLE devices (id INTEGER NOT NULL, name VARCHAR(64) NOT NULL, host VARCHAR(255) NOT NULL, port INTEGER NOT NULL, + username VARCHAR(64) NOT NULL, password_enc TEXT NOT NULL, verify_tls BOOLEAN NOT NULL, created_at DATETIME NOT NULL, + online BOOLEAN, status_json TEXT NOT NULL, status_at DATETIME, last_error TEXT, last_backup_requested_at DATETIME, + PRIMARY KEY (id), UNIQUE (name)); +CREATE TABLE jobs (id INTEGER NOT NULL, device_id INTEGER, device_name VARCHAR(64) NOT NULL, type VARCHAR(32) NOT NULL, + status VARCHAR(16) NOT NULL, message TEXT NOT NULL, created_at DATETIME NOT NULL, finished_at DATETIME, PRIMARY KEY (id)); +CREATE TABLE backups (id INTEGER NOT NULL, device_id INTEGER NOT NULL, requested_at DATETIME NOT NULL, status VARCHAR(16) NOT NULL, + key_binary VARCHAR(512), key_rsc VARCHAR(512), error TEXT, PRIMARY KEY (id), + FOREIGN KEY(device_id) REFERENCES devices (id) ON DELETE CASCADE); +CREATE TABLE device_groups (id INTEGER NOT NULL, name VARCHAR(64) NOT NULL, PRIMARY KEY (id), UNIQUE (name)); +""" + + +def test_migration_adds_legacy_columns_before_id_migration(tmp_path): + """Схема до появления групп/TLS-настроек/примечаний (без use_tls/group_id/note): колонки добавляются + раньше миграции ID — _to_v1 читает их из старой таблицы.""" + path = tmp_path / "legacy2.db" + con = sqlite3.connect(path) + con.executescript(LEGACY_DDL_NO_EXTRA_COLUMNS) + con.execute("INSERT INTO devices (id, name, host, port, username, password_enc, verify_tls, created_at, status_json)" + " VALUES (1, 'r1', '10.0.0.1', 80, 'u', 'enc', 0, '2026-09-10 10:00:00.000000', '{}')") + con.commit() + con.close() + + db.init_db(f"sqlite:///{path}") + con = sqlite3.connect(path) + assert con.execute("PRAGMA user_version").fetchone()[0] == 2 + dev_id, use_tls, group_id, note = con.execute("SELECT id, use_tls, group_id, note FROM devices").fetchone() + assert ids.is_id(dev_id, "dev") and (use_tls, group_id, note) == (1, None, None) # значения по умолчанию + con.close() + + +@pytest.mark.asyncio +async def test_batch_channel_runs_as_jobs(monkeypatch): + """Групповая смена канала — фоновыми задачами: 202 + job_ids, задачи завершаются done, канал — в data job.created.""" + calls = [] + + async def fake_set_channel(device_id, channel): + calls.append((device_id, channel)) + + monkeypatch.setattr(ops, "set_channel", fake_set_channel) + d1 = devices.create_device("r1", "10.0.0.1", 443, "admin", "pw") + d2 = devices.create_device("r2", "10.0.0.2", 443, "admin", "pw") + + with TestClient(create_app()) as c: + h = {"Authorization": "Bearer test-token"} + r = c.put("/api/v1/batch/channel", headers=h, json={"device_ids": [d1.id, d2.id], "channel": "testing"}) + assert r.status_code == 202 + job_ids = r.json()["job_ids"] + assert len(job_ids) == 2 + await asyncio.sleep(0.3) + + assert sorted(calls) == sorted([(d1.id, "testing"), (d2.id, "testing")]) + for jid in job_ids: + assert jobs.get_job(jid).status == "done" + created = [e for e in events.list_events(type_="job.created") if e.entity_id in job_ids] + assert len(created) == 2 and all(json.loads(e.data) == {"type": "set_channel", "channel": "testing"} for e in created) + # неверный канал отклоняется до создания задач + assert c.put("/api/v1/batch/channel", headers=h, json={"device_ids": [d1.id], "channel": "bogus"}).status_code == 422 + + @pytest.mark.asyncio async def test_events_link_entities(monkeypatch): """Журнал: у каждой записи свой ID, ссылка на ID сущности и актор; в журнал попадают только смены online/offline."""