diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index a5f799444..9c8ad44ee 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -238,6 +238,9 @@ Request objects Also mind that the :meth:`copy` and :meth:`replace` request methods :doc:`shallow-copy ` request metadata. + .. seealso:: :class:`~scrapy.spidermiddlewares.metacopy.MetaCopyDetectionMiddleware` + for a built-in middleware that warns about this issue at run time. + .. autoattribute:: dont_filter .. autoattribute:: Request.attributes diff --git a/docs/topics/spider-middleware.rst b/docs/topics/spider-middleware.rst index 870adbc8c..aa14f6801 100644 --- a/docs/topics/spider-middleware.rst +++ b/docs/topics/spider-middleware.rst @@ -316,6 +316,35 @@ Default: ``False`` Pass all responses, regardless of its status code. +MetaCopyDetectionMiddleware +--------------------------- + +.. module:: scrapy.spidermiddlewares.metacopy + :synopsis: Meta Copy Detection Spider Middleware + +.. class:: MetaCopyDetectionMiddleware + + Warns when a spider yields a request that contains internal meta keys which + should not be copied from :attr:`response.meta ` + into new requests. See :attr:`~scrapy.http.Request.meta` to learn why. + + Only 1 warning is emitted per crawl. + + MetaCopyDetectionMiddleware settings + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + .. setting:: META_COPY_WARN_SKIP_KEYS + + META_COPY_WARN_SKIP_KEYS + ^^^^^^^^^^^^^^^^^^^^^^^^ + + Default: ``[]`` + + A list of internal meta key names to exclude from the internal-keys check. + Use this when you intentionally copy one of the monitored keys and want to + suppress the resulting warning without disabling the middleware entirely. + + RefererMiddleware ----------------- diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 3a7dfd018..504b58e1c 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -556,6 +556,7 @@ SPIDER_MIDDLEWARES_BASE = { "scrapy.spidermiddlewares.referer.RefererMiddleware": 700, "scrapy.spidermiddlewares.urllength.UrlLengthMiddleware": 800, "scrapy.spidermiddlewares.depth.DepthMiddleware": 900, + "scrapy.spidermiddlewares.metacopy.MetaCopyDetectionMiddleware": 1000, # Spider side } diff --git a/scrapy/spidermiddlewares/base.py b/scrapy/spidermiddlewares/base.py index 6c62dccb8..3b14d2b0a 100644 --- a/scrapy/spidermiddlewares/base.py +++ b/scrapy/spidermiddlewares/base.py @@ -48,7 +48,10 @@ class BaseSpiderMiddleware: @_warn_spider_arg def process_spider_output( - self, response: Response, result: Iterable[Any], spider: Spider | None = None + self, + response: Response | None, + result: Iterable[Any], + spider: Spider | None = None, ) -> Iterable[Any]: for o in result: if (o := self._get_processed(o, response)) is not None: @@ -57,7 +60,7 @@ class BaseSpiderMiddleware: @_warn_spider_arg async def process_spider_output_async( self, - response: Response, + response: Response | None, result: AsyncIterator[Any], spider: Spider | None = None, ) -> AsyncIterator[Any]: diff --git a/scrapy/spidermiddlewares/depth.py b/scrapy/spidermiddlewares/depth.py index 9c387530c..054804119 100644 --- a/scrapy/spidermiddlewares/depth.py +++ b/scrapy/spidermiddlewares/depth.py @@ -55,19 +55,24 @@ class DepthMiddleware(BaseSpiderMiddleware): @_warn_spider_arg def process_spider_output( - self, response: Response, result: Iterable[Any], spider: Spider | None = None + self, + response: Response | None, + result: Iterable[Any], + spider: Spider | None = None, ) -> Iterable[Any]: - self._init_depth(response) + if response is not None: + self._init_depth(response) yield from super().process_spider_output(response, result) @_warn_spider_arg async def process_spider_output_async( self, - response: Response, + response: Response | None, result: AsyncIterator[Any], spider: Spider | None = None, ) -> AsyncIterator[Any]: - self._init_depth(response) + if response is not None: + self._init_depth(response) async for o in super().process_spider_output_async(response, result): yield o diff --git a/scrapy/spidermiddlewares/metacopy.py b/scrapy/spidermiddlewares/metacopy.py new file mode 100644 index 000000000..aa5a120c6 --- /dev/null +++ b/scrapy/spidermiddlewares/metacopy.py @@ -0,0 +1,65 @@ +from __future__ import annotations + +import logging +from typing import TYPE_CHECKING + +from scrapy.spidermiddlewares.base import BaseSpiderMiddleware + +if TYPE_CHECKING: + from scrapy.crawler import Crawler + from scrapy.http import Request, Response + + +logger = logging.getLogger(__name__) + + +class MetaCopyDetectionMiddleware(BaseSpiderMiddleware): + """Warn when a spider yields a request with internal meta keys that should + not be copied from response.meta, or when two requests share the same meta + dict object. + + Each warning is emitted at most once per crawl. + """ + + _INTERNAL_KEYS: frozenset[str] = frozenset( + { + "_auth_proxy", + "_dont_cache", + "_scheme_proxy", + "download_latency", + "redirect_reasons", + "redirect_times", + "redirect_ttl", + "redirect_urls", + "retry_times", + } + ) + + def __init__(self, crawler: Crawler) -> None: + super().__init__(crawler) + skip = frozenset(crawler.settings.getlist("META_COPY_WARN_SKIP_KEYS", [])) + self._keys: frozenset[str] = self._INTERNAL_KEYS - skip + self._warned: bool = False + + def get_processed_request( + self, request: Request, response: Response | None + ) -> Request | None: + if response is None: + return request + + if not self._warned: + found = self._keys & request.meta.keys() + if found: + spider_name = type(self.crawler.spider).__name__ + logger.warning( + f"{spider_name} yielded a request containing internal " + f"meta keys that were likely copied from response.meta " + f"and should not be forwarded to new requests: " + f"{sorted(found)}. See the MetaCopyDetectionMiddleware " + f"documentation for more information. Source response: " + f"{response}, target request: {request}", + extra={"spider": self.crawler.spider}, + ) + self._warned = True + + return request diff --git a/tests/test_spidermiddleware_metacopy.py b/tests/test_spidermiddleware_metacopy.py new file mode 100644 index 000000000..bfa27ac68 --- /dev/null +++ b/tests/test_spidermiddleware_metacopy.py @@ -0,0 +1,121 @@ +from __future__ import annotations + +from logging import WARNING +from typing import TYPE_CHECKING + +import pytest + +from scrapy.http import Request, Response +from scrapy.spidermiddlewares.metacopy import MetaCopyDetectionMiddleware +from scrapy.spiders import Spider +from scrapy.utils.test import get_crawler + +if TYPE_CHECKING: + from scrapy.crawler import Crawler + + +def make_response(url: str = "https://example.com") -> Response: + response = Response(url) + response.request = Request(url) + return response + + +@pytest.fixture +def crawler() -> Crawler: + return get_crawler(Spider) + + +@pytest.fixture +def mw(crawler: Crawler) -> MetaCopyDetectionMiddleware: + return MetaCopyDetectionMiddleware.from_crawler(crawler) + + +def process( + mw: MetaCopyDetectionMiddleware, + requests: list[Request], + response: Response | None = None, +) -> list[Request]: + if response is None: + response = make_response() + return list(mw.process_spider_output(response, requests)) + + +class TestInternalKeysCheck: + def test_no_warning_for_clean_request( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + req = Request("https://example.com/1", meta={"my_key": "value"}) + with caplog.at_level(WARNING): + process(mw, [req]) + assert not caplog.records + + def test_warns_on_internal_key( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + req = Request("https://example.com/1", meta={"retry_times": 1}) + with caplog.at_level(WARNING): + process(mw, [req]) + assert len(caplog.records) == 1 + assert "retry_times" in caplog.text + assert "https://example.com/1" in caplog.text + + def test_warns_once_across_multiple_requests( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + reqs = [ + Request("https://example.com/1", meta={"retry_times": 1}), + Request("https://example.com/2", meta={"retry_times": 2}), + ] + with caplog.at_level(WARNING): + process(mw, reqs) + assert len(caplog.records) == 1 + + def test_reports_all_found_keys( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + req = Request( + "https://example.com/1", + meta={"retry_times": 1, "redirect_times": 2}, + ) + with caplog.at_level(WARNING): + process(mw, [req]) + assert "retry_times" in caplog.text + assert "redirect_times" in caplog.text + + def test_includes_source_response_in_message( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + response = make_response("https://source.example.com") + req = Request("https://example.com/1", meta={"retry_times": 1}) + with caplog.at_level(WARNING): + process(mw, [req], response=response) + assert "https://source.example.com" in caplog.text + + def test_skip_keys_setting(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(WARNING): + crawler = get_crawler(Spider, {"META_COPY_WARN_SKIP_KEYS": ["retry_times"]}) + mw = MetaCopyDetectionMiddleware.from_crawler(crawler) + req = Request("https://example.com/1", meta={"retry_times": 1}) + process(mw, [req]) + assert not caplog.records + + def test_skip_keys_setting_partial(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(WARNING): + crawler = get_crawler(Spider, {"META_COPY_WARN_SKIP_KEYS": ["retry_times"]}) + mw = MetaCopyDetectionMiddleware.from_crawler(crawler) + req = Request( + "https://example.com/1", + meta={"retry_times": 1, "redirect_times": 2}, + ) + process(mw, [req]) + assert len(caplog.records) == 1 + assert "retry_times" not in caplog.text + assert "redirect_times" in caplog.text + + def test_no_warning_for_start_requests( + self, mw: MetaCopyDetectionMiddleware, caplog: pytest.LogCaptureFixture + ) -> None: + req = Request("https://example.com/1", meta={"retry_times": 1}) + with caplog.at_level(WARNING): + list(mw.process_spider_output(None, [req])) + assert not caplog.records