0 Fork 0

Files

T

ayurishchevandClaude Opus 5.5 123b5abdfc Ревью кодовой базы и исправления корректности по его итогам

Ревью кодовой базы: 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 <noreply@anthropic.com>

2026-09-27 21:23:34 +03:00

8.2 KiB

Raw Blame History

Ревью кодовой базы 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 вида `<префикс>_<uuid7>`; версия схемы в `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`	Низкий

8.2 KiB Raw Blame History Unescape Escape