From 6fdd5a9d4f62cc989b8c96b68a895aee89108f86 Mon Sep 17 00:00:00 2001 From: Adrian Chaves Date: Tue, 28 Jul 2026 17:16:21 +0200 Subject: [PATCH] Unify HTTP code handling configuration for the error and redirect middlewares --- docs/topics/downloader-middleware.rst | 13 +- docs/topics/request-response.rst | 3 +- docs/topics/settings.rst | 50 ++++ docs/topics/spider-middleware.rst | 55 +--- scrapy/commands/fetch.py | 7 +- scrapy/downloadermiddlewares/redirect.py | 22 +- scrapy/pipelines/media.py | 14 +- scrapy/settings/default_settings.py | 3 + scrapy/shell.py | 10 +- scrapy/spidermiddlewares/httperror.py | 37 ++- scrapy/utils/_httpstatus.py | 184 ++++++++++++ tests/test_downloadermiddleware_redirect.py | 110 ++++++-- tests/test_pipeline_media.py | 8 +- tests/test_spidermiddleware_httperror.py | 294 ++++++++++++++++---- 14 files changed, 633 insertions(+), 177 deletions(-) create mode 100644 scrapy/utils/_httpstatus.py diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst index 934eb19ac..45f290dd5 100644 --- a/docs/topics/downloader-middleware.rst +++ b/docs/topics/downloader-middleware.rst @@ -909,8 +909,9 @@ settings (see the settings documentation for more info): If :attr:`Request.meta ` has ``dont_redirect`` key set to True, the request will be ignored by this middleware. -If you want to handle some redirect status codes in your spider, you can -specify these in the ``handle_httpstatus_list`` spider attribute. +If you want to handle some redirect status codes in your spider, declare them +through the :setting:`HANDLE_HTTP_CODES` setting or the +:reqmeta:`handle_http_codes` request meta key. For example, if you want the redirect middleware to ignore 301 and 302 responses (and pass them through to your spider) you can do this: @@ -918,13 +919,7 @@ responses (and pass them through to your spider) you can do this: .. code-block:: python class MySpider(CrawlSpider): - handle_httpstatus_list = [301, 302] - -The ``handle_httpstatus_list`` key of :attr:`Request.meta -` can also be used to specify which response codes to -allow on a per-request basis. You can also set the meta key -``handle_httpstatus_all`` to ``True`` if you want to allow any response code -for a request. + custom_settings = {"HANDLE_HTTP_CODES": [301, 302]} RedirectMiddleware settings diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index b83a04032..e4ddabbbe 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -728,8 +728,7 @@ Those are: * ``ftp_password`` (See :setting:`FTP_PASSWORD` for more info) * ``ftp_user`` (See :setting:`FTP_USER` for more info) * :reqmeta:`give_up_log_level` -* :reqmeta:`handle_httpstatus_all` -* :reqmeta:`handle_httpstatus_list` +* :reqmeta:`handle_http_codes` * :reqmeta:`http_auth_domain` * :reqmeta:`http_pass` * :reqmeta:`http_user` diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 8068824e3..66ffa9b8b 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -1448,6 +1448,56 @@ Default: ``None`` The Project ID that will be used when storing data on `Google Cloud Storage`_. +.. setting:: HANDLE_HTTP_CODES + +HANDLE_HTTP_CODES +----------------- + +.. versionadded:: VERSION + +Default: ``None`` + +Response status codes that your spider handles itself, instead of letting +Scrapy handle them. + +Supported values are: + +- ``None`` or ``False``, to handle no status code yourself. This is the + default. + +- ``True``, to handle every status code yourself. + +- An integer, or a container of integers, to handle only those status codes + yourself, e.g. ``404`` or ``[404, 410]``. + +Handling a status code yourself has 2 effects: + +- :class:`~scrapy.spidermiddlewares.httperror.HttpErrorMiddleware` sends + responses with that status code to your callback, instead of to your + errback as an + :exc:`~scrapy.spidermiddlewares.httperror.HttpError` exception. + +- :class:`~scrapy.downloadermiddlewares.redirect.RedirectMiddleware` does not + follow responses with that status code, so that your callback gets the + redirect response itself. Note that + :class:`~scrapy.downloadermiddlewares.redirect.MetaRefreshMiddleware` is + not affected, because ``meta`` refresh redirects come in successful + responses; use :reqmeta:`dont_redirect` to stop those. + +Retries are a separate matter: handling a status code yourself does not +prevent :class:`~scrapy.downloadermiddlewares.retry.RetryMiddleware` from +retrying it. A retried request either succeeds, in which case you get the new +response, or runs out of retries, in which case you get the response with the +status code that you handle. To also stop retries, remove the status code from +:setting:`RETRY_HTTP_CODES`, or set the :reqmeta:`dont_retry` request meta key. + +.. reqmeta:: handle_http_codes + +Use the ``handle_http_codes`` key of :attr:`Request.meta ` +to set this on a per-request basis. It supports the same values as the setting, +and it takes precedence over the setting, e.g. ``False`` makes a request handle +no status code even when the setting is ``True``. + .. setting:: ITEM_PIPELINES ITEM_PIPELINES diff --git a/docs/topics/spider-middleware.rst b/docs/topics/spider-middleware.rst index aa14f6801..1158af9fa 100644 --- a/docs/topics/spider-middleware.rst +++ b/docs/topics/spider-middleware.rst @@ -250,21 +250,16 @@ HttpErrorMiddleware .. module:: scrapy.spidermiddlewares.httperror :synopsis: HTTP Error Spider Middleware -.. class:: HttpErrorMiddleware - - Filter out unsuccessful (erroneous) HTTP responses so that spiders don't - have to deal with them, which (most of the time) imposes an overhead, - consumes more resources, and makes the spider logic more complex. +.. autoclass:: HttpErrorMiddleware According to the `HTTP standard`_, successful responses are those whose status codes are in the 200-300 range. .. _HTTP standard: https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html -If you still want to process response codes outside that range, you can -specify which response codes the spider is able to handle using the -``handle_httpstatus_list`` spider attribute or -:setting:`HTTPERROR_ALLOWED_CODES` setting. +If you still want to process response codes outside that range, use the +:setting:`HANDLE_HTTP_CODES` setting or the :reqmeta:`handle_http_codes` +request meta key to declare which status codes your spider handles itself. For example, if you want your spider to handle 404 responses you can do this: @@ -275,45 +270,13 @@ this: class MySpider(CrawlSpider): - handle_httpstatus_list = [404] + custom_settings = {"HANDLE_HTTP_CODES": [404]} -.. reqmeta:: handle_httpstatus_list +Responses whose status code your spider does not handle reach your errback as +an :exc:`HttpError` exception: -.. reqmeta:: handle_httpstatus_all - -The ``handle_httpstatus_list`` key of :attr:`Request.meta -` can also be used to specify which response codes to -allow on a per-request basis. You can also set the meta key ``handle_httpstatus_all`` -to ``True`` if you want to allow any response code for a request, and ``False`` to -disable the effects of the ``handle_httpstatus_all`` key. - -Keep in mind, however, that it's usually a bad idea to handle non-200 -responses, unless you really know what you're doing. - -For more information see: `HTTP Status Code Definitions`_. - -.. _HTTP Status Code Definitions: https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html - -HttpErrorMiddleware settings -~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -.. setting:: HTTPERROR_ALLOWED_CODES - -HTTPERROR_ALLOWED_CODES -^^^^^^^^^^^^^^^^^^^^^^^ - -Default: ``[]`` - -Pass all responses with non-200 status codes contained in this list. - -.. setting:: HTTPERROR_ALLOW_ALL - -HTTPERROR_ALLOW_ALL -^^^^^^^^^^^^^^^^^^^ - -Default: ``False`` - -Pass all responses, regardless of its status code. +.. autoexception:: HttpError + :members: MetaCopyDetectionMiddleware diff --git a/scrapy/commands/fetch.py b/scrapy/commands/fetch.py index 0b8311efb..2ba5e1b54 100644 --- a/scrapy/commands/fetch.py +++ b/scrapy/commands/fetch.py @@ -77,10 +77,9 @@ class Command(ScrapyCommand): ) # by default, let the framework handle redirects, # i.e. command handles all codes expect 3xx - if not opts.no_redirect: - request.meta["handle_httpstatus_list"] = SequenceExclude(range(300, 400)) - else: - request.meta["handle_httpstatus_all"] = True + request.meta["handle_http_codes"] = ( + True if opts.no_redirect else SequenceExclude(range(300, 400)) + ) spidercls: type[Spider] = DefaultSpider assert self.crawler_process diff --git a/scrapy/downloadermiddlewares/redirect.py b/scrapy/downloadermiddlewares/redirect.py index 45af5c67a..0de25b551 100644 --- a/scrapy/downloadermiddlewares/redirect.py +++ b/scrapy/downloadermiddlewares/redirect.py @@ -10,6 +10,7 @@ from scrapy import signals from scrapy.exceptions import IgnoreRequest, NotConfigured from scrapy.http import HtmlResponse, Response from scrapy.spidermiddlewares.referer import RefererMiddleware +from scrapy.utils._httpstatus import StatusHandling from scrapy.utils.decorators import _warn_spider_arg from scrapy.utils.httpobj import urlparse_cached from scrapy.utils.python import global_object_name @@ -198,16 +199,25 @@ class BaseRedirectMiddleware: class RedirectMiddleware(BaseRedirectMiddleware): """Handle redirection of requests based on response status.""" + def __init__(self, settings: BaseSettings): + super().__init__(settings) + self._status_handling = StatusHandling(settings, union_legacy_meta=True) + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: + o = super().from_crawler(crawler) + crawler.signals.connect(o.spider_opened, signal=signals.spider_opened) + return o + + def spider_opened(self, spider: Spider) -> None: + self._status_handling.spider_opened(spider) + @_warn_spider_arg def process_response( self, request: Request, response: Response, spider: Spider | None = None ) -> Request | Response: - if ( - request.meta.get("dont_redirect", False) - or response.status - in getattr(self.crawler.spider, "handle_httpstatus_list", ()) - or response.status in request.meta.get("handle_httpstatus_list", ()) - or request.meta.get("handle_httpstatus_all", False) + if request.meta.get("dont_redirect", False) or self._status_handling.handles( + response.status, request.meta ): return response diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 764f82a78..b33a48f58 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -29,7 +29,7 @@ from scrapy.utils.misc import arg_to_iter from scrapy.utils.python import global_object_name if TYPE_CHECKING: - from collections.abc import Awaitable, Callable + from collections.abc import Awaitable, Callable, Container # typing.Self requires Python 3.11 from typing_extensions import Self @@ -115,9 +115,10 @@ class MediaPipeline(ABC): self._handle_statuses(self.allow_redirects) def _handle_statuses(self, allow_redirects: bool) -> None: - self.handle_httpstatus_list = None - if allow_redirects: - self.handle_httpstatus_list = SequenceExclude(range(300, 400)) + # Redirects are either followed by the framework or handled here. + self.handle_http_codes: bool | Container[int] = ( + SequenceExclude(range(300, 400)) if allow_redirects else True + ) def _key_for_pipe( self, @@ -218,10 +219,7 @@ class MediaPipeline(ABC): return await maybe_deferred_to_future(wad) # it must return wad at last def _modify_media_request(self, request: Request) -> None: - if self.handle_httpstatus_list: - request.meta["handle_httpstatus_list"] = self.handle_httpstatus_list - else: - request.meta["handle_httpstatus_all"] = True + request.meta["handle_http_codes"] = self.handle_http_codes async def _check_media_to_download( self, request: Request, info: SpiderInfo, item: Any diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 993f2436d..6549ad04b 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -107,6 +107,7 @@ __all__ = [ "FTP_PASSWORD", "FTP_USER", "GCS_PROJECT_ID", + "HANDLE_HTTP_CODES", "HTTPAUTH_DOMAIN", "HTTPAUTH_PASS", "HTTPAUTH_USER", @@ -399,6 +400,8 @@ FTP_PASSWORD = "guest" # noqa: S105 GCS_PROJECT_ID = None +HANDLE_HTTP_CODES = None + HTTPAUTH_USER = "" HTTPAUTH_PASS = "" HTTPAUTH_DOMAIN = None diff --git a/scrapy/shell.py b/scrapy/shell.py index dfea00c46..c30a8c1a7 100644 --- a/scrapy/shell.py +++ b/scrapy/shell.py @@ -220,12 +220,10 @@ class Shell: else: url = any_to_uri(request_or_url) request = Request(url, dont_filter=True, **kwargs) - if redirect: - request.meta["handle_httpstatus_list"] = SequenceExclude( - range(300, 400) - ) - else: - request.meta["handle_httpstatus_all"] = True + # Redirects are either followed by the framework or handled here. + request.meta["handle_http_codes"] = ( + SequenceExclude(range(300, 400)) if redirect else True + ) response: Response | None = None if self._use_reactor: from twisted.internet import reactor diff --git a/scrapy/spidermiddlewares/httperror.py b/scrapy/spidermiddlewares/httperror.py index 156b73e7e..7fa432f8e 100644 --- a/scrapy/spidermiddlewares/httperror.py +++ b/scrapy/spidermiddlewares/httperror.py @@ -9,7 +9,9 @@ from __future__ import annotations import logging from typing import TYPE_CHECKING, Any +from scrapy import signals from scrapy.exceptions import IgnoreRequest +from scrapy.utils._httpstatus import StatusHandling from scrapy.utils.decorators import _warn_spider_arg if TYPE_CHECKING: @@ -28,48 +30,43 @@ logger = logging.getLogger(__name__) class HttpError(IgnoreRequest): - """A non-2xx response was filtered""" + """Raised by :class:`HttpErrorMiddleware` for a response whose status code + is not successful and that the spider does not handle itself. See + :setting:`HANDLE_HTTP_CODES`.""" def __init__(self, response: Response, *args: Any, **kwargs: Any): - self.response = response + #: The response that was filtered out. + self.response: Response = response super().__init__(*args, **kwargs) class HttpErrorMiddleware: + """Filter out unsuccessful (erroneous) HTTP responses so that spiders don't + have to deal with them, which (most of the time) imposes an overhead, + consumes more resources, and makes the spider logic more complex.""" + crawler: Crawler def __init__(self, settings: BaseSettings): - self.handle_httpstatus_all: bool = settings.getbool("HTTPERROR_ALLOW_ALL") - self.handle_httpstatus_list: list[int] = settings.getlist( - "HTTPERROR_ALLOWED_CODES" - ) + self._status_handling = StatusHandling(settings, legacy_settings=True) @classmethod def from_crawler(cls, crawler: Crawler) -> Self: o = cls(crawler.settings) o.crawler = crawler + crawler.signals.connect(o.spider_opened, signal=signals.spider_opened) return o + def spider_opened(self, spider: Spider) -> None: + self._status_handling.spider_opened(spider) + @_warn_spider_arg def process_spider_input( self, response: Response, spider: Spider | None = None ) -> None: if 200 <= response.status < 300: # common case return - meta = response.meta - if meta.get("handle_httpstatus_all", False): - return - if "handle_httpstatus_list" in meta: - allowed_statuses = meta["handle_httpstatus_list"] - elif self.handle_httpstatus_all: - return - else: - allowed_statuses = getattr( - self.crawler.spider, - "handle_httpstatus_list", - self.handle_httpstatus_list, - ) - if response.status in allowed_statuses: + if self._status_handling.handles(response.status, response.meta): return raise HttpError(response, "Ignoring non-200 response") diff --git a/scrapy/utils/_httpstatus.py b/scrapy/utils/_httpstatus.py new file mode 100644 index 000000000..97f5e6bb7 --- /dev/null +++ b/scrapy/utils/_httpstatus.py @@ -0,0 +1,184 @@ +"""Shared resolution of the statuses that a spider handles itself. + +Used by :class:`~scrapy.spidermiddlewares.httperror.HttpErrorMiddleware` and +:class:`~scrapy.downloadermiddlewares.redirect.RedirectMiddleware`. +""" + +from __future__ import annotations + +import warnings +from typing import TYPE_CHECKING, Any, cast +from weakref import WeakKeyDictionary + +from scrapy.exceptions import ScrapyDeprecationWarning +from scrapy.utils.deprecate import warn_on_deprecated_spider_attribute + +if TYPE_CHECKING: + from collections.abc import Container, Mapping + + from scrapy import Spider + from scrapy.settings import BaseSettings + + # True means every status, False means no status, and a container is + # checked for membership. + HandledCodes = bool | Container[int] + + +SETTING = "HANDLE_HTTP_CODES" +META_KEY = "handle_http_codes" + +_LEGACY_SETTING_ALL = "HTTPERROR_ALLOW_ALL" +_LEGACY_SETTING_LIST = "HTTPERROR_ALLOWED_CODES" +# Both a spider attribute and a request meta key. +_LEGACY_LIST = "handle_httpstatus_list" +_LEGACY_ALL = "handle_httpstatus_all" + +# Same string values that BaseSettings.getbool() accepts. +_TRUE_STRINGS = frozenset({"1", "True", "true"}) +_FALSE_STRINGS = frozenset({"0", "False", "false"}) + +_warned_meta_keys: WeakKeyDictionary[Any, set[str]] = WeakKeyDictionary() + + +def normalize(value: Any) -> HandledCodes: + """Return *value* as either a boolean or a container of status codes. + + Booleans are returned as they are, integers become single-code containers, + strings are parsed as they come from the command line or the environment, + and sequences have their items coerced to integers. Any other container is + returned untouched, so that objects such as + :class:`~scrapy.utils.datatypes.SequenceExclude` keep working. + """ + if isinstance(value, bool): + return value + if isinstance(value, int): + return frozenset({value}) + if isinstance(value, str): + value = value.strip() + if not value: + return False + if value in _TRUE_STRINGS: + return True + if value in _FALSE_STRINGS: + return False + return frozenset(int(code) for code in value.split(",")) + if isinstance(value, (list, tuple, set, frozenset)): + return frozenset(int(code) for code in value) + if not hasattr(value, "__contains__"): + raise ValueError( + f"Unsupported {SETTING} value: {value!r}. Expected a boolean, an " + f"integer, a string, or a container of integers." + ) + return cast("Container[int]", value) + + +def matches(value: HandledCodes | None, status: int) -> bool: + if value is True: + return True + if not value: # False, None or an empty container + return False + return status in value + + +class StatusHandling: + """Tell whether the spider handles a response status code itself. + + *legacy_settings* enables reading the deprecated + :setting:`HTTPERROR_ALLOWED_CODES` and :setting:`HTTPERROR_ALLOW_ALL` + settings, which only ever applied to + :class:`~scrapy.spidermiddlewares.httperror.HttpErrorMiddleware`. + + *union_legacy_meta* combines a deprecated request meta key with the + deprecated spider attribute, instead of overriding it, as + :class:`~scrapy.downloadermiddlewares.redirect.RedirectMiddleware` used to + do. + + :meth:`spider_opened` must be called on the ``spider_opened`` signal, so + that the deprecated ``handle_httpstatus_list`` spider attribute is taken + into account. + """ + + def __init__( + self, + settings: BaseSettings, + *, + legacy_settings: bool = False, + union_legacy_meta: bool = False, + ): + self._settings_value: HandledCodes = self._from_settings( + settings, legacy_settings + ) + self._union_legacy_meta = union_legacy_meta + self._spider_value: HandledCodes | None = None + self._warning_scope: Any = self + + def spider_opened(self, spider: Spider) -> None: + # Deprecation warnings about request meta keys are emitted once per + # crawl, no matter how many components ask about the same key. + self._warning_scope = getattr(spider, "crawler", None) or self + value = getattr(spider, _LEGACY_LIST, None) + if value is None: + return + warn_on_deprecated_spider_attribute(_LEGACY_LIST, SETTING) + self._spider_value = normalize(value) + + def handles(self, status: int, meta: Mapping[str, Any]) -> bool: + value, from_legacy_meta = self._from_meta(meta) + if value is None: + value = self._spider_value + if value is None: + value = self._settings_value + handled = matches(value, status) + if handled or not (from_legacy_meta and self._union_legacy_meta): + return handled + return matches(self._spider_value, status) + + @staticmethod + def _from_settings(settings: BaseSettings, legacy: bool) -> HandledCodes: + value = settings.get(SETTING) + if value is not None: + return normalize(value) + if not legacy: + return False + for name in (_LEGACY_SETTING_ALL, _LEGACY_SETTING_LIST): + if (settings.getpriority(name) or 0) > 0: + warnings.warn( + f"The {name} setting is deprecated, use {SETTING} instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + if settings.getbool(_LEGACY_SETTING_ALL): + return True + return normalize(settings.getlist(_LEGACY_SETTING_LIST)) + + def _from_meta(self, meta: Mapping[str, Any]) -> tuple[HandledCodes | None, bool]: + """Return the value that *meta* defines, if any, and whether it comes + from a deprecated meta key.""" + value = meta.get(META_KEY) + if value is not None: + return normalize(value), False + # The deprecated keys used to be checked in this order: a true + # handle_httpstatus_all took precedence over handle_httpstatus_list, + # and a false one was only taken into account on its own. + legacy_all = meta.get(_LEGACY_ALL) + if legacy_all: + self._warn_meta_key(_LEGACY_ALL) + return True, True + if _LEGACY_LIST in meta: + self._warn_meta_key(_LEGACY_LIST) + return normalize(meta[_LEGACY_LIST]), True + if legacy_all is not None: + self._warn_meta_key(_LEGACY_ALL) + return False, True + return None, False + + def _warn_meta_key(self, key: str) -> None: + warned = _warned_meta_keys.setdefault(self._warning_scope, set()) + if key in warned: + return + warned.add(key) + warnings.warn( + f"The {key!r} request meta key is deprecated, use {META_KEY!r} instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) diff --git a/tests/test_downloadermiddleware_redirect.py b/tests/test_downloadermiddleware_redirect.py index ef2774a93..0f58f1591 100644 --- a/tests/test_downloadermiddleware_redirect.py +++ b/tests/test_downloadermiddleware_redirect.py @@ -4,7 +4,7 @@ from unittest.mock import MagicMock import pytest from scrapy.downloadermiddlewares.redirect import RedirectMiddleware -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.http import Request, Response from scrapy.spidermiddlewares.referer import ( POLICY_NO_REFERRER, @@ -296,28 +296,106 @@ class TestRedirectMiddleware(TestRedirectBase): assert req2.url == url3 assert req2.method == "HEAD" - def test_spider_handling(self): - self.mw.crawler.spider.handle_httpstatus_list = [404, 301, 302] - url = "http://www.example.com/301" + def _assert_passthrough(self, req, mw=None): url2 = "http://www.example.com/redirected" - req = Request(url) - rsp = Response(url, headers={"Location": url2}, status=301) - r = self.mw.process_response(req, rsp) - assert r is rsp + rsp = Response(req.url, headers={"Location": url2}, status=301, request=req) + assert (mw or self.mw).process_response(req, rsp) is rsp + + def _assert_redirected(self, req, mw=None): + url2 = "http://www.example.com/redirected" + rsp = Response(req.url, headers={"Location": url2}, status=301, request=req) + assert isinstance((mw or self.mw).process_response(req, rsp), Request) + + def test_setting_handling(self): + url = "http://www.example.com/301" + crawler = get_crawler(DefaultSpider, {"HANDLE_HTTP_CODES": [404, 301, 302]}) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + self._assert_passthrough(Request(url), mw) + + def test_setting_handling_all(self): + url = "http://www.example.com/301" + crawler = get_crawler(DefaultSpider, {"HANDLE_HTTP_CODES": True}) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + self._assert_passthrough(Request(url), mw) + + def test_setting_handling_unrelated_code(self): + url = "http://www.example.com/301" + crawler = get_crawler(DefaultSpider, {"HANDLE_HTTP_CODES": [404]}) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + self._assert_redirected(Request(url), mw) + + def test_httperror_settings_ignored(self): + """The deprecated HTTPERROR_* settings never applied to redirects.""" + url = "http://www.example.com/301" + crawler = get_crawler(DefaultSpider, {"HTTPERROR_ALLOW_ALL": True}) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + self._assert_redirected(Request(url), mw) + + def test_spider_handling(self): + mw = self._spider_attribute_mw([404, 301, 302]) + self._assert_passthrough(Request("http://www.example.com/301"), mw) def test_request_meta_handling(self): url = "http://www.example.com/301" - url2 = "http://www.example.com/redirected" + self._assert_passthrough(Request(url, meta={"handle_http_codes": [301]})) + self._assert_passthrough(Request(url, meta={"handle_http_codes": True})) + self._assert_redirected(Request(url, meta={"handle_http_codes": [404]})) + self._assert_redirected(Request(url, meta={"handle_http_codes": False})) - def _test_passthrough(req): - rsp = Response(url, headers={"Location": url2}, status=301, request=req) - r = self.mw.process_response(req, rsp) - assert r is rsp + def test_request_meta_handling_deprecated(self): + url = "http://www.example.com/301" + with pytest.warns(ScrapyDeprecationWarning): + self._assert_passthrough( + Request(url, meta={"handle_httpstatus_list": [404, 301, 302]}) + ) + with pytest.warns(ScrapyDeprecationWarning): + self._assert_passthrough(Request(url, meta={"handle_httpstatus_all": True})) + # Already warned about above, hence no warning expected here. + self._assert_redirected(Request(url, meta={"handle_httpstatus_list": [404]})) - _test_passthrough( - Request(url, meta={"handle_httpstatus_list": [404, 301, 302]}) + def _spider_attribute_mw(self, codes): + class HandlingSpider(DefaultSpider): + handle_httpstatus_list = codes + + crawler = get_crawler(HandlingSpider) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + with pytest.warns(ScrapyDeprecationWarning): + mw.spider_opened(crawler.spider) + return mw + + @pytest.mark.parametrize( + "meta", + [ + {"handle_httpstatus_list": [404]}, + {"handle_httpstatus_all": False}, + ], + ) + def test_deprecated_meta_combines_with_spider_attribute(self, meta): + """This middleware used to combine the deprecated meta key with the + deprecated spider attribute, instead of overriding it.""" + mw = self._spider_attribute_mw([301]) + with pytest.warns(ScrapyDeprecationWarning): + self._assert_passthrough( + Request("http://www.example.com/301", meta=meta), mw + ) + + def test_meta_overrides_spider_attribute(self): + mw = self._spider_attribute_mw([301]) + self._assert_redirected( + Request("http://www.example.com/301", meta={"handle_http_codes": [404]}), mw ) - _test_passthrough(Request(url, meta={"handle_httpstatus_all": True})) + + def test_request_meta_overrides_setting(self): + url = "http://www.example.com/301" + crawler = get_crawler(DefaultSpider, {"HANDLE_HTTP_CODES": True}) + crawler.spider = crawler._create_spider() + mw = self.mwcls.from_crawler(crawler) + self._assert_redirected(Request(url, meta={"handle_http_codes": False}), mw) def test_latin1_location(self): req = Request("http://scrapytest.org/first") diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index 23c19e4be..426c93fe7 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -58,7 +58,7 @@ class TestBaseMediaPipeline: def test_modify_media_request(self): request = Request("http://url") self.pipe._modify_media_request(request) - assert request.meta == {"handle_httpstatus_all": True} + assert request.meta == {"handle_http_codes": True} def test_should_remove_req_res_references_before_caching_the_results(self): """Regression test case to prevent a memory leak in the Media Pipeline. @@ -391,7 +391,7 @@ class TestMediaPipelineAllowRedirectSettings: request = Request("http://url") pipe._modify_media_request(request) - assert "handle_httpstatus_list" in request.meta + assert "handle_http_codes" in request.meta for status, check in [ (200, True), # These are the status codes we want @@ -407,9 +407,9 @@ class TestMediaPipelineAllowRedirectSettings: (500, True), ]: if check: - assert status in request.meta["handle_httpstatus_list"] + assert status in request.meta["handle_http_codes"] else: - assert status not in request.meta["handle_httpstatus_list"] + assert status not in request.meta["handle_http_codes"] def test_subclass_standard_setting(self): self._assert_request_no3xx(UserDefinedPipeline, {"MEDIA_ALLOW_REDIRECTS": True}) diff --git a/tests/test_spidermiddleware_httperror.py b/tests/test_spidermiddleware_httperror.py index 2ba083fd6..8fe16f379 100644 --- a/tests/test_spidermiddleware_httperror.py +++ b/tests/test_spidermiddleware_httperror.py @@ -1,18 +1,22 @@ from __future__ import annotations import logging -from typing import TYPE_CHECKING +import warnings +from typing import TYPE_CHECKING, Any import pytest +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http import Request, Response from scrapy.spidermiddlewares.httperror import HttpError, HttpErrorMiddleware +from scrapy.utils.datatypes import SequenceExclude from scrapy.utils.spider import DefaultSpider from scrapy.utils.test import get_crawler from tests.spiders import MockServerSpider from tests.utils.decorators import coroutine_test if TYPE_CHECKING: + from scrapy import Spider from tests.mockserver.http import MockServer @@ -74,12 +78,20 @@ def res404() -> Response: return _response(req, 404) +def _mw( + settings: dict[str, Any] | None = None, spidercls: type[Spider] = DefaultSpider +) -> HttpErrorMiddleware: + crawler = get_crawler(spidercls, settings) + crawler.spider = crawler._create_spider() + mw = HttpErrorMiddleware.from_crawler(crawler) + mw.spider_opened(crawler.spider) + return mw + + class TestHttpErrorMiddleware: @pytest.fixture def mw(self) -> HttpErrorMiddleware: - crawler = get_crawler(DefaultSpider) - crawler.spider = crawler._create_spider() - return HttpErrorMiddleware.from_crawler(crawler) + return _mw() def test_process_spider_input( self, mw: HttpErrorMiddleware, res200: Response, res404: Response @@ -94,18 +106,13 @@ class TestHttpErrorMiddleware: assert mw.process_spider_exception(res404, HttpError(res404)) == () assert mw.process_spider_exception(res404, Exception()) is None - def test_handle_httpstatus_list( - self, mw: HttpErrorMiddleware, res404: Response - ) -> None: - request = Request( - "http://scrapytest.org", meta={"handle_httpstatus_list": [404]} - ) - res = _response(request, 404) - mw.process_spider_input(res) - - assert mw.crawler.spider - mw.crawler.spider.handle_httpstatus_list = [404] # type: ignore[attr-defined] - mw.process_spider_input(res404) + def test_meta(self, mw: HttpErrorMiddleware, res404: Response) -> None: + request = Request("http://example.com", meta={"handle_http_codes": [404]}) + mw.process_spider_input(_response(request, 404)) + with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 402)) + with pytest.raises(HttpError): + mw.process_spider_input(res404) class TestHttpErrorMiddlewareSettings: @@ -113,9 +120,7 @@ class TestHttpErrorMiddlewareSettings: @pytest.fixture def mw(self) -> HttpErrorMiddleware: - crawler = get_crawler(DefaultSpider, {"HTTPERROR_ALLOWED_CODES": (402,)}) - crawler.spider = crawler._create_spider() - return HttpErrorMiddleware.from_crawler(crawler) + return _mw({"HANDLE_HTTP_CODES": (402,)}) def test_process_spider_input( self, @@ -130,32 +135,25 @@ class TestHttpErrorMiddlewareSettings: mw.process_spider_input(res402) def test_meta_overrides_settings(self, mw: HttpErrorMiddleware) -> None: - request = Request( - "http://scrapytest.org", meta={"handle_httpstatus_list": [404]} - ) - res404 = _response(request, 404) - res402 = _response(request, 402) - - mw.process_spider_input(res404) + request = Request("http://example.com", meta={"handle_http_codes": [404]}) + mw.process_spider_input(_response(request, 404)) with pytest.raises(HttpError): - mw.process_spider_input(res402) + mw.process_spider_input(_response(request, 402)) - def test_spider_override_settings( - self, mw: HttpErrorMiddleware, res402: Response, res404: Response - ) -> None: - assert mw.crawler.spider - mw.crawler.spider.handle_httpstatus_list = [404] # type: ignore[attr-defined] - mw.process_spider_input(res404) + def test_meta_false_overrides_settings(self, mw: HttpErrorMiddleware) -> None: + request = Request("http://example.com", meta={"handle_http_codes": False}) with pytest.raises(HttpError): - mw.process_spider_input(res402) + mw.process_spider_input(_response(request, 402)) + + def test_meta_none_ignored(self, mw: HttpErrorMiddleware) -> None: + request = Request("http://example.com", meta={"handle_http_codes": None}) + mw.process_spider_input(_response(request, 402)) class TestHttpErrorMiddlewareHandleAll: @pytest.fixture def mw(self) -> HttpErrorMiddleware: - crawler = get_crawler(DefaultSpider, {"HTTPERROR_ALLOW_ALL": True}) - crawler.spider = crawler._create_spider() - return HttpErrorMiddleware.from_crawler(crawler) + return _mw({"HANDLE_HTTP_CODES": True}) def test_process_spider_input( self, @@ -167,31 +165,199 @@ class TestHttpErrorMiddlewareHandleAll: mw.process_spider_input(res404) def test_meta_overrides_settings(self, mw: HttpErrorMiddleware) -> None: - request = Request( - "http://scrapytest.org", meta={"handle_httpstatus_list": [404]} - ) - res404 = _response(request, 404) - res402 = _response(request, 402) - - mw.process_spider_input(res404) + request = Request("http://example.com", meta={"handle_http_codes": [404]}) + mw.process_spider_input(_response(request, 404)) with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 402)) + + def test_meta_false_overrides_settings(self, mw: HttpErrorMiddleware) -> None: + request = Request("http://example.com", meta={"handle_http_codes": False}) + with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 402)) + + +class TestHttpErrorMiddlewareValues: + """Every supported way to express a HANDLE_HTTP_CODES value.""" + + @pytest.mark.parametrize( + ("value", "handled"), + [ + (True, True), + (False, False), + (None, False), + (402, True), + ("402", True), + ("402,404", True), + ("True", True), + ("true", True), + ("1", True), + ("False", False), + ("false", False), + ("0", False), + ("", False), + ([402], True), + (["402"], True), + ((402,), True), + ({402}, True), + (frozenset({402}), True), + ([], False), + ([404], False), + (SequenceExclude(range(300, 400)), True), + ], + ) + def test_setting(self, value: Any, handled: bool) -> None: + mw = _mw({"HANDLE_HTTP_CODES": value}) + res402 = _response(Request("http://example.com"), 402) + if handled: mw.process_spider_input(res402) + else: + with pytest.raises(HttpError): + mw.process_spider_input(res402) - def test_httperror_allow_all_false(self) -> None: - crawler = get_crawler(_HttpErrorSpider) - mw = HttpErrorMiddleware.from_crawler(crawler) - request_httpstatus_false = Request( - "http://scrapytest.org", meta={"handle_httpstatus_all": False} - ) - request_httpstatus_true = Request( - "http://scrapytest.org", meta={"handle_httpstatus_all": True} - ) - res404 = _response(request_httpstatus_false, 404) - res402 = _response(request_httpstatus_true, 402) + @pytest.mark.parametrize( + ("value", "handled"), + [ + (True, True), + (False, False), + (402, True), + ("402", True), + ([402], True), + ([404], False), + (SequenceExclude(range(300, 400)), True), + ], + ) + def test_meta(self, value: Any, handled: bool) -> None: + mw = _mw() + request = Request("http://example.com", meta={"handle_http_codes": value}) + if handled: + mw.process_spider_input(_response(request, 402)) + else: + with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 402)) + def test_unsupported_value(self) -> None: + with pytest.raises(ValueError, match="Unsupported HANDLE_HTTP_CODES value"): + _mw({"HANDLE_HTTP_CODES": object()}) + + +class TestHttpErrorMiddlewareDeprecated: + def test_settings(self) -> None: + with pytest.warns( + ScrapyDeprecationWarning, match="HTTPERROR_ALLOWED_CODES setting" + ): + mw = _mw({"HTTPERROR_ALLOWED_CODES": (402,)}) + res402 = _response(Request("http://example.com"), 402) + res404 = _response(Request("http://example.com"), 404) + mw.process_spider_input(res402) with pytest.raises(HttpError): mw.process_spider_input(res404) - mw.process_spider_input(res402) + + def test_allow_all_setting(self) -> None: + with pytest.warns( + ScrapyDeprecationWarning, match="HTTPERROR_ALLOW_ALL setting" + ): + mw = _mw({"HTTPERROR_ALLOW_ALL": True}) + mw.process_spider_input(_response(Request("http://example.com"), 404)) + + def test_new_setting_wins(self) -> None: + mw = _mw({"HANDLE_HTTP_CODES": False, "HTTPERROR_ALLOW_ALL": True}) + with pytest.raises(HttpError): + mw.process_spider_input(_response(Request("http://example.com"), 404)) + + def test_spider_attribute(self) -> None: + class HandlingSpider(DefaultSpider): + handle_httpstatus_list = [404] + + with pytest.warns( + ScrapyDeprecationWarning, match="'handle_httpstatus_list' spider attribute" + ): + mw = _mw(spidercls=HandlingSpider) + mw.process_spider_input(_response(Request("http://example.com"), 404)) + with pytest.raises(HttpError): + mw.process_spider_input(_response(Request("http://example.com"), 402)) + + def test_spider_attribute_overrides_settings(self) -> None: + class HandlingSpider(DefaultSpider): + handle_httpstatus_list = [404] + + with pytest.warns(ScrapyDeprecationWarning): + mw = _mw({"HANDLE_HTTP_CODES": [402]}, spidercls=HandlingSpider) + mw.process_spider_input(_response(Request("http://example.com"), 404)) + with pytest.raises(HttpError): + mw.process_spider_input(_response(Request("http://example.com"), 402)) + + def test_meta_overrides_spider_attribute(self) -> None: + """Unlike RedirectMiddleware, this middleware never combined the + deprecated meta key with the deprecated spider attribute.""" + + class HandlingSpider(DefaultSpider): + handle_httpstatus_list = [404] + + with pytest.warns(ScrapyDeprecationWarning): + mw = _mw(spidercls=HandlingSpider) + request = Request("http://example.com", meta={"handle_httpstatus_list": [402]}) + with pytest.warns(ScrapyDeprecationWarning), pytest.raises(HttpError): + mw.process_spider_input(_response(request, 404)) + + def test_meta_list(self) -> None: + mw = _mw() + request = Request("http://example.com", meta={"handle_httpstatus_list": [404]}) + with pytest.warns( + ScrapyDeprecationWarning, match="'handle_httpstatus_list' request meta key" + ): + mw.process_spider_input(_response(request, 404)) + with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 402)) + + def test_meta_all(self) -> None: + mw = _mw() + request = Request("http://example.com", meta={"handle_httpstatus_all": True}) + with pytest.warns( + ScrapyDeprecationWarning, match="'handle_httpstatus_all' request meta key" + ): + mw.process_spider_input(_response(request, 404)) + + def test_meta_all_false(self) -> None: + mw = _mw({"HANDLE_HTTP_CODES": True}) + request = Request("http://example.com", meta={"handle_httpstatus_all": False}) + with pytest.warns(ScrapyDeprecationWarning), pytest.raises(HttpError): + mw.process_spider_input(_response(request, 404)) + + def test_meta_all_true_beats_meta_list(self) -> None: + mw = _mw() + request = Request( + "http://example.com", + meta={"handle_httpstatus_all": True, "handle_httpstatus_list": [404]}, + ) + with pytest.warns(ScrapyDeprecationWarning): + mw.process_spider_input(_response(request, 402)) + + def test_meta_all_false_defers_to_meta_list(self) -> None: + mw = _mw() + request = Request( + "http://example.com", + meta={"handle_httpstatus_all": False, "handle_httpstatus_list": [404]}, + ) + with pytest.warns(ScrapyDeprecationWarning): + mw.process_spider_input(_response(request, 404)) + + def test_new_meta_key_wins(self) -> None: + mw = _mw() + request = Request( + "http://example.com", + meta={"handle_http_codes": False, "handle_httpstatus_all": True}, + ) + with pytest.raises(HttpError): + mw.process_spider_input(_response(request, 404)) + + def test_meta_key_warning_once_per_crawl(self) -> None: + mw = _mw() + request = Request("http://example.com", meta={"handle_httpstatus_all": True}) + with pytest.warns(ScrapyDeprecationWarning): + mw.process_spider_input(_response(request, 404)) + with warnings.catch_warnings(): + warnings.simplefilter("error") + mw.process_spider_input(_response(request, 404)) class TestHttpErrorMiddlewareIntegrational: @@ -211,6 +377,22 @@ class TestHttpErrorMiddlewareIntegrational: assert get_value("httperror/response_ignored_status_count/402") == 1 assert get_value("httperror/response_ignored_status_count/500") == 1 + @coroutine_test + async def test_setting(self, mockserver: MockServer) -> None: + crawler = get_crawler(_HttpErrorSpider, {"HANDLE_HTTP_CODES": [402, 404]}) + await crawler.crawl_async(mockserver=mockserver) + assert isinstance(crawler.spider, _HttpErrorSpider) + assert crawler.spider.parsed == {"200", "402", "404"} + assert crawler.spider.failed == {"500"} + + @coroutine_test + async def test_setting_all(self, mockserver: MockServer) -> None: + crawler = get_crawler(_HttpErrorSpider, {"HANDLE_HTTP_CODES": True}) + await crawler.crawl_async(mockserver=mockserver) + assert isinstance(crawler.spider, _HttpErrorSpider) + assert crawler.spider.parsed == {"200", "402", "404", "500"} + assert not crawler.spider.failed + @coroutine_test async def test_logging( self, caplog: pytest.LogCaptureFixture, mockserver: MockServer