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
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.
This commit is contained in:
parent
346767ba90
commit
e042e9bbfd
2 changed files with 97 additions and 2 deletions
|
|
@ -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)
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -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
|
||||||
Loading…
Add table
Reference in a new issue