diff --git a/tradein-mvp/backend/app/services/estimator.py b/tradein-mvp/backend/app/services/estimator.py index 52d85280..3c9a84a9 100644 --- a/tradein-mvp/backend/app/services/estimator.py +++ b/tradein-mvp/backend/app/services/estimator.py @@ -394,8 +394,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 = ( @@ -403,48 +410,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 ─────────────────────────────────────────────────────────────────── @@ -718,25 +728,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", @@ -870,13 +880,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 @@ -889,13 +899,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()