diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index 8fd3de621..660dbd4cf 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -231,6 +231,10 @@ Request objects Also mind that the :meth:`copy` and :meth:`replace` request methods :doc:`shallow-copy ` request metadata. + .. seealso:: :ref:`unsafe-meta-copy` for a more detailed explanation + and :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 99bbdf292..197e2f183 100644 --- a/docs/topics/spider-middleware.rst +++ b/docs/topics/spider-middleware.rst @@ -316,6 +316,58 @@ 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, or when two requests share the same meta dict object. + + Each warning is emitted at most once per crawl to avoid noise, but includes + the source response and the target request so the issue is easy to locate. + + .. _unsafe-meta-copy: + + Why copying response.meta is unsafe + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + + :attr:`response.meta ` is an alias for + :attr:`response.request.meta `, which is the meta + dict of the request that produced the response. That dict may contain keys + set by Scrapy's internal components — such as ``retry_times`` from + :class:`~scrapy.downloadermiddlewares.retry.RetryMiddleware` — that are + specific to that one request and must not be forwarded to unrelated + requests. Copying the entire dict (e.g. ``meta={**response.meta}``) or + passing the dict object directly (e.g. ``meta=response.meta``) propagates + those internal keys and can cause subtle bugs. For example, forwarding + ``retry_times`` reduces the number of retries available for the new request. + + To pass custom data between callbacks, prefer + :attr:`~scrapy.http.Request.cb_kwargs`. If you need to carry data through + :attr:`~scrapy.http.Request.meta`, copy only the specific keys your code + uses rather than the whole dict. See also `scrapy-sticky-meta-params + `_. + + 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 7d5612026..ce0544b1e 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -512,6 +512,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/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..d137822cf --- /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: + 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}) + with caplog.at_level(WARNING): + process(mw, [req]) + assert not caplog.records + + def test_skip_keys_setting_partial(self, caplog: pytest.LogCaptureFixture) -> None: + 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}, + ) + with caplog.at_level(WARNING): + 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