Merge 72685e0659 into d8ba1571e7

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 <library/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

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 <scrapy.http.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
 -----------------
 

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
 }
 

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]:

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
 

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

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