Compare commits

...
Sign in to create a new pull request.

1 commit

Author SHA1 Message Date
bot-backend
e042e9bbfd fix(tradein/estimator): scope year_built NULL cohort-bypass away from avito (#2013)
Some checks failed
CI Trade-In / changes (pull_request) Successful in 10s
CI / changes (pull_request) Successful in 10s
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / backend-tests (pull_request) Failing after 54s
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.
2026-07-12 15:22:32 +03:00
2 changed files with 97 additions and 2 deletions

View file

@ -4217,9 +4217,13 @@ _COMMON_WHERE = """
-- Tier W имеет свою копию этого блока в inline SQL. Если cohort_year_min IS NULL -- Tier W имеет свою копию этого блока в inline SQL. Если cohort_year_min IS NULL
-- фильтр прозрачен. CAST обязателен psycopg3 prepared statement не выводит -- фильтр прозрачен. CAST обязателен psycopg3 prepared statement не выводит
-- тип $N при IS NULL в predicate (см. PR #518 fix). -- тип $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 ( AND (
CAST(:cohort_year_min AS integer) IS NULL 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) OR year_built BETWEEN CAST(:cohort_year_min AS integer)
AND CAST(:cohort_year_max AS integer) AND CAST(:cohort_year_max AS integer)
) )
@ -4637,9 +4641,11 @@ def _fetch_analogs(
-- (хрущёвка vs новостройка). Если cohort_year_min IS NULL -- (хрущёвка vs новостройка). Если cohort_year_min IS NULL
-- фильтр прозрачен. CAST обязателен psycopg3 prepared statement -- фильтр прозрачен. CAST обязателен psycopg3 prepared statement
-- не выводит тип $N при IS NULL в predicate (см. PR #518 fix). -- не выводит тип $N при IS NULL в predicate (см. PR #518 fix).
-- #2013: сужаем year_built IS NULL escape — не должен пропускать
-- avito any-era comp мимо cohort фильтра (sync с _COMMON_WHERE).
AND ( AND (
CAST(:cohort_year_min AS integer) IS NULL 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) OR year_built BETWEEN CAST(:cohort_year_min AS integer)
AND CAST(:cohort_year_max AS integer) AND CAST(:cohort_year_max AS integer)
) )

View file

@ -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