From db9bec8142f7781a6be8b24d6aca91713058391d Mon Sep 17 00:00:00 2001 From: bot-backend Date: Sat, 4 Jul 2026 23:16:25 +0300 Subject: [PATCH] =?UTF-8?q?fix(tradein/cian):=20management=5Fcompanies=20d?= =?UTF-8?q?edup=20=E2=80=94=20SELECT-before-INSERT,=20not=20ON=20CONFLICT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ext_id=NULL rows never conflict under Postgres default NULLS DISTINCT, so the ON CONFLICT (ext_source, name, ext_id) DO UPDATE in _upsert_cian_valuation_management_company was dead code — every valuation cache-miss for the same building inserted a fresh duplicate management_companies row instead of deduping by name. Replaced with an explicit SELECT-then-branch (UPDATE existing / INSERT new); docstring corrected to stop claiming the constraint dedups it. Found by deep-code-reviewer on #2435. Refs #2435 --- .../tests/test_cian_bti_house_persist.py | 61 +++++++++++++++++-- .../scraper_kit/providers/cian/valuation.py | 50 +++++++++++---- 2 files changed, 95 insertions(+), 16 deletions(-) diff --git a/tradein-mvp/backend/tests/test_cian_bti_house_persist.py b/tradein-mvp/backend/tests/test_cian_bti_house_persist.py index 5c63e0d2..e41a2790 100644 --- a/tradein-mvp/backend/tests/test_cian_bti_house_persist.py +++ b/tradein-mvp/backend/tests/test_cian_bti_house_persist.py @@ -225,8 +225,16 @@ def _cian_result( ) -def _mock_db_valuation(mc_insert_id: int | None = 900) -> MagicMock: - """Mock db для _save_to_cache: INSERT management_companies RETURNING id + UPDATE houses.""" +def _mock_db_valuation( + mc_insert_id: int | None = 900, mc_existing_id: int | None = None +) -> MagicMock: + """Mock db для _save_to_cache: SELECT (dedup) → UPDATE-or-INSERT management_companies, + затем UPDATE houses. + + `mc_existing_id` simulates the dedup SELECT finding a prior row (existing.fetchone() + returns that id) — the UPDATE branch is taken instead of INSERT. Default None means + "no existing row", exercising the INSERT branch (the original test-suite behavior). + """ db = MagicMock() @contextmanager @@ -238,7 +246,11 @@ def _mock_db_valuation(mc_insert_id: int | None = 900) -> MagicMock: def _exec(sql, params=None): sql_str = str(sql) mock_result = MagicMock() - if "INSERT INTO management_companies" in sql_str: + if "SELECT id FROM management_companies" in sql_str: + mock_result.fetchone.return_value = ( + (mc_existing_id,) if mc_existing_id is not None else None + ) + elif "INSERT INTO management_companies" in sql_str: mock_result.fetchone.return_value = ( (mc_insert_id,) if mc_insert_id is not None else None ) @@ -250,6 +262,22 @@ def _mock_db_valuation(mc_insert_id: int | None = 900) -> MagicMock: return db +def _mc_calls(db: MagicMock) -> list[str]: + """SQL statement kinds touching management_companies, in call order.""" + kinds = [] + for call_args in db.execute.call_args_list: + if not call_args.args: + continue + sql = str(call_args.args[0]) + if "SELECT id FROM management_companies" in sql: + kinds.append("select") + elif "INSERT INTO management_companies" in sql: + kinds.append("insert") + elif "UPDATE management_companies" in sql: + kinds.append("update") + return kinds + + def _houses_update_call(db: MagicMock) -> tuple[str, dict] | None: for call_args in db.execute.call_args_list: if not call_args.args: @@ -278,7 +306,8 @@ def _call_save_to_cache(db: MagicMock, result: CianValuationResult, house_id: in def test_valuation_house_info_and_management_company_land_in_houses_update(): - """house_id задан → UPDATE houses с management_company_id + house_info-полями.""" + """house_id задан, УК ранее не встречалась → SELECT (не найдено) → INSERT management_companies, + UPDATE houses с management_company_id + house_info-полями.""" db = _mock_db_valuation(mc_insert_id=900) result = _cian_result( house_info=_HOUSE_INFO_SAMPLE, @@ -293,6 +322,7 @@ def test_valuation_house_info_and_management_company_land_in_houses_update(): _call_save_to_cache(db, result, house_id=42) + assert _mc_calls(db) == ["select", "insert"] call = _houses_update_call(db) assert call is not None sql, params = call @@ -315,6 +345,29 @@ def test_valuation_house_info_and_management_company_land_in_houses_update(): db.commit.assert_called_once() +def test_valuation_management_company_dedups_on_repeat_call_same_name(): + """Regression for the NULLS-DISTINCT dedup bug (#2435 review): a repeat valuation + call for a management company already persisted (same ext_source/name, ext_id=NULL) + must find it via the explicit SELECT and UPDATE it — not INSERT a duplicate row. + `ON CONFLICT (ext_source, name, ext_id)` alone can't dedup ext_id=NULL rows under + Postgres's default NULLS DISTINCT (two NULLs never compare equal), which is why + dedup is done via SELECT-before-INSERT rather than relying on the constraint.""" + db = _mock_db_valuation(mc_existing_id=900) + result = _cian_result( + house_info=[], + management_company={"name": 'ТСЖ "Учителей, 18"', "phones": ["+7 (343) 216-59-12"]}, + ) + + _call_save_to_cache(db, result, house_id=42) + + assert _mc_calls(db) == ["select", "update"] + call = _houses_update_call(db) + assert call is not None + _, params = call + assert params["mcid"] == 900 + db.commit.assert_called_once() + + def test_valuation_house_id_none_skips_house_update(): """house_id=None → нет UPDATE houses (best-effort no-op, тот же no-crash контракт, что test_extval_house_id_write_path.py::test_cian_save_null_house_id_no_exception).""" diff --git a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/valuation.py b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/valuation.py index 9ff819bd..9f62c966 100644 --- a/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/valuation.py +++ b/tradein-mvp/packages/scraper-kit/src/scraper_kit/providers/cian/valuation.py @@ -656,11 +656,14 @@ def _upsert_cian_valuation_management_company( ) -> int | None: """UPSERT management_companies из Cian Valuation Calculator's managementCompany.data. - Тот же UPSERT-паттерн, что providers/cian/newbuilding.py::save_newbuilding_enrichment - (независимая копия — newbuilding.py вне scope #2435, не рефакторим/не переиспользуем). - ext_source='cian_valuation' (не 'cian') — валюация не даёт числового id УК (в отличие - от newbuilding.managementCompany.id), поэтому dedup-ключ (ext_source, name, ext_id=NULL) - сходится по имени УК среди повторных valuation-запросов. + Тот же намерение, что providers/cian/newbuilding.py::save_newbuilding_enrichment + (независимая копия — newbuilding.py вне scope #2435, не рефакторим/не переиспользуем), + НО другой механизм dedup. ext_source='cian_valuation' (не 'cian') — валюация не даёт + числового id УК (в отличие от newbuilding.managementCompany.id), поэтому пишем + ext_id=NULL. `UNIQUE (ext_source, name, ext_id)` под Postgres-дефолтом NULLS DISTINCT + не считает два NULL равными — `ON CONFLICT` с ext_id=NULL никогда бы не сработал (каждый + вызов создавал бы новую дублирующую строку). Поэтому dedup сделан явно: SELECT по + (ext_source, name) WHERE ext_id IS NULL перед INSERT, а не через constraint. opening_hours — Cian отдаёт список словарей ([{"Пн–Пт": ["10:00-20:00"]}, ...]), не строку; json.dumps в text-колонку (в отличие от newbuilding.py, которая пишет объект @@ -673,6 +676,32 @@ def _upsert_cian_valuation_management_company( return None opening_hours = mc.get("openingHours") + phones = mc.get("phones") or [] + email = mc.get("email") + oh_json = json.dumps(opening_hours, ensure_ascii=False) if opening_hours else None + chief = mc.get("chiefName") + + existing = db.execute( + text(""" + SELECT id FROM management_companies + WHERE ext_source = 'cian_valuation' AND name = :name AND ext_id IS NULL + LIMIT 1 + """), + {"name": name}, + ).fetchone() + if existing: + mc_id = int(existing[0]) + db.execute( + text(""" + UPDATE management_companies SET + phones = COALESCE(CAST(:phones AS text[]), phones), + email = COALESCE(:email, email) + WHERE id = CAST(:mc_id AS bigint) + """), + {"phones": phones, "email": email, "mc_id": mc_id}, + ) + return mc_id + row = db.execute( text(""" INSERT INTO management_companies ( @@ -680,17 +709,14 @@ def _upsert_cian_valuation_management_company( ) VALUES ( :name, CAST(:phones AS text[]), :email, :oh, :chief, 'cian_valuation', NULL ) - ON CONFLICT (ext_source, name, ext_id) DO UPDATE SET - phones = COALESCE(EXCLUDED.phones, management_companies.phones), - email = COALESCE(EXCLUDED.email, management_companies.email) RETURNING id """), { "name": name, - "phones": mc.get("phones") or [], - "email": mc.get("email"), - "oh": json.dumps(opening_hours, ensure_ascii=False) if opening_hours else None, - "chief": mc.get("chiefName"), + "phones": phones, + "email": email, + "oh": oh_json, + "chief": chief, }, ).fetchone() return int(row[0]) if row else None