From 37e738c802a111866ece7d9afb6c5fd6e831b89d Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 15 Aug 2026 21:20:43 +0300 Subject: [PATCH] fix(tradein/coverage): resolve city by coordinates, not sweep-context city_hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Повторная проверка /coverage закрыла оба MAJOR из #2894, но выявила три новых дефекта: 1. Город больше не резолвится из моды listings.city найденной когорты — эта колонка хранит город SWEEP-контекста скрейпера (миграция 196), не геокод адреса объявления. Замер на проде: 90/90 строк в радиусе 1000м вокруг Берёзовского имеют city='Екатеринбург', 74/74 вокруг Ревды — city='Первоуральск'. Города-спутники из COVERAGE_GREEN/YELLOW_CITIES были физически недостижимы. Город теперь резолвится детерминированно по lat/lon запроса — ближайший центроид из статичной константы (8 городов, рядом с ручкой, не в БД — comment объясняет почему) в пределах 25 км. city_hint остаётся в схеме (фронт его шлёт для соседних ручек), но чисто информационный — на порог/статус не влияет. 2. test_max_age_outlier_days_passed_to_sql проверял подстроку, которая встречается в SQL дважды (count и percentile_cont) — мутация «убрать FILTER у percentile_cont, оставив у count» проходила зелёной. Добавлен живой поведенческий тест (вставляет когорту + выброс days_on_market=4000, проверяет что медиана не сдвигается) — ловит эту мутацию (подтверждено: median 8→9 при мутации). 3. _live_session() вызывался в pytest.mark.skipif на этапе сбора тестов и создавал никогда не закрываемый Session, плюс дублировался в теле теста. Заменено на _live_db_available() (open+close голого connection) для skipif и pytest-фикстуру live_session с гарантированным close/dispose. 4. Nit: пустая когорта в поддерживаемом городе отдавала status=not_covered вместе с ненулевым threshold — противоречило докстрингу CoverageProbeResponse.threshold ("0, когда порог неприменим"). threshold теперь всегда 0 при not_covered, независимо от причины. --- tradein-mvp/backend/app/api/v1/trade_in.py | 104 +++- tradein-mvp/backend/app/schemas/trade_in.py | 25 +- tradein-mvp/backend/tests/skip_allowlist.txt | 10 +- .../tests/test_coverage_probe_endpoint.py | 446 ++++++++++++------ 4 files changed, 408 insertions(+), 177 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/trade_in.py b/tradein-mvp/backend/app/api/v1/trade_in.py index 71402948..2e4b391e 100644 --- a/tradein-mvp/backend/app/api/v1/trade_in.py +++ b/tradein-mvp/backend/app/api/v1/trade_in.py @@ -9,6 +9,7 @@ import asyncio import calendar import json import logging +import math from datetime import UTC, date, datetime, timedelta from typing import Annotated, Any, Literal from uuid import UUID @@ -2592,28 +2593,72 @@ _COVERAGE_CITY_THRESHOLDS: dict[str, tuple[str, int]] = { **{_fold_city(c): (c, COVERAGE_YELLOW_MIN_N) for c in COVERAGE_YELLOW_CITIES}, } +# Повторная проверка ручки #2894 (2026-08): город раньше резолвился модой +# `listings.city` найденной когорты — оказалось, что `listings.city` это город +# СВИП-контекста скрейпера (миграция 196 — колонка заполняется тем городом, +# который скрейпер обходил, не геокодом самого объявления). Замер на проде: +# в радиусе 1000 м вокруг Берёзовского 90/90 строк имеют city='Екатеринбург'; +# вокруг Ревды 74/74 — city='Первоуральск'. Следствие: продавец в Берёзовском +# видел на лэндинге «Екатеринбург», а сами COVERAGE_GREEN/YELLOW_CITIES для +# городов-спутников были НЕДОСТИЖИМЫ (в БД нет ни одной строки с их city). +# Фикс — детерминированный резолв по координатам ЗАПРОСА (никакого участия +# клиента, никакой моды когорты): ближайший центроид города из списка ниже, +# если он в пределах COVERAGE_CITY_MATCH_RADIUS_KM. +# +# Координаты — константа РЯДОМ С РУЧКОЙ, не таблица в БД: единственный +# существующий кандидат на "готовый реестр городов" — это +# frontend/src/lib/city-registry.ts (OBLAST_CITIES) и backend +# geocoder.py::SVERDLOVSK_OBLAST_CITIES — оба хранят ТОЛЬКО текстовые лейблы +# (city_hint для геокодера), без координат. Заводить миграцию + таблицу ради +# статичного справочника из 8 географических центров населённых пунктов — +# оверинжиниринг; координаты (WGS84, общедоступные центры НП) живут здесь же, +# рядом с порогами, которые они резолвят. +COVERAGE_CITY_MATCH_RADIUS_KM = 25.0 # дальше — город не определён (not_covered) -def _resolve_coverage_city(city_hint: str | None, cohort_city: str | None) -> tuple[str, int, bool]: - """Резолвит (display_city, threshold, is_supported) для пробы покрытия. +_CITY_CENTROIDS_DEG: dict[str, tuple[float, float]] = { + "Екатеринбург": (56.8389, 60.6057), + "Верхняя Пышма": (56.9789, 60.5636), + "Берёзовский": (56.9096, 60.8034), + "Среднеуральск": (56.9848, 60.4759), + "Нижний Тагил": (57.9099, 59.9819), + "Каменск-Уральский": (56.4110, 61.9243), + "Первоуральск": (56.9083, 59.9483), + "Ревда": (56.7986, 59.9298), +} - MINOR fix (независимый ревью #2894): city_hint — это НЕ проверенный вход, - клиент им управляет напрямую (lat/lon в Серове + city_hint='Екатеринбург' - раньше давал threshold=8 и status='ok' — клиент выбирал себе порог). Источник - истины — мода city найденной SQL-когорты (то, что реально лежит в БД рядом с - переданными lat/lon); city_hint используется ТОЛЬКО как фолбэк, когда когорта - пуста (cohort_city is None) — в этом случае n_listings тоже 0, и caller всё - равно форсирует status="not_covered" независимо от threshold/supported, так - что подмена клиентом порога здесь не даёт эффекта. - Эхо произвольной клиентской строки в поле city убрано: candidate, не нашедший - совпадения в зелёном/жёлтом списке, отдаётся как "" (не supported), а не как - сырой ввод. + +def _haversine_km(lat1: float, lon1: float, lat2: float, lon2: float) -> float: + """Расстояние по большому кругу (км), радиус Земли 6371 км.""" + r_earth_km = 6371.0 + phi1, phi2 = math.radians(lat1), math.radians(lat2) + dphi = math.radians(lat2 - lat1) + dlambda = math.radians(lon2 - lon1) + a = math.sin(dphi / 2) ** 2 + math.cos(phi1) * math.cos(phi2) * math.sin(dlambda / 2) ** 2 + return 2 * r_earth_km * math.asin(math.sqrt(a)) + + +def _resolve_coverage_city(lat: float, lon: float) -> tuple[str, int, bool]: + """Резолвит (display_city, threshold, is_supported) для пробы покрытия — ПО КООРДИНАТАМ. + + Город = ближайший центроид из `_CITY_CENTROIDS_DEG`, если расстояние до него + < `COVERAGE_CITY_MATCH_RADIUS_KM`; иначе город не определён. Детерминированно + и без участия клиента — см. комментарий над `_CITY_CENTROIDS_DEG` про то, + почему `listings.city` (мода когорты) и `city_hint` (клиентский вход) сюда + больше НЕ допускаются в качестве источника истины. """ - candidate = (cohort_city or city_hint or "").strip() - match = _COVERAGE_CITY_THRESHOLDS.get(_fold_city(candidate)) if candidate else None - if match is not None: - display, threshold = match - return display, threshold, True - return "", 0, False + nearest_city: str | None = None + nearest_km = math.inf + for city, (clat, clon) in _CITY_CENTROIDS_DEG.items(): + distance_km = _haversine_km(lat, lon, clat, clon) + if distance_km < nearest_km: + nearest_km = distance_km + nearest_city = city + + if nearest_city is None or nearest_km > COVERAGE_CITY_MATCH_RADIUS_KM: + return "", 0, False + + display, threshold = _COVERAGE_CITY_THRESHOLDS[_fold_city(nearest_city)] + return display, threshold, True @router.post("/coverage", response_model=CoverageProbeResponse) @@ -2654,6 +2699,14 @@ def coverage_probe( сведение двух разных когорт усложнило бы «один дешёвый SQL» без выигрыша в честности (у нас и так честное имя поля — age активного объявления, не срок продажи). См. openQuestions PR #2894 при ревью. + + Повторная проверка ручки (2026-08): город больше НЕ берётся из моды + `listings.city` найденной когорты и НЕ зависит от `payload.city_hint` — + оба источника ненадёжны (см. комментарий над `_CITY_CENTROIDS_DEG`). + Город резолвится детерминированно по `payload.lat/lon` через + `_resolve_coverage_city` — `city_hint` в payload остаётся только + информационным полем (см. `CoverageProbeInput.city_hint`), на результат + не влияет. """ from app.services.estimator import _RN_DUP_WINDOW, MAX_ANALOGS_PER_ADDRESS @@ -2666,7 +2719,6 @@ def coverage_probe( f""" WITH base AS ( SELECT - city, days_on_market, row_number() OVER ( PARTITION BY address ORDER BY scraped_at DESC @@ -2700,9 +2752,7 @@ def coverage_probe( FILTER ( WHERE days_on_market IS NOT NULL AND days_on_market <= :max_age_days - ) AS median_age_days, - mode() WITHIN GROUP (ORDER BY city) - FILTER (WHERE city IS NOT NULL) AS cohort_city + ) AS median_age_days FROM base WHERE rn_addr <= :max_per_addr AND rn_dup = 1 @@ -2733,12 +2783,16 @@ def coverage_probe( and n_with_age >= COVERAGE_MIN_AGE_SAMPLES else None ) - cohort_city = row["cohort_city"] if row else None - city, threshold, supported = _resolve_coverage_city(payload.city_hint, cohort_city) + city, threshold, supported = _resolve_coverage_city(payload.lat, payload.lon) if not supported or n_listings == 0: status: Literal["ok", "thin", "not_covered"] = "not_covered" + # Nit-fix (повторная проверка #2894): threshold неприменим при + # not_covered — см. CoverageProbeResponse.threshold docstring. Раньше + # поддерживаемый (по координатам) город с пустой когортой отдавал + # реальный порог (8/12) вместе с not_covered — противоречило докстрингу. + threshold = 0 elif n_listings >= threshold: status = "ok" else: diff --git a/tradein-mvp/backend/app/schemas/trade_in.py b/tradein-mvp/backend/app/schemas/trade_in.py index 4686b3b5..8f245bb0 100644 --- a/tradein-mvp/backend/app/schemas/trade_in.py +++ b/tradein-mvp/backend/app/schemas/trade_in.py @@ -763,9 +763,18 @@ class CoverageProbeInput(BaseModel): lat/lon — координаты, уже разрезолвленные фронтом (тот же контракт, что TradeInEstimateInput.lat/lon — geocode делает фронт/автокомплит, эта ручка - сама НИКОГО не геокодирует). city_hint — опционально, из того же - автокомплита (см. TradeInEstimateInput.city_hint); без него город - резолвится best-effort из моды city найденной когорты. + сама НИКОГО не геокодирует). Город (и, соответственно, порог ok/thin) для + ответа резолвится ИСКЛЮЧИТЕЛЬНО из lat/lon — см. + `app.api.v1.trade_in._resolve_coverage_city`. + + city_hint — ИНФОРМАЦИОННОЕ поле, на результат НЕ влияет (повторная проверка + #2894, 2026-08). Раньше оно участвовало в резолве города как фолбэк — + убрано вместе с модой `listings.city`: оба источника ненадёжны (`city_hint` + — непроверенный клиентский вход, `listings.city` — город свип-контекста + скрейпера, не адреса объявления, см. комментарий в trade_in.py). Поле + оставлено в схеме, потому что фронт его уже шлёт в других ручках того же + автокомплита (см. TradeInEstimateInput.city_hint) — принимаем и молча + игнорируем, чтобы не ронять запрос лишней 422. """ lat: float = Field(ge=-90, le=90) @@ -804,8 +813,14 @@ class CoverageProbeResponse(BaseModel): маленьком n_with_age — цифра "медиана" по 1-2 объявлениям не медиана. threshold — n, начиная с которого статус переходит в "ok" для резолвленного - города; 0, если город не входит ни в один список (порог неприменим — - статус в этом случае всегда "not_covered" вне зависимости от n_listings). + города; 0 всегда, когда status == "not_covered" (порог неприменим — ни для + города вне зелёного/жёлтого списка, ни для поддерживаемого города с пустой + когортой), НЕ только для неподдерживаемого города. + + city — резолвится ИСКЛЮЧИТЕЛЬНО из lat/lon запроса (ближайший центроид из + зелёного/жёлтого списка в пределах `COVERAGE_CITY_MATCH_RADIUS_KM`), не из + `city_hint` и не из моды `listings.city` найденной когорты — см. + `app.api.v1.trade_in._resolve_coverage_city`. """ status: Literal["ok", "thin", "not_covered"] diff --git a/tradein-mvp/backend/tests/skip_allowlist.txt b/tradein-mvp/backend/tests/skip_allowlist.txt index fb780107..d97627f7 100644 --- a/tradein-mvp/backend/tests/skip_allowlist.txt +++ b/tradein-mvp/backend/tests/skip_allowlist.txt @@ -70,8 +70,16 @@ tests/test_house_imv_retry_stuck.py::test_explicit_only_status_still_takes_exhau tests/test_house_imv_retry_stuck.py::test_stuck_transient_house_returns_to_the_queue_by_itself tests/test_house_imv_retry_stuck.py::test_transient_attempts_counter_only_counts_transient -# MAJOR-1 fix, coverage probe (#2894, независимый ревью) — тот же `_live_session()`. +# MAJOR-1 fix, coverage probe (#2894, независимый ревью) — тот же `live_session` fixture +# (self-skip через `_live_db_available()`, живёт только при реальном Postgres DSN). # Проверяет, что novostroyki-строка / geo_precision='city'-строка / price_rub=0-строка # физически не попадают в когорту (не только SQL-текст, который проверяется отдельным # статическим тестом test_cohort_sql_excludes_* в этом же файле, идущим на обоих лэйнах). tests/test_coverage_probe_endpoint.py::test_major1_cohort_excludes_novostroyki_and_city_precision_live + +# MAJOR-2 поведенческий пин (повторная проверка #2894) — та же `live_session` fixture. +# Ловит мутацию «убрать FILTER у percentile_cont, оставив у count(*)», которую +# текстовый тест test_max_age_outlier_days_passed_to_sql пропускал (подстрока +# `days_on_market <= :max_age_days` встречается в SQL дважды). На мок-лэйне +# (deploy-tradein.yml, DSN-заглушка) реальной БД нет — self-skip. +tests/test_coverage_probe_endpoint.py::test_max_age_outlier_excluded_from_median_live diff --git a/tradein-mvp/backend/tests/test_coverage_probe_endpoint.py b/tradein-mvp/backend/tests/test_coverage_probe_endpoint.py index 1b6abdc3..65d85921 100644 --- a/tradein-mvp/backend/tests/test_coverage_probe_endpoint.py +++ b/tradein-mvp/backend/tests/test_coverage_probe_endpoint.py @@ -4,15 +4,19 @@ видит, сколько похожих квартир продаётся рядом и как быстро они уходят, без единой рублёвой цифры в ответе. Covers: - пороги ok/thin/not_covered для зелёных/жёлтых/неподдерживаемых городов - - пустая когорта (n=0) → not_covered даже в поддерживаемом городе + - пустая когорта (n=0) → not_covered даже в поддерживаемом городе; threshold + принудительно 0 в этом случае (nit-fix, повторная проверка #2894) - в ответе НЕТ ни одного price-подобного поля (падающий тест на регресс схемы) - MAJOR-1 (независимый ревью #2894): когорта пробы — sync с estimator._COMMON_WHERE / Tier W (novostroyki guard, geo_precision != 'city', price_rub > 0), не шире когорты платного эстиматора - MAJOR-2: median_listing_age_days честно null при тонкой n_with_age выборке, - выбросы (> COVERAGE_MAX_AGE_DAYS) не тянут медиану - - MINOR: когортный город (мода) побеждает city_hint при расхождении — клиент - не управляет порогом; неизвестный город не эхуется сырой строкой + выбросы (> COVERAGE_MAX_AGE_DAYS) не тянут медиану — запинено ЖИВЫМ SQL + (см. test_max_age_outlier_excluded_from_median_live), не только подстрокой + - Повторная проверка #2894 (2026-08): город резолвится ИСКЛЮЧИТЕЛЬНО по + lat/lon (ближайший центроид), НЕ по моде `listings.city` (город + свип-контекста скрейпера, не адреса объявления) и НЕ по `city_hint` + (непроверенный клиентский вход) — см. app.api.v1.trade_in._resolve_coverage_city """ from __future__ import annotations @@ -57,18 +61,19 @@ def trade_in_app() -> FastAPI: def _row( n_listings: int, median_age_days: float | None, - cohort_city: str | None, n_with_age: int | None = None, ) -> dict: """Строка, которую coverage_probe читает через db.execute(...).mappings().fetchone(). n_with_age по умолчанию = n_listings, если не задан явно (большинство старых тестов не проверяют MAJOR-2 отдельно — сохраняем их поведение). + + Повторная проверка #2894: строка больше не несёт cohort_city — город + резолвится по lat/lon запроса, не по SQL-агрегату (см. модуль-докстринг). """ return { "n_listings": n_listings, "median_age_days": median_age_days, - "cohort_city": cohort_city, "n_with_age": n_with_age if n_with_age is not None else n_listings, } @@ -90,8 +95,22 @@ def _override(app: FastAPI, db: MagicMock) -> None: app.dependency_overrides[get_db] = lambda: (yield db) +# Екатеринбург — совпадает (с точностью до сотен метров) с центроидом +# _CITY_CENTROIDS_DEG["Екатеринбург"], поэтому дефолтный payload детерминированно +# резолвится в зелёный город без доп. настройки координат в каждом тесте. _BASE_PAYLOAD = {"lat": 56.8384, "lon": 60.6057, "rooms": 2, "area_m2": 50.0} +# Координаты других городов из COVERAGE_GREEN/YELLOW_CITIES (те же значения, что +# _CITY_CENTROIDS_DEG в trade_in.py) — используются, когда тесту нужен НЕ ЕКБ. +_NIZHNY_TAGIL = {"lat": 57.9099, "lon": 59.9819} +_REVDA = {"lat": 56.7986, "lon": 59.9298} +_BEREZOVSKY = {"lat": 56.9096, "lon": 60.8034} + +# Реальные координаты Серова — ближайший поддерживаемый центроид (Нижний Тагил) +# в ~190 км, далеко за пределами COVERAGE_CITY_MATCH_RADIUS_KM=25 — гарантированно +# "город не определён", без совпадения ни с одним из 8 центроидов. +_FAR_AWAY_CITY = {"lat": 59.6047, "lon": 60.1970} + # ── Response schema: NO price anywhere (issue #2894 hard rule) ──────────────── @@ -109,7 +128,7 @@ def test_coverage_response_has_no_price_fields(trade_in_app: FastAPI) -> None: def test_coverage_actual_response_has_no_price_fields(trade_in_app: FastAPI) -> None: """Same guard but on a live serialized response (belt-and-suspenders).""" - db = _db_mock_returning(_row(10, 21.0, "Екатеринбург")) + db = _db_mock_returning(_row(10, 21.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -125,7 +144,7 @@ def test_coverage_actual_response_has_no_price_fields(trade_in_app: FastAPI) -> def test_green_city_ok_at_threshold(trade_in_app: FastAPI) -> None: """Екатеринбург (зелёный, порог 8) — n=8 ровно на границе → ok.""" - db = _db_mock_returning(_row(8, 15.0, "Екатеринбург")) + db = _db_mock_returning(_row(8, 15.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -143,7 +162,7 @@ def test_green_city_ok_at_threshold(trade_in_app: FastAPI) -> None: def test_green_city_thin_below_threshold(trade_in_app: FastAPI) -> None: """Екатеринбург, n=7 (< порог 8) → thin, не ok и не not_covered.""" - db = _db_mock_returning(_row(7, 10.0, "Екатеринбург")) + db = _db_mock_returning(_row(7, 10.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -158,15 +177,12 @@ def test_green_city_thin_below_threshold(trade_in_app: FastAPI) -> None: def test_yellow_city_ok_at_threshold(trade_in_app: FastAPI) -> None: - """Нижний Тагил (жёлтый, порог 12) — n=12 → ok. cohort_city совпадает с hint.""" - db = _db_mock_returning(_row(12, 30.0, "Нижний Тагил")) + """Нижний Тагил (жёлтый, порог 12) — n=12 → ok. Город резолвится из lat/lon.""" + db = _db_mock_returning(_row(12, 30.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) - resp = client.post( - "/api/v1/trade-in/coverage", - json={**_BASE_PAYLOAD, "city_hint": "Нижний Тагил"}, - ) + resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_NIZHNY_TAGIL}) data = resp.json() assert data["status"] == "ok" assert data["threshold"] == 12 @@ -174,51 +190,57 @@ def test_yellow_city_ok_at_threshold(trade_in_app: FastAPI) -> None: def test_yellow_city_thin_below_threshold(trade_in_app: FastAPI) -> None: - """Ревда, n=11 (< порог 12) → thin. Когорта пуста по городу → используем hint.""" - db = _db_mock_returning(_row(11, 40.0, None)) + """Ревда, n=11 (< порог 12) → thin.""" + db = _db_mock_returning(_row(11, 40.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) - resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, "city_hint": "Ревда"}) + resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_REVDA}) data = resp.json() assert data["status"] == "thin" assert data["threshold"] == 12 -# ── City outside both lists → always not_covered ──────────────────────────────── +# ── City outside all centroids → always not_covered ───────────────────────────── def test_unsupported_city_not_covered_even_with_high_n(trade_in_app: FastAPI) -> None: - """Город вне списков → not_covered независимо от n_listings (даже n=500).""" - db = _db_mock_returning(_row(500, 5.0, "Серов")) + """Точка вне 25-км радиуса всех центроидов → not_covered независимо от n_listings + (даже n=500).""" + db = _db_mock_returning(_row(500, 5.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) - resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, "city_hint": "Серов"}) + resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_FAR_AWAY_CITY}) data = resp.json() assert data["status"] == "not_covered" assert data["threshold"] == 0 assert data["n_listings"] == 500 # честно отдаём счётчик, статус его игнорирует - assert data["city"] == "" # MINOR: неизвестный город не эхуется сырой строкой + assert data["city"] == "" # город не определён — не эхуется сырой строкой # ── Empty cohort ────────────────────────────────────────────────────────────── def test_empty_cohort_supported_city_not_covered(trade_in_app: FastAPI) -> None: - """n=0 в поддерживаемом (зелёном) городе → not_covered, не thin — честнее.""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + """n=0 в поддерживаемом (зелёном) городе → not_covered, не thin — честнее. + + Nit-fix (повторная проверка #2894): threshold обязан быть 0, а не реальным + порогом города (8) — при not_covered threshold "неприменим" по докстрингу + CoverageProbeResponse, независимо от ПРИЧИНЫ not_covered. + """ + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) - resp = client.post( - "/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, "city_hint": "Екатеринбург"} - ) + resp = client.post("/api/v1/trade-in/coverage", json=_BASE_PAYLOAD) data = resp.json() assert data["status"] == "not_covered" assert data["n_listings"] == 0 assert data["median_listing_age_days"] is None assert data["n_with_age"] == 0 + assert data["city"] == "Екатеринбург" # город резолвится по координатам всегда + assert data["threshold"] == 0 # nit: не 8, хотя город поддерживаемый def test_empty_cohort_no_row_at_all(trade_in_app: FastAPI) -> None: @@ -235,63 +257,70 @@ def test_empty_cohort_no_row_at_all(trade_in_app: FastAPI) -> None: assert data["n_listings"] == 0 assert data["median_listing_age_days"] is None assert data["n_with_age"] == 0 - - -# ── MINOR: cohort mode (реальные данные из БД) побеждает city_hint ────────────── - - -def test_cohort_mode_overrides_city_hint_on_mismatch(trade_in_app: FastAPI) -> None: - """lat/lon в Серове + city_hint='Екатеринбург' — клиент не управляет порогом. - - Когорта реально нашлась в Серове (cohort_city="Серов", город вне списков) — - ответ обязан игнорировать спуфленный hint и не выдавать зелёный threshold=8. - Регресс на прод-инцидент из независимого ревью #2894. - """ - db = _db_mock_returning(_row(9, 12.0, "Серов")) - _override(trade_in_app, db) - - client = TestClient(trade_in_app) - resp = client.post( - "/api/v1/trade-in/coverage", - json={**_BASE_PAYLOAD, "city_hint": "Екатеринбург"}, - ) - data = resp.json() - assert data["city"] != "Екатеринбург" - assert data["status"] == "not_covered" # Серов вне зелёного/жёлтого списка assert data["threshold"] == 0 -def test_city_hint_used_only_as_fallback_for_empty_cohort(trade_in_app: FastAPI) -> None: - """Когорта пуста (cohort_city=None) — hint используется как фолбэк для display, - но status всё равно not_covered (n_listings=0), так что подмена без эффекта.""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) +# ── Город резолвится ТОЛЬКО по координатам — не по listings.city, не по city_hint ── + + +def test_city_resolved_from_coordinates_not_cohort_mode(trade_in_app: FastAPI) -> None: + """Точка в Берёзовском → city='Берёзовский' (а не 'Екатеринбург'). + + Регресс на прод-замер (повторная проверка #2894): в радиусе 1000м вокруг + Берёзовского 90/90 строк listings имеют city='Екатеринбург' (город + свип-контекста скрейпера, миграция 196) — старая логика (мода когорты) + отдала бы 'Екатеринбург'. Ручка больше НЕ читает cohort city из SQL вовсе. + """ + db = _db_mock_returning(_row(8, 5.0)) _override(trade_in_app, db) client = TestClient(trade_in_app) - resp = client.post( - "/api/v1/trade-in/coverage", - json={**_BASE_PAYLOAD, "city_hint": "Екатеринбург"}, - ) - data = resp.json() - assert data["city"] == "Екатеринбург" - assert data["status"] == "not_covered" - - -def test_yo_fold_city_hint_matches(trade_in_app: FastAPI) -> None: - """«Березовский» без ё должен резолвиться в тот же зелёный порог, что «Берёзовский».""" - db = _db_mock_returning(_row(8, 5.0, None, n_with_age=8)) - _override(trade_in_app, db) - - client = TestClient(trade_in_app) - resp = client.post( - "/api/v1/trade-in/coverage", - json={**_BASE_PAYLOAD, "city_hint": "березовский"}, - ) + resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_BEREZOVSKY}) data = resp.json() + assert data["city"] == "Берёзовский" assert data["status"] == "ok" assert data["threshold"] == 8 +def test_far_from_all_centroids_not_covered(trade_in_app: FastAPI) -> None: + """Точка за пределами 25 км от всех 8 центроидов → not_covered, city="".""" + db = _db_mock_returning(_row(0, None, n_with_age=0)) + _override(trade_in_app, db) + + client = TestClient(trade_in_app) + resp = client.post("/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_FAR_AWAY_CITY}) + data = resp.json() + assert data["status"] == "not_covered" + assert data["city"] == "" + assert data["threshold"] == 0 + + +def test_city_hint_does_not_change_threshold_or_status(trade_in_app: FastAPI) -> None: + """city_hint — чисто информационное поле (повторная проверка #2894): точка в + Берёзовском + city_hint='Екатеринбург' обязана резолвиться в Берёзовский + (threshold=8, зелёный порог — оба города зелёные, поэтому дополнительно + проверяем n=8 → ok именно для Берёзовского, а не подмену клиентом города). + """ + db_with_hint = _db_mock_returning(_row(8, 5.0)) + _override(trade_in_app, db_with_hint) + client = TestClient(trade_in_app) + resp_with_hint = client.post( + "/api/v1/trade-in/coverage", + json={**_BASE_PAYLOAD, **_BEREZOVSKY, "city_hint": "Екатеринбург"}, + ) + + db_without_hint = _db_mock_returning(_row(8, 5.0)) + _override(trade_in_app, db_without_hint) + resp_without_hint = client.post( + "/api/v1/trade-in/coverage", json={**_BASE_PAYLOAD, **_BEREZOVSKY} + ) + + data_with, data_without = resp_with_hint.json(), resp_without_hint.json() + assert data_with["city"] == data_without["city"] == "Берёзовский" + assert data_with["threshold"] == data_without["threshold"] == 8 + assert data_with["status"] == data_without["status"] == "ok" + + # ── MAJOR-2: median age — n_with_age threshold + outlier clamp ───────────────── @@ -301,7 +330,7 @@ def test_median_age_null_below_min_age_samples(trade_in_app: FastAPI) -> None: from app.api.v1.trade_in import COVERAGE_MIN_AGE_SAMPLES assert COVERAGE_MIN_AGE_SAMPLES == 5 - db = _db_mock_returning(_row(20, 40.0, "Екатеринбург", n_with_age=2)) + db = _db_mock_returning(_row(20, 40.0, n_with_age=2)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -314,7 +343,7 @@ def test_median_age_null_below_min_age_samples(trade_in_app: FastAPI) -> None: def test_median_age_present_at_min_age_samples_threshold(trade_in_app: FastAPI) -> None: """n_with_age=5 (== порог) → median_listing_age_days отдаётся.""" - db = _db_mock_returning(_row(20, 40.0, "Екатеринбург", n_with_age=5)) + db = _db_mock_returning(_row(20, 40.0, n_with_age=5)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -326,11 +355,17 @@ def test_median_age_present_at_min_age_samples_threshold(trade_in_app: FastAPI) def test_max_age_outlier_days_passed_to_sql(trade_in_app: FastAPI) -> None: """COVERAGE_MAX_AGE_DAYS=365 передаётся в SQL как параметр — выбросы (мёртвые - объявления) отсекаются percentile_cont FILTER на стороне БД, не в Python.""" + объявления) отсекаются percentile_cont FILTER на стороне БД, не в Python. + + Слабая (текстовая) проверка — подстрока встречается в SQL ДВАЖДЫ (count и + percentile_cont), поэтому `assert "..." in sql_text` одна ловит только + "убрали оба FILTER", не "убрали один из двух". Реальный поведенческий пин — + test_max_age_outlier_excluded_from_median_live ниже (живой Postgres). + """ from app.api.v1.trade_in import COVERAGE_MAX_AGE_DAYS assert COVERAGE_MAX_AGE_DAYS == 365 - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -341,7 +376,10 @@ def test_max_age_outlier_days_passed_to_sql(trade_in_app: FastAPI) -> None: assert params["max_age_days"] == 365 sql_text = str(call_args[0][0]) - assert "days_on_market <= :max_age_days" in sql_text + # count==2: и в count(*) FILTER, и в percentile_cont(...) FILTER — обе нужны, + # чтобы n_with_age и median_listing_age_days считались по ОДНОМУ и тому же + # предикату (иначе честный n_with_age маскирует нечестный медианный расчёт). + assert sql_text.count("days_on_market <= :max_age_days") == 2 # ── DB dedup / cap params passed through ──────────────────────────────────────── @@ -349,7 +387,7 @@ def test_max_age_outlier_days_passed_to_sql(trade_in_app: FastAPI) -> None: def test_coverage_sql_uses_radius_1000_and_area_tolerance(trade_in_app: FastAPI) -> None: """SQL params: radius=1000 (строго), area ±15%, rooms exact.""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -378,7 +416,7 @@ def test_coverage_sql_uses_radius_1000_and_area_tolerance(trade_in_app: FastAPI) def test_cohort_sql_excludes_novostroyki(trade_in_app: FastAPI) -> None: """Guard новостроек — sync с estimator._COMMON_WHERE (5460) / Tier W (5932).""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -390,7 +428,7 @@ def test_cohort_sql_excludes_novostroyki(trade_in_app: FastAPI) -> None: def test_cohort_sql_excludes_city_precision_geocodes(trade_in_app: FastAPI) -> None: """geo_precision != 'city' — sync с estimator Tier W (5910/5945-5948, #769 Part E).""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -402,7 +440,7 @@ def test_cohort_sql_excludes_city_precision_geocodes(trade_in_app: FastAPI) -> N def test_cohort_sql_excludes_zero_price(trade_in_app: FastAPI) -> None: """price_rub > 0 — sync с estimator._COMMON_WHERE (5441) / Tier W (5916).""" - db = _db_mock_returning(_row(0, None, None, n_with_age=0)) + db = _db_mock_returning(_row(0, None, n_with_age=0)) _override(trade_in_app, db) client = TestClient(trade_in_app) @@ -412,31 +450,66 @@ def test_cohort_sql_excludes_zero_price(trade_in_app: FastAPI) -> None: assert "price_rub > 0" in sql_text -# ── MAJOR-1 (real-DB variant): предикаты реально фильтруют, не только в тексте ── +# ── Live-DB tests (self-skip без реальной Postgres+PostGIS) ──────────────────── # -# Опциональный тест против настоящего Postgres (тот же паттерн self-skip, что -# test_gar_flats_loader.py::_live_session) — вставляет novostroyki-строку и -# строку с geo_precision='city' в когорту и проверяет, что они физически НЕ -# посчитаны. Требует TEST_DATABASE_URL/DATABASE_URL, указывающий на реальную -# Postgres+PostGIS БД (не дефолтный localhost:5432/test-заглушку) — иначе skip. +# Опциональные тесты против настоящего Postgres (тот же паттерн self-skip, что +# test_gar_flats_loader.py::_live_session) — требуют TEST_DATABASE_URL/ +# DATABASE_URL, указывающий на реальную БД (не дефолтный localhost:5432/test- +# заглушку); иначе skip. В CI (ci-tradein.yml) этот DSN всегда живой Postgres+ +# PostGIS-контейнер. +# +# Fix (повторная проверка #2894): раньше `_live_session()` вызывался И в +# `pytest.mark.skipif(...)` (на этапе СБОРА тестов — соединение открывалось и +# никогда не закрывалось, при реальном DSN это утечка на КАЖДЫЙ импорт файла), +# И повторно внутри тела единственного live-теста. Теперь доступность БД +# проверяется отдельной дешёвой функцией с явным закрытием соединения +# (`_live_db_available`), а сама Session выдаётся pytest-фикстурой +# (`live_session`) с гарантированным close() в finally, а не ручным вызовом. -def _live_session(): # type: ignore[no-untyped-def] +def _live_db_available() -> bool: + """Дешёвая проверка доступности live-Postgres — соединение открывается и + СРАЗУ закрывается (`with engine.connect()`), никакого висящего ORM Session. + + Используется только в `pytest.mark.skipif(...)`, который вычисляется на + этапе сбора тестов — до фикстур. + """ try: from sqlalchemy import create_engine from sqlalchemy import text as sa_text - from sqlalchemy.orm import sessionmaker dsn = os.environ.get("TEST_DATABASE_URL") or os.environ.get("DATABASE_URL", "") if not dsn or "localhost:5432/test" in dsn: - return None + return False engine = create_engine(dsn, future=True) - conn = engine.connect() - conn.execute(sa_text("SELECT 1")) - conn.close() - return sessionmaker(bind=engine, future=True)() + try: + with engine.connect() as conn: + conn.execute(sa_text("SELECT 1")) + return True + finally: + engine.dispose() except Exception: - return None + return False + + +@pytest.fixture() +def live_session(): # type: ignore[no-untyped-def] + """Session для live-Postgres тестов — гарантированно закрывается после теста + (rollback + close + dispose в finally), в отличие от прежнего ручного вызова + `_live_session()` внутри тела каждого теста.""" + from sqlalchemy import create_engine + from sqlalchemy.orm import sessionmaker + + dsn = os.environ.get("TEST_DATABASE_URL") or os.environ.get("DATABASE_URL", "") + engine = create_engine(dsn, future=True) + session_factory = sessionmaker(bind=engine, future=True) + session = session_factory() + try: + yield session + finally: + session.rollback() + session.close() + engine.dispose() # Координаты вне Свердловской обл. (реальные данные там ~56-60/58-64) — изолируют @@ -445,58 +518,139 @@ def _live_session(): # type: ignore[no-untyped-def] _LIVE_LAT, _LIVE_LON = 1.111, 2.222 -@pytest.mark.skipif(_live_session() is None, reason="нет доступной Postgres test-БД") -def test_major1_cohort_excludes_novostroyki_and_city_precision_live() -> None: +@pytest.mark.skipif(not _live_db_available(), reason="нет доступной Postgres test-БД") +def test_major1_cohort_excludes_novostroyki_and_city_precision_live(live_session) -> None: # type: ignore[no-untyped-def] from sqlalchemy import text as sa_text from app.api.v1.trade_in import coverage_probe from app.schemas.trade_in import CoverageProbeInput - db = _live_session() - assert db is not None - try: - rows = [ - # (source_url suffix, listing_segment, geo_precision, price_rub) — все - # остальные поля общие: rooms=2, area_m2=50, is_active, scraped_at=NOW(). - ("ok-vtorichka", None, None, 5_000_000), # counted - ("bad-novostroyka", "novostroyki", None, 5_000_000), # excluded - ("bad-city-precision", None, "city", 5_000_000), # excluded - ("bad-zero-price", None, None, 0), # excluded - ] - for suffix, segment, geo_precision, price in rows: - url = f"https://test.invalid/coverage-major1-{suffix}" - db.execute( - sa_text( - """ - INSERT INTO listings - (source, source_url, source_id, dedup_hash, address, lat, lon, - rooms, area_m2, price_rub, is_active, scraped_at, - listing_segment, geo_precision) - VALUES - ('test', :url, :url, :url, 'test addr', :lat, :lon, - 2, 50.0, :price, true, NOW(), :segment, :geo_precision) - """ - ), - { - "url": url, - "lat": _LIVE_LAT, - "lon": _LIVE_LON, - "price": price, - "segment": segment, - "geo_precision": geo_precision, - }, - ) + db = live_session + rows = [ + # (source_url suffix, listing_segment, geo_precision, price_rub) — все + # остальные поля общие: rooms=2, area_m2=50, is_active, scraped_at=NOW(). + ("ok-vtorichka", None, None, 5_000_000), # counted + ("bad-novostroyka", "novostroyki", None, 5_000_000), # excluded + ("bad-city-precision", None, "city", 5_000_000), # excluded + ("bad-zero-price", None, None, 0), # excluded + ] + for suffix, segment, geo_precision, price in rows: + url = f"https://test.invalid/coverage-major1-{suffix}" + db.execute( + sa_text( + """ + INSERT INTO listings + (source, source_url, source_id, dedup_hash, address, lat, lon, + rooms, area_m2, price_rub, is_active, scraped_at, + listing_segment, geo_precision) + VALUES + ('test', :url, :url, :url, 'test addr', :lat, :lon, + 2, 50.0, :price, true, NOW(), :segment, :geo_precision) + """ + ), + { + "url": url, + "lat": _LIVE_LAT, + "lon": _LIVE_LON, + "price": price, + "segment": segment, + "geo_precision": geo_precision, + }, + ) - result = coverage_probe( - CoverageProbeInput(lat=_LIVE_LAT, lon=_LIVE_LON, rooms=2, area_m2=50.0), db - ) - # Только первая (ok-vtorichka) строка должна попадать в когорту — - # каждая следующая вставка не должна сдвигать счётчик. - assert result.n_listings == 1, ( - f"predicate regression: n_listings={result.n_listings} after inserting " - f"{suffix!r} (segment={segment!r} geo_precision={geo_precision!r} " - f"price={price}) — expected still 1 (only ok-vtorichka counted)" - ) - finally: - db.rollback() - db.close() + result = coverage_probe( + CoverageProbeInput(lat=_LIVE_LAT, lon=_LIVE_LON, rooms=2, area_m2=50.0), db + ) + # Только первая (ok-vtorichka) строка должна попадать в когорту — + # каждая следующая вставка не должна сдвигать счётчик. + assert result.n_listings == 1, ( + f"predicate regression: n_listings={result.n_listings} after inserting " + f"{suffix!r} (segment={segment!r} geo_precision={geo_precision!r} " + f"price={price}) — expected still 1 (only ok-vtorichka counted)" + ) + + +@pytest.mark.skipif(not _live_db_available(), reason="нет доступной Postgres test-БД") +def test_max_age_outlier_excluded_from_median_live(live_session) -> None: # type: ignore[no-untyped-def] + """MAJOR-2 поведенческий пин (повторная проверка #2894). + + Текстовый тест (test_max_age_outlier_days_passed_to_sql) проверял, что + подстрока `days_on_market <= :max_age_days` встречается в SQL — но она там + ДВАЖДЫ (count и percentile_cont), и мутация «убрать FILTER у + percentile_cont, оставив у count» проходила зелёной: n_with_age (из count) + оставался честным, а percentile_cont без FILTER считал медиану по ВСЕМ + days_on_market, включая выбросы. + + Вставляет когорту из 5 "нормальных" объявлений (days_on_market + 4/6/8/10/12, честная медиана — 8) и один выброс (days_on_market=4000, + > COVERAGE_MAX_AGE_DAYS=365). Проверяет, что после вставки выброса + n_with_age и median_listing_age_days НЕ меняются (выброс попадает только + в n_listings) — с правильными двумя FILTER это так; без FILTER у + percentile_cont медиана сдвинулась бы 8 → 9 (percentile_cont(0.5) по + [4,6,8,10,12,4000] = среднее 3-го и 4-го отсортированных значений = 9). + """ + from sqlalchemy import text as sa_text + + from app.api.v1.trade_in import coverage_probe + from app.schemas.trade_in import CoverageProbeInput + + db = live_session + normal_ages = [4, 6, 8, 10, 12] + for i, age in enumerate(normal_ages): + url = f"https://test.invalid/coverage-major2-normal-{i}" + db.execute( + sa_text( + """ + INSERT INTO listings + (source, source_url, source_id, dedup_hash, address, lat, lon, + rooms, area_m2, price_rub, is_active, scraped_at, days_on_market) + VALUES + ('test', :url, :url, :url, :addr, :lat, :lon, + 2, 50.0, 5000000, true, NOW(), :age) + """ + ), + { + "url": url, + "addr": f"test addr coverage-major2-{i}", + "lat": _LIVE_LAT, + "lon": _LIVE_LON, + "age": age, + }, + ) + + result = coverage_probe( + CoverageProbeInput(lat=_LIVE_LAT, lon=_LIVE_LON, rooms=2, area_m2=50.0), db + ) + assert result.n_listings == 5 + assert result.n_with_age == 5 + assert result.median_listing_age_days == 8 + + outlier_url = "https://test.invalid/coverage-major2-outlier" + db.execute( + sa_text( + """ + INSERT INTO listings + (source, source_url, source_id, dedup_hash, address, lat, lon, + rooms, area_m2, price_rub, is_active, scraped_at, days_on_market) + VALUES + ('test', :url, :url, :url, 'test addr coverage-major2-outlier', :lat, :lon, + 2, 50.0, 5000000, true, NOW(), 4000) + """ + ), + {"url": outlier_url, "lat": _LIVE_LAT, "lon": _LIVE_LON}, + ) + + result_with_outlier = coverage_probe( + CoverageProbeInput(lat=_LIVE_LAT, lon=_LIVE_LON, rooms=2, area_m2=50.0), db + ) + assert result_with_outlier.n_listings == 6 # выброс всё же попадает в n_listings + assert result_with_outlier.n_with_age == 5, ( + f"MAJOR-2 regression: outlier (days_on_market=4000 > MAX=365) leaked into " + f"n_with_age={result_with_outlier.n_with_age} — count(*) FILTER пропал/сломан" + ) + assert result_with_outlier.median_listing_age_days == 8, ( + f"MAJOR-2 regression: median_listing_age_days=" + f"{result_with_outlier.median_listing_age_days} shifted by outlier — " + f"percentile_cont(...) FILTER пропал (мутация «убрать FILTER у " + f"percentile_cont, оставив у count»)" + )