From fa84705ec71646b15105eb71da90545692f5140c Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 15 Aug 2026 18:50:17 +0300 Subject: [PATCH] fix(tradein/geocoder): stop apt number leaking into house + houses bbox/sibling guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 on #2626 (local houses fallback) found two HIGH-severity bugs verified live against prod data: 1. _extract_local_house_token took the LAST digit-like token in the raw address, so "...Педагогическая, д 15, кв 11" resolved house=11 (apartment number) instead of 15 -- confidently returning a stranger's building with confidence='exact', written to geocode_cache. Fixed by stripping the apartment/office/floor/entrance tail (кв/оф/пом/подъезд/этаж -- NOT корп/к, which is part of the house number) before extracting the token. Fixes the exact prod case from the review plus the corpus+apartment combo ("д 26 к 1, кв 41" -> 26к1, not 41). 2. houses is not an EKB-only table (21% of rows with coords are outside the metro, some as far as another city) -- "улица Маяковского, 7" in houses resolves to Серов, not Екатеринбург, and use_local_ekb only gates the user's query text, not the source row. Added an is_within_ekb_bbox_wide check on every candidate row before it can become a match. Also addressed two MEDIUM findings from the same review: 3. The "<номер> -> <номер>к1" corpus guess only checked uniqueness among к1-labelled rows, so real multi-building addresses (Онуфриева 24: к1/к2/к3, 250-400m apart) resolved confidently to к1 anyway. Guess is now skipped when any other corpus/slash variant of the same base number exists among the street's candidates. 4. Houses-fallback results are no longer cached in geocode_cache -- the source (scraped listings) is less reliable than geoportal/cadastral/ Nominatim, and the lookup is cheap/local, so caching only extended the lifetime of a possible bad match. Side benefit: address_refined now survives every repeat request of the same raw address, not just the first. Also added ORDER BY address, id to the underlying query so the coordinate dedup picks a deterministic row (LOW finding #5). 14 new/updated tests in test_geocoder_local_houses_fallback.py cover all five findings against real prod address/houses-row fixtures. Full geocoder + dadata + estimator/pdf regression suite (402 tests) green. --- tradein-mvp/backend/app/services/geocoder.py | 132 ++++++++++++--- .../test_geocoder_local_houses_fallback.py | 154 +++++++++++++++++- 2 files changed, 256 insertions(+), 30 deletions(-) diff --git a/tradein-mvp/backend/app/services/geocoder.py b/tradein-mvp/backend/app/services/geocoder.py index 8d798476..420728dd 100644 --- a/tradein-mvp/backend/app/services/geocoder.py +++ b/tradein-mvp/backend/app/services/geocoder.py @@ -51,11 +51,14 @@ class GeocodeResult: # корпус («49» вместо реального «49к1») — houses-фолбэк нашёл ОДНОЗНАЧНЫЙ дом по # нормализованному совпадению. Честный сигнал вызывающему коду «адрес уточнён # автоматически», НЕ эвристика на корректность — см. `geocode()`/`_local_houses_match`. - # Известный предел: `geocode_cache` НЕ хранит этот флаг (схему не трогаем) — - # на повторный запрос ТОГО ЖЕ сырого адреса из кэша координаты корректные, но - # `address_refined` вернётся `False` (та же судьба у `city_ambiguous` при - # cache-hit — см. `_geocode_resolve`, восстанавливается `replace()` из - # текущего вызова, а не из кэша). + # Houses-фолбэк НЕ пишет свой результат в `geocode_cache` (менее надёжный + # источник координат, чем geoportal/cadastral/Nominatim — #2626 review R2 #4), + # поэтому этот сигнал переживает КАЖДЫЙ повторный запрос того же сырого + # адреса. `geocode_cache` вообще не хранит этот флаг (схему не трогаем) — + # если бы houses-хит когда-нибудь попал в кэш, на cache-hit `address_refined` + # вернулся бы `False` (та же судьба у `city_ambiguous` при cache-hit — см. + # `_geocode_resolve`, восстанавливается `replace()` из текущего вызова, а не + # из кэша). address_refined: bool = False @@ -1361,13 +1364,33 @@ def _norm_local_house(raw: str) -> str: return s +# Хвостовой мусор ПОСЛЕ номера дома — квартира/офис/помещение/подъезд/этаж. +# НЕ включает «корп/корпус/к» (в отличие от `_RE_APT_TAIL` выше) — корпус тут +# ЧАСТЬ номера дома, который должен остаться видимым для `_LOCAL_HOUSE_TOKEN_RE` +# («49к1», «26 к 1» — корпус нельзя терять). Без этой зачистки +# `_extract_local_house_token` (берёт ПОСЛЕДНЕЕ число в строке) находит номер +# квартиры/этажа вместо дома — прод-баг #2626 review R2 #1: «...Педагогическая, +# д 15, кв 11» отдавал дом «11» (координаты ЧУЖОГО здания) вместо «15». +_RE_LOCAL_APT_TAIL = re.compile( + r"[,\s]\s*(?:кв|квартира|оф|офис|пом|помещение|лит|подъезд|этаж)\.?\s*\d.*$", + re.IGNORECASE, +) + + def _extract_local_house_token(address: str) -> str | None: """Номер дома из ПОЛЬЗОВАТЕЛЬСКОГО адреса — с учётом «/N» и «корпус N» хвостов, которые `_parse_street_house`/`_HOUSE_NUM` обрезают (см. коммент у `_LOCAL_HOUSE_TOKEN_RE`). Берём ПОСЛЕДНЕЕ совпадение — номер дома в русском адресе почти всегда в хвосте строки. None, если цифр нет вовсе. + + Квартирный/этажный/подъездный хвост зачищается ДО поиска номера + (`_RE_LOCAL_APT_TAIL`) — иначе «последнее число в строке» это номер + квартиры/этажа, а не дома (см. докстринг у `_RE_LOCAL_APT_TAIL`). """ s = _RE_POSTAL.sub(" ", " ".join(address.lower().strip().split())).strip(" ,.") + if not s: + return None + s = _RE_LOCAL_APT_TAIL.sub(" ", s).strip(" ,.") if not s: return None matches = list(_LOCAL_HOUSE_TOKEN_RE.finditer(s)) @@ -1431,25 +1454,53 @@ def _street_tail_matches(row_street_norm: str, query_street_norm: str) -> bool: return row_street_norm == query_street_norm or row_street_norm.endswith(" " + query_street_norm) +# «24к1» → «24» (базовый номер варианта с корпусом/слэшем); «44» (голый номер, +# без суффикса) → None. Используется ТОЛЬКО для sibling-guard (см. ниже) — +# отличить «этот дом однозначно к1» от «этого дома несколько корпусов, а у +# нас в вводе просто нет данных, какой именно». +_LOCAL_HOUSE_VARIANT_BASE_RE = re.compile(r"^(\d+)(?:к\d+|/\d+)$") + + def _local_houses_match(db: Session, street: str, house: str) -> GeocodeSuggestion | None: """Последний локальный тир `geocode()` (#2626) — fallback на `houses` (скрейпленные листинги avito/cian/derived/yandex, own DB table, БЕЗ FDW). Вызывается ТОЛЬКО когда geoportal/cadastral/Nominatim уже не дали результата. - Два независимых допущения, оба defensive (при неоднозначности — None, не гадаем): + Допущения, все defensive (при неоднозначности — None, не гадаем): 1. Улица матчится «по хвосту» (`_street_tail_matches`) — ловит расхождение разговорного/сокращённого имени («Онуфриева») и канонического ГАР-имени в houses («Начдива Онуфриева»). - 2. Номер дома — сперва точное совпадение; нет — пробуем `<номер>к1` (частый - случай: пользователь ввёл «49», у дома есть только корпус «49к1»). ЛЮБОЙ - шаг, где кандидатов больше одного (после дедупа по координатам — разные - source-строки ОДНОГО дома не в счёт), возвращает None — угадывать нельзя. + 2. Координаты строки-кандидата обязаны лежать в широком ЕКБ-bbox + (`is_within_ekb_bbox_wide`) — `houses` НЕ ЕКБ-only реестр (в отличие от + geoportal/cad_buildings): 21% строк с координатами лежат вне области ЕКБ, + местами вплоть до другого региона (#2626 review R2 #2 — прод-пример + «улица Маяковского, 7» в houses это Серов, а не запрошенный + Екатеринбург). `use_local_ekb` в `geocode()` гейтит только ЗАПРОС + пользователя, не страхует от грязной строки-источника. + 3. Номер дома — сперва точное совпадение; нет — пробуем `<номер>к1` (частый + случай: пользователь ввёл «49», у дома есть только корпус «49к1»), но + ТОЛЬКО если среди кандидатов улицы НЕТ других корпусов/дробей этого же + номера («24к2», «24/2» и т.п.) — иначе «к1» такая же угадайка, как и + любой другой корпус, и реальные дома могут быть в 250-400м друг от друга + (#2626 review R2 #3, прод-пример «Начдива Онуфриева, 24»: 24к1/24к2/24к3 + — три разных здания). + 4. ЛЮБОЙ шаг, где кандидатов больше одного (после дедупа по округлённым + координатам — разные source-строки ОДНОГО дома не в счёт), возвращает + None — угадывать нельзя. SQL — дешёвый ILIKE-префильтр по последнему слову улицы (нет индекса на - `houses.address`, но тир последний и редкий — не на каждый запрос), вся - точная логика (суффикс улицы + равенство номера) — в Python, что и делает - её юнит-тестируемой без реальной БД (см. `test_geocoder_local_houses_fallback.py`). + `houses.address`, но тир последний и редкий — не на каждый запрос) с + детерминированным ORDER BY (дедуп по координатам иначе непредсказуемо + выбирал бы, какая из двух ~идентичных source-строк станет ответом — + #2626 review R2 #5); вся точная логика (суффикс улицы, bbox, равенство + номера) — в Python, что и делает её юнит-тестируемой без реальной БД + (см. `test_geocoder_local_houses_fallback.py`). + + Результат этого тира НЕ кэшируется в `geocode_cache` вызывающей стороной + (см. `geocode()`) — `houses`-координаты из скрейпленных объявлений менее + надёжны, чем geoportal/cadastral/Nominatim, а сам lookup дешёвый и локальный + (#2626 review R2 #4). """ query_street_norm = _clean_local_house_street(street) if not query_street_norm: @@ -1466,6 +1517,7 @@ def _local_houses_match(db: Session, street: str, house: str) -> GeocodeSuggesti FROM houses WHERE address ILIKE CAST('%' || :w || '%' AS text) AND lat IS NOT NULL AND lon IS NOT NULL + ORDER BY address, id """), {"w": last_word}, ).fetchall() @@ -1478,23 +1530,32 @@ def _local_houses_match(db: Session, street: str, house: str) -> GeocodeSuggesti ) return None + # Street-tail + bbox фильтр — один проход, дальше переиспользуется и для + # точного совпадения, и для corpus-1 догадки, и для sibling-guard. + street_rows: list[tuple[str, float, float, str]] = [] # (house_norm, lat, lon, addr) + for r in rows: + parsed = _row_local_house(str(r.address or "")) + if parsed is None: + continue + row_street_norm, row_house_norm = parsed + if not _street_tail_matches(row_street_norm, query_street_norm): + continue + lat, lon = float(r.lat), float(r.lon) + if not is_within_ekb_bbox_wide(lat, lon): + continue + street_rows.append((row_house_norm, lat, lon, str(r.address))) + def _candidates(house_norm: str) -> list[tuple[str, float, float]]: out: list[tuple[str, float, float]] = [] seen_coords: set[tuple[float, float]] = set() - for r in rows: - parsed = _row_local_house(str(r.address or "")) - if parsed is None: - continue - row_street_norm, row_house_norm = parsed + for row_house_norm, lat, lon, addr in street_rows: if row_house_norm != house_norm: continue - if not _street_tail_matches(row_street_norm, query_street_norm): - continue - coord_key = (round(float(r.lat), 4), round(float(r.lon), 4)) # ~11m — дедуп источников + coord_key = (round(lat, 4), round(lon, 4)) # ~11m — дедуп источников if coord_key in seen_coords: continue seen_coords.add(coord_key) - out.append((str(r.address), float(r.lat), float(r.lon))) + out.append((addr, lat, lon)) return out exact = _candidates(query_house_norm) @@ -1514,6 +1575,21 @@ def _local_houses_match(db: Session, street: str, house: str) -> GeocodeSuggesti # если запрошенный номер — голое число (не пытаемся достраивать «49/2» → «49/2к1»). if query_house_norm.isdigit(): corpus1 = f"{query_house_norm}к1" + siblings = { + row_house_norm + for row_house_norm, _lat, _lon, _addr in street_rows + if row_house_norm != corpus1 + and (m := _LOCAL_HOUSE_VARIANT_BASE_RE.match(row_house_norm)) is not None + and m.group(1) == query_house_norm + } + if siblings: + logger.info( + "local houses fallback: корпус-1 %r неоднозначен — есть другие " + "корпуса/дроби %s — skip", + corpus1, + sorted(siblings), + ) + return None guessed = _candidates(corpus1) if len(guessed) == 1: addr, lat, lon = guessed[0] @@ -1810,7 +1886,10 @@ async def _geocode_resolve( # резолвился Nominatim'ом (разговорное/усечённое имя улицы или отсутствующий # в вводе корпус). См. `_local_houses_match`. EKB-only гейт — тот же, что у # geoportal/cadastral (houses — преимущественно ЕКБ-трафик, тот же риск - # коллизии улица+дом с другим городом региона, что и мотивировал #2582). + # коллизии улица+дом с другим городом региона, что и мотивировал #2582); + # координаты строки-кандидата ДОПОЛНИТЕЛЬНО проверяются bbox-ом внутри + # `_local_houses_match` (гейт здесь фильтрует только запрос пользователя, + # не грязь в самой таблице — #2626 review R2 #2). if use_local_ekb and parsed is not None: local_street, _parsed_house = parsed local_house = _extract_local_house_token(address) or _parsed_house @@ -1825,7 +1904,12 @@ async def _geocode_resolve( city_ambiguous=city_ambiguous, address_refined=True, ) - await asyncio.to_thread(_cache_put, db, addr_norm, result) + # НЕ кэшируем: houses-координаты (скрейпленные листинги) менее + # надёжны, чем geoportal/cadastral/Nominatim, а сам lookup дешёвый + # и локальный — кэш только продлевал бы жизнь возможной ошибке + # источника (#2626 review R2 #4). Побочный эффект: `address_refined` + # переживает КАЖДЫЙ повторный запрос этого сырого адреса, а не + # только первый (было известным пределом до этого фикса). logger.info( "geocode local houses fallback: %s → (%.5f, %.5f) [%s]", addr_norm, diff --git a/tradein-mvp/backend/tests/test_geocoder_local_houses_fallback.py b/tradein-mvp/backend/tests/test_geocoder_local_houses_fallback.py index fef919a4..d28941c2 100644 --- a/tradein-mvp/backend/tests/test_geocoder_local_houses_fallback.py +++ b/tradein-mvp/backend/tests/test_geocoder_local_houses_fallback.py @@ -83,6 +83,32 @@ def test_norm_local_house(raw: str, expected: str) -> None: ("Крестинского 49к1", "49к1"), ("8 Марта 204", "204"), # digit-leading street name doesn't confuse it ("Малышева 30", "30"), + # #2626 review R2 #1 — прод-баг: квартира подменяла дом («д 15, кв 11» + # → дом «11», чужое здание). Реальные строки из trade_in_estimates: + ( + "620078, Свердловская обл, г Екатеринбург, Кировский р-н, " + "ул Педагогическая, д 15, кв 11", + "15", + ), + ( + "620078, Свердловская обл, г Екатеринбург, Кировский р-н, " + "ул Педагогическая, д 15, кв 48", + "15", + ), + # корпус ПЕРЕД квартирой — «26 к 1» обязан остаться частью номера дома, + # «кв 41» — уйти: + ( + "620149, Свердловская обл, г Екатеринбург, Ленинский р-н, " + "ул Начдива Онуфриева, д 26 к 1, кв 41", + "26к1", + ), + # подъезд/этаж — тот же класс бага, что и квартира (последнее число в + # строке — не дом): + ( + "Россия, Свердловская область, Екатеринбург, Трамвайный переулок, " + "2к2, подъезд 1, этаж 25, кв. 205", + "2к2", + ), ], ) def test_extract_local_house_token(address: str, expected: str) -> None: @@ -186,7 +212,30 @@ def test_local_houses_match_exact_house_number() -> None: def test_local_houses_match_street_tail_and_corpus1_guess() -> None: - """«Онуфриева, 24» (без «Начдива», без корпуса) → единственный «24к1» реестра.""" + """«Онуфриева, 24» (без «Начдива», без корпуса), реестр — ЕДИНСТВЕННЫЙ + корпус «24к1» → уверенная догадка (нет sibling-корпусов — не угадайка).""" + db = _db_with_rows( + [ + _make_row( + "р-н Ленинский, мкр. Юго-Западный, улица Начдива Онуфриева, 24к1", + 56.802928, + 60.551696, + ), + ] + ) + + hit = _local_houses_match(db, "онуфриева", "24") + + assert hit is not None + assert hit.lat == pytest.approx(56.802928) + assert hit.lon == pytest.approx(60.551696) + + +def test_local_houses_match_corpus1_guess_skipped_when_sibling_corpus_exists() -> None: + """#2626 review R2 #3, прод-данные: «Начдива Онуфриева, 24» реально ТРИ + разных здания (24к1/24к2/24к3, 250-400м друг от друга). Догадка «→24к1» + не угадывает конкретное здание среди known-siblings — честный None, не + «уверенный» результат с confidence='exact' на случайно выбранном доме.""" db = _db_with_rows( [ _make_row( @@ -199,11 +248,19 @@ def test_local_houses_match_street_tail_and_corpus1_guess() -> None: ] ) - hit = _local_houses_match(db, "онуфриева", "24") + assert _local_houses_match(db, "онуфриева", "24") is None - assert hit is not None - assert hit.lat == pytest.approx(56.802928) - assert hit.lon == pytest.approx(60.551696) + +def test_local_houses_match_corpus1_guess_skipped_when_slash_sibling_exists() -> None: + """Sibling-guard ловит не только «кN», но и «/N» вариант того же номера.""" + db = _db_with_rows( + [ + _make_row("улица X, 24к1", 56.80, 60.60), + _make_row("улица X, 24/2", 56.81, 60.61), + ] + ) + + assert _local_houses_match(db, "x", "24") is None def test_local_houses_match_no_corpus1_candidate_returns_none() -> None: @@ -278,6 +335,50 @@ def test_local_houses_match_returns_none_on_db_error() -> None: assert _local_houses_match(db, "онуфриева", "24") is None +# ── bbox guard: `houses` is NOT EKB-only (#2626 review R2 #2) ─────────────── + + +def test_local_houses_match_rejects_row_outside_ekb_bbox() -> None: + """Прод-кейс: «улица Маяковского, 7» в `houses` — это Серов (56.6/60.66 — + ~310км от ЕКБ), не Екатеринбург. `use_local_ekb` в `geocode()` гейтит только + ЗАПРОС пользователя, не координаты строки-источника — bbox-фильтр внутри + `_local_houses_match` обязан отбросить такую строку, а не вернуть её как + confidence='exact' совпадение чужого города.""" + db = _db_with_rows( + [_make_row("улица Маяковского, 7", 59.652903, 60.659674)], # Серов, не ЕКБ + ) + + assert _local_houses_match(db, "маяковского", "7") is None + + +def test_local_houses_match_accepts_row_inside_ekb_bbox_wide() -> None: + """Контроль: легитимная ЕКБ-строка (в т.ч. приграничье, в WIDE, не в TIGHT) + по-прежнему проходит — bbox-фильтр не режет реальные ЕКБ-дома.""" + db = _db_with_rows( + [_make_row("Екатеринбург, улица Маяковского, 8", 56.862701, 60.620274)], + ) + + hit = _local_houses_match(db, "маяковского", "8") + + assert hit is not None + assert hit.lat == pytest.approx(56.862701) + + +# ── deterministic ORDER BY (#2626 review R2 #5) ────────────────────────────── + + +def test_local_houses_match_query_has_deterministic_order_by() -> None: + """Без ORDER BY дедуп по округлённым координатам оставлял бы ПЕРВУЮ строку + в порядке сканирования — недетерминированно между вызовами. SQL обязан + сортировать явно.""" + db = _db_with_rows([]) + + _local_houses_match(db, "x", "1") + + sql_text = str(db.execute.call_args[0][0]) + assert "ORDER BY" in sql_text.upper() + + # ── geocode() wiring — last-resort tier, sets address_refined ─────────────── @@ -298,7 +399,7 @@ async def test_geocode_falls_back_to_local_houses_after_nominatim_miss() -> None patch("app.services.geocoder._geoportal_house_match", return_value=None), patch("app.services.geocoder._cadastral_house_match", return_value=None), patch("app.services.geocoder._cadastral_forward_sync", return_value=[]), - patch("app.services.geocoder._cache_put"), + patch("app.services.geocoder._cache_put") as mock_cache_put, patch( "app.services.geocoder._nominatim_lookup", new_callable=AsyncMock, @@ -316,6 +417,9 @@ async def test_geocode_falls_back_to_local_houses_after_nominatim_miss() -> None assert result.confidence == "exact" assert result.address_refined is True mock_local.assert_called_once() + # #2626 review R2 #4 — houses-фолбэк дешёвый и менее надёжный источник + # координат, чем geoportal/cadastral/Nominatim — свой результат не кэширует. + mock_cache_put.assert_not_called() async def test_geocode_address_refined_false_when_earlier_tier_hits() -> None: @@ -366,3 +470,41 @@ async def test_geocode_returns_none_when_local_houses_also_misses() -> None: assert result is None mock_local.assert_called_once() + + +async def test_geocode_local_houses_apartment_number_does_not_leak_into_house() -> None: + """End-to-end regression, #2626 review R2 #1: реальный прод-адрес с хвостом + «кв 11» должен резолвиться в дом 15 (`Педагогическая ул.,15`), а НЕ в дом 11 + (`Педагогическая ул.,11` — чужое здание) — `_local_houses_match` не + замокан, проверяем полную цепочку `geocode()` → `_extract_local_house_token` + → SQL-lookup.""" + db = _db_with_rows( + [ + _make_row("Педагогическая ул.,11", 56.835387, 60.654104), + _make_row("Педагогическая ул.,15", 56.835284, 60.655829), + ] + ) + + with ( + patch("app.services.geocoder._cache_get", return_value=None), + patch("app.services.geocoder._geoportal_house_match", return_value=None), + patch("app.services.geocoder._cadastral_house_match", return_value=None), + patch("app.services.geocoder._cadastral_forward_sync", return_value=[]), + patch("app.services.geocoder._cache_put") as mock_cache_put, + patch( + "app.services.geocoder._nominatim_lookup", + new_callable=AsyncMock, + return_value=None, + ), + ): + result = await geocode( + "620078, Свердловская обл, г Екатеринбург, Кировский р-н, " + "ул Педагогическая, д 15, кв 11", + db, + ) + + assert result is not None + assert result.lat == pytest.approx(56.835284) + assert result.lon == pytest.approx(60.655829) + assert result.address_refined is True + mock_cache_put.assert_not_called()