From e042e9bbfd7b4bf506e7c5212d1ea28de33eaf83 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sun, 12 Jul 2026 15:22:32 +0300 Subject: [PATCH] fix(tradein/estimator): scope year_built NULL cohort-bypass away from avito (#2013) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cohort/era WHERE clause carried `OR year_built IS NULL`, meant to let avito's frequently-missing year_built field through the filter. In practice this let ANY-era avito listing bypass the cohort filter entirely once cohort_year_min/ max were set — pollution bug: khrushchevka target could pull in a modern avito comp with no year parsed, and vice versa. Narrow the escape to `OR (year_built IS NULL AND source <> 'avito')` in both places that must stay in sync: - _COMMON_WHERE (Tier S/H, ~estimator.py:4210) - Tier W inline SQL copy (~estimator.py:4640) Non-avito NULL-year rows (cian/domklik/etc without a parsed year) still pass through untouched; avito NULL-year rows now must satisfy the same year_built BETWEEN cohort_year_min AND cohort_year_max test as everything else. Added tests/test_estimator_avito_cohort_null_2013.py: SQL-source assertions that both cohort blocks carry the new guard, stay in sync (exactly 2 occurrences), and that the old unscoped escape is gone. A full DB-backed integration test was impractical — this suite's estimator tests run against a dummy DATABASE_URL with no live DB. --- tradein-mvp/backend/app/services/estimator.py | 10 ++- .../test_estimator_avito_cohort_null_2013.py | 89 +++++++++++++++++++ 2 files changed, 97 insertions(+), 2 deletions(-) create mode 100644 tradein-mvp/backend/tests/test_estimator_avito_cohort_null_2013.py diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 827a4b37..d02724bd 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -4217,9 +4217,13 @@ _COMMON_WHERE = """ -- Tier W имеет свою копию этого блока в inline SQL. Если cohort_year_min IS NULL — -- фильтр прозрачен. CAST обязателен — psycopg3 prepared statement не выводит -- тип $N при IS NULL в predicate (см. PR #518 fix). + -- #2013: year_built IS NULL escape изначально был для avito (missing-year + -- listings), но пропускал ЛЮБОЙ NULL-year comp мимо cohort/era фильтра — + -- пуллюция any-era avito в comp-пул. Сужаем escape: только не-avito NULL + -- проходит прозрачно; avito NULL-year обязан пройти year_built BETWEEN. AND ( CAST(:cohort_year_min AS integer) IS NULL - OR year_built IS NULL + OR (year_built IS NULL AND source <> 'avito') OR year_built BETWEEN CAST(:cohort_year_min AS integer) AND CAST(:cohort_year_max AS integer) ) @@ -4637,9 +4641,11 @@ def _fetch_analogs( -- (хрущёвка vs новостройка). Если cohort_year_min IS NULL — -- фильтр прозрачен. CAST обязателен — psycopg3 prepared statement -- не выводит тип $N при IS NULL в predicate (см. PR #518 fix). + -- #2013: сужаем year_built IS NULL escape — не должен пропускать + -- avito any-era comp мимо cohort фильтра (sync с _COMMON_WHERE). AND ( CAST(:cohort_year_min AS integer) IS NULL - OR year_built IS NULL + OR (year_built IS NULL AND source <> 'avito') OR year_built BETWEEN CAST(:cohort_year_min AS integer) AND CAST(:cohort_year_max AS integer) ) diff --git a/tradein-mvp/backend/tests/test_estimator_avito_cohort_null_2013.py b/tradein-mvp/backend/tests/test_estimator_avito_cohort_null_2013.py new file mode 100644 index 00000000..c9f2dd64 --- /dev/null +++ b/tradein-mvp/backend/tests/test_estimator_avito_cohort_null_2013.py @@ -0,0 +1,89 @@ +"""Regression test for #2013 — avito NULL-year listings bypassing cohort filter. + +Bug: the cohort/era WHERE clause carried an `OR year_built IS NULL` escape +meant for avito's frequently-missing year_built field. In practice this let +ANY-era avito listing (not just genuinely unknown-year ones) into the comp +pool once cohort_year_min/max were set — pollution bug (#2013). + +Fix: narrow the escape to `OR (year_built IS NULL AND source <> 'avito')` so +avito NULL-year rows are held to the same year_built BETWEEN test as every +other source, while non-avito NULL-year rows (e.g. cian/domklik legacy data +without a parsed year) still pass through untouched. + +The clause exists in TWO places that must stay byte-identical in intent: + - `_COMMON_WHERE` (Tier S / Tier H, shared string constant) + - the Tier W inline SQL copy (own CTE, per code comments "must stay in sync") + +Wiring a real Postgres-backed integration test is impractical here (this +suite's estimator tests run against a dummy DATABASE_URL with no live DB — +see tests/test_estimator_pure_units.py / test_estimator_cohort.py). Instead +we assert directly on the SQL source: both cohort blocks must contain the new +guard, and the old unscoped `OR year_built IS NULL` escape must be gone from +both (guarding against a partial fix that only patches one copy). +""" + +import os + +# Settings requires DATABASE_URL at init time. Set dummy DSN before any app import. +os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test") + +from pathlib import Path + +from app.services import estimator + +ESTIMATOR_SRC = Path(estimator.__file__).read_text(encoding="utf-8") + +GUARD = "OR (year_built IS NULL AND source <> 'avito')" + + +def _cohort_block(anchor: str) -> str: + """Extract the `AND ( ... )` cohort-filter block that follows `anchor`. + + Balances parens from the first `AND (` after `anchor` so we grab exactly + the cohort predicate group, not neighbouring clauses. + """ + idx = ESTIMATOR_SRC.index(anchor) + tail = ESTIMATOR_SRC[idx:] + start = tail.index("AND (") + open_idx = tail.index("(", start) + depth = 0 + for i, ch in enumerate(tail[open_idx:], start=open_idx): + if ch == "(": + depth += 1 + elif ch == ")": + depth -= 1 + if depth == 0: + return tail[start : i + 1] + raise AssertionError(f"unbalanced parens extracting cohort block after {anchor!r}") + + +def test_common_where_cohort_block_has_avito_guard() -> None: + """Tier S/H (_COMMON_WHERE): NULL-year avito rows must not bypass cohort.""" + block = _cohort_block("_COMMON_WHERE = ") + assert "cohort_year_min" in block + assert "cohort_year_max" in block + assert GUARD in block + + +def test_tier_w_inline_cohort_block_has_avito_guard() -> None: + """Tier W inline copy: same guard, kept in sync with _COMMON_WHERE.""" + block = _cohort_block("-- Когортный фильтр (PR 9)") + assert "cohort_year_min" in block + assert "cohort_year_max" in block + assert GUARD in block + + +def test_avito_guard_present_in_exactly_both_copies() -> None: + """Exactly two occurrences: _COMMON_WHERE + Tier W inline, kept in sync.""" + assert ESTIMATOR_SRC.count(GUARD) == 2 + + +def test_old_unscoped_null_escape_is_fully_removed() -> None: + """No leftover `OR year_built IS NULL` (unscoped) predicate anywhere. + + Catches a partial fix that patches only one of the two WHERE copies. + The one remaining textual mention of the bare phrase is a prose comment + (ends in a period, not immediately followed by a newline like the SQL + predicate form did), so it does not match this pattern. + """ + assert "OR year_built IS NULL\n" not in ESTIMATOR_SRC