370 lines
15 KiB
Markdown
370 lines
15 KiB
Markdown
# Code Review: находки и предложения
|
||
|
||
> Статус: **только чтение** — ничего не изменено. Каждый пункт сопровождается предлагаемым исправлением.
|
||
|
||
---
|
||
|
||
## 1. Дублирование кода — `_safe_name` / `_safe_chapter_name`
|
||
|
||
**Файлы:** `src/api.py:251`, `src/worker.py:26–32`, `src/cli.py` (аналогичные функции)
|
||
|
||
Одна и та же функция определена в трёх местах. При изменении логики нужно менять сразу в трёх файлах — риск расхождения.
|
||
|
||
**Исправление:** вынести в `src/utils.py`, импортировать везде:
|
||
|
||
```python
|
||
# src/utils.py
|
||
import re
|
||
from .sources.base import Chapter
|
||
|
||
def safe_name(s: str) -> str:
|
||
return re.sub(r'[^\w\s\-]', '', s).strip().replace(" ", "_")[:80]
|
||
|
||
def safe_chapter_name(ch: Chapter) -> str:
|
||
vol = f"v{ch.volume:02d}_" if ch.volume else ""
|
||
return f"{vol}ch{ch.number:06.1f}"
|
||
```
|
||
|
||
---
|
||
|
||
## 2. Прямой доступ к `db.conn` в API-эндпоинтах
|
||
|
||
**Файлы:** `src/api.py`
|
||
|
||
| Место | Строки |
|
||
|-------|--------|
|
||
| `_enrich_manga` | 269–278 |
|
||
| `retry_errors` | 680–688 |
|
||
| `force_redownload` | 819–823 |
|
||
| `delete_manga` | 882–885 |
|
||
| `rename_folder` | 801–803 |
|
||
|
||
`api.py` напрямую исполняет SQL через `db.conn.execute(...)` вместо методов `StateDB`. Это означает, что логика разбросана между двумя слоями, и рефакторинг `StateDB` может незаметно сломать эндпоинты.
|
||
|
||
Для `retry_errors` и `force_redownload` в `StateDB` уже есть готовый метод `reset_failed_chapters` — он просто не используется в API.
|
||
|
||
**Исправление для `retry_errors`:**
|
||
|
||
```python
|
||
# api.py — было:
|
||
now = db.conn.execute("SELECT datetime('now')").fetchone()[0]
|
||
db.conn.execute("UPDATE chapters SET status='pending' ...", (now, url))
|
||
db.conn.commit()
|
||
|
||
# стало:
|
||
db.reset_failed_chapters(url)
|
||
```
|
||
|
||
Для `delete_manga` и `rename_folder` добавить соответствующие методы в `StateDB`.
|
||
|
||
---
|
||
|
||
## 3. `datetime.utcnow()` устарел
|
||
|
||
**Файлы:** `src/api.py:369`, `src/state.py:628`
|
||
|
||
`datetime.utcnow()` объявлен deprecated в Python 3.12. Используется в двух местах.
|
||
|
||
**Исправление:**
|
||
|
||
```python
|
||
# src/state.py
|
||
from datetime import datetime, timezone
|
||
|
||
def _now() -> str:
|
||
return datetime.now(timezone.utc).replace(tzinfo=None).isoformat()
|
||
|
||
# src/api.py — в login():
|
||
expires_at = (datetime.now(timezone.utc) + timedelta(days=30)).replace(tzinfo=None).isoformat()
|
||
```
|
||
|
||
---
|
||
|
||
## 4. `check_for_updates` не использует `db_lock`
|
||
|
||
**Файл:** `src/worker.py:343–400`
|
||
|
||
`download_manga` тщательно оборачивает все обращения к БД в `db_lock` (`asyncio.Lock`). `check_for_updates` вызывает `db.update_manga_info`, `db.get_all_chapters`, `db.set_last_checked` напрямую без блокировки. При параллельном запуске нескольких авто-обновлений возможна конкуренция записей в одну и ту же строку SQLite.
|
||
|
||
**Исправление:** добавить `db_lock` в `check_for_updates` по аналогии с `download_manga`, либо убедиться, что авто-обновления всегда запускаются последовательно (сейчас в `_run_auto_updates` они идут через `for manga in candidates` — это последовательно, но ручной `check_now` и авто-обновление могут пересечься).
|
||
|
||
---
|
||
|
||
## 5. Хак `pages_done_count = [0]`
|
||
|
||
**Файл:** `src/worker.py:196`
|
||
|
||
```python
|
||
pages_done_count = [0] # мутабельный список вместо nonlocal
|
||
|
||
async def on_page(page_idx: int, pages_total: int):
|
||
pages_done_count[0] += 1
|
||
```
|
||
|
||
Это классический обходной путь для `nonlocal` в Python 2. В Python 3 достаточно `nonlocal`.
|
||
|
||
**Исправление:**
|
||
|
||
```python
|
||
pages_done = 0
|
||
|
||
async def on_page(page_idx: int, pages_total: int):
|
||
nonlocal pages_done
|
||
pages_done += 1
|
||
await db_call(db.update_chapter_pages, ch.url, pages_total, pages_done)
|
||
```
|
||
|
||
---
|
||
|
||
## 6. Мёртвый код в `StateDB`
|
||
|
||
**Файл:** `src/state.py:405–407`
|
||
|
||
```python
|
||
def increment_manga_chapters_done(self, url: str):
|
||
# Оставлен для совместимости, но не используется в воркере
|
||
pass
|
||
```
|
||
|
||
Метод ничего не делает и нигде не вызывается.
|
||
|
||
**Исправление:** удалить.
|
||
|
||
---
|
||
|
||
## 7. Отложенный импорт `BrowserManager` в `_fetch_preview`
|
||
|
||
**Файл:** `src/api.py:548`
|
||
|
||
```python
|
||
async def _fetch_preview(url: str):
|
||
try:
|
||
from .browser import BrowserManager # импорт внутри функции
|
||
```
|
||
|
||
`BrowserManager` уже импортируется в `worker.py`. Импорт внутри функции — признак того, что его пытались скрыть из-за каких-то проблем при старте, но сейчас оснований для этого нет. Это замедляет первый вызов и затрудняет поиск зависимостей.
|
||
|
||
**Исправление:** добавить `from .browser import BrowserManager` в топ-уровневые импорты `api.py`.
|
||
|
||
Аналогично — `import shutil` внутри тел `rename_folder` (строка 789) и `delete_manga` (строка 879). Вынести в топ.
|
||
|
||
---
|
||
|
||
## 8. O(n²) назначение позиций в очереди
|
||
|
||
**Файл:** `src/api.py:486–491`
|
||
|
||
```python
|
||
queue_list = list(download_queue._queue)
|
||
for i, job in enumerate(queue_list):
|
||
for r in result: # ← внутренний цикл по всем мангам
|
||
if r["url"] == job["url"]:
|
||
r["queue_position"] = i + 1
|
||
```
|
||
|
||
При 100 мангах в очереди и 100 в БД — 10 000 итераций на каждый запрос `/api/mangas`.
|
||
|
||
**Исправление:**
|
||
|
||
```python
|
||
queue_positions = {job["url"]: i + 1 for i, job in enumerate(queue_list)}
|
||
for r in result:
|
||
r["queue_position"] = queue_positions.get(r["url"])
|
||
```
|
||
|
||
---
|
||
|
||
## 9. Утечка памяти в `_export_pdf_pillow`
|
||
|
||
**Файл:** `src/exporter.py:131–135`
|
||
|
||
```python
|
||
def _export_pdf_pillow(images: list[Path], out: Path):
|
||
from PIL import Image
|
||
pil_images = [Image.open(p).convert("RGB") for p in images]
|
||
if pil_images:
|
||
pil_images[0].save(out, save_all=True, append_images=pil_images[1:], format="PDF")
|
||
# pil_images не закрываются — файловые дескрипторы висят до GC
|
||
```
|
||
|
||
**Исправление:**
|
||
|
||
```python
|
||
def _export_pdf_pillow(images: list[Path], out: Path):
|
||
from PIL import Image
|
||
pil_images = [Image.open(p).convert("RGB") for p in images]
|
||
try:
|
||
if pil_images:
|
||
pil_images[0].save(out, save_all=True, append_images=pil_images[1:], format="PDF")
|
||
finally:
|
||
for img in pil_images:
|
||
img.close()
|
||
```
|
||
|
||
---
|
||
|
||
## 10. Потенциальный SQL-инъекционный путь в `mark_done`
|
||
|
||
**Файл:** `src/state.py:453–459`
|
||
|
||
```python
|
||
def mark_done(self, chapter_url: str, fmt: str, output_path: str):
|
||
col = f"output_{fmt}"
|
||
self.conn.execute(f"""
|
||
UPDATE chapters SET status='done', {col}=?, updated_at=?
|
||
WHERE chapter_url=?
|
||
""", (output_path, _now(), chapter_url))
|
||
```
|
||
|
||
Сейчас `fmt` всегда приходит из `["cbz", "pdf", "epub"]` в `worker.py`, поэтому эксплуатации нет. Но паттерн хрупкий: если завтра `fmt` придёт от пользователя через API, это станет инъекцией.
|
||
|
||
**Исправление:**
|
||
|
||
```python
|
||
_ALLOWED_FMTS = {"cbz", "pdf", "epub"}
|
||
|
||
def mark_done(self, chapter_url: str, fmt: str, output_path: str):
|
||
if fmt not in _ALLOWED_FMTS:
|
||
raise ValueError(f"Unknown format: {fmt}")
|
||
col = f"output_{fmt}"
|
||
...
|
||
```
|
||
|
||
---
|
||
|
||
## 11. Неиспользуемый метод `BrowserManager.navigate()`
|
||
|
||
**Файл:** `src/browser.py`
|
||
|
||
`BrowserManager` содержит метод `navigate()`, который нигде не вызывается — `readmanga.py` определяет собственный `_navigate`. Это мёртвый код.
|
||
|
||
**Исправление:** удалить `navigate()` из `BrowserManager` или убрать `_navigate` из `readmanga.py` и использовать единый метод.
|
||
|
||
---
|
||
|
||
## 12. `cli.py` использует устаревший шим вместо реестра источников
|
||
|
||
**Файл:** `src/cli.py`
|
||
|
||
```python
|
||
from .scraper import get_manga_info, get_chapter_images_and_download # shim
|
||
```
|
||
|
||
`scraper.py` — это обёртка-пустышка, делегирующая в `ReadmangaSource`. CLI не использует `SourceRegistry`, не поддерживает `MangaMeta` полноценно. При добавлении нового источника CLI не заработает автоматически.
|
||
|
||
**Исправление:** переписать `cli.py` на использование `registry` и `get_source_for_url`, как это сделано в `worker.py`.
|
||
|
||
---
|
||
|
||
## 13. Двойное чтение тела ответа в `saveRenameFolder`
|
||
|
||
**Файл:** `frontend/index.html`
|
||
|
||
```javascript
|
||
async function saveRenameFolder() {
|
||
const r = await fetch('/api/mangas/rename_folder', ...);
|
||
if (!r.ok) {
|
||
const err = await r.json(); // ← первое чтение
|
||
...
|
||
}
|
||
const data = await r.json(); // ← второе чтение (тело уже прочитано!)
|
||
```
|
||
|
||
`Response.body` — это `ReadableStream`, читается один раз. Второй `r.json()` вернёт ошибку или пустой объект.
|
||
|
||
**Исправление:**
|
||
|
||
```javascript
|
||
const data = await r.json();
|
||
if (!r.ok) {
|
||
showError(data.detail || 'Ошибка');
|
||
return;
|
||
}
|
||
```
|
||
|
||
---
|
||
|
||
## 14. `escHtml()` не защищает от JS-инъекции в `onclick`
|
||
|
||
**Файл:** `frontend/index.html` — различные места типа:
|
||
|
||
```javascript
|
||
`<button onclick="openEditUserModal(${u.id}, '${escHtml(u.username)}', ...)">
|
||
```
|
||
|
||
`escHtml` экранирует HTML-спецсимволы (`<`, `>`, `&`), но не защищает от инъекции в контекст JavaScript-строки. Если `u.username` содержит `'); evil() //` — экранирование не поможет.
|
||
|
||
**Исправление:** вместо `onclick="..."` в строке шаблона использовать `data-*` атрибуты + делегирование событий:
|
||
|
||
```javascript
|
||
`<button class="edit-user-btn" data-id="${u.id}" data-username="${escAttr(u.username)}">`
|
||
|
||
// один раз:
|
||
document.addEventListener('click', e => {
|
||
const btn = e.target.closest('.edit-user-btn');
|
||
if (btn) openEditUserModal(+btn.dataset.id, btn.dataset.username, ...);
|
||
});
|
||
```
|
||
|
||
---
|
||
|
||
## 15. Дублирование `forceRedownload` / `forceRedownloadModal`
|
||
|
||
**Файл:** `frontend/index.html`
|
||
|
||
Две функции с практически идентичным телом — `forceRedownload(url)` и `forceRedownloadModal(url)`. Отличие только в том, что вторая закрывает модал перед вызовом.
|
||
|
||
**Исправление:** одна функция с необязательным флагом или вызов `closeModal()` перед `forceRedownload()` там, где нужно.
|
||
|
||
---
|
||
|
||
## 16. `check_for_updates` в `worker.py` импортирует `scraper` шим
|
||
|
||
**Файл:** `src/worker.py:16`
|
||
|
||
```python
|
||
from .scraper import get_manga_info, get_chapter_images_and_download # shim для обратной совместимости
|
||
```
|
||
|
||
Эти символы нигде в файле не используются — `worker.py` работает через `registry`. Мёртвый импорт.
|
||
|
||
**Исправление:** удалить строку.
|
||
|
||
---
|
||
|
||
## 17. Одиночный SQLite-коннект с `check_same_thread=False` в async-среде
|
||
|
||
**Файл:** `src/state.py:27`
|
||
|
||
```python
|
||
self.conn = sqlite3.connect(str(db_path), check_same_thread=False)
|
||
```
|
||
|
||
Каждый `StateDB()` создаёт новый коннект, но внутри одного воркера `db` живёт на всё время скачивания манги. `check_same_thread=False` отключает встроенную защиту SQLite от использования коннекта в нескольких потоках. В текущей архитектуре с `asyncio` и `db_lock` это безопасно, но хрупко — одно забытое `await` без блокировки может привести к `database is locked` или повреждению данных.
|
||
|
||
**Рекомендация:** ничего не менять прямо сейчас, но при следующем крупном рефакторинге рассмотреть `aiosqlite` или SQLite WAL-режим (`PRAGMA journal_mode=WAL`) для снижения вероятности блокировок при параллельных читателях.
|
||
|
||
---
|
||
|
||
## Сводная таблица приоритетов
|
||
|
||
| # | Файл | Проблема | Приоритет |
|
||
|---|------|----------|-----------|
|
||
| 2 | api.py | Прямой `db.conn` в эндпоинтах (retry, force, delete) | Высокий |
|
||
| 8 | api.py | O(n²) очередь позиций | Высокий |
|
||
| 13 | frontend | Двойное чтение `r.json()` — баг | Высокий |
|
||
| 4 | worker.py | `check_for_updates` без `db_lock` | Средний |
|
||
| 9 | exporter.py | Утечка PIL-объектов в PDF Pillow | Средний |
|
||
| 10 | state.py | Хрупкий f-string в `mark_done` | Средний |
|
||
| 3 | api/state | `datetime.utcnow()` deprecated | Низкий |
|
||
| 1 | api/worker/cli | Дублирование `_safe_name` | Низкий |
|
||
| 5 | worker.py | Хак `pages_done_count = [0]` | Низкий |
|
||
| 6 | state.py | Мёртвый метод `increment_manga_chapters_done` | Низкий |
|
||
| 7 | api.py | Поздний `import` внутри функций | Низкий |
|
||
| 11 | browser.py | Мёртвый метод `navigate()` | Низкий |
|
||
| 12 | cli.py | Устаревший шим, нет поддержки реестра | Низкий |
|
||
| 14 | frontend | JS-инъекция через `onclick` + `escHtml` | Низкий (internal tool) |
|
||
| 15 | frontend | Дублирование `forceRedownload*` | Низкий |
|
||
| 16 | worker.py | Мёртвый импорт scraper shim | Низкий |
|
||
| 17 | state.py | `check_same_thread=False` в async | На будущее |
|