From afa93f9a1e0aacc3cd7b5b2f340019adfd4ab7f9 Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sun, 6 Sep 2026 01:08:09 +0500 Subject: [PATCH] =?UTF-8?q?fix(tradein):=20=D0=BC=D0=B8=D0=B3=D1=80=D0=B0?= =?UTF-8?q?=D1=86=D0=B8=D1=8F=20285=20=D0=BD=D0=B5=20=D1=82=D1=80=D0=BE?= =?UTF-8?q?=D0=B3=D0=B0=D0=B5=D1=82=20=D1=81=D1=82=D1=80=D0=BE=D0=BA=D0=B8?= =?UTF-8?q?=20=D1=82=D1=80=D0=B8=D0=B3=D0=B3=D0=B5=D1=80=D0=B0=20(deep-rev?= =?UTF-8?q?iew=20=D0=B1=D0=BB=D0=BE=D0=BA=D0=B5=D1=80)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Утверждение «формула триггера = формула миграции» было ложным по ИСТОЧНИКУ базы: 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 не чиним. --- .../285_domklik_diff_percent_recompute.sql | 74 ++++++++++++++----- .../tests/scrapers/test_domclick_detail.py | 31 ++++++++ 2 files changed, 87 insertions(+), 18 deletions(-) diff --git a/tradein-mvp/backend/data/sql/285_domklik_diff_percent_recompute.sql b/tradein-mvp/backend/data/sql/285_domklik_diff_percent_recompute.sql index 2acaaf57..f00e2dd9 100644 --- a/tradein-mvp/backend/data/sql/285_domklik_diff_percent_recompute.sql +++ b/tradein-mvp/backend/data/sql/285_domklik_diff_percent_recompute.sql @@ -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 $$; diff --git a/tradein-mvp/backend/tests/scrapers/test_domclick_detail.py b/tradein-mvp/backend/tests/scrapers/test_domclick_detail.py index 1d3835dc..537bfc00 100644 --- a/tradein-mvp/backend/tests/scrapers/test_domclick_detail.py +++ b/tradein-mvp/backend/tests/scrapers/test_domclick_detail.py @@ -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