fix(tradein): миграция 285 не трогает строки триггера (deep-review блокер)
All checks were successful
CI Trade-In / changes (pull_request) Successful in 8s
CI Trade-In / browser-tests (pull_request) Has been skipped
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / changes (pull_request) Successful in 11s
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) Successful in 4m56s
All checks were successful
CI Trade-In / changes (pull_request) Successful in 8s
CI Trade-In / browser-tests (pull_request) Has been skipped
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI / changes (pull_request) Successful in 11s
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) Successful in 4m56s
Утверждение «формула триггера = формула миграции» было ложным по ИСТОЧНИКУ базы: record_listing_price_change (131:76-86) считает процент от listings.OLD.price_rub, а не от предыдущей строки offer_price_history. Пересчёт по lag() портил честные значения: у листинга, чья история начинается с триггерной строки, lag() = NULL → −0.82 уходил в NULL; у триггерной строки с соседом-строкой загрузчика база чужая. Теперь пересчитываем и берём как базу ТОЛЬКО строки загрузчика (change_time <> recorded_at — триггер ставит обе метки одним now()). Это ровно то, что делает починенный код: процент внутри истории самой карточки. Окно lag() сужено до листингов с domklik-строками — иначе оконная функция шла по всей таблице под lock_timeout = 5s и валила деплой. Признак закреплён тестом на INSERT загрузчика (recorded_at не указан → DEFAULT NOW()); при добавлении recorded_at в INSERT тест краснеет — проверено. В шапке миграции отмечена асимметрия: старые cian-строки с |x| > 100 не чиним.
This commit is contained in:
parent
769a36098b
commit
afa93f9a1e
2 changed files with 87 additions and 18 deletions
|
|
@ -11,21 +11,47 @@
|
|||
-- процент считается из соседних цен, неправдоподобное значение отвергается, а не
|
||||
-- зажимается). Эта миграция отрабатывает задним числом по уже собранным строкам.
|
||||
--
|
||||
-- ПОЧЕМУ ПЕРЕСЧИТЫВАЕМ ВСЕ DOMKLIK-СТРОКИ, А НЕ ТОЛЬКО ИСПОРЧЕННЫЕ
|
||||
-- Отличить строку загрузчика от строки триггера по данным нечем: триггер пишет
|
||||
-- source = NEW.source, то есть тоже 'domklik' (131_fix_diff_percent_overflow.sql).
|
||||
-- Косвенный признак есть — у триггерной строки change_time = recorded_at (обе now()),
|
||||
-- у загрузчика change_time историческая и много раньше recorded_at, — но опираться
|
||||
-- на него незачем: формула триггера («предыдущая цена → текущая, round 2») это ровно
|
||||
-- то, что считает оконная lag() ниже, поэтому у триггерных строк пересчёт даёт то же
|
||||
-- самое значение и UPDATE их не трогает (WHERE ... IS DISTINCT FROM). Фильтр по
|
||||
-- |x| > 100 тоже не берём: он пропустил бы испорченные строки с мелким рублёвым
|
||||
-- diff (например −50 рублей выглядит как правдоподобные −50%).
|
||||
-- ТРОГАЕМ ТОЛЬКО СТРОКИ ЗАГРУЗЧИКА: change_time <> recorded_at
|
||||
-- В таблице два писателя с РАЗНОЙ базой отсчёта, и смешивать их нельзя:
|
||||
-- • загрузчик (domclick/detail.py) считает процент внутри истории самой карточки —
|
||||
-- база это предыдущая запись priceHistory, то есть предыдущая строка загрузчика;
|
||||
-- • триггер record_listing_price_change (131_fix_diff_percent_overflow.sql:76-86)
|
||||
-- считает от listings.OLD.price_rub — от цены В КАРТОЧКЕ ЛИСТИНГА на момент
|
||||
-- upsert'а. Эта база в offer_price_history может вообще не лежать отдельной
|
||||
-- строкой.
|
||||
-- Поэтому раннее утверждение «формула триггера = формула миграции, пересчёт даст то
|
||||
-- же значение» ЛОЖНО, и опереться на признак происхождения как раз нужно. Без него
|
||||
-- миграция портит честные данные двумя способами: (1) у листинга, чья история
|
||||
-- начинается с триггерной строки, lag() = NULL → честный −0.82 перезаписывается в
|
||||
-- NULL; (2) у триггерной строки, соседом которой по lag() оказалась строка
|
||||
-- загрузчика, честный процент пересчитывается от чужой базы.
|
||||
-- Признак точный, а не эвристический: триггер подставляет now() и в change_time, и
|
||||
-- в recorded_at ОДНИМ INSERT'ом, а now() стабилен внутри транзакции → у триггерной
|
||||
-- строки метки равны побайтово. Загрузчик пишет в change_time дату источника, а
|
||||
-- recorded_at не указывает вовсе (DEFAULT NOW(), 023_offer_price_history.sql:19) →
|
||||
-- расхождение в месяцы. Совпадение исторической даты с моментом вставки с точностью
|
||||
-- до микросекунды недостижимо. Признак закреплён тестом
|
||||
-- test_save_detail_enrichment_leaves_recorded_at_to_default.
|
||||
--
|
||||
-- ОКНО СЧИТАЕМ ПО ВСЕЙ ИСТОРИИ ЛИСТИНГА, А ПИШЕМ ТОЛЬКО В DOMKLIK
|
||||
-- PARTITION BY listing_id без фильтра по source: «предыдущая цена» — это предыдущая
|
||||
-- запись листинга, кто бы её ни записал. Если сузить окно до source = 'domklik',
|
||||
-- у листинга со смешанными источниками соседом станет не та строка.
|
||||
-- ОКНО lag() — ТОЖЕ ТОЛЬКО ПО СТРОКАМ ЗАГРУЗЧИКА
|
||||
-- Триггерные строки не переписываем И не используем как базу. Иначе строка
|
||||
-- загрузчика получила бы базой триггерную строку, которой в истории карточки нет, —
|
||||
-- и результат разошёлся бы с тем, что теперь пишет починенный код. Задача миграции
|
||||
-- ровно в том, чтобы задним числом дать те же значения, что даёт код: prev — это
|
||||
-- предыдущая запись priceHistory карточки. По source окно не сужаем: «предыдущая
|
||||
-- цена» — это предыдущая запись листинга, а у листинга со смешанными источниками
|
||||
-- фильтр по source подсунул бы не ту строку.
|
||||
--
|
||||
-- ФИЛЬТР ПО |x| > 100 НЕ БЕРЁМ
|
||||
-- Он пропустил бы испорченные строки с мелким рублёвым diff (−50 рублей выглядит
|
||||
-- как правдоподобные −50%).
|
||||
--
|
||||
-- АСИММЕТРИЯ: СТАРЫЕ CIAN-СТРОКИ НЕ ЧИНИМ
|
||||
-- Гейт validate_diff_percent общий для всех источников (|x| > 100 → NULL + warning),
|
||||
-- а миграция — только про domklik. Уже лежащие в таблице cian-строки с |x| > 100
|
||||
-- остаются как есть: их история приходит из cian_price_history.py со своим полем и
|
||||
-- своей базой, отдельным замером не подтверждена, а править вслепую по чужому
|
||||
-- источнику — это второй #3225, а не его починка. Отдельная задача.
|
||||
--
|
||||
-- САМАЯ РАННЯЯ ЗАПИСЬ ЛИСТИНГА → NULL, НЕ 0
|
||||
-- Предыдущей цены нет — процента не существует. Ноль здесь читался бы как «цена не
|
||||
|
|
@ -52,18 +78,29 @@ BEGIN
|
|||
count(*) FILTER (WHERE abs(diff_percent) > 100)
|
||||
INTO before_notnull, before_bad
|
||||
FROM offer_price_history
|
||||
WHERE source = 'domklik';
|
||||
WHERE source = 'domklik'
|
||||
AND change_time <> recorded_at;
|
||||
|
||||
RAISE NOTICE 'domklik ДО: diff_percent непустых = %, из них |x| > 100 = %',
|
||||
RAISE NOTICE 'domklik (строки загрузчика) ДО: diff_percent непустых = %, из них |x| > 100 = %',
|
||||
before_notnull, before_bad;
|
||||
|
||||
WITH neighbours AS (
|
||||
-- Окно только по листингам, у которых есть domklik-строки: без этого сужения
|
||||
-- lag() прогоняется по ВСЕЙ таблице, и под lock_timeout = 5s деплой падает.
|
||||
-- Оба скана идут по индексам 023: oph_source_time_idx (source, change_time)
|
||||
-- для подзапроса и oph_listing_time_idx (listing_id, change_time) для окна.
|
||||
SELECT id,
|
||||
source,
|
||||
price_rub,
|
||||
lag(price_rub) OVER (PARTITION BY listing_id ORDER BY change_time, id)
|
||||
AS prev_price
|
||||
FROM offer_price_history
|
||||
WHERE listing_id IN (
|
||||
SELECT DISTINCT listing_id
|
||||
FROM offer_price_history
|
||||
WHERE source = 'domklik'
|
||||
)
|
||||
AND change_time <> recorded_at
|
||||
),
|
||||
recomputed AS (
|
||||
SELECT id,
|
||||
|
|
@ -85,9 +122,10 @@ BEGIN
|
|||
count(*) FILTER (WHERE abs(diff_percent) > 100)
|
||||
INTO after_notnull, after_bad
|
||||
FROM offer_price_history
|
||||
WHERE source = 'domklik';
|
||||
WHERE source = 'domklik'
|
||||
AND change_time <> recorded_at;
|
||||
|
||||
RAISE NOTICE 'domklik ПОСЛЕ: diff_percent непустых = % (было %), |x| > 100 = % (было %), переписано строк = %',
|
||||
RAISE NOTICE 'domklik (строки загрузчика) ПОСЛЕ: diff_percent непустых = % (было %), |x| > 100 = % (было %), переписано строк = %',
|
||||
after_notnull, before_notnull, after_bad, before_bad, touched;
|
||||
END $$;
|
||||
|
||||
|
|
|
|||
|
|
@ -578,6 +578,37 @@ def test_save_detail_enrichment_writes_columns() -> None:
|
|||
db.commit.assert_called_once()
|
||||
|
||||
|
||||
def test_save_detail_enrichment_leaves_recorded_at_to_default() -> None:
|
||||
"""Строка загрузчика обязана иметь change_time <> recorded_at.
|
||||
|
||||
По этому признаку миграция 285 отличает строки загрузчика от строк триггера
|
||||
record_listing_price_change (тот пишет обе метки одним now(), то есть равными) и
|
||||
пересчитывает diff_percent только у первых. У триггера ДРУГАЯ база отсчёта —
|
||||
listings.OLD.price_rub, — поэтому пересчёт его строк ломает честные значения.
|
||||
Признак держится на двух вещах: change_time = дата источника (глубоко в прошлом),
|
||||
а recorded_at загрузчик не указывает вовсе → DEFAULT NOW() на вставке.
|
||||
Пропишут recorded_at в этот INSERT — признак сломается и тест покраснеет.
|
||||
"""
|
||||
db = MagicMock()
|
||||
db.execute.return_value.rowcount = 1
|
||||
ct = datetime(2026, 4, 1, tzinfo=UTC)
|
||||
e = DomClickDetailEnrichment(
|
||||
item_id="2075729321",
|
||||
source_url=_CARD_URL,
|
||||
price_changes=[{"change_time": ct, "price_rub": 5000000, "diff_percent": -2.5}],
|
||||
)
|
||||
|
||||
assert save_detail_enrichment(db, 999, e) is True
|
||||
|
||||
insert_calls = [c for c in db.execute.call_args_list if "offer_price_history" in str(c[0][0])]
|
||||
assert len(insert_calls) == 1
|
||||
insert_sql = str(insert_calls[0][0][0])
|
||||
assert "recorded_at" not in insert_sql
|
||||
# change_time — историческая дата источника, а не момент вставки.
|
||||
assert insert_calls[0][0][1]["ct"] == ct
|
||||
assert ct < datetime.now(UTC)
|
||||
|
||||
|
||||
def test_save_detail_enrichment_listing_not_found() -> None:
|
||||
db = MagicMock()
|
||||
db.execute.return_value.rowcount = 0
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue