From 872c83caf5a588f6fd92c89132f4f34ff640ab13 Mon Sep 17 00:00:00 2001 From: lekss361 Date: Sun, 24 May 2026 14:01:02 +0300 Subject: [PATCH] fix(tradein-estimator): atomic IMV link, batched yandex history, drop redundant CAST MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3 fixes inside services/estimator.py from 2026-05-24 audit: * IMV link UPDATE moved inside main tx (finding #4) — closes race where two concurrent estimates sharing a cache_key could overwrite each other's estimate_id after the main commit. * _save_yandex_history_items batched under single try/except (finding #5) — prior per-item db.rollback() destroyed the parent transaction; next iteration could run on a rolled-back session. * Drop redundant CAST inside IS NOT NULL predicates in _fetch_analogs (finding #9) — CAST(NULL AS T) IS NOT NULL is equivalent to NULL IS NOT NULL but evaluates per-row. THEN-branch CASTs preserved (typed arithmetic). test: update test_save_history_items_db_error_continues → batch rollback semantics --- tradein-mvp/backend/app/services/estimator.py | 136 ++++++++++-------- .../test_estimator_yandex_integration.py | 8 +- 2 files changed, 78 insertions(+), 66 deletions(-) diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 0e6aad9a..fa565c91 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -343,8 +343,15 @@ def _save_yandex_history_items( Idempotent via UNIQUE (source, ext_item_id); we synthesize ext_item_id from (address|date|area|floor) hash since Yandex history items don't carry an explicit ID. + + Batch semantics (closes finding #5 from 2026-05-24 audit): all items + inserted under a single try/except; on any failure the whole batch is + rolled back as a unit (prior per-item rollback() destroyed the parent tx). """ - saved = 0 + if not result.history_items: + return 0 + + rows = [] for item in result.history_items: # Synthesize stable ext_item_id (no native ID in valuation page) ext_seed = ( @@ -352,48 +359,51 @@ def _save_yandex_history_items( f"{item.start_price}|{item.last_price}" ) ext_item_id = hashlib.sha256(ext_seed.encode("utf-8")).hexdigest()[:32] - try: - db.execute( - text( - """ - INSERT INTO house_placement_history ( - source, ext_item_id, - rooms, area_m2, floor, - start_price, start_price_date, - last_price, last_price_date, - exposure_days, - raw_payload - ) VALUES ( - 'yandex_valuation', :ext_id, - :rooms, :area, :floor, - :start_price, :publish_date, - :last_price, :publish_date, - :exposure, - CAST(:raw AS jsonb) - ) - ON CONFLICT (source, ext_item_id) DO NOTHING - """ - ), - { - "ext_id": ext_item_id, - "rooms": item.rooms, - "area": item.area_m2, - "floor": item.floor, - "start_price": item.start_price, - "last_price": item.last_price, - "publish_date": item.publish_date, - "exposure": item.exposure_days, - "raw": json.dumps(item.model_dump(mode="json"), ensure_ascii=False), - }, - ) - saved += 1 - except Exception as e: - logger.warning("yandex_valuation: failed to save history item: %s", e) - db.rollback() - continue - if saved: + rows.append({ + "ext_id": ext_item_id, + "rooms": item.rooms, + "area": item.area_m2, + "floor": item.floor, + "start_price": item.start_price, + "last_price": item.last_price, + "publish_date": item.publish_date, + "exposure": item.exposure_days, + "raw": json.dumps(item.model_dump(mode="json"), ensure_ascii=False), + }) + + sql = text( + """ + INSERT INTO house_placement_history ( + source, ext_item_id, + rooms, area_m2, floor, + start_price, start_price_date, + last_price, last_price_date, + exposure_days, + raw_payload + ) VALUES ( + 'yandex_valuation', :ext_id, + :rooms, :area, :floor, + :start_price, :publish_date, + :last_price, :publish_date, + :exposure, + CAST(:raw AS jsonb) + ) + ON CONFLICT (source, ext_item_id) DO NOTHING + """ + ) + + try: + for row in rows: + db.execute(sql, row) db.commit() - return saved + return len(rows) + except Exception as e: + logger.warning( + "yandex_valuation: failed to save history batch (%d items): %s", + len(rows), e, + ) + db.rollback() + return 0 # ── Public ─────────────────────────────────────────────────────────────────── @@ -667,25 +677,25 @@ async def estimate_quality( "expires_at": expires_at, }, ) - db.commit() - # Link saved IMV evaluation к этому estimate_id (для analytics joining) + # Link saved IMV evaluation к этому estimate_id атомарно с основным INSERT + # (closes finding #4 from 2026-05-24 audit — prior code committed estimate first, + # then UPDATEd IMV in a separate tx, racing against concurrent estimators + # sharing the same cache_key). if imv_eval is not None: - try: - db.execute( - text( - """ - UPDATE avito_imv_evaluations - SET estimate_id = CAST(:estimate_id AS uuid) - WHERE cache_key = :cache_key - AND (estimate_id IS NULL OR estimate_id = CAST(:estimate_id AS uuid)) - """ - ), - {"estimate_id": str(estimate_id), "cache_key": imv_eval.cache_key}, - ) - db.commit() - except Exception as e: - logger.warning("imv: failed to link estimate_id to evaluation: %s", e) + db.execute( + text( + """ + UPDATE avito_imv_evaluations + SET estimate_id = CAST(:estimate_id AS uuid) + WHERE cache_key = :cache_key + AND (estimate_id IS NULL OR estimate_id = CAST(:estimate_id AS uuid)) + """ + ), + {"estimate_id": str(estimate_id), "cache_key": imv_eval.cache_key}, + ) + + db.commit() logger.info( "estimate: id=%s addr=%s rooms=%d area=%.1f → median=%d (n=%d, conf=%s)%s%s", @@ -819,13 +829,13 @@ def _fetch_analogs( -- без типа → PostgreSQL "could not determine data type of parameter" -- (AmbiguousParameter). Явный тип снимает неоднозначность. + CASE - WHEN CAST(:target_year AS integer) IS NOT NULL + WHEN :target_year IS NOT NULL AND year_built IS NOT NULL THEN abs(year_built - CAST(:target_year AS integer)) / 12.0 ELSE 0 END + CASE - WHEN CAST(:target_house_type AS text) IS NOT NULL + WHEN :target_house_type IS NOT NULL AND house_type IS NOT NULL AND house_type <> CAST(:target_house_type AS text) THEN 1.5 @@ -838,13 +848,13 @@ def _fetch_analogs( ST_Distance(geom::geography, ST_MakePoint(:lon, :lat)::geography) / 1000.0 + CASE - WHEN CAST(:target_year AS integer) IS NOT NULL + WHEN :target_year IS NOT NULL AND year_built IS NOT NULL THEN abs(year_built - CAST(:target_year AS integer)) / 12.0 ELSE 0 END + CASE - WHEN CAST(:target_house_type AS text) IS NOT NULL + WHEN :target_house_type IS NOT NULL AND house_type IS NOT NULL AND house_type <> CAST(:target_house_type AS text) THEN 1.5 diff --git a/tradein-mvp/backend/tests/test_estimator_yandex_integration.py b/tradein-mvp/backend/tests/test_estimator_yandex_integration.py index d5c4db23..7d56ab12 100644 --- a/tradein-mvp/backend/tests/test_estimator_yandex_integration.py +++ b/tradein-mvp/backend/tests/test_estimator_yandex_integration.py @@ -188,10 +188,12 @@ def test_save_history_items_ext_id_stable_across_calls(): assert ext_ids_1 == ext_ids_2 -def test_save_history_items_db_error_continues(): - """One item failing doesn't abort the others.""" +def test_save_history_items_db_error_rolls_back_batch(): + """Any item failing rolls back the whole batch — batch semantics (finding #5).""" db = MagicMock() db.execute.side_effect = [RuntimeError("first row fails"), None] result = _sample_result() saved = _save_yandex_history_items(db, result) - assert saved == 1 # second item still saved + assert saved == 0 # whole batch rolled back + db.rollback.assert_called_once() + db.commit.assert_not_called()