diff --git a/tradein-mvp/backend/app/tasks/newbuilding_enrich_backfill.py b/tradein-mvp/backend/app/tasks/newbuilding_enrich_backfill.py index 855eb473..b6f5dda1 100644 --- a/tradein-mvp/backend/app/tasks/newbuilding_enrich_backfill.py +++ b/tradein-mvp/backend/app/tasks/newbuilding_enrich_backfill.py @@ -356,21 +356,26 @@ async def backfill_newbuilding_enrichment( # when the task actually runs — mirrors cian_history_backfill's lazy import and keeps # unit tests able to patch fetch_newbuilding cheaply. # - # NOT migrated to scraper_kit (issue #2310, Group C): kit's - # scraper_kit.providers.cian.newbuilding.fetch_newbuilding() constructs - # BrowserFetcher(source="cian") WITHOUT the now-mandatory endpoint= kwarg — - # every call raises TypeError, with no caller-side fix possible (the kit - # provider function doesn't expose a config/endpoint hook at all). This is - # issue #2322, verified STILL OPEN by reading the actual provider source - # (not just the issue's open/closed status) at the time of this migration. - # Left on legacy entirely, exactly like Group A (#2305) did for the same - # bug in admin.py's debug endpoints. - from app.services.scrapers.cian_newbuilding import ( + # Migrated to scraper_kit (#2397 Part D3): issue #2322 (BrowserFetcher(source="cian") + # constructed WITHOUT the mandatory endpoint= kwarg) was fixed for this provider — + # fetch_newbuilding() / resolve_cian_zhk_url_via_search() now accept an explicit + # `config: ScraperConfig | None` kwarg (see test_scraper_kit_newbuilding_endpoint.py). + # config=RealScraperConfig() is REQUIRED below for both calls: + # - fetch_newbuilding: without it, BrowserFetcher.__aenter__ raises AssertionError. + # - resolve_cian_zhk_url_via_search: this one goes through curl_cffi (not + # BrowserFetcher), so a missing config does NOT crash — it silently drops + # cian_proxy_url and falls back to a direct (non-proxied) connection. Both call + # sites below pass config= explicitly to avoid that silent footgun. + from scraper_kit.providers.cian.newbuilding import ( fetch_newbuilding, resolve_cian_zhk_url_via_search, save_newbuilding_enrichment, ) + from app.services.scraper_adapters import RealScraperConfig + + scraper_config = RealScraperConfig() + result = NewbuildingEnrichBackfillResult() t0 = time.time() delay = request_delay_sec if request_delay_sec is not None else get_scraper_delay("cian") @@ -444,7 +449,7 @@ async def backfill_newbuilding_enrichment( continue try: - resolved = await resolve_cian_zhk_url_via_search(nb_id) + resolved = await resolve_cian_zhk_url_via_search(nb_id, config=scraper_config) except Exception as exc: # defensive — resolver already catches internally logger.warning( "zhk-url resolve raised house_id=%s nb_id=%s: %s", house_id, nb_id, exc @@ -491,7 +496,7 @@ async def backfill_newbuilding_enrichment( # ── Fetch (network; anti-bot surface) ────────────────────────────── enrichment = None try: - enrichment = await fetch_newbuilding(zhk_url) + enrichment = await fetch_newbuilding(zhk_url, config=scraper_config) except Exception as exc: logger.warning( "newbuilding fetch failed house_id=%s url=%s: %s", house_id, zhk_url, exc diff --git a/tradein-mvp/backend/tests/tasks/test_newbuilding_enrich_backfill.py b/tradein-mvp/backend/tests/tasks/test_newbuilding_enrich_backfill.py index 452ffc3b..16c32b25 100644 --- a/tradein-mvp/backend/tests/tasks/test_newbuilding_enrich_backfill.py +++ b/tradein-mvp/backend/tests/tasks/test_newbuilding_enrich_backfill.py @@ -19,8 +19,8 @@ _wp_mock = MagicMock() sys.modules.setdefault("weasyprint", _wp_mock) import pytest # noqa: E402 +from scraper_kit.providers.cian.newbuilding import NewbuildingEnrichment # noqa: E402 -from app.services.scrapers.cian_newbuilding import NewbuildingEnrichment # noqa: E402 from app.tasks.newbuilding_enrich_backfill import ( # noqa: E402 NewbuildingEnrichBackfillResult, _save_cian_reviews, @@ -233,7 +233,7 @@ def test_save_cian_reviews_clamps_bad_score() -> None: async def test_backfill_dry_run_counts_only() -> None: db = FakeDB(pending_rows=[{"house_id": 1, "cian_zhk_url": "https://zhk-a.cian.ru/"}]) with patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", new_callable=AsyncMock + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", new_callable=AsyncMock ) as mock_fetch: result = await backfill_newbuilding_enrichment(db, limit=5, dry_run=True) mock_fetch.assert_not_called() @@ -261,11 +261,11 @@ async def test_backfill_populates_all_three_tables() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -293,12 +293,12 @@ async def test_backfill_force_rerun_no_dup() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", new_callable=AsyncMock, return_value=enr, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -335,9 +335,9 @@ async def test_backfill_default_rerun_skips_and_no_reliability_dup() -> None: return enr with ( - patch("app.services.scrapers.cian_newbuilding.fetch_newbuilding", side_effect=_fetch), + patch("scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -370,11 +370,11 @@ async def test_backfill_fetch_failure_does_not_abort_batch() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -411,15 +411,15 @@ async def test_backfill_resolves_zhk_url_from_ext_id_then_enriches() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.resolve_cian_zhk_url_via_search", + "scraper_kit.providers.cian.newbuilding.resolve_cian_zhk_url_via_search", side_effect=_resolve, ), patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -450,16 +450,16 @@ async def test_backfill_unresolved_ext_id_counts_fail_and_skips_fetch() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.resolve_cian_zhk_url_via_search", + "scraper_kit.providers.cian.newbuilding.resolve_cian_zhk_url_via_search", new_callable=AsyncMock, return_value=None, ), patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): @@ -494,15 +494,15 @@ async def test_backfill_resolve_then_idempotent_rerun_no_reresolve() -> None: with ( patch( - "app.services.scrapers.cian_newbuilding.resolve_cian_zhk_url_via_search", + "scraper_kit.providers.cian.newbuilding.resolve_cian_zhk_url_via_search", side_effect=_resolve, ), patch( - "app.services.scrapers.cian_newbuilding.fetch_newbuilding", + "scraper_kit.providers.cian.newbuilding.fetch_newbuilding", side_effect=_fetch, ), patch( - "app.services.scrapers.cian_newbuilding.save_newbuilding_enrichment", + "scraper_kit.providers.cian.newbuilding.save_newbuilding_enrichment", side_effect=_fake_save_newbuilding_enrichment, ), ): diff --git a/tradein-mvp/backend/tests/test_scraper_kit_newbuilding_endpoint.py b/tradein-mvp/backend/tests/test_scraper_kit_newbuilding_endpoint.py index e0d41687..b372a63e 100644 --- a/tradein-mvp/backend/tests/test_scraper_kit_newbuilding_endpoint.py +++ b/tradein-mvp/backend/tests/test_scraper_kit_newbuilding_endpoint.py @@ -14,6 +14,15 @@ These tests prove `config.browser_http_endpoint` is actually threaded through to `BrowserFetcher(...)` constructor call args — not merely that the function "doesn't crash" on a happy path. `BrowserFetcher` itself is replaced by a spy so the constructor call is directly inspectable. + +Also covers a SIBLING but importantly DIFFERENT footgun (#2397 Part D3, migrating +`app.tasks.newbuilding_enrich_backfill` onto this kit provider): +`resolve_cian_zhk_url_via_search` does NOT use `BrowserFetcher` at all — it goes +through `build_curl_cffi_session` — so a missing `config=` does NOT raise like the +`BrowserFetcher.__aenter__` assertion above. It silently degrades to a direct +(non-proxied) connection (`cian_proxy_url` dropped). That is much harder to notice in +prod (no crash, just quietly unproxied traffic that anti-bot can throttle/block), so a +caller that forgets `config=` here gets no signal at all. """ from __future__ import annotations @@ -22,13 +31,17 @@ from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest -from scraper_kit.providers.cian.newbuilding import fetch_newbuilding +from scraper_kit.providers.cian.newbuilding import ( + fetch_newbuilding, + resolve_cian_zhk_url_via_search, +) from scraper_kit.providers.yandex.newbuilding import ( YandexNewbuildingScraper, resolve_yandex_jk_slug, ) _TEST_ENDPOINT = "http://test-tradein-browser:9009/fetch" +_TEST_PROXY = "http://mobile-proxy.example:8080" def _spy_browser_fetcher(html: str) -> MagicMock: @@ -138,3 +151,58 @@ async def test_resolve_yandex_jk_slug_endpoint_none_without_config(monkeypatch): _, kwargs = spy.call_args assert kwargs.get("source") == "yandex" assert kwargs.get("endpoint") is None + + +# ── cian.newbuilding.resolve_cian_zhk_url_via_search ───────────────────────── +# NOTE (#2397 Part D3): this provider does NOT use BrowserFetcher (unlike +# fetch_newbuilding above) — it builds its own curl_cffi session via +# build_curl_cffi_session(proxy_url=...). Unlike the BrowserFetcher assertion, a +# missing config= here does NOT crash — it silently drops the proxy and falls back to +# a direct (non-proxied) connection. These tests prove the threading in BOTH directions +# so a caller forgetting config= has a red test instead of quiet unproxied traffic. + +_SERP_HTML = ( + '

Купить квартиру в ' + 'ЖК «Парковый квартал»

' +) + + +def _spy_curl_session(html: str) -> MagicMock: + """build_curl_cffi_session spy: records the (proxy_url=...) call kwargs and returns + a fake curl_cffi AsyncSession whose .get() resolves to a 200 response carrying `html`.""" + mock_resp = SimpleNamespace(status_code=200, text=html) + mock_session = MagicMock() + mock_session.get = AsyncMock(return_value=mock_resp) + mock_session.close = AsyncMock() + return MagicMock(return_value=mock_session) + + +@pytest.mark.asyncio +async def test_resolve_cian_zhk_url_via_search_passes_proxy_from_config(monkeypatch): + """config.cian_proxy_url должен попасть в build_curl_cffi_session(proxy_url=...).""" + spy = _spy_curl_session(_SERP_HTML) + monkeypatch.setattr("scraper_kit.providers.cian.newbuilding.build_curl_cffi_session", spy) + + config = SimpleNamespace(cian_proxy_url=_TEST_PROXY) + url = await resolve_cian_zhk_url_via_search(48853, config=config) + + assert url == "https://zhk-parkovyy-kvartal-ekb-i.cian.ru" + spy.assert_called_once() + _, kwargs = spy.call_args + assert kwargs.get("proxy_url") == _TEST_PROXY + + +@pytest.mark.asyncio +async def test_resolve_cian_zhk_url_via_search_proxy_none_without_config(monkeypatch): + """Без config= (SILENT footgun): вызов НЕ падает (в отличие от BrowserFetcher + assertion выше в этом файле), но proxy_url молча становится None — резолвер уходит + на прямое (непроксированное) соединение вместо настроенного мобильного прокси.""" + spy = _spy_curl_session(_SERP_HTML) + monkeypatch.setattr("scraper_kit.providers.cian.newbuilding.build_curl_cffi_session", spy) + + url = await resolve_cian_zhk_url_via_search(48853) + + assert url == "https://zhk-parkovyy-kvartal-ekb-i.cian.ru" # "works" — silently unproxied + spy.assert_called_once() + _, kwargs = spy.call_args + assert kwargs.get("proxy_url") is None