From ec17886d60769d3a709a89b8bc13b81220f542d1 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Wed, 5 Aug 2026 17:36:07 +0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(tradein/geocode):=20=D0=BF=D1=80=D0=BE?= =?UTF-8?q?=D1=88=D0=B8=D1=82=D1=8C=20city=5Fhint=20=D0=B2=20deals-=D1=81?= =?UTF-8?q?=D0=BA=D1=80=D0=B8=D0=BF=D1=82=20+=20=D1=80=D0=B0=D0=B7=D0=B2?= =?UTF-8?q?=D0=B5=D1=81=D1=82=D0=B8=20=D1=81=D1=87=D1=91=D1=82=D1=87=D0=B8?= =?UTF-8?q?=D0=BA=D0=B8=20=D0=B3=D0=B5=D0=B9=D1=82=D0=B0=20(#2603)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Хвосты после #2601 (замыкание петли «город → геокодер»). 1. scripts/geocode_deals_nominatim.py — непрошитый sibling-caller. Скрипт группировал `GROUP BY address` и звал `geocode(address, db)` без города, хотя deals.city (миграция 177) заполнена на 100%: один и тот же текст адреса из разных городов схлопывался в одну группу, один geocode-вызов и один UPDATE по тексту адреса. Теперь — та же форма, что в #2601: группировка по паре (address, city), city_hint в geocode(), UPDATE и mark-tried через `city IS NOT DISTINCT FROM` (обычное `=` не ловит NULL-город → NULL-группа не обновлялась бы вовсе). Хинт передаётся ТОЛЬКО для значений из geocoder.SVERDLOVSK_OBLAST_CITIES: deals.city росреестровое, в хвосте лежит мусор («Бессонова», «Бердюгина», «Билейский рыбопитомник»), а любой не-ЕКБ хинт жёстко закрывает EKB-локальные тиры и подставляется в запрос провайдеру — мусорный хинт хуже отсутствия хинта. Словарь переиспользован, а не заведён свой: тот же набор уже питает гейты самого геокодера (_names_non_ekb_city / _ekb_local_tiers_allowed) и estimator._resolve_target_city. 2. tasks/backfill_listings_coords_geoportal.py — наблюдаемость городского гейта. Добавлен skipped_non_ekb_by_column (+ в to_counters и в DONE-логи): колоночный гейт стоит перед парсером адреса, поэтому по мере раскатки областных развёрток (#2598) строки потекут из no_address в skipped_non_ekb и общий счётчик поменяет смысл ровно тогда, когда по нему валидируют раскатку. Старый счётчик не тронут — остаётся суммой обоих гейтов, вклад текстового считается разностью. 3. tests: test_admin_geocode_missing_passes_city_hint параметризован на target="deals" (колонка city есть в обеих таблицах, ветка была не покрыта). 4. tasks/geocode_missing.py: dry-run лог печатает city — он с #2594 часть ключа группы, без него две строки dry-run неотличимы. Refs #2603 --- .../backfill_listings_coords_geoportal.py | 25 ++- .../backend/app/tasks/geocode_missing.py | 6 +- .../scripts/geocode_deals_nominatim.py | 212 ++++++++++++------ .../scripts/test_geocode_deals_nominatim.py | 164 ++++++++++++-- ...test_backfill_listings_coords_geoportal.py | 45 ++++ .../tests/tasks/test_geocode_missing.py | 16 +- 6 files changed, 382 insertions(+), 86 deletions(-) diff --git a/tradein-mvp/backend/app/tasks/backfill_listings_coords_geoportal.py b/tradein-mvp/backend/app/tasks/backfill_listings_coords_geoportal.py index 9bd37f38..286d1cf8 100644 --- a/tradein-mvp/backend/app/tasks/backfill_listings_coords_geoportal.py +++ b/tradein-mvp/backend/app/tasks/backfill_listings_coords_geoportal.py @@ -85,7 +85,17 @@ class BackfillCoordsResult: updated: int = 0 # реально обновлено (UPDATE rowcount) no_address: int = 0 # listing.address IS NULL / не распарсился no_match: int = 0 # адрес распарсился, но в реестре здания нет - skipped_non_ekb: int = 0 # non-ЕКБ гейт: колонка city (#2594) ИЛИ текст адреса (#2583) + skipped_non_ekb: int = 0 # non-ЕКБ гейт ВСЕГО: колонка city (#2594) ИЛИ текст (#2583) + # Подмножество skipped_non_ekb — только те, кого отсёк гейт по КОЛОНКЕ city + # (#2603). Зачем отдельный счётчик: колоночный гейт стоит ПЕРЕД парсером + # адреса, поэтому по мере раскатки областных развёрток (#2598) строки, которые + # сейчас падают в no_address (город неизвестен, адрес не парсится), начнут + # перетекать в skipped_non_ekb — и общий счётчик поменяет смысл ровно тогда, + # когда по нему хотят валидировать раскатку. Разность + # skipped_non_ekb - skipped_non_ekb_by_column = вклад ТЕКСТОВОГО гейта, т.е. + # старая метрика #2583 остаётся вычислимой. Обратная совместимость: + # skipped_non_ekb продолжает означать то же, что и раньше (гейт целиком). + skipped_non_ekb_by_column: int = 0 errors: int = 0 # исключения при обработке отдельной записи duration_sec: float = field(default=0.0) @@ -97,6 +107,7 @@ class BackfillCoordsResult: "no_address": self.no_address, "no_match": self.no_match, "skipped_non_ekb": self.skipped_non_ekb, + "skipped_non_ekb_by_column": self.skipped_non_ekb_by_column, "errors": self.errors, "duration_sec": int(self.duration_sec), } @@ -227,6 +238,10 @@ def backfill_coords_from_geoportal( city: str | None = row.get("city") if city is not None and city != "Екатеринбург": res.skipped_non_ekb += 1 + # Отдельный срез (#2603) — общий skipped_non_ekb смешивает + # колоночный и текстовый гейты, а по мере раскатки #2598 + # колоночный будет забирать строки из no_address. + res.skipped_non_ekb_by_column += 1 continue # Текстовый гейт (#2583, H3) — fallback для листингов, у которых @@ -313,13 +328,15 @@ def backfill_coords_from_geoportal( logger.info( "backfill_coords: DONE — candidates=%d matched=%d updated=%d " - "no_address=%d no_match=%d skipped_non_ekb=%d errors=%d duration=%.1fs", + "no_address=%d no_match=%d skipped_non_ekb=%d (by_column=%d) " + "errors=%d duration=%.1fs", res.candidates, res.matched, res.updated, res.no_address, res.no_match, res.skipped_non_ekb, + res.skipped_non_ekb_by_column, res.errors, res.duration_sec, ) @@ -369,7 +386,8 @@ def run_geoportal_coords_backfill( runs_mod.mark_done(db, run_id, counters) logger.info( "run_geoportal_coords_backfill: run_id=%d DONE candidates=%d matched=%d " - "updated=%d no_address=%d no_match=%d skipped_non_ekb=%d errors=%d duration=%.1fs", + "updated=%d no_address=%d no_match=%d skipped_non_ekb=%d (by_column=%d) " + "errors=%d duration=%.1fs", run_id, res.candidates, res.matched, @@ -377,6 +395,7 @@ def run_geoportal_coords_backfill( res.no_address, res.no_match, res.skipped_non_ekb, + res.skipped_non_ekb_by_column, res.errors, res.duration_sec, ) diff --git a/tradein-mvp/backend/app/tasks/geocode_missing.py b/tradein-mvp/backend/app/tasks/geocode_missing.py index bcb1d18d..35823593 100644 --- a/tradein-mvp/backend/app/tasks/geocode_missing.py +++ b/tradein-mvp/backend/app/tasks/geocode_missing.py @@ -223,9 +223,13 @@ async def geocode_missing_listings( result.addresses_geocoded += 1 if dry_run: + # city в логе (#2603) — с #2594 это часть ключа группы: без него две + # строки dry-run с одинаковым текстом адреса неотличимы друг от друга. logger.info( - "geocode_missing[dry]: '%s' → (%.5f, %.5f) provider=%s would update %d listings", + "geocode_missing[dry]: '%s' city=%r → (%.5f, %.5f) provider=%s " + "would update %d listings", address[:60], + city, geo.lat, geo.lon, geo.provider, diff --git a/tradein-mvp/backend/scripts/geocode_deals_nominatim.py b/tradein-mvp/backend/scripts/geocode_deals_nominatim.py index 80947023..99e981cd 100644 --- a/tradein-mvp/backend/scripts/geocode_deals_nominatim.py +++ b/tradein-mvp/backend/scripts/geocode_deals_nominatim.py @@ -18,12 +18,30 @@ result into `geocode_cache`. We do NOT call Nominatim directly — that keeps a single source of truth for provider order, ЕКБ bbox filtering, and the 1 req/sec policy. -Why dedup by address (not one call per row) -------------------------------------------- -`deals.address` is street-only ('Екатеринбург, '), so thousands of rows -share the same address. The geocoder caches by normalized address, but we also -GROUP BY address up front so the real call count is driven by DISTINCT streets, -not the ~6,951 row backlog. One geocode call → UPDATE every deal on that street. +Why dedup by (address, city) — not one call per row +--------------------------------------------------- +`deals.address` is street-only (', '), so thousands of rows share +the same address. The geocoder caches by normalized address, but we also GROUP +BY up front so the real call count is driven by DISTINCT streets, not the +~6,951 row backlog. One geocode call → UPDATE every deal on that street. + +Grouping is by the PAIR (address, city), not by address alone (#2603, same +shape as #2601 fixed in app/tasks/geocode_missing.py): the same address text in +two different cities must not collapse into one geocode call, and the UPDATE +must not spill onto the other city's rows — hence `city IS NOT DISTINCT FROM` +(plain `=` never matches a NULL city, so a NULL-city group would update nothing). + +City hint (and why it is filtered) +---------------------------------- +`deals.city` comes from Rosreestr (migration 177) and is filled on 100% of the +rows, so a hint is available for every group — but its long tail holds values +that are not cities at all ('Бессонова', 'Бердюгина', 'Билейский рыбопитомник'). +A non-EKB hint is a HARD signal inside the geocoder: it closes the EKB-only +local tiers (`_ekb_local_tiers_allowed`) and gets prefixed into the provider +query, so a junk hint makes the result strictly WORSE than no hint at all. +We therefore pass the hint only for values that are recognised cities of oblast +66 (`_city_hint` below); everything else degrades to the previous behaviour +(no hint, address text only). Street-level precision is accepted: the estimator's comparable search uses a 1000-2000 m radius, so a street-level point lands every deal on that street in @@ -87,6 +105,7 @@ except ImportError: # pragma: no cover — fallback for adhoc invocation try: from app.services.geocoder import ( # type: ignore[import-not-found] + SVERDLOVSK_OBLAST_CITIES, GeocodeResult, geocode, ) @@ -94,7 +113,7 @@ except ImportError: # pragma: no cover import sys sys.path.insert(0, str(Path(__file__).resolve().parents[1])) - from app.services.geocoder import GeocodeResult, geocode + from app.services.geocoder import SVERDLOVSK_OBLAST_CITIES, GeocodeResult, geocode logging.basicConfig( level=logging.INFO, @@ -122,17 +141,43 @@ _LOG_EVERY = 25 @dataclass class AddressGroup: - """One distinct deals.address and how many lat-IS-NULL rows share it.""" + """One distinct (deals.address, deals.city) pair + its lat-IS-NULL row count. + + `city` is the raw Rosreestr value (may be NULL for non-rosreestr sources, and + may be junk — see `_city_hint`), kept verbatim so the UPDATE can target + exactly the rows this group came from. + """ address: str deals_count: int + city: str | None = None + + +def _city_hint(city: str | None) -> str | None: + """`city` if it is a recognised oblast-66 city name, else None (#2603). + + Dictionary = `geocoder.SVERDLOVSK_OBLAST_CITIES` — the single source of truth + already used by the geocoder's own city gates (`_names_non_ekb_city`, + `_ekb_local_tiers_allowed`) and by `estimator._resolve_target_city`. No local + list: a second one would drift from the gates the hint actually feeds. + + Filtering is not optional politeness: `deals.city` is Rosreestr-sourced and + its tail holds non-cities ('Бессонова', 'Бердюгина', 'Билейский + рыбопитомник'). Any non-EKB hint closes the EKB-only local tiers and is + prefixed into the provider query, so an unrecognised value would make the + geocode worse than passing nothing. + """ + if not city: + return None + normalized = " ".join(city.lower().split()) + return city if normalized in SVERDLOVSK_OBLAST_CITIES else None @dataclass class Stats: """Final-summary counters. - - processed distinct addresses fed to the geocoder this run. + - processed distinct (address, city) groups fed to the geocoder this run. - geocoded addresses the geocoder resolved to coords. - geocode_failed addresses the geocoder returned None for (still stamped). - skipped distinct addresses skipped before any geocode call @@ -156,10 +201,8 @@ class Stats: # --------------------------------------------------------------------------- -def _select_pending_addresses( - db: Session, *, limit: int, stale_days: int -) -> list[AddressGroup]: - """Distinct deals.address still needing coords — resume-safe candidate set. +def _select_pending_addresses(db: Session, *, limit: int, stale_days: int) -> list[AddressGroup]: + """Distinct (address, city) pairs still needing coords — resume-safe set. Combines the `deals_geocode_pending_idx` partial index predicate (`lat IS NULL`) with a staleness filter so failed/un-geocodable addresses @@ -167,44 +210,68 @@ def _select_pending_addresses( The GROUP BY collapses the ~6,951-row backlog into its distinct streets; `:limit` caps the deal ROWS fanned out, computed from the running SUM of - per-address counts so a single huge street can't blow past the cap. We - order by occurrence DESC (biggest ROI per geocode call first) then address - for a deterministic resume order. + per-group counts so a single huge street can't blow past the cap. We order + by occurrence DESC (biggest ROI per geocode call first) then address, city + for a deterministic resume order (the window ORDER BY must match the outer + one, otherwise the running_rows cap slices a different ordering). + + GROUP BY address, city — NOT address alone (#2603, the shape #2601 fixed in + app/tasks/geocode_missing.py): the same street name in two cities is two + geocode calls with two different hints, not one call whose result lands on + both. SQL groups NULL cities together, so a NULL-city group stays its own + group rather than merging into an arbitrary city. """ - rows = db.execute( - text( - "SELECT address, deals_count FROM (" - " SELECT address, " - " COUNT(*) AS deals_count, " - " SUM(COUNT(*)) OVER (" - " ORDER BY COUNT(*) DESC, address ASC" - " ) AS running_rows " - " FROM deals " - " WHERE lat IS NULL " - " AND address IS NOT NULL " - " AND length(trim(address)) >= 3 " - " AND (geocode_tried_at IS NULL " - " OR geocode_tried_at < NOW() " - " - make_interval(days => CAST(:stale_days AS int))) " - " GROUP BY address " - ") g " - "WHERE running_rows - deals_count < CAST(:limit AS int) " - "ORDER BY deals_count DESC, address ASC" - ), - {"limit": limit, "stale_days": stale_days}, - ).mappings().all() - return [AddressGroup(address=r["address"], deals_count=r["deals_count"]) for r in rows] + rows = ( + db.execute( + text( + "SELECT address, city, deals_count FROM (" + " SELECT address, city, " + " COUNT(*) AS deals_count, " + " SUM(COUNT(*)) OVER (" + " ORDER BY COUNT(*) DESC, address ASC, city ASC NULLS FIRST" + " ) AS running_rows " + " FROM deals " + " WHERE lat IS NULL " + " AND address IS NOT NULL " + " AND length(trim(address)) >= 3 " + " AND (geocode_tried_at IS NULL " + " OR geocode_tried_at < NOW() " + " - make_interval(days => CAST(:stale_days AS int))) " + " GROUP BY address, city " + ") g " + "WHERE running_rows - deals_count < CAST(:limit AS int) " + "ORDER BY deals_count DESC, address ASC, city ASC NULLS FIRST" + ), + {"limit": limit, "stale_days": stale_days}, + ) + .mappings() + .all() + ) + return [ + AddressGroup( + address=r["address"], + city=r.get("city"), + deals_count=r["deals_count"], + ) + for r in rows + ] def _count_pending_total(db: Session, *, stale_days: int) -> tuple[int, int]: - """Full backlog: (distinct addresses, total rows) eligible this pass. + """Full backlog: (distinct (address, city) pairs, total rows) this pass. Denominators for the dry-run projection — counts every lat-IS-NULL deal - that passes the staleness filter, ignoring --limit. + that passes the staleness filter, ignoring --limit. Counts PAIRS, matching + what `_select_pending_addresses` actually feeds the geocoder (#2603) — + counting distinct addresses here would understate the denominator and the + projection would read above 100%. COALESCE(city, '') keeps a NULL-city pair + countable: `count()` skips NULL inputs, and a bare row expression with a + NULL field invites exactly that argument — the empty string can't collide + with a real city name, so the pair count stays honest either way. """ row = db.execute( text( - "SELECT COUNT(DISTINCT address) AS streets, COUNT(*) AS rows " + "SELECT COUNT(DISTINCT (address, COALESCE(city, ''))) AS streets, COUNT(*) AS rows " "FROM deals " "WHERE lat IS NULL " " AND address IS NOT NULL " @@ -225,8 +292,10 @@ def _count_pending_total(db: Session, *, stale_days: int) -> tuple[int, int]: # --------------------------------------------------------------------------- -def _update_deals_geocoded(db: Session, *, address: str, lat: float, lon: float) -> int: - """UPDATE every lat-IS-NULL deal on `address`; geom auto-fills via trigger. +def _update_deals_geocoded( + db: Session, *, address: str, city: str | None, lat: float, lon: float +) -> int: + """UPDATE every lat-IS-NULL deal on (address, city); geom via trigger. The `deals_set_geom_trg` BEFORE UPDATE OF lat, lon trigger (002_core_tables.sql, reuses listings_set_geom()) populates geom from the @@ -234,6 +303,12 @@ def _update_deals_geocoded(db: Session, *, address: str, lat: float, lon: float) so the row drops out of the candidate set. The `AND lat IS NULL` guard keeps this idempotent and avoids clobbering rows another pass already set. + `city IS NOT DISTINCT FROM` (not `=`) scopes the write to the group the + geocode was made for: plain `=` is never true for a NULL city, so a + NULL-city group would update zero rows and re-run forever; `IS NOT DISTINCT + FROM` treats NULL=NULL as a match while staying strict for a real city, so + the same street text in another city keeps its own coords (#2603). + Returns the number of deal rows updated. """ result = db.execute( @@ -243,14 +318,15 @@ def _update_deals_geocoded(db: Session, *, address: str, lat: float, lon: float) " lon = CAST(:lon AS double precision), " " geocode_tried_at = NOW() " " WHERE address = CAST(:addr AS text) " + " AND city IS NOT DISTINCT FROM CAST(:city AS text) " " AND lat IS NULL" ), - {"addr": address, "lat": lat, "lon": lon}, + {"addr": address, "city": city, "lat": lat, "lon": lon}, ) return result.rowcount -def _mark_deals_tried(db: Session, *, address: str) -> int: +def _mark_deals_tried(db: Session, *, address: str, city: str | None) -> int: """Stamp `geocode_tried_at = NOW()` WITHOUT touching lat/lon (geocode miss). Critical for resume: an address the geocoder can't resolve must still drop @@ -259,6 +335,9 @@ def _mark_deals_tried(db: Session, *, address: str) -> int: it for `stale_days`. The `AND lat IS NULL` guard means a concurrent success can't be downgraded. + Scoped to the (address, city) pair for the same reason as the coords write + — a miss in one city must not defer the other city's retry (#2603). + Returns the number of deal rows stamped. """ result = db.execute( @@ -266,9 +345,10 @@ def _mark_deals_tried(db: Session, *, address: str) -> int: "UPDATE deals " " SET geocode_tried_at = NOW() " " WHERE address = CAST(:addr AS text) " + " AND city IS NOT DISTINCT FROM CAST(:city AS text) " " AND lat IS NULL" ), - {"addr": address}, + {"addr": address, "city": city}, ) return result.rowcount @@ -285,7 +365,7 @@ async def _run_backfill( batch: str, dry_run: bool, ) -> Stats: - """For each distinct address: geocode once, then UPDATE all its deals. + """For each (address, city) pair: geocode once, then UPDATE all its deals. Per-address SAVEPOINT (`db.begin_nested()`) so one bad UPDATE can't abort the batch (backend.md SAVEPOINT rule). Per-address commit on success → a @@ -301,6 +381,11 @@ async def _run_backfill( for i, group in enumerate(groups, start=1): address = group.address + city = group.city + # Only a recognised oblast-66 city is fed to the geocoder; junk Rosreestr + # values degrade to None (see `_city_hint`). The raw `city` is still used + # for the UPDATE scope — it identifies the group either way. + hint = _city_hint(city) # The geocoder itself rejects <3 chars, but skip here too so the dry-run # report and counters stay honest (no phantom "processed" address). @@ -310,13 +395,14 @@ async def _run_backfill( result: GeocodeResult | None = None try: - result = await geocode(address, db) + result = await geocode(address, db, city_hint=hint) except Exception as exc: # defensive — one geocode error must not kill batch stats.geocode_failed += 1 stats.processed += 1 logger.warning( - "geocode raised for addr=%r (%d deals): %s", + "geocode raised for addr=%r city=%r (%d deals): %s", address[:60], + city, group.deals_count, exc, ) @@ -331,18 +417,21 @@ async def _run_backfill( stats.processed += 1 if dry_run: logger.info( - "DRY-RUN addr=%r (%d deals) → NOT FOUND (would stamp tried)", + "DRY-RUN addr=%r city=%r (%d deals) → NOT FOUND (would stamp tried)", address[:60], + city, group.deals_count, ) else: try: with db.begin_nested(): - _mark_deals_tried(db, address=address) + _mark_deals_tried(db, address=address, city=city) db.commit() except Exception as exc: # defensive — isolate one bad UPDATE db.rollback() - logger.warning("mark_tried failed for addr=%r: %s", address[:60], exc) + logger.warning( + "mark_tried failed for addr=%r city=%r: %s", address[:60], city, exc + ) _maybe_log_progress(i, groups, batch, stats) continue @@ -356,8 +445,9 @@ async def _run_backfill( if dry_run: logger.info( - "DRY-RUN addr=%r → (%.5f, %.5f) provider=%s would update %d deals", + "DRY-RUN addr=%r city=%r → (%.5f, %.5f) provider=%s would update %d deals", address[:60], + city, result.lat, result.lon, result.provider, @@ -369,7 +459,7 @@ async def _run_backfill( try: with db.begin_nested(): n = _update_deals_geocoded( - db, address=address, lat=result.lat, lon=result.lon + db, address=address, city=city, lat=result.lat, lon=result.lon ) # Per-address commit so resume picks up exactly where we crashed. db.commit() @@ -378,7 +468,7 @@ async def _run_backfill( db.rollback() # The geocode itself succeeded (and is cached); only the write # failed. Count the address as geocoded but log the write failure. - logger.warning("db_write failed for addr=%r: %s", address[:60], exc) + logger.warning("db_write failed for addr=%r city=%r: %s", address[:60], city, exc) _maybe_log_progress(i, groups, batch, stats) @@ -513,12 +603,10 @@ async def main(argv: list[str] | None = None) -> int: db = SessionLocal() try: - groups = _select_pending_addresses( - db, limit=args.limit, stale_days=args.stale_days - ) + groups = _select_pending_addresses(db, limit=args.limit, stale_days=args.stale_days) total_rows = sum(g.deals_count for g in groups) logger.info( - "loaded %d distinct addresses (%d deal rows) needing coords", + "loaded %d distinct (address, city) groups (%d deal rows) needing coords", len(groups), total_rows, ) @@ -533,9 +621,7 @@ async def main(argv: list[str] | None = None) -> int: stats = await _run_backfill(db, groups, batch=args.batch, dry_run=args.dry_run) if args.dry_run: - total_streets, backlog_rows = _count_pending_total( - db, stale_days=args.stale_days - ) + total_streets, backlog_rows = _count_pending_total(db, stale_days=args.stale_days) _report_dry_run( stats, total_streets=total_streets, diff --git a/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py b/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py index 794555cc..dc47e02a 100644 --- a/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py +++ b/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py @@ -12,6 +12,9 @@ Coverage (per the issue's test plan): batch. - dedup: distinct addresses drive geocode call count (one call per address). - main() wiring: SessionLocal, --limit bind, returns geocoded count. + - #2603: grouping by the (address, city) PAIR, city_hint filtered against + geocoder.SVERDLOVSK_OBLAST_CITIES (junk Rosreestr city → no hint), and + writes scoped with `city IS NOT DISTINCT FROM`. No real Postgres. `geocode()` is async → patched with AsyncMock (mirrors tests/tasks/test_geocode_missing.py). The Session is a MagicMock that routes @@ -31,6 +34,7 @@ from app.services.geocoder import GeocodeResult from scripts.geocode_deals_nominatim import ( AddressGroup, Stats, + _city_hint, _mark_deals_tried, _run_backfill, _select_pending_addresses, @@ -93,7 +97,7 @@ def _make_db_mock( tried_updates.append(dict(params) if params else {}) result.rowcount = update_rowcount return result - if "COUNT(DISTINCT address)" in sql_str: + if "COUNT(DISTINCT" in sql_str: result.first.return_value = (total_streets, total_rows) return result if "FROM deals" in sql_str: @@ -176,13 +180,16 @@ async def test_mark_deals_tried_sql_stamps_tried_at_only(): """The mark-tried writer sets geocode_tried_at and nothing else (no lat/lon).""" db = MagicMock() db.execute.return_value = MagicMock(rowcount=4) - n = _mark_deals_tried(db, address="Екатеринбург, X") + n = _mark_deals_tried(db, address="Екатеринбург, X", city="Екатеринбург") args, _kw = db.execute.call_args sql_str = str(args[0]) assert "UPDATE deals" in sql_str assert "geocode_tried_at = NOW()" in sql_str assert "SET lat" not in sql_str assert "lat IS NULL" in sql_str # idempotency / no-clobber guard + # #2603: a miss in one city must not defer the other city's retry. + assert "city IS NOT DISTINCT FROM CAST(:city AS text)" in sql_str + assert args[1]["city"] == "Екатеринбург" assert n == 4 @@ -312,7 +319,9 @@ async def test_run_backfill_db_write_failure_isolated_to_address(): def test_update_deals_geocoded_sets_lat_lon_tried_at_not_geom(): db = MagicMock() db.execute.return_value = MagicMock(rowcount=7) - n = _update_deals_geocoded(db, address="Екатеринбург, Y", lat=56.1, lon=60.2) + n = _update_deals_geocoded( + db, address="Екатеринбург, Y", city="Екатеринбург", lat=56.1, lon=60.2 + ) args, _kw = db.execute.call_args sql_str = str(args[0]) binds = args[1] @@ -321,10 +330,28 @@ def test_update_deals_geocoded_sets_lat_lon_tried_at_not_geom(): # geom must NOT be set manually — the deals_set_geom_trg trigger fills it. assert "geom" not in sql_str assert "lat IS NULL" in sql_str # no-clobber guard - assert binds == {"addr": "Екатеринбург, Y", "lat": 56.1, "lon": 60.2} + # #2603: the write is scoped to the (address, city) group it was made for. + assert "city IS NOT DISTINCT FROM CAST(:city AS text)" in sql_str + assert binds == { + "addr": "Екатеринбург, Y", + "city": "Екатеринбург", + "lat": 56.1, + "lon": 60.2, + } assert n == 7 +def test_update_deals_geocoded_null_city_group_uses_null_safe_predicate(): + """A NULL-city group must still update its rows: plain `city = NULL` is never + true, so `IS NOT DISTINCT FROM` is what keeps those rows reachable.""" + db = MagicMock() + db.execute.return_value = MagicMock(rowcount=2) + _update_deals_geocoded(db, address="Тестовая, 1", city=None, lat=56.1, lon=60.2) + args, _kw = db.execute.call_args + assert "city IS NOT DISTINCT FROM CAST(:city AS text)" in str(args[0]) + assert args[1]["city"] is None + + # --------------------------------------------------------------------------- # resume filter — candidate SQL carries lat IS NULL + staleness predicate # --------------------------------------------------------------------------- @@ -333,23 +360,19 @@ def test_update_deals_geocoded_sets_lat_lon_tried_at_not_geom(): def test_select_pending_addresses_filters_null_and_stale(): """The candidate query must combine lat IS NULL with the staleness window and bind --stale-days, so already-tried-recently rows are excluded.""" - db, _, _ = _make_db_mock( - address_rows=[{"address": "Екатеринбург, Z", "deals_count": 3}] - ) + db, _, _ = _make_db_mock(address_rows=[{"address": "Екатеринбург, Z", "deals_count": 3}]) groups = _select_pending_addresses(db, limit=2000, stale_days=30) assert groups == [AddressGroup(address="Екатеринбург, Z", deals_count=3)] # Inspect the SQL + binds of the SELECT. - select_call = next( - c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0]) - ) + select_call = next(c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0])) sql_str = str(select_call[0][0]) binds = select_call[0][1] assert "lat IS NULL" in sql_str assert "geocode_tried_at IS NULL" in sql_str assert "make_interval(days => CAST(:stale_days AS int))" in sql_str - assert "GROUP BY address" in sql_str + assert "GROUP BY address, city" in sql_str # #2603 — pair, not address alone assert binds["stale_days"] == 30 assert binds["limit"] == 2000 @@ -357,9 +380,7 @@ def test_select_pending_addresses_filters_null_and_stale(): def test_select_pending_addresses_passes_custom_stale_days(): db, _, _ = _make_db_mock(address_rows=[]) _select_pending_addresses(db, limit=500, stale_days=7) - select_call = next( - c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0]) - ) + select_call = next(c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0])) binds = select_call[0][1] assert binds["stale_days"] == 7 assert binds["limit"] == 500 @@ -419,9 +440,7 @@ async def test_main_respects_limit_bind(): ): await main(["--limit", "50"]) - select_call = next( - c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0]) - ) + select_call = next(c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0])) assert select_call[0][1]["limit"] == 50 @@ -440,6 +459,117 @@ async def test_main_no_pending_returns_zero(): db.close.assert_called_once() +# --------------------------------------------------------------------------- +# #2603 — (address, city) pair grouping + filtered city_hint +# --------------------------------------------------------------------------- + + +def test_select_pending_addresses_returns_city_in_group(): + """The candidate SELECT reads `city` and carries it into the group.""" + db, _, _ = _make_db_mock( + address_rows=[ + {"address": "Нижний Тагил, Победы", "city": "Нижний Тагил", "deals_count": 3}, + {"address": "Тестовая, 1", "city": None, "deals_count": 1}, + ] + ) + + groups = _select_pending_addresses(db, limit=2000, stale_days=30) + + assert groups == [ + AddressGroup(address="Нижний Тагил, Победы", deals_count=3, city="Нижний Тагил"), + AddressGroup(address="Тестовая, 1", deals_count=1, city=None), + ] + sql_str = str(next(c for c in db.execute.call_args_list if "FROM deals" in str(c[0][0]))[0][0]) + assert "SELECT address, city, deals_count" in sql_str + + +def test_city_hint_keeps_known_oblast_city(): + """A recognised oblast-66 city passes through as the hint (case/spacing-insensitive).""" + assert _city_hint("Нижний Тагил") == "Нижний Тагил" + assert _city_hint("екатеринбург") == "екатеринбург" + assert _city_hint(" Каменск-Уральский ") == " Каменск-Уральский " # raw value kept + + +def test_city_hint_drops_junk_rosreestr_value(): + """Rosreestr tail values are NOT cities — a junk hint is worse than none. + + Any non-EKB hint closes the geocoder's EKB-only local tiers and is prefixed + into the provider query, so 'Бессонова' would actively degrade the result. + """ + assert _city_hint("Бессонова") is None + assert _city_hint("Бердюгина") is None + assert _city_hint("Билейский рыбопитомник") is None + assert _city_hint(None) is None + assert _city_hint("") is None + + +async def test_run_backfill_passes_known_city_as_hint(): + groups = [AddressGroup(address="Победы, 30", deals_count=2, city="Нижний Тагил")] + db, coord_updates, _ = _make_db_mock(update_rowcount=2) + + with patch( + _GEOCODE_PATH, new_callable=AsyncMock, return_value=_result("nominatim") + ) as mock_geo: + await _run_backfill(db, groups, batch="b", dry_run=False) + + mock_geo.assert_called_once_with("Победы, 30", db, city_hint="Нижний Тагил") + assert coord_updates[0]["city"] == "Нижний Тагил" + + +async def test_run_backfill_junk_city_geocodes_without_hint_but_scopes_update(): + """Junk city → geocode WITHOUT a hint (old behaviour), yet the UPDATE is still + scoped to that exact group so it can't spill onto another city's rows.""" + groups = [AddressGroup(address="Бессонова, 11", deals_count=1, city="Бессонова")] + db, coord_updates, _ = _make_db_mock(update_rowcount=1) + + with patch( + _GEOCODE_PATH, new_callable=AsyncMock, return_value=_result("nominatim") + ) as mock_geo: + await _run_backfill(db, groups, batch="b", dry_run=False) + + mock_geo.assert_called_once_with("Бессонова, 11", db, city_hint=None) + assert coord_updates[0]["city"] == "Бессонова" + + +async def test_run_backfill_same_address_two_cities_independent(): + """Key #2603 scenario: identical address text in two cities → two geocode + calls with their own hints, and two UPDATEs scoped to their own city.""" + groups = [ + AddressGroup(address="Победы, 30", deals_count=4, city="Екатеринбург"), + AddressGroup(address="Победы, 30", deals_count=2, city="Нижний Тагил"), + ] + db, coord_updates, _ = _make_db_mock(update_rowcount=1) + + with patch( + _GEOCODE_PATH, + new_callable=AsyncMock, + side_effect=[_result("nominatim"), _result("nominatim", lat=57.910, lon=59.985)], + ) as mock_geo: + stats = await _run_backfill(db, groups, batch="b", dry_run=False) + + assert mock_geo.call_count == 2 + assert [c.kwargs["city_hint"] for c in mock_geo.call_args_list] == [ + "Екатеринбург", + "Нижний Тагил", + ] + assert stats.geocoded == 2 + assert [(u["addr"], u["city"]) for u in coord_updates] == [ + ("Победы, 30", "Екатеринбург"), + ("Победы, 30", "Нижний Тагил"), + ] + + +async def test_run_backfill_miss_marks_only_its_own_city(): + groups = [AddressGroup(address="Победы, 30", deals_count=1, city="Нижний Тагил")] + db, coord_updates, tried_updates = _make_db_mock() + + with patch(_GEOCODE_PATH, new_callable=AsyncMock, return_value=None): + await _run_backfill(db, groups, batch="b", dry_run=False) + + assert coord_updates == [] + assert tried_updates == [{"addr": "Победы, 30", "city": "Нижний Тагил"}] + + # --------------------------------------------------------------------------- # Stats dataclass # --------------------------------------------------------------------------- diff --git a/tradein-mvp/backend/tests/tasks/test_backfill_listings_coords_geoportal.py b/tradein-mvp/backend/tests/tasks/test_backfill_listings_coords_geoportal.py index 709ab7e0..bcfafebc 100644 --- a/tradein-mvp/backend/tests/tasks/test_backfill_listings_coords_geoportal.py +++ b/tradein-mvp/backend/tests/tasks/test_backfill_listings_coords_geoportal.py @@ -359,6 +359,51 @@ def test_city_column_null_falls_back_to_text_gate() -> None: mock_geo.assert_not_called() +# ── наблюдаемость гейта: отдельный срез по колонке (#2603) ─────────────────── + + +def test_skipped_non_ekb_by_column_counts_only_column_gate() -> None: + """Колоночный и текстовый гейты различимы: by_column считает ТОЛЬКО первый. + + Общий skipped_non_ekb остаётся суммой обоих (обратная совместимость), но по + мере раскатки областных развёрток (#2598) колоночный гейт начнёт забирать + строки из no_address — без отдельного среза общий счётчик поменял бы смысл + ровно тогда, когда по нему валидируют раскатку. + """ + rows = [ + # 1. Гейт по колонке: город в колонке, в тексте адреса города НЕТ. + {"id": 300, "address": "ул. Победы, 30", "city": "Нижний Тагил"}, + # 2. Текстовый гейт: колонка пуста, город назван в тексте. + {"id": 301, "address": "г. Нижний Тагил, проспект Ленина, 1", "city": None}, + ] + db = _make_db([rows, []]) + + with ( + patch( + "app.tasks.backfill_listings_coords_geoportal._geoportal_house_match", + return_value=_HIT, + ), + patch("app.tasks.backfill_listings_coords_geoportal._parse_street_house"), + ): + res = backfill_coords_from_geoportal(db, batch_size=500) + + assert res.candidates == 2 + assert res.skipped_non_ekb == 2 # оба гейта, как и раньше + assert res.skipped_non_ekb_by_column == 1 # только колоночный + # Вклад текстового гейта вычисляется разностью — старая метрика #2583 жива. + assert res.skipped_non_ekb - res.skipped_non_ekb_by_column == 1 + + +def test_to_counters_exposes_skipped_non_ekb_by_column() -> None: + """Новый счётчик уходит в scrape_runs.counters, старый ключ на месте.""" + counters = BackfillCoordsResult(skipped_non_ekb=7, skipped_non_ekb_by_column=5).to_counters() + + assert counters["skipped_non_ekb"] == 7 + assert counters["skipped_non_ekb_by_column"] == 5 + # Дефолт нулевой — прогон без колоночных скипов пишет 0, а не теряет ключ. + assert BackfillCoordsResult().to_counters()["skipped_non_ekb_by_column"] == 0 + + # ── idempotency ─────────────────────────────────────────────────────────────── diff --git a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py index c14cd8e1..d1e24921 100644 --- a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py +++ b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py @@ -761,11 +761,17 @@ def test_admin_geocode_missing_post_dry_run_endpoint_exists() -> None: @pytest.mark.asyncio -async def test_admin_geocode_missing_passes_city_hint() -> None: +@pytest.mark.parametrize("target", ["listings", "deals"]) +async def test_admin_geocode_missing_passes_city_hint(target: str) -> None: """POST /admin/geocode-missing читает city из SELECT и передаёт как city_hint. Раньше endpoint читал только row["address"] и звал geocode(clean, db) без города — голый тагильский адрес без города в тексте уходил в Екатеринбург. + + Оба target'а (#2603): колонка city есть и в listings (миграция 196), и в + deals (миграция 177) — SELECT + прокидывание хинта общие для обеих веток, + расходится только extra_filter, поэтому deals обязан покрываться тем же + контрактом. """ from app.api.v1 import admin as admin_module @@ -792,11 +798,17 @@ async def test_admin_geocode_missing_passes_city_hint() -> None: new_callable=AsyncMock, return_value=geo, ) as mock_geo: - result = await admin_module.geocode_missing(db, limit=100, target="listings") + result = await admin_module.geocode_missing( + db, + limit=100, + target=target, # type: ignore[arg-type] + ) mock_geo.assert_called_once_with("ул. Победы, 30", db, city_hint="Нижний Тагил") assert result["geocoded"] == 1 assert result["skipped"] == 0 + # SELECT адресован именно запрошенной таблице (обе несут колонку city). + assert f"FROM {target}" in str(db.execute.call_args_list[0][0][0]) @pytest.mark.asyncio -- 2.45.3 From 5ecd5361fdee0a0ff6fb202f20ef39db002afc26 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Wed, 5 Aug 2026 20:20:41 +0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(tradein/geocode):=20=D0=B3=D0=B5=D0=B9?= =?UTF-8?q?=D1=82=20=D0=BC=D1=83=D1=81=D0=BE=D1=80=D0=BD=D0=BE=D0=B3=D0=BE?= =?UTF-8?q?=20=D0=B3=D0=BE=D1=80=D0=BE=D0=B4=D0=B0=20=D0=B2=D1=8B=D0=BD?= =?UTF-8?q?=D0=B5=D1=81=D1=82=D0=B8=20=D0=B2=20=D0=BE=D0=B1=D1=89=D0=B8?= =?UTF-8?q?=D0=B9=20=D1=85=D0=B5=D0=BB=D0=BF=D0=B5=D1=80=20=D0=B8=20=D0=BF?= =?UTF-8?q?=D1=80=D0=BE=D1=88=D0=B8=D1=82=D1=8C=20=D0=B2=20admin-=D0=BF?= =?UTF-8?q?=D1=83=D1=82=D1=8C=20(#2603)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Первый коммит починил только scripts/geocode_deals_nominatim.py — ручной скрипт. Тот же дефект оставался на живом пути: POST /admin/geocode-missing?target=deals отдавал сырой row["city"] в city_hint, а deals.city росреестровое и в хвосте распределения содержит не-города («Бессонова», «Билейский рыбопитомник»). Любой не-ЕКБ хинт жёстко закрывает EKB-локальные тиры и уезжает префиксом в запрос провайдеру, то есть мусорный хинт хуже отсутствия хинта. Гейт вынесен в geocoder.known_city_hint (сверка с SVERDLOVSK_OBLAST_CITIES — тем же набором, который уже питает _names_non_ekb_city / _ekb_local_tiers_allowed) и переиспользуется всеми тремя потребителями city_hint: скриптом, admin-ручкой и задачей geocode_missing. Копий функции нет — четвёртый потребитель, если появится, получит гейт сам. Тесты: мусорный город -> хинт не передаётся, валидный -> передаётся; проверено фальсификацией (без фикса все три новых теста краснеют). --- tradein-mvp/backend/app/api/v1/admin.py | 11 ++- tradein-mvp/backend/app/services/geocoder.py | 35 +++++++++ .../backend/app/tasks/geocode_missing.py | 14 +++- .../scripts/geocode_deals_nominatim.py | 40 +++-------- .../scripts/test_geocode_deals_nominatim.py | 28 ++------ .../tests/tasks/test_geocode_missing.py | 72 +++++++++++++++++++ .../backend/tests/test_geocoder_city_hint.py | 28 ++++++++ 7 files changed, 170 insertions(+), 58 deletions(-) diff --git a/tradein-mvp/backend/app/api/v1/admin.py b/tradein-mvp/backend/app/api/v1/admin.py index 1e5add94..c564ed2b 100644 --- a/tradein-mvp/backend/app/api/v1/admin.py +++ b/tradein-mvp/backend/app/api/v1/admin.py @@ -72,7 +72,7 @@ from app.services import cian_session as cian_session_svc from app.services import domclick_session as domclick_session_svc from app.services import proxy_rotation as proxy_rotation_svc from app.services import scrape_runs as runs_mod -from app.services.geocoder import geocode +from app.services.geocoder import geocode, known_city_hint from app.services.scheduler import has_running_run from app.services.scraper_adapters import ( RealEnrichmentJobs, @@ -318,7 +318,14 @@ async def geocode_missing( # развёртки/импорта. Прокидываем как city_hint, а не полагаемся на то, что # геокодер угадает город по тексту address (голый "ул. Победы, 30" без # города в тексте иначе уходит в Екатеринбург). - city = row.get("city") + # + # known_city_hint (#2603) — гейт по словарю городов области: при + # target="deals" сюда приходит росреестровое поле, в хвосте которого + # лежат не-города («Бессонова», «Билейский рыбопитомник»), а мусорный + # хинт закрывает EKB-локальные тиры и уезжает префиксом в запрос + # провайдеру, т.е. вреднее отсутствия хинта. Общий хелпер, тот же, что у + # scripts/geocode_deals_nominatim.py и tasks/geocode_missing.py. + city = known_city_hint(row.get("city")) result = await geocode(clean, db, city_hint=city) if result is None: # Помечаем что пробовали — иначе ретрай на каждом cron. diff --git a/tradein-mvp/backend/app/services/geocoder.py b/tradein-mvp/backend/app/services/geocoder.py index d3c9eb55..f3cdb12e 100644 --- a/tradein-mvp/backend/app/services/geocoder.py +++ b/tradein-mvp/backend/app/services/geocoder.py @@ -156,6 +156,41 @@ SVERDLOVSK_OBLAST_CITIES = frozenset( # без district-префикса ложно ушёл бы в non-EKB gate. } ) + + +def known_city_hint(value: str | None) -> str | None: + """`value` как city_hint, если это узнаваемое имя города региона 66, иначе None. + + Для callers, которые берут город из КОЛОНКИ БД и передают его в `geocode()` + (#2603): `deals.city` — росреестровое поле, заполнено на 100%, но в хвосте + распределения лежит мусор («Бессонова», «Бердюгина», «Билейский + рыбопитомник» — улицы/урочища, попавшие в поле города). Мусорный хинт хуже + отсутствия хинта: любой не-ЕКБ `city_hint` жёстко закрывает EKB-локальные + тиры (`_ekb_local_tiers_allowed`) И подставляется префиксом в запрос + провайдеру (`_resolve_city_for_geocode`) — «Бессонова, Бессонова 10» + провайдер не резолвит вовсе. + + Словарь — `SVERDLOVSK_OBLAST_CITIES`, тот же, на котором стоят городские + гейты самого геокодера. Отдельного списка сознательно НЕ заводим: город, + отсутствующий в этом наборе, и так обрабатывается геокодером как незнакомый + (`_names_non_ekb_city` его не увидит, `estimator._resolve_target_city` не + резолвит) — т.е. новый город области в любом случае добавляется СЮДА, и + гейт хинта не создаёт новой связности. + + Цена решения (осознанная): легитимный, но не перечисленный населённый пункт + («Реж», «Арамиль», сёла/посёлки) хинта не получит и вернётся к поведению «по + тексту адреса» — то же, что было до прошивки хинта, без регрессии. + + Пользовательский ввод (`/geocode/lookup`, `/geocode/suggest`, + `TradeInEstimateInput.city_hint`) сюда НЕ заворачиваем: там город назвал + человек, и молча его игнорировать нельзя — для произвольной строки + fail-closed отрабатывает `_ekb_local_tiers_allowed` (#2580/#2589). + """ + if not value: + return None + return value if " ".join(value.lower().split()) in SVERDLOVSK_OBLAST_CITIES else None + + # Значение для DaData-констрейнта `locations: [{"region": ...}]`. # ВАЖНО: DaData хранит имя региона БЕЗ типа — `region="Свердловская"`, # `region_type="обл"` (тип лежит в отдельных полях `region_type` / diff --git a/tradein-mvp/backend/app/tasks/geocode_missing.py b/tradein-mvp/backend/app/tasks/geocode_missing.py index 35823593..e93d0289 100644 --- a/tradein-mvp/backend/app/tasks/geocode_missing.py +++ b/tradein-mvp/backend/app/tasks/geocode_missing.py @@ -41,7 +41,7 @@ from sqlalchemy.orm import Session from app.services import scrape_runs as runs_mod from app.services.estimator import _geocode_is_coarse -from app.services.geocoder import geocode +from app.services.geocoder import geocode, known_city_hint logger = logging.getLogger(__name__) @@ -77,7 +77,8 @@ async def geocode_missing_listings( не попадут в выдачу пользователю) 2. Для каждой пары (address, city): - - geocode(address, db, city_hint=city) — auto-cache (hit или miss) + - geocode(address, db, city_hint=known_city_hint(city)) — auto-cache + (hit или miss); хинт гейтится словарём городов области (#2603) - Если есть результат: UPDATE listings SET lat, lon WHERE address = :addr AND city IS NOT DISTINCT FROM :city AND lat IS NULL (IS NOT DISTINCT FROM, а не `=` — стандартная SQL NULL-семантика: `city = NULL` @@ -160,7 +161,14 @@ async def geocode_missing_listings( result.addresses_processed += 1 try: - geo = await geocode(address, db, city_hint=city) + # known_city_hint (#2603) — общий гейт по словарю городов области для + # всех DB-колоночных callers. Для listings.city он сегодня no-op + # (скрапер пишет только шесть кураторских имён из + # scraper_kit CITY_DISPLAY_NAMES, все они есть в словаре), но держит + # инвариант единым с deals-путями, где колонка росреестровая и в + # хвосте лежит мусор. Сырой `city` ниже остаётся ключом группы для + # UPDATE — гейт влияет только на подсказку геокодеру. + geo = await geocode(address, db, city_hint=known_city_hint(city)) except Exception as exc: logger.warning("geocode_missing: geocode raised for '%s': %s", address[:60], exc) result.addresses_failed += 1 diff --git a/tradein-mvp/backend/scripts/geocode_deals_nominatim.py b/tradein-mvp/backend/scripts/geocode_deals_nominatim.py index 99e981cd..c6876d83 100644 --- a/tradein-mvp/backend/scripts/geocode_deals_nominatim.py +++ b/tradein-mvp/backend/scripts/geocode_deals_nominatim.py @@ -40,8 +40,9 @@ A non-EKB hint is a HARD signal inside the geocoder: it closes the EKB-only local tiers (`_ekb_local_tiers_allowed`) and gets prefixed into the provider query, so a junk hint makes the result strictly WORSE than no hint at all. We therefore pass the hint only for values that are recognised cities of oblast -66 (`_city_hint` below); everything else degrades to the previous behaviour -(no hint, address text only). +66 — `geocoder.known_city_hint`, shared with the other DB-column callers +(`app/api/v1/admin.py`, `app/tasks/geocode_missing.py`); everything else +degrades to the previous behaviour (no hint, address text only). Street-level precision is accepted: the estimator's comparable search uses a 1000-2000 m radius, so a street-level point lands every deal on that street in @@ -105,15 +106,15 @@ except ImportError: # pragma: no cover — fallback for adhoc invocation try: from app.services.geocoder import ( # type: ignore[import-not-found] - SVERDLOVSK_OBLAST_CITIES, GeocodeResult, geocode, + known_city_hint, ) except ImportError: # pragma: no cover import sys sys.path.insert(0, str(Path(__file__).resolve().parents[1])) - from app.services.geocoder import SVERDLOVSK_OBLAST_CITIES, GeocodeResult, geocode + from app.services.geocoder import GeocodeResult, geocode, known_city_hint logging.basicConfig( level=logging.INFO, @@ -144,8 +145,8 @@ class AddressGroup: """One distinct (deals.address, deals.city) pair + its lat-IS-NULL row count. `city` is the raw Rosreestr value (may be NULL for non-rosreestr sources, and - may be junk — see `_city_hint`), kept verbatim so the UPDATE can target - exactly the rows this group came from. + may be junk — see `geocoder.known_city_hint`), kept verbatim so the UPDATE + can target exactly the rows this group came from. """ address: str @@ -153,26 +154,6 @@ class AddressGroup: city: str | None = None -def _city_hint(city: str | None) -> str | None: - """`city` if it is a recognised oblast-66 city name, else None (#2603). - - Dictionary = `geocoder.SVERDLOVSK_OBLAST_CITIES` — the single source of truth - already used by the geocoder's own city gates (`_names_non_ekb_city`, - `_ekb_local_tiers_allowed`) and by `estimator._resolve_target_city`. No local - list: a second one would drift from the gates the hint actually feeds. - - Filtering is not optional politeness: `deals.city` is Rosreestr-sourced and - its tail holds non-cities ('Бессонова', 'Бердюгина', 'Билейский - рыбопитомник'). Any non-EKB hint closes the EKB-only local tiers and is - prefixed into the provider query, so an unrecognised value would make the - geocode worse than passing nothing. - """ - if not city: - return None - normalized = " ".join(city.lower().split()) - return city if normalized in SVERDLOVSK_OBLAST_CITIES else None - - @dataclass class Stats: """Final-summary counters. @@ -383,9 +364,10 @@ async def _run_backfill( address = group.address city = group.city # Only a recognised oblast-66 city is fed to the geocoder; junk Rosreestr - # values degrade to None (see `_city_hint`). The raw `city` is still used - # for the UPDATE scope — it identifies the group either way. - hint = _city_hint(city) + # values degrade to None (geocoder.known_city_hint — shared with the + # other DB-column callers). The raw `city` is still used for the UPDATE + # scope — it identifies the group either way. + hint = known_city_hint(city) # The geocoder itself rejects <3 chars, but skip here too so the dry-run # report and counters stay honest (no phantom "processed" address). diff --git a/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py b/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py index dc47e02a..3e656a98 100644 --- a/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py +++ b/tradein-mvp/backend/tests/scripts/test_geocode_deals_nominatim.py @@ -12,9 +12,10 @@ Coverage (per the issue's test plan): batch. - dedup: distinct addresses drive geocode call count (one call per address). - main() wiring: SessionLocal, --limit bind, returns geocoded count. - - #2603: grouping by the (address, city) PAIR, city_hint filtered against - geocoder.SVERDLOVSK_OBLAST_CITIES (junk Rosreestr city → no hint), and - writes scoped with `city IS NOT DISTINCT FROM`. + - #2603: grouping by the (address, city) PAIR, city_hint filtered through + the shared `geocoder.known_city_hint` (junk Rosreestr city → no hint; the + helper's own cases live in tests/test_geocoder_city_hint.py), and writes + scoped with `city IS NOT DISTINCT FROM`. No real Postgres. `geocode()` is async → patched with AsyncMock (mirrors tests/tasks/test_geocode_missing.py). The Session is a MagicMock that routes @@ -34,7 +35,6 @@ from app.services.geocoder import GeocodeResult from scripts.geocode_deals_nominatim import ( AddressGroup, Stats, - _city_hint, _mark_deals_tried, _run_backfill, _select_pending_addresses, @@ -483,26 +483,6 @@ def test_select_pending_addresses_returns_city_in_group(): assert "SELECT address, city, deals_count" in sql_str -def test_city_hint_keeps_known_oblast_city(): - """A recognised oblast-66 city passes through as the hint (case/spacing-insensitive).""" - assert _city_hint("Нижний Тагил") == "Нижний Тагил" - assert _city_hint("екатеринбург") == "екатеринбург" - assert _city_hint(" Каменск-Уральский ") == " Каменск-Уральский " # raw value kept - - -def test_city_hint_drops_junk_rosreestr_value(): - """Rosreestr tail values are NOT cities — a junk hint is worse than none. - - Any non-EKB hint closes the geocoder's EKB-only local tiers and is prefixed - into the provider query, so 'Бессонова' would actively degrade the result. - """ - assert _city_hint("Бессонова") is None - assert _city_hint("Бердюгина") is None - assert _city_hint("Билейский рыбопитомник") is None - assert _city_hint(None) is None - assert _city_hint("") is None - - async def test_run_backfill_passes_known_city_as_hint(): groups = [AddressGroup(address="Победы, 30", deals_count=2, city="Нижний Тагил")] db, coord_updates, _ = _make_db_mock(update_rowcount=2) diff --git a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py index d1e24921..0e786ffb 100644 --- a/tradein-mvp/backend/tests/tasks/test_geocode_missing.py +++ b/tradein-mvp/backend/tests/tasks/test_geocode_missing.py @@ -811,6 +811,78 @@ async def test_admin_geocode_missing_passes_city_hint(target: str) -> None: assert f"FROM {target}" in str(db.execute.call_args_list[0][0][0]) +@pytest.mark.asyncio +@pytest.mark.parametrize("target", ["listings", "deals"]) +async def test_admin_geocode_missing_drops_junk_city_hint(target: str) -> None: + """Мусорный город из колонки НЕ уходит в city_hint (#2603). + + `deals.city` росреестровое: в хвосте «Бессонова», «Билейский рыбопитомник» — + улицы/урочища, а не города. Хинт из такого значения закрывает EKB-локальные + тиры и уезжает префиксом в запрос провайдеру, т.е. хуже отсутствия хинта. + Гейт общий с двумя другими DB-колоночными callers (geocoder.known_city_hint). + """ + from app.api.v1 import admin as admin_module + + rows = [{"id": 56, "address": "ул. Бессонова, 11", "city": "Бессонова"}] + + db = MagicMock() + select_result = MagicMock() + select_result.mappings.return_value.all.return_value = rows + update_result = MagicMock() + remaining_result = MagicMock() + remaining_result.scalar.return_value = 0 + db.execute.side_effect = [select_result, update_result, remaining_result] + + geo = GeocodeResult( + lat=56.838, + lon=60.605, + full_address="Екатеринбург, ул. Бессонова, 11", + provider="nominatim", # type: ignore[arg-type] + confidence="exact", + ) + + with patch( + "app.api.v1.admin.geocode", + new_callable=AsyncMock, + return_value=geo, + ) as mock_geo: + await admin_module.geocode_missing( + db, + limit=100, + target=target, # type: ignore[arg-type] + ) + + mock_geo.assert_called_once_with("ул. Бессонова, 11", db, city_hint=None) + + +@pytest.mark.asyncio +async def test_geocode_missing_listings_drops_junk_city_hint() -> None: + """Тот же гейт в ночной задаче: мусорный city → city_hint=None (#2603). + + Для listings.city это сегодня no-op (скрапер пишет шесть кураторских имён), + но инвариант «в geocode() уходит только словарный город» держим единым для + всех трёх DB-колоночных callers, чтобы седьмой не пришлось чинить заново. + Сырой city при этом остаётся ключом группы для UPDATE. + """ + rows = [{"address": "ул. Бессонова, 11", "city": "Бессонова", "listings_count": 2}] + db = _mock_db_rows(rows) + + with patch( + "app.tasks.geocode_missing.geocode", + new_callable=AsyncMock, + return_value=_make_geocode_result(), + ) as mock_geo: + await geocode_missing_listings(db, batch_size=10) + + mock_geo.assert_called_once_with("ул. Бессонова, 11", db, city_hint=None) + update_binds = [ + c[0][1] + for c in db.execute.call_args_list + if "UPDATE listings" in str(c[0][0]) and "SET lat" in str(c[0][0]) + ] + assert update_binds[0]["city"] == "Бессонова" # группа UPDATE — по сырому городу + + @pytest.mark.asyncio async def test_admin_geocode_missing_select_includes_city_column() -> None: """SELECT в admin.geocode_missing содержит колонку city (#2594).""" diff --git a/tradein-mvp/backend/tests/test_geocoder_city_hint.py b/tradein-mvp/backend/tests/test_geocoder_city_hint.py index 0565aa0d..7daa2023 100644 --- a/tradein-mvp/backend/tests/test_geocoder_city_hint.py +++ b/tradein-mvp/backend/tests/test_geocoder_city_hint.py @@ -38,9 +38,37 @@ from app.services.geocoder import ( _nominatim_suggest, _resolve_city_for_geocode, geocode, + known_city_hint, suggest, ) +# ── known_city_hint (#2603) ────────────────────────────────────────────────── + + +@pytest.mark.parametrize( + "value,expected", + [ + # Известные города области — значение проходит КАК ЕСТЬ (регистр/пробелы + # нормализует сам геокодер, caller'ы не должны его причёсывать). + ("Екатеринбург", "Екатеринбург"), + ("Нижний Тагил", "Нижний Тагил"), + ("нижний тагил", "нижний тагил"), + ("Каменск-Уральский", "Каменск-Уральский"), + # Мусор из хвоста росреестрового deals.city — это улицы/урочища, а не + # города; хинт из них закрыл бы EKB-тиры и уехал префиксом в запрос. + ("Бессонова", None), + ("Бердюгина", None), + ("Билейский рыбопитомник", None), + # Пусто — хинта нет. + (None, None), + ("", None), + (" ", None), + ], +) +def test_known_city_hint(value: str | None, expected: str | None) -> None: + assert known_city_hint(value) == expected + + # ── _resolve_city_for_geocode ──────────────────────────────────────────────── -- 2.45.3