From bc5b5fb1f6d8fef2f57ef74a728dd609b5c00cf4 Mon Sep 17 00:00:00 2001 From: Youssef Mohamed <114195599+MegumiinUwU@users.noreply.github.com> Date: Wed, 29 Jul 2026 11:47:15 +0300 Subject: [PATCH 1/4] Add Request.to_curl() (#7743) (#7802) --- docs/topics/request-response.rst | 2 ++ scrapy/http/request/__init__.py | 14 ++++++++++++++ tests/test_utils_request.py | 11 +++++++++++ tests/utils/bases/http_request.py | 9 +++++++++ 4 files changed, 36 insertions(+) diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index b83a04032..8e565907f 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -260,6 +260,8 @@ Request objects .. automethod:: from_curl + .. automethod:: to_curl + .. automethod:: to_dict diff --git a/scrapy/http/request/__init__.py b/scrapy/http/request/__init__.py index 57517a65b..68847283a 100644 --- a/scrapy/http/request/__init__.py +++ b/scrapy/http/request/__init__.py @@ -387,6 +387,20 @@ class Request(object_ref): request_kwargs.update(kwargs) return cls(**request_kwargs) + def to_curl(self) -> str: + """Return a string with a `cURL `_ command equivalent + to this request. + + Inverse of :meth:`from_curl`. See also + :func:`scrapy.utils.request.request_to_curl`. + + .. versionadded:: VERSION + """ + # Imported here to avoid a circular import. + from scrapy.utils.request import request_to_curl # noqa: PLC0415 + + return request_to_curl(self) + def to_dict(self, *, spider: scrapy.Spider | None = None) -> dict[str, Any]: """Return a dictionary containing the Request's data. diff --git a/tests/test_utils_request.py b/tests/test_utils_request.py index eb95be06c..935447bc4 100644 --- a/tests/test_utils_request.py +++ b/tests/test_utils_request.py @@ -475,3 +475,14 @@ class TestRequestToCurl: " --data-raw '{\"foo\": \"bar\"}' --cookie 'foo=1'" ) self._test_request(request_object, expected_curl_command) + + def test_request_to_curl_method(self) -> None: + request_object = Request( + "https://www.httpbin.org/post", + method="POST", + body=json.dumps({"foo": "bar"}), + ) + expected_curl_command = ( + 'curl -X POST https://www.httpbin.org/post --data-raw \'{"foo": "bar"}\'' + ) + assert request_object.to_curl() == expected_curl_command diff --git a/tests/utils/bases/http_request.py b/tests/utils/bases/http_request.py index c255b5e4c..3a1e588ef 100644 --- a/tests/utils/bases/http_request.py +++ b/tests/utils/bases/http_request.py @@ -6,6 +6,7 @@ import pytest from scrapy.http import Headers, Request from scrapy.http.request import NO_CALLBACK +from scrapy.utils.request import request_to_curl class TestRequestBase(ABC): @@ -488,3 +489,11 @@ class TestRequestBase(ABC): 'curl -X PATCH "http://example.org" --foo -z', ignore_unknown_options=False, ) + + def test_to_curl(self): + # Note: more curated tests regarding curl conversion are in + # `test_utils_request.py` + r = self.request_class( + "http://www.example.com/", method="POST", body=b"foo=bar" + ) + assert r.to_curl() == request_to_curl(r) From 8b5147ae2ec62bca66e3b957a8d0f1c3654ecf2e Mon Sep 17 00:00:00 2001 From: Adrian Date: Wed, 29 Jul 2026 11:32:18 +0200 Subject: [PATCH 2/4] Improve test coverage for scrapy.pipelines (#7798) * Improve test coverage for scrapy.pipelines * Restore old Pillow support --- tests/test_pipeline_files.py | 220 ++++++++++++++++++++++++++++++--- tests/test_pipeline_images.py | 52 +++++++- tests/test_pipeline_media.py | 60 +++++++++ tests/utils/media_pipelines.py | 6 + 4 files changed, 321 insertions(+), 17 deletions(-) diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index 4f6fa21a0..97daa514e 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -33,7 +33,7 @@ from scrapy.pipelines.files import ( GCSFilesStore, S3FilesStore, ) -from scrapy.pipelines.media import MediaPipeline, _MediaRequestFiltered +from scrapy.pipelines.media import _MediaRequestFiltered from scrapy.settings import Settings from scrapy.utils.asyncio import call_later from scrapy.utils.defer import maybe_deferred_to_future @@ -43,11 +43,7 @@ from tests.mockserver.ftp import MockFTPServer from tests.utils.decorators import coroutine_test, inline_callbacks_test from .utils.cloud import mock_google_cloud_storage -from .utils.media_pipelines import mocked_download_func - -# required by persist_file() and stat_file(), but as some stores don't use the argument -# we can pass this singleton to keep type hints correct -DUMMY_SPIDER_INFO = MediaPipeline.SpiderInfo(DefaultSpider()) +from .utils.media_pipelines import DUMMY_SPIDER_INFO, mocked_download_func def get_ftp_content_and_delete( @@ -94,16 +90,19 @@ class DeferredFSFilesStore(FSFilesStore): class TestFilesPipeline: def setup_method(self): self.tempdir = mkdtemp() - settings_dict = {"FILES_STORE": self.tempdir} - crawler = get_crawler(DefaultSpider, settings_dict=settings_dict) - crawler.spider = crawler._create_spider() - crawler.engine = MagicMock(download_async=mocked_download_func) - self.pipeline = FilesPipeline.from_crawler(crawler) - self.pipeline.open_spider() + self.pipeline = self._create_pipeline(FilesPipeline) def teardown_method(self): rmtree(self.tempdir) + def _create_pipeline(self, pipeline_cls: type[FilesPipeline]) -> FilesPipeline: + crawler = get_crawler(DefaultSpider, {"FILES_STORE": self.tempdir}) + crawler.spider = crawler._create_spider() + crawler.engine = MagicMock(download_async=mocked_download_func) + pipeline = pipeline_cls.from_crawler(crawler) + pipeline.open_spider() + return pipeline + def test_file_path_query_parameters(self): file_path = self.pipeline.file_path @@ -254,6 +253,107 @@ class TestFilesPipeline: assert result["files"][0]["checksum"] != "abc" assert result["files"][0]["status"] == "cached" + @coroutine_test + async def test_file_stat_without_last_modified(self) -> None: + """A stat result without a last modification time forces a download.""" + item_url = "http://example.com/file4.pdf" + item = _create_item_with_files(item_url) + with ( + mock.patch.object(FilesPipeline, "inc_stats", return_value=True), + mock.patch.object( + FSFilesStore, "stat_file", return_value={"checksum": "abc"} + ), + mock.patch.object( + FilesPipeline, + "get_media_requests", + return_value=[_prepare_request_object(item_url)], + ), + ): + result = await self.pipeline.process_item(item) + assert result["files"][0]["checksum"] != "abc" + assert result["files"][0]["status"] == "downloaded" + + @coroutine_test + async def test_file_empty_content(self, caplog: pytest.LogCaptureFixture) -> None: + item_url = "http://example.com/empty.pdf" + item = _create_item_with_files(item_url) + request = Request( + item_url, meta={"response": Response(item_url, status=200, body=b"")} + ) + with ( + caplog.at_level(logging.WARNING), + mock.patch.object( + FilesPipeline, "get_media_requests", return_value=[request] + ), + ): + result = await self.pipeline.process_item(item) + assert result["files"] == [] + assert "File (empty-content): Empty file from" in caplog.text + + @coroutine_test + async def test_file_downloaded_file_exception( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """A FileException from file_downloaded() is logged as a warning and + kept as is.""" + + class FailingFilesPipeline(FilesPipeline): + def file_downloaded(self, response, request, info, *, item=None): + raise FileException("boom") + + item_url = "http://example.com/file5.pdf" + item = _create_item_with_files(item_url) + pipeline = self._create_pipeline(FailingFilesPipeline) + with ( + caplog.at_level(logging.WARNING), + mock.patch.object( + FilesPipeline, + "get_media_requests", + return_value=[_prepare_request_object(item_url)], + ), + ): + result = await pipeline.process_item(item) + assert result["files"] == [] + records = [ + r for r in caplog.records if "Error processing file" in r.getMessage() + ] + assert len(records) == 1 + assert records[0].levelname == "WARNING" + assert "boom" in records[0].getMessage() + + @coroutine_test + async def test_file_downloaded_unknown_error( + self, caplog: pytest.LogCaptureFixture + ) -> None: + """Any other exception from file_downloaded() is logged as an error and + reported as a FileException.""" + + class FailingFilesPipeline(FilesPipeline): + def file_downloaded(self, response, request, info, *, item=None): + raise RuntimeError("boom") + + item_url = "http://example.com/file6.pdf" + item = _create_item_with_files(item_url) + pipeline = self._create_pipeline(FailingFilesPipeline) + with ( + caplog.at_level(logging.WARNING), + mock.patch.object( + FilesPipeline, + "get_media_requests", + return_value=[_prepare_request_object(item_url)], + ), + ): + result = await pipeline.process_item(item) + assert result["files"] == [] + records = [ + r for r in caplog.records if "Error processing file" in r.getMessage() + ] + assert len(records) == 1 + assert records[0].levelname == "ERROR" + exc_info = records[0].exc_info + assert exc_info is not None + assert exc_info[0] is RuntimeError + @coroutine_test async def test_async_store(self) -> None: """Test that async persist_file() works and is awaited.""" @@ -648,9 +748,24 @@ class TestFilesPipelineCustomSettings: request = Request("http://example.com/image01.jpg") assert pipeline.file_path(request) == Path("subdir/image01.jpg") - def test_files_store_constructor_with_pathlike_object(self, tmp_path): - fs_store = FSFilesStore(tmp_path) - assert fs_store.basedir == str(tmp_path) + +class TestFSFilesStore: + def test_constructor_with_pathlike_object(self, tmp_path: Path) -> None: + assert FSFilesStore(tmp_path).basedir == str(tmp_path) + + def test_constructor_with_uri(self, tmp_path: Path) -> None: + assert FSFilesStore(f"file://{tmp_path}").basedir == str(tmp_path) + + def test_stat_file(self, tmp_path: Path) -> None: + store = FSFilesStore(tmp_path) + store.persist_file("full/filename", BytesIO(b"data"), DUMMY_SPIDER_INFO) + stat = store.stat_file("full/filename", DUMMY_SPIDER_INFO) + assert stat["checksum"] == "8d777f385d3dfec8815d20f7496026dc" + assert stat["last_modified"] == pytest.approx(time.time(), abs=60) + + def test_stat_missing_file(self, tmp_path: Path) -> None: + store = FSFilesStore(tmp_path) + assert store.stat_file("full/filename", DUMMY_SPIDER_INFO) == {} @pytest.mark.requires_botocore @@ -695,6 +810,59 @@ class TestS3FilesStore: # The call to read does not happen with Stubber assert buffer.method_calls == [mock.call.seek(0)] + @inline_callbacks_test + def test_persist_without_headers(self): + """Without custom headers only the default ones are sent.""" + bucket = "mybucket" + key = "export.csv" + buffer = mock.MagicMock() + + store = S3FilesStore(f"s3://{bucket}/{key}") + from botocore.stub import Stubber # noqa: PLC0415 + + with Stubber(store.s3_client) as stub: + stub.add_response( + "put_object", + expected_params={ + "ACL": S3FilesStore.POLICY, + "Body": buffer, + "Bucket": bucket, + "CacheControl": S3FilesStore.HEADERS["Cache-Control"], + "Key": key, + "Metadata": {}, + }, + service_response={}, + ) + + yield store.persist_file("", buffer, info=DUMMY_SPIDER_INFO) + + stub.assert_no_pending_responses() + + def test_missing_botocore(self): + with ( + mock.patch( + "scrapy.pipelines.files.is_botocore_available", return_value=False + ), + pytest.raises(NotConfigured, match="missing botocore library"), + ): + S3FilesStore("s3://mybucket/key") + + def test_wrong_uri_scheme(self): + with pytest.raises( + ValueError, + match=re.escape( + "Incorrect URI scheme in ftp://mybucket/key, expected 's3'" + ), + ): + S3FilesStore("ftp://mybucket/key") + + def test_unsupported_header(self): + store = S3FilesStore("s3://mybucket/key") + with pytest.raises( + TypeError, match='Header "X-Custom" is not supported by botocore' + ): + store._headers_to_botocore_kwargs({"X-Custom": "value"}) + @inline_callbacks_test def test_stat(self): bucket = "mybucket" @@ -901,6 +1069,28 @@ class TestFTPFileStore: ) assert data == content + @inline_callbacks_test + def test_persist_active_mode(self, monkeypatch: pytest.MonkeyPatch): + data = b"active mode" + path = "full/filename" + monkeypatch.setattr(FTPFilesStore, "FTP_USERNAME", "anonymous") + monkeypatch.setattr(FTPFilesStore, "FTP_PASSWORD", "guest") + monkeypatch.setattr(FTPFilesStore, "USE_ACTIVE_MODE", True) + with MockFTPServer() as ftp_server: + store = FTPFilesStore(ftp_server.url("/")) + yield store.persist_file(path, BytesIO(data), info=DUMMY_SPIDER_INFO) + stat = yield store.stat_file(path, info=DUMMY_SPIDER_INFO) + assert stat["checksum"] == "ff1575649a39a27c13faa0d37c84bab3" + + def test_wrong_uri_scheme(self): + with pytest.raises( + ValueError, + match=re.escape( + "Incorrect URI scheme in http://example.com/, expected 'ftp'" + ), + ): + FTPFilesStore("http://example.com/") + class ItemWithFiles(Item): file_urls = Field() diff --git a/tests/test_pipeline_images.py b/tests/test_pipeline_images.py index 1b73dd157..19e61579f 100644 --- a/tests/test_pipeline_images.py +++ b/tests/test_pipeline_images.py @@ -3,20 +3,26 @@ from __future__ import annotations import dataclasses import io import random +import sys from abc import ABC, abstractmethod +from pathlib import Path from shutil import rmtree from tempfile import mkdtemp +from types import SimpleNamespace from typing import Any import attr import pytest from itemadapter import ItemAdapter +from scrapy.exceptions import NotConfigured from scrapy.http import Request, Response from scrapy.item import Field, Item -from scrapy.pipelines.files import GCSFilesStore, S3FilesStore +from scrapy.pipelines.files import GCSFilesStore, S3FilesStore, _md5sum from scrapy.pipelines.images import ImageException, ImagesPipeline from scrapy.utils.test import get_crawler +from tests.utils.decorators import coroutine_test +from tests.utils.media_pipelines import DUMMY_SPIDER_INFO try: from PIL import Image @@ -40,6 +46,11 @@ class TestImagesPipeline: def teardown_method(self): rmtree(self.tempdir) + def test_missing_pillow(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setitem(sys.modules, "PIL", None) + with pytest.raises(NotConfigured, match="requires installing Pillow"): + ImagesPipeline(self.tempdir, crawler=get_crawler()) + def test_file_path(self): file_path = self.pipeline.file_path assert ( @@ -197,6 +208,25 @@ class TestImagesPipeline: assert path == "full/3fd165099d8e71b8a48b2683946e64dbfad8b52d.jpg" assert new_im.getpixel((0, 0)) == (255, 0, 0) + @coroutine_test + async def test_image_downloaded(self) -> None: + """The image and its thumbnails are stored, and the checksum of the + full-size image is returned.""" + self.pipeline.thumbs = {"small": (20, 20)} + _, buf = _create_image("JPEG", "RGB", (50, 50), (0, 0, 0)) + url = "https://dev.mydeco.com/mydeco.gif" + response = Response(url=url, body=buf.getvalue()) + + checksum = await self.pipeline.image_downloaded( + response, Request(url=url), DUMMY_SPIDER_INFO + ) + + buf.seek(0) + assert checksum == _md5sum(buf) + name = "3fd165099d8e71b8a48b2683946e64dbfad8b52d.jpg" + assert Path(self.tempdir, "full", name).read_bytes() == buf.getvalue() + assert Path(self.tempdir, "thumbs", "small", name).exists() + def test_convert_image(self): SIZE = (100, 100) # straight forward case: RGB and JPEG @@ -230,6 +260,24 @@ class TestImagesPipeline: assert converted.mode == "RGB" assert converted.getcolors() == [(10000, (205, 230, 255))] + def test_convert_image_legacy_resampling_filter( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Pillow older than 9.1.0 has Image.ANTIALIAS instead of + Image.Resampling.LANCZOS.""" + # Image.LANCZOS is the only spelling that exists in every supported + # Pillow version, but Pillow defines it dynamically, hence the ignore. + monkeypatch.setattr( + self.pipeline, + "_Image", + SimpleNamespace(ANTIALIAS=Image.LANCZOS), # type: ignore[attr-defined] + ) + im, buf = _create_image("JPEG", "RGB", (100, 100), (0, 127, 255)) + + thumbnail, _ = self.pipeline.convert_image(im, size=(10, 25), response_body=buf) + + assert thumbnail.size == (10, 10) + @pytest.mark.parametrize( "bad_type", [ @@ -581,7 +629,7 @@ class TestImagesPipelineCustomSettings: GCSFilesStore.POLICY = old_policy -def _create_image(format_, *a, **kw): +def _create_image(format_: str, *a: Any, **kw: Any) -> tuple[Image.Image, io.BytesIO]: buf = io.BytesIO() Image.new(*a, **kw).save(buf, format_) buf.seek(0) diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index 23c19e4be..ba1c18006 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -319,6 +319,42 @@ class TestMediaPipeline(TestBaseMediaPipeline): assert self.fingerprint(req1) == self.fingerprint(req2) assert new_item["results"] == [(True, {})] + @coroutine_test + async def test_failures_are_cached_across_multiple_items(self): + self.pipe.LOG_FAILED_RESULTS = False + exc = Exception("foo") + req1 = Request("http://url1", meta={"response": exc}) + new_item = await self.pipe.process_item({"requests": req1}) + assert new_item["results"][0][1].value is exc + + # rsp2 is ignored, the cached failure must be reused because request + # fingerprints are the same + req2 = Request( + req1.url, meta={"response": Response("http://donot.download.me")} + ) + new_item = await self.pipe.process_item({"requests": req2}) + assert new_item["results"][0][0] is False + assert new_item["results"][0][1].value is exc + assert self.pipe._mockcalled.count("media_to_download") == 1 + + @coroutine_test + async def test_cached_failure_calls_errback(self): + """The errback of a request is called for a cached failure as well.""" + self.pipe.LOG_FAILED_RESULTS = False + exc = Exception("foo") + await self.pipe.process_item( + {"requests": Request("http://url1", meta={"response": exc})} + ) + + def errback(failure): + self.pipe._mockcalled.append("request_errback") + return {"recovered": failure.value} + + req = Request("http://url1", errback=errback) + new_item = await self.pipe.process_item({"requests": req}) + assert new_item["results"] == [(True, {"recovered": exc})] + assert self.pipe._mockcalled.count("request_errback") == 1 + @coroutine_test async def test_results_are_cached_for_requests_of_single_item(self): rsp1 = Response("http://url1") @@ -472,6 +508,30 @@ class TestBuildFromCrawler: assert pipe._from_crawler_called +class MediaFailedNonePipeline(MockedMediaPipeline): + def media_failed(self, failure, request, info): + self._mockcalled.append("media_failed") + + +class TestMediaFailedNone(TestBaseMediaPipeline): + """Test what happens when media_failed() neither raises an exception nor + returns a failure.""" + + pipeline_class = MediaFailedNonePipeline + + @coroutine_test + async def test_result_none(self): + req = Request("http://url1", meta={"response": Exception("foo")}) + new_item = await self.pipe.process_item({"requests": req}) + assert new_item["results"] == [(True, None)] + assert self.pipe._mockcalled == [ + "get_media_requests", + "media_to_download", + "media_failed", + "item_completed", + ] + + class MediaFailedFailurePipeline(MockedMediaPipeline): def media_failed(self, failure, request, info): self._mockcalled.append("media_failed") diff --git a/tests/utils/media_pipelines.py b/tests/utils/media_pipelines.py index 283c95940..7e013b797 100644 --- a/tests/utils/media_pipelines.py +++ b/tests/utils/media_pipelines.py @@ -3,6 +3,12 @@ from __future__ import annotations from typing import Any from scrapy.http.request import NO_CALLBACK, Request +from scrapy.pipelines.media import MediaPipeline +from scrapy.utils.spider import DefaultSpider + +# required by persist_file() and stat_file(), but as some stores don't use the argument +# we can pass this singleton to keep type hints correct +DUMMY_SPIDER_INFO = MediaPipeline.SpiderInfo(DefaultSpider()) async def mocked_download_func(request: Request) -> Any: From 0cbb20e8e83996a4d8a3339254c2d7e9b763ce7f Mon Sep 17 00:00:00 2001 From: Adrian Date: Wed, 29 Jul 2026 11:39:27 +0200 Subject: [PATCH 3/4] Treat broken cache records as cache misses (#7805) --- docs/topics/downloader-middleware.rst | 4 + scrapy/downloadermiddlewares/httpcache.py | 21 +++++- tests/test_downloadermiddleware_httpcache.py | 77 +++++++++++++++++--- 3 files changed, 90 insertions(+), 12 deletions(-) diff --git a/docs/topics/downloader-middleware.rst b/docs/topics/downloader-middleware.rst index 934eb19ac..fcfe7fd29 100644 --- a/docs/topics/downloader-middleware.rst +++ b/docs/topics/downloader-middleware.rst @@ -564,6 +564,10 @@ defines the methods described below. Return response if present in cache, or ``None`` otherwise. + If this method raises an exception, e.g. because the cache entry is + corrupted, the middleware logs a warning and handles the request as a + cache miss. + :param spider: the spider which generated the request :type spider: :class:`~scrapy.Spider` object diff --git a/scrapy/downloadermiddlewares/httpcache.py b/scrapy/downloadermiddlewares/httpcache.py index c6c811809..e7ca0ac0e 100644 --- a/scrapy/downloadermiddlewares/httpcache.py +++ b/scrapy/downloadermiddlewares/httpcache.py @@ -1,5 +1,6 @@ from __future__ import annotations +import logging from email.utils import formatdate from typing import TYPE_CHECKING @@ -28,6 +29,9 @@ if TYPE_CHECKING: from scrapy.statscollectors import StatsCollector +logger = logging.getLogger(__name__) + + class HttpCacheMiddleware: DOWNLOAD_EXCEPTIONS = ( ConnectionDone, @@ -77,9 +81,20 @@ class HttpCacheMiddleware: return None # Look for cached response and check if expired - cachedresponse: Response | None = self.storage.retrieve_response( - self.crawler.spider, request - ) + cachedresponse: Response | None + try: + cachedresponse = self.storage.retrieve_response( + self.crawler.spider, request + ) + except Exception: + self.stats.inc_value("httpcache/retrieve_error") + logger.warning( + f"Could not read the cache entry for {request}, treating it as a " + f"cache miss.", + exc_info=True, + extra={"spider": self.crawler.spider}, + ) + cachedresponse = None if cachedresponse is None: self.stats.inc_value("httpcache/miss") if self.ignore_missing: diff --git a/tests/test_downloadermiddleware_httpcache.py b/tests/test_downloadermiddleware_httpcache.py index 9d5d6874e..ce56ee11d 100644 --- a/tests/test_downloadermiddleware_httpcache.py +++ b/tests/test_downloadermiddleware_httpcache.py @@ -1,10 +1,12 @@ from __future__ import annotations import email.utils +import logging import shutil import tempfile import time from contextlib import contextmanager +from pathlib import Path from typing import TYPE_CHECKING, Any from unittest import mock @@ -93,6 +95,12 @@ class TestBase: class StorageTestMixin: """Mixin containing storage-specific test methods.""" + def _corrupt_cache_entry( + self, storage: Any, spider: Spider, request: Request + ) -> None: + """Make the cache entry of *request* unreadable for *storage*.""" + raise NotImplementedError + def test_storage(self): with self._storage(HTTPCACHE_EXPIRATION_SECS=1) as (storage, crawler): request2 = self.request.copy() @@ -115,6 +123,38 @@ class StorageTestMixin: with mock.patch("scrapy.extensions.httpcache.time", return_value=future): assert storage.retrieve_response(crawler.spider, self.request) + def test_corrupted_cache_entry_is_a_miss(self, caplog): + with self._middleware() as mw: + spider = mw.crawler.spider + mw.storage.store_response(spider, self.request, self.response) + self._corrupt_cache_entry(mw.storage, spider, self.request) + + caplog.clear() + with caplog.at_level(logging.WARNING): + assert mw.process_request(self.request) is None + + assert "treating it as a cache miss" in caplog.text + assert mw.crawler.stats.get_value("httpcache/retrieve_error") == 1 + assert mw.crawler.stats.get_value("httpcache/miss") == 1 + + # Storing the response again replaces the corrupted cache entry. + mw.storage.store_response(spider, self.request, self.response) + self.assertEqualResponse( + self.response, mw.storage.retrieve_response(spider, self.request) + ) + + def test_corrupted_cache_entry_ignore_missing(self): + with self._middleware(HTTPCACHE_IGNORE_MISSING=True) as mw: + spider = mw.crawler.spider + mw.storage.store_response(spider, self.request, self.response) + self._corrupt_cache_entry(mw.storage, spider, self.request) + + with pytest.raises(IgnoreRequest): + mw.process_request(self.request) + + assert mw.crawler.stats.get_value("httpcache/retrieve_error") == 1 + assert mw.crawler.stats.get_value("httpcache/ignore") == 1 + def test_storage_no_content_type_header(self): """Test that the response body is used to get the right response class even if there is no Content-Type header""" @@ -556,29 +596,43 @@ class RFC2616PolicyTestMixin(PolicyTestMixin): # Concrete test classes that combine storage and policy mixins -class TestFilesystemStorageWithDummyPolicy( - TestBase, StorageTestMixin, DummyPolicyTestMixin -): +class FilesystemStorageTestMixin(StorageTestMixin): storage_class = "scrapy.extensions.httpcache.FilesystemCacheStorage" + + def _corrupt_cache_entry(self, storage, spider, request) -> None: + rpath = Path(storage._get_request_path(spider, request)) + (rpath / "response_body").unlink() + + +class DbmStorageTestMixin(StorageTestMixin): + storage_class = "scrapy.extensions.httpcache.DbmCacheStorage" + + def _corrupt_cache_entry(self, storage, spider, request) -> None: + key = storage._fingerprinter.fingerprint(request).hex() + storage.db[f"{key}_data"] = b"not a pickle" + + +class TestFilesystemStorageWithDummyPolicy( + TestBase, FilesystemStorageTestMixin, DummyPolicyTestMixin +): policy_class = "scrapy.extensions.httpcache.DummyPolicy" class TestFilesystemStorageWithRFC2616Policy( - TestBase, StorageTestMixin, RFC2616PolicyTestMixin + TestBase, FilesystemStorageTestMixin, RFC2616PolicyTestMixin ): - storage_class = "scrapy.extensions.httpcache.FilesystemCacheStorage" policy_class = "scrapy.extensions.httpcache.RFC2616Policy" -class TestDbmStorageWithDummyPolicy(TestBase, StorageTestMixin, DummyPolicyTestMixin): - storage_class = "scrapy.extensions.httpcache.DbmCacheStorage" +class TestDbmStorageWithDummyPolicy( + TestBase, DbmStorageTestMixin, DummyPolicyTestMixin +): policy_class = "scrapy.extensions.httpcache.DummyPolicy" class TestDbmStorageWithRFC2616Policy( - TestBase, StorageTestMixin, RFC2616PolicyTestMixin + TestBase, DbmStorageTestMixin, RFC2616PolicyTestMixin ): - storage_class = "scrapy.extensions.httpcache.DbmCacheStorage" policy_class = "scrapy.extensions.httpcache.RFC2616Policy" @@ -599,3 +653,8 @@ class TestFilesystemStorageGzipWithDummyPolicy(TestFilesystemStorageWithDummyPol def _get_settings(self, **new_settings) -> dict[str, Any]: new_settings.setdefault("HTTPCACHE_GZIP", True) return super()._get_settings(**new_settings) + + def _corrupt_cache_entry(self, storage, spider, request) -> None: + # A spider killed while writing a gzip file leaves it truncated. + body_path = Path(storage._get_request_path(spider, request), "response_body") + body_path.write_bytes(body_path.read_bytes()[:-5]) From 01447f996500367fb999560d08a40cd2ff68815a Mon Sep 17 00:00:00 2001 From: Laerte Pereira <5853172+Laerte@users.noreply.github.com> Date: Wed, 29 Jul 2026 08:00:24 -0300 Subject: [PATCH 4/4] fix(commands/parse): Restore request callback before invoking spider (#7803) * fix(commands/parse): restore request callback before invoking spider * Add other test --- scrapy/commands/parse.py | 2 ++ tests/test_command_parse.py | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/scrapy/commands/parse.py b/scrapy/commands/parse.py index 2ac65bf3f..93194ded7 100644 --- a/scrapy/commands/parse.py +++ b/scrapy/commands/parse.py @@ -346,6 +346,8 @@ class Command(BaseRunSpiderCommand): self.first_response = response cb = self._get_callback(spider=spider, opts=opts, response=response) + assert response.request + response.request.callback = cb # parse items and requests depth: int = response.meta["_depth"] diff --git a/tests/test_command_parse.py b/tests/test_command_parse.py index 9b7131c7a..772cc82e2 100644 --- a/tests/test_command_parse.py +++ b/tests/test_command_parse.py @@ -126,6 +126,32 @@ class MySpider(scrapy.Spider): else: self.logger.debug('It Does Not Work :(') +class RetryRequestSpider(BaseSpider): + name = 'retry_request' + + def parse(self, response): + if response.meta.get('retried'): + yield {{'retried': True}} + return + response.meta['retried'] = True + yield response.request.replace(dont_filter=True) + +class CustomCallbackRetryRequestSpider(BaseSpider): + name = 'retry_request_custom_callback' + + def parse(self, response): + yield response.request.replace( + callback=self.parse_retry, + dont_filter=True, + ) + + def parse_retry(self, response): + if response.meta.get('retried'): + yield {{'retried_with_custom_callback': True}} + return + response.meta['retried'] = True + yield response.request.replace(dont_filter=True) + class MyGoodCrawlSpider(CrawlSpider): name = 'goodcrawl{self.spider_name}' @@ -381,6 +407,36 @@ ITEM_PIPELINES = {{'{self.project_name}.pipelines.MyPipeline': 1}} ) assert "[{}, {'foo': 'bar'}]" in out + def test_retry_response_request( + self, proj_path: Path, mockserver: MockServer + ) -> None: + _, out, stderr = proc( + "parse", + "--spider", + "retry_request", + "-d", + "2", + mockserver.url("/html"), + cwd=proj_path, + ) + assert "RecursionError" not in stderr + assert "{'retried': True}" in out + + def test_retry_response_request_with_custom_callback( + self, proj_path: Path, mockserver: MockServer + ) -> None: + _, out, stderr = proc( + "parse", + "--spider", + "retry_request_custom_callback", + "-d", + "3", + mockserver.url("/html"), + cwd=proj_path, + ) + assert "RecursionError" not in stderr + assert "{'retried_with_custom_callback': True}" in out + def test_wrong_callback_passed( self, proj_path: Path, mockserver: MockServer ) -> None: