fix(ptica): три места, где код делал не то, что говорил (#2464)
All checks were successful
CI Trade-In / changes (pull_request) Successful in 10s
CI Trade-In / backend-tests (pull_request) Has been skipped
CI / changes (pull_request) Successful in 10s
CI Trade-In / browser-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Successful in 2m17s
CI / backend-tests (pull_request) Successful in 17m20s
All checks were successful
CI Trade-In / changes (pull_request) Successful in 10s
CI Trade-In / backend-tests (pull_request) Has been skipped
CI / changes (pull_request) Successful in 10s
CI Trade-In / browser-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Successful in 2m17s
CI / backend-tests (pull_request) Successful in 17m20s
1. `load_water_reserves_from_docx` собирал `result` с ключом `period`,
печатал ПОЛНЫЙ словарь в лог, а возвращал
`{k: v for … if isinstance(v, int)}` — период это строка или None,
поэтому выбрасывался всегда. По логам казалось, что период отдаётся;
вызывающий не получал его ни разу.
Фильтр стоял ради аннотации `dict[str, int]` и ничего не защищал:
соседняя ветка `load_water_reserves` кладёт в тот же словарь
`{"error": str(...)}`, а единственный потребитель — задача
`sync_water_reserves` — результат логирует и возвращает как есть.
Период полезен: без него «загружено 42 записи» не отличить от
прошлогодних. Аннотация исправлена, иначе следующий проход mypy вернул
бы фильтр обратно — на это поставлен отдельный тест.
2. `get_sqlite_info` — TOCTOU: `p.exists()`, затем незащищённый
`p.stat()`. try/except покрывал только `sqlite3.connect` ниже, поэтому
OSError из stat улетал наружу и превращал диагностическую функцию в
источник отказа. Файл между проверками реально исчезает — его
переписывает выгрузка Объектива. Теперь отдаём то, что успели узнать,
с ключом `stat_error`.
3. `place_program` — предупреждение «участок мал» печатало КАТАЛОЖНЫЕ
`house.footprint_*`, хотя ставили по `fp_w`/`fp_d`. При
переопределённом в программе габарите сообщение называло размер,
которым никто не пытался ставить, и уводило от причины.
Двусторонне: против origin/main четыре теста красные с конкретными
значениями («период выброшен из ответа: {'records': 1, 'inserted': 1,
'updated': 0}», «аннотация всё ещё требует только int: dict[str, int]»).
Контроли зелёные с обеих сторон: отсутствующий файл по-прежнему даёт
exists=False без ошибки; `fp_w`/`fp_d` — действительно те размеры,
которыми ставят (иначе первый тест сверял бы имена, а не смысл).
pytest backend/tests/services/ — 3207 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
c979cf886a
commit
f227768a53
4 changed files with 161 additions and 6 deletions
|
|
@ -295,13 +295,17 @@ def place_program(
|
|||
)
|
||||
placed_for_item += 1
|
||||
if placed_for_item < item.count:
|
||||
# Печатаем ФАКТИЧЕСКИ использованные размеры fp_w/fp_d, а не каталожные
|
||||
# house.footprint_* (#2464): если элемент программы переопределил габарит,
|
||||
# прежнее сообщение называло размер, которым никто не пытался ставить, —
|
||||
# диагностика уводила от причины «участок мал».
|
||||
logger.warning(
|
||||
"program: type=%s placed %d of %d sections (%.0fx%.0f m) — участок мал",
|
||||
item.section_type,
|
||||
placed_for_item,
|
||||
item.count,
|
||||
house.footprint_w_m,
|
||||
house.footprint_d_m,
|
||||
fp_w,
|
||||
fp_d,
|
||||
)
|
||||
|
||||
result = PlacedProgram(
|
||||
|
|
|
|||
|
|
@ -463,7 +463,15 @@ def get_sqlite_info(sqlite_path: str | Path) -> dict[str, Any]:
|
|||
}
|
||||
if not p.exists():
|
||||
return info
|
||||
st = p.stat()
|
||||
# stat() под защитой (#2464): между exists() и stat() файл может исчезнуть —
|
||||
# его переписывает выгрузка Объектива. Раньше try/except покрывал только
|
||||
# sqlite3.connect ниже, и OSError отсюда улетал наружу, превращая
|
||||
# диагностическую функцию в источник отказа. Отдаём то, что успели узнать.
|
||||
try:
|
||||
st = p.stat()
|
||||
except OSError as e:
|
||||
info["stat_error"] = f"{type(e).__name__}: {e}"
|
||||
return info
|
||||
info["size_bytes"] = st.st_size
|
||||
info["modified_at"] = st.st_mtime # epoch seconds
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -458,11 +458,15 @@ def load_water_reserves_from_docx(
|
|||
system_kind: str,
|
||||
docx_bytes: bytes,
|
||||
source_url: str = "",
|
||||
) -> dict[str, int]:
|
||||
) -> dict[str, object]:
|
||||
"""Парсит docx-байты → UPSERT ЦСВ/ЦСК в water_supply_reserves.
|
||||
|
||||
Выделено из load_water_reserves для юнит-теста на синтетическом docx.
|
||||
Читает word/document.xml из zip, forward-fill vMerge, извлечение записей.
|
||||
|
||||
Возвращает счётчики (`records`, `inserted`, `updated`, …) И `period` — строку
|
||||
вида «III кв. 2025» либо None. Раньше тип был `dict[str, int]`, и ради него
|
||||
период выбрасывался фильтром на выходе, хотя в лог печатался (#2464).
|
||||
"""
|
||||
with zipfile.ZipFile(io.BytesIO(docx_bytes)) as zf:
|
||||
document_xml = zf.read("word/document.xml")
|
||||
|
|
@ -485,9 +489,19 @@ def load_water_reserves_from_docx(
|
|||
logger.exception("load_water_reserves_from_docx: outer tx rolled back: %s", e)
|
||||
raise
|
||||
|
||||
result = {"records": len(records), **counts, "period": period} # type: ignore[dict-item]
|
||||
# `period` возвращаем вместе с остальным (#2464). Раньше стоял фильтр
|
||||
# `isinstance(v, int)`, который выбрасывал его ВСЕГДА — период это строка или
|
||||
# None. В лог при этом печатался полный словарь, поэтому по логам казалось, что
|
||||
# период отдаётся, а вызывающий его не получал никогда.
|
||||
#
|
||||
# Фильтр ничего не защищал: соседняя ветка `load_water_reserves` кладёт в тот же
|
||||
# словарь `{"error": str(...)}`, то есть «только int» контрактом не было, а
|
||||
# единственный потребитель (задача sync_water_reserves) результат логирует и
|
||||
# возвращает как есть. Период при этом полезен: он говорит, за какой квартал
|
||||
# данные, — без него «загружено 42 записи» не отличить от прошлогодних.
|
||||
result: dict[str, object] = {"records": len(records), **counts, "period": period}
|
||||
logger.info("water_reserves[%s] done: %s", system_kind, result)
|
||||
return {k: v for k, v in result.items() if isinstance(v, int)}
|
||||
return result
|
||||
|
||||
|
||||
def load_water_reserves(db: Session | None = None) -> dict[str, dict]:
|
||||
|
|
|
|||
129
backend/tests/services/test_2464_three_small_leaks.py
Normal file
129
backend/tests/services/test_2464_three_small_leaks.py
Normal file
|
|
@ -0,0 +1,129 @@
|
|||
"""Три места, где код делал не то, что говорил (#2464).
|
||||
|
||||
1. `load_water_reserves_from_docx` (vodokanal): собирал `result` с ключом `period`,
|
||||
печатал полный словарь в лог, а возвращал `{k: v for … if isinstance(v, int)}` —
|
||||
период это строка или None, поэтому он выбрасывался ВСЕГДА. По логам казалось,
|
||||
что период отдаётся; вызывающий не получал его ни разу.
|
||||
|
||||
2. `get_sqlite_info` (objective_etl): TOCTOU — `p.exists()`, затем незащищённый
|
||||
`p.stat()`. try/except покрывал только `sqlite3.connect` ниже, поэтому OSError
|
||||
из stat улетал наружу и превращал диагностическую функцию в источник отказа.
|
||||
|
||||
3. `place_program` (placement): предупреждение «участок мал» печатало КАТАЛОЖНЫЕ
|
||||
`house.footprint_*`, хотя ставили по `fp_w`/`fp_d`. Если элемент программы
|
||||
переопределил габарит, сообщение называло размер, которым никто не пытался
|
||||
ставить, — диагностика уводила от причины.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import inspect
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
# ── 1. period больше не выбрасывается ────────────────────────────────────────
|
||||
|
||||
|
||||
def test_water_result_keeps_period() -> None:
|
||||
"""Головной: период обязан доехать до вызывающего, раз он попал в лог.
|
||||
|
||||
На origin/main фильтр `isinstance(v, int)` выбрасывает его всегда.
|
||||
"""
|
||||
from app.services.site_finder import vodokanal_reserve_loader as mod
|
||||
|
||||
with (
|
||||
patch.object(mod, "parse_docx_table_rows", lambda _x: [["шапка"]]),
|
||||
patch.object(mod, "extract_water_rows", lambda _m: [{"name": "ЦСВ-1"}]),
|
||||
patch.object(mod, "_dedupe_names", lambda r: r),
|
||||
patch.object(mod, "infer_period", lambda _u: "III кв. 2025"),
|
||||
patch.object(mod, "_upsert_water_rows", lambda *a, **k: {"inserted": 1, "updated": 0}),
|
||||
patch.object(mod.zipfile, "ZipFile", MagicMock()),
|
||||
):
|
||||
res = mod.load_water_reserves_from_docx(MagicMock(), "supply", b"", "http://x")
|
||||
assert (
|
||||
res.get("period") == "III кв. 2025"
|
||||
), f"период выброшен из ответа: {res} — по логам он есть, у вызывающего нет"
|
||||
assert res.get("records") == 1 and res.get("inserted") == 1, res
|
||||
|
||||
|
||||
def test_water_return_annotation_allows_non_int() -> None:
|
||||
"""Контроль: фильтр стоял ради аннотации `dict[str, int]` — она тоже исправлена.
|
||||
|
||||
Иначе следующий проход mypy вернул бы фильтр обратно.
|
||||
"""
|
||||
from app.services.site_finder.vodokanal_reserve_loader import load_water_reserves_from_docx
|
||||
|
||||
ann = inspect.signature(load_water_reserves_from_docx).return_annotation
|
||||
assert "int]" not in str(ann), f"аннотация всё ещё требует только int: {ann}"
|
||||
|
||||
|
||||
# ── 2. TOCTOU в get_sqlite_info ──────────────────────────────────────────────
|
||||
|
||||
|
||||
def test_sqlite_info_survives_file_vanishing_between_exists_and_stat() -> None:
|
||||
"""Головной: файл исчез между exists() и stat() — функция не падает.
|
||||
|
||||
На origin/main OSError улетает наружу: try/except покрывает только connect.
|
||||
"""
|
||||
from app.services import objective_etl as mod
|
||||
|
||||
настоящий_stat = Path.stat
|
||||
|
||||
def _stat(self, *a, **k):
|
||||
if str(self).endswith("исчезающий.sqlite"):
|
||||
raise FileNotFoundError(2, "No such file or directory")
|
||||
return настоящий_stat(self, *a, **k)
|
||||
|
||||
with (
|
||||
patch.object(Path, "exists", lambda self: True),
|
||||
patch.object(Path, "stat", _stat),
|
||||
):
|
||||
info = mod.get_sqlite_info("/tmp/исчезающий.sqlite")
|
||||
|
||||
assert info["exists"] is True
|
||||
assert "stat_error" in info, f"ошибка stat не отражена в ответе: {info}"
|
||||
assert "size_bytes" not in info, "размер выдуман при отсутствующем файле"
|
||||
|
||||
|
||||
def test_sqlite_info_missing_file_unchanged() -> None:
|
||||
"""Контроль: отсутствующий файл по-прежнему даёт exists=False без ошибок."""
|
||||
from app.services.objective_etl import get_sqlite_info
|
||||
|
||||
info = get_sqlite_info("/tmp/такого-файла-нет-2464.sqlite")
|
||||
assert info["exists"] is False
|
||||
assert "stat_error" not in info, info
|
||||
|
||||
|
||||
# ── 3. предупреждение placement называет фактические габариты ────────────────
|
||||
|
||||
|
||||
def test_placement_warning_uses_actual_footprint_not_catalog() -> None:
|
||||
"""Головной: в сообщении должны стоять fp_w/fp_d, а не house.footprint_*.
|
||||
|
||||
На origin/main при переопределённом габарите печатается каталожный размер.
|
||||
"""
|
||||
from app.services.generative import placement as mod
|
||||
|
||||
src = inspect.getsource(mod.place_program)
|
||||
хвост = src[src.index("участок мал") :]
|
||||
assert (
|
||||
"fp_w," in хвост and "fp_d," in хвост
|
||||
), f"в предупреждении не фактические габариты:\n{хвост[:320]}"
|
||||
assert (
|
||||
"house.footprint_w_m," not in хвост and "house.footprint_d_m," not in хвост
|
||||
), f"в предупреждении остался каталожный размер:\n{хвост[:320]}"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("имя", ["fp_w", "fp_d"])
|
||||
def test_placement_uses_those_names_for_placement(имя: str) -> None:
|
||||
"""Контроль: fp_w/fp_d — это действительно те размеры, которыми ставят.
|
||||
|
||||
Без него тест выше проверял бы совпадение имён, а не смысл.
|
||||
"""
|
||||
from app.services.generative import placement as mod
|
||||
|
||||
src = inspect.getsource(mod.place_program)
|
||||
assert "_centered_footprint(cell.cx, cell.cy, fp_w, fp_d)" in src, src[:200]
|
||||
assert имя in src
|
||||
Loading…
Add table
Reference in a new issue