diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index 4a8639c22..f83037e6c 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -12,6 +12,7 @@ import hashlib import logging import mimetypes import time +import warnings from collections import defaultdict from contextlib import suppress from ftplib import FTP @@ -24,16 +25,17 @@ from itemadapter import ItemAdapter from twisted.internet.defer import Deferred, maybeDeferred from twisted.internet.threads import deferToThread -from scrapy.exceptions import IgnoreRequest, NotConfigured +from scrapy.exceptions import IgnoreRequest, NotConfigured, ScrapyDeprecationWarning from scrapy.http import Request, Response from scrapy.http.request import NO_CALLBACK from scrapy.pipelines.media import FileInfo, FileInfoOrError, MediaPipeline from scrapy.settings import Settings from scrapy.utils.boto import is_botocore_available from scrapy.utils.datatypes import CaseInsensitiveDict +from scrapy.utils.deprecate import method_is_overridden from scrapy.utils.ftp import ftp_store_file from scrapy.utils.log import failure_to_exc_info -from scrapy.utils.python import to_bytes +from scrapy.utils.python import get_func_args, to_bytes from scrapy.utils.request import referer_str if TYPE_CHECKING: @@ -46,6 +48,7 @@ if TYPE_CHECKING: from typing_extensions import Self from scrapy import Spider + from scrapy.crawler import Crawler logger = logging.getLogger(__name__) @@ -443,6 +446,8 @@ class FilesPipeline(MediaPipeline): store_uri: str | PathLike[str], download_func: Callable[[Request, Spider], Response] | None = None, settings: Settings | dict[str, Any] | None = None, + *, + crawler: Crawler | None = None, ): store_uri = _to_string(store_uri) if not store_uri: @@ -467,10 +472,35 @@ class FilesPipeline(MediaPipeline): resolve("FILES_RESULT_FIELD"), self.FILES_RESULT_FIELD ) - super().__init__(download_func=download_func, settings=settings) + super().__init__( + download_func=download_func, settings=settings, crawler=crawler + ) @classmethod def from_settings(cls, settings: Settings) -> Self: + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings, None) + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: + if method_is_overridden(cls, FilesPipeline, "from_settings"): + warnings.warn( + f"{cls.__name__} overrides FilesPipeline.from_settings()." + f" This method is deprecated and won't be called in future Scrapy versions," + f" please update your code so that it overrides from_crawler() instead.", + category=ScrapyDeprecationWarning, + ) + o = cls.from_settings(crawler.settings) + o._finish_init(crawler) + return o + return cls._from_settings(crawler.settings, crawler) + + @classmethod + def _from_settings(cls, settings: Settings, crawler: Crawler | None) -> Self: s3store: type[S3FilesStore] = cast(type[S3FilesStore], cls.STORE_SCHEMES["s3"]) s3store.AWS_ACCESS_KEY_ID = settings["AWS_ACCESS_KEY_ID"] s3store.AWS_SECRET_ACCESS_KEY = settings["AWS_SECRET_ACCESS_KEY"] @@ -495,7 +525,18 @@ class FilesPipeline(MediaPipeline): ftp_store.USE_ACTIVE_MODE = settings.getbool("FEED_STORAGE_FTP_ACTIVE") store_uri = settings["FILES_STORE"] - return cls(store_uri, settings=settings) + if "crawler" in get_func_args(cls.__init__): + o = cls(store_uri, settings=settings, crawler=crawler) + else: + o = cls(store_uri, settings=settings) + if crawler: + o._finish_init(crawler) + warnings.warn( + f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + " This is deprecated and the argument will be required in future Scrapy versions.", + category=ScrapyDeprecationWarning, + ) + return o def _get_store(self, uri: str) -> FilesStoreProtocol: if Path(uri).is_absolute(): # to support win32 paths like: C:\\some\dir diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index 2c4c9376e..71da6a196 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -8,13 +8,14 @@ from __future__ import annotations import functools import hashlib +import warnings from contextlib import suppress from io import BytesIO from typing import TYPE_CHECKING, Any, cast from itemadapter import ItemAdapter -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.http import Request, Response from scrapy.http.request import NO_CALLBACK from scrapy.pipelines.files import ( @@ -26,7 +27,7 @@ from scrapy.pipelines.files import ( _md5sum, ) from scrapy.settings import Settings -from scrapy.utils.python import to_bytes +from scrapy.utils.python import get_func_args, to_bytes if TYPE_CHECKING: from collections.abc import Callable, Iterable @@ -38,6 +39,7 @@ if TYPE_CHECKING: from typing_extensions import Self from scrapy import Spider + from scrapy.crawler import Crawler from scrapy.pipelines.media import FileInfoOrError, MediaPipeline @@ -64,6 +66,8 @@ class ImagesPipeline(FilesPipeline): store_uri: str | PathLike[str], download_func: Callable[[Request, Spider], Response] | None = None, settings: Settings | dict[str, Any] | None = None, + *, + crawler: Crawler | None = None, ): try: from PIL import Image @@ -74,7 +78,9 @@ class ImagesPipeline(FilesPipeline): "ImagesPipeline requires installing Pillow 4.0.0 or later" ) - super().__init__(store_uri, settings=settings, download_func=download_func) + super().__init__( + store_uri, settings=settings, download_func=download_func, crawler=crawler + ) if isinstance(settings, dict) or settings is None: settings = Settings(settings) @@ -108,7 +114,7 @@ class ImagesPipeline(FilesPipeline): ) @classmethod - def from_settings(cls, settings: Settings) -> Self: + def _from_settings(cls, settings: Settings, crawler: Crawler | None) -> Self: s3store: type[S3FilesStore] = cast(type[S3FilesStore], cls.STORE_SCHEMES["s3"]) s3store.AWS_ACCESS_KEY_ID = settings["AWS_ACCESS_KEY_ID"] s3store.AWS_SECRET_ACCESS_KEY = settings["AWS_SECRET_ACCESS_KEY"] @@ -133,7 +139,18 @@ class ImagesPipeline(FilesPipeline): ftp_store.USE_ACTIVE_MODE = settings.getbool("FEED_STORAGE_FTP_ACTIVE") store_uri = settings["IMAGES_STORE"] - return cls(store_uri, settings=settings) + if "crawler" in get_func_args(cls.__init__): + o = cls(store_uri, settings=settings, crawler=crawler) + else: + o = cls(store_uri, settings=settings) + if crawler: + o._finish_init(crawler) + warnings.warn( + f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + " This is deprecated and the argument will be required in future Scrapy versions.", + category=ScrapyDeprecationWarning, + ) + return o def file_downloaded( self, diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index b10ec147b..99abed09e 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -2,6 +2,7 @@ from __future__ import annotations import functools import logging +import warnings from abc import ABC, abstractmethod from collections import defaultdict from typing import ( @@ -20,12 +21,14 @@ from twisted.internet.defer import Deferred, DeferredList from twisted.python.failure import Failure from twisted.python.versions import Version +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http.request import NO_CALLBACK, Request from scrapy.settings import Settings from scrapy.utils.datatypes import SequenceExclude from scrapy.utils.defer import defer_result, mustbe_deferred from scrapy.utils.log import failure_to_exc_info from scrapy.utils.misc import arg_to_iter +from scrapy.utils.python import get_func_args if TYPE_CHECKING: from collections.abc import Callable @@ -38,7 +41,6 @@ if TYPE_CHECKING: from scrapy.http import Response from scrapy.utils.request import RequestFingerprinter - _T = TypeVar("_T") @@ -51,13 +53,13 @@ class FileInfo(TypedDict): FileInfoOrError = Union[tuple[Literal[True], FileInfo], tuple[Literal[False], Failure]] - logger = logging.getLogger(__name__) class MediaPipeline(ABC): crawler: Crawler _fingerprinter: RequestFingerprinter + _modern_init = False LOG_FAILED_RESULTS: bool = True @@ -74,6 +76,8 @@ class MediaPipeline(ABC): self, download_func: Callable[[Request, Spider], Response] | None = None, settings: Settings | dict[str, Any] | None = None, + *, + crawler: Crawler | None = None, ): self.download_func = download_func @@ -87,6 +91,28 @@ class MediaPipeline(ABC): ) self._handle_statuses(self.allow_redirects) + if crawler: + # TODO use crawler.settings + self._finish_init(crawler) + self._modern_init = True + else: + warnings.warn( + f"MediaPipeline.__init__() was called without the crawler argument" + f" when creating {self.__class__.__qualname__}." + f" This is deprecated and the argument will be required in future Scrapy versions.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + + def _finish_init(self, crawler: Crawler) -> None: + # This was done in from_crawler() before 2.12, now it's done in __init__() + # if the crawler was passed to it and may be needed to be called in other + # deprecated code paths explicitly too. After the crawler argument of __init__() + # becomes mandatory this should be inlined there. + self.crawler = crawler + assert crawler.request_fingerprinter + self._fingerprinter = crawler.request_fingerprinter + def _handle_statuses(self, allow_redirects: bool) -> None: self.handle_httpstatus_list = None if allow_redirects: @@ -112,13 +138,19 @@ class MediaPipeline(ABC): @classmethod def from_crawler(cls, crawler: Crawler) -> Self: pipe: Self - try: + if hasattr(cls, "from_settings"): pipe = cls.from_settings(crawler.settings) # type: ignore[attr-defined] - except AttributeError: + elif "crawler" in get_func_args(cls.__init__): + pipe = cls(crawler=crawler) + else: pipe = cls() - pipe.crawler = crawler - assert crawler.request_fingerprinter - pipe._fingerprinter = crawler.request_fingerprinter + warnings.warn( + f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + " This is deprecated and the argument will be required in future Scrapy versions.", + category=ScrapyDeprecationWarning, + ) + if not pipe._modern_init: + pipe._finish_init(crawler) return pipe def open_spider(self, spider: Spider) -> None: diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index 83eaa1fdd..5e94f9271 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -17,7 +17,6 @@ from itemadapter import ItemAdapter from twisted.internet import defer from twisted.trial import unittest -from scrapy import Spider from scrapy.http import Request, Response from scrapy.item import Field, Item from scrapy.pipelines.files import ( @@ -27,7 +26,6 @@ from scrapy.pipelines.files import ( GCSFilesStore, S3FilesStore, ) -from scrapy.settings import Settings from scrapy.utils.test import ( assert_gcs_environ, get_crawler, @@ -219,8 +217,8 @@ class FilesPipelineTestCase(unittest.TestCase): def file_path(self, request, response=None, info=None, item=None): return f'full/{item.get("path")}' - file_path = CustomFilesPipeline.from_settings( - Settings({"FILES_STORE": self.tempdir}) + file_path = CustomFilesPipeline.from_crawler( + get_crawler(None, {"FILES_STORE": self.tempdir}) ).file_path item = {"path": "path-to-store-file"} request = Request("http://example.com") @@ -237,7 +235,9 @@ class FilesPipelineTestCaseFieldsMixin: def test_item_fields_default(self): url = "http://www.example.com/files/1.txt" item = self.item_class(name="item1", file_urls=[url]) - pipeline = FilesPipeline.from_settings(Settings({"FILES_STORE": self.tempdir})) + pipeline = FilesPipeline.from_crawler( + get_crawler(None, {"FILES_STORE": self.tempdir}) + ) requests = list(pipeline.get_media_requests(item, None)) self.assertEqual(requests[0].url, url) results = [(True, {"url": url})] @@ -249,13 +249,14 @@ class FilesPipelineTestCaseFieldsMixin: def test_item_fields_override_settings(self): url = "http://www.example.com/files/1.txt" item = self.item_class(name="item1", custom_file_urls=[url]) - pipeline = FilesPipeline.from_settings( - Settings( + pipeline = FilesPipeline.from_crawler( + get_crawler( + None, { "FILES_STORE": self.tempdir, "FILES_URLS_FIELD": "custom_file_urls", "FILES_RESULT_FIELD": "custom_files", - } + }, ) ) requests = list(pipeline.get_media_requests(item, None)) @@ -373,8 +374,10 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): different settings. """ custom_settings = self._generate_fake_settings() - another_pipeline = FilesPipeline.from_settings(Settings(custom_settings)) - one_pipeline = FilesPipeline(self.tempdir) + another_pipeline = FilesPipeline.from_crawler( + get_crawler(None, custom_settings) + ) + one_pipeline = FilesPipeline(self.tempdir, crawler=get_crawler(None)) for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: default_value = self.default_cls_settings[pipe_attr] self.assertEqual(getattr(one_pipeline, pipe_attr), default_value) @@ -387,7 +390,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): If subclasses override class attributes and there are no special settings those values should be kept. """ pipe_cls = self._generate_fake_pipeline() - pipe = pipe_cls.from_settings(Settings({"FILES_STORE": self.tempdir})) + pipe = pipe_cls.from_crawler(get_crawler(None, {"FILES_STORE": self.tempdir})) for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: custom_value = getattr(pipe, pipe_ins_attr) self.assertNotEqual(custom_value, self.default_cls_settings[pipe_attr]) @@ -400,7 +403,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): """ pipeline_cls = self._generate_fake_pipeline() settings = self._generate_fake_settings() - pipeline = pipeline_cls.from_settings(Settings(settings)) + pipeline = pipeline_cls.from_crawler(get_crawler(None, settings)) for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: value = getattr(pipeline, pipe_ins_attr) setting_value = settings.get(settings_attr) @@ -416,8 +419,8 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): class UserDefinedFilesPipeline(FilesPipeline): pass - user_pipeline = UserDefinedFilesPipeline.from_settings( - Settings({"FILES_STORE": self.tempdir}) + user_pipeline = UserDefinedFilesPipeline.from_crawler( + get_crawler(None, {"FILES_STORE": self.tempdir}) ) for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: # Values from settings for custom pipeline should be set on pipeline instance. @@ -435,7 +438,9 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): prefix = UserDefinedFilesPipeline.__name__.upper() settings = self._generate_fake_settings(prefix=prefix) - user_pipeline = UserDefinedFilesPipeline.from_settings(Settings(settings)) + user_pipeline = UserDefinedFilesPipeline.from_crawler( + get_crawler(None, settings) + ) for pipe_attr, settings_attr, pipe_inst_attr in self.file_cls_attr_settings_map: # Values from settings for custom pipeline should be set on pipeline instance. custom_value = settings.get(prefix + "_" + settings_attr) @@ -450,7 +455,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): pipeline_cls = self._generate_fake_pipeline() prefix = pipeline_cls.__name__.upper() settings = self._generate_fake_settings(prefix=prefix) - user_pipeline = pipeline_cls.from_settings(Settings(settings)) + user_pipeline = pipeline_cls.from_crawler(get_crawler(None, settings)) for ( pipe_cls_attr, settings_attr, @@ -465,8 +470,8 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): DEFAULT_FILES_RESULT_FIELD = "this" DEFAULT_FILES_URLS_FIELD = "that" - pipeline = UserDefinedFilesPipeline.from_settings( - Settings({"FILES_STORE": self.tempdir}) + pipeline = UserDefinedFilesPipeline.from_crawler( + get_crawler(None, {"FILES_STORE": self.tempdir}) ) self.assertEqual( pipeline.files_result_field, @@ -486,7 +491,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): class UserPipe(FilesPipeline): pass - pipeline_cls = UserPipe.from_settings(Settings(settings)) + pipeline_cls = UserPipe.from_crawler(get_crawler(None, settings)) for pipe_attr, settings_attr, pipe_inst_attr in self.file_cls_attr_settings_map: expected_value = settings.get(settings_attr) @@ -497,8 +502,8 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): def file_path(self, request, response=None, info=None, *, item=None): return Path("subdir") / Path(request.url).name - pipeline = CustomFilesPipelineWithPathLikeDir.from_settings( - Settings({"FILES_STORE": Path("./Temp")}) + pipeline = CustomFilesPipelineWithPathLikeDir.from_crawler( + get_crawler(None, {"FILES_STORE": Path("./Temp")}) ) request = Request("http://example.com/image01.jpg") self.assertEqual(pipeline.file_path(request), Path("subdir/image01.jpg")) @@ -695,7 +700,7 @@ def _prepare_request_object(item_url, flags=None): class BuildFromCrawlerTestCase(unittest.TestCase): def setUp(self): self.tempdir = mkdtemp() - self.crawler = get_crawler(Spider, {"FILES_STORE": self.tempdir}) + self.crawler = get_crawler(None, {"FILES_STORE": self.tempdir}) def tearDown(self): rmtree(self.tempdir) @@ -711,8 +716,23 @@ class BuildFromCrawlerTestCase(unittest.TestCase): self.assertEqual(len(w), 0) assert pipe.store + def test_has_old_init(self): + class Pipeline(FilesPipeline): + def __init__(self, store_uri, download_func=None, settings=None): + super().__init__(store_uri, download_func, settings) + self._init_called = True + + with warnings.catch_warnings(record=True) as w: + pipe = Pipeline.from_crawler(self.crawler) + assert pipe.crawler == self.crawler + assert pipe._fingerprinter + self.assertEqual(len(w), 2) + assert pipe._init_called + def test_has_from_settings(self): class Pipeline(FilesPipeline): + _from_settings_called = False + @classmethod def from_settings(cls, settings): o = super().from_settings(settings) @@ -723,27 +743,24 @@ class BuildFromCrawlerTestCase(unittest.TestCase): pipe = Pipeline.from_crawler(self.crawler) assert pipe.crawler == self.crawler assert pipe._fingerprinter - self.assertEqual(len(w), 0) + self.assertEqual(len(w), 3) assert pipe.store assert pipe._from_settings_called - @pytest.mark.xfail( - reason="No way to override MediaPipeline.from_crawler having non-trivial __init__" - ) def test_has_from_crawler_and_init(self): class Pipeline(FilesPipeline): + _from_crawler_called = False + @classmethod def from_crawler(cls, crawler): settings = crawler.settings store_uri = settings["FILES_STORE"] - # you can either call super().from_crawler() or cls.__init__() but you need both - o = cls(store_uri, settings=settings) + o = cls(store_uri, settings=settings, crawler=crawler) o._from_crawler_called = True return o with warnings.catch_warnings(record=True) as w: pipe = Pipeline.from_crawler(self.crawler) - # this and the next assert will fail as MediaPipeline.from_crawler() wasn't called assert pipe.crawler == self.crawler assert pipe._fingerprinter self.assertEqual(len(w), 0) diff --git a/tests/test_pipeline_images.py b/tests/test_pipeline_images.py index dfeead999..3f18c83f7 100644 --- a/tests/test_pipeline_images.py +++ b/tests/test_pipeline_images.py @@ -14,6 +14,7 @@ from scrapy.http import Request, Response from scrapy.item import Field, Item from scrapy.pipelines.images import ImageException, ImagesPipeline from scrapy.settings import Settings +from scrapy.utils.test import get_crawler skip_pillow: str | None try: @@ -33,7 +34,8 @@ class ImagesPipelineTestCase(unittest.TestCase): def setUp(self): self.tempdir = mkdtemp() - self.pipeline = ImagesPipeline(self.tempdir) + crawler = get_crawler() + self.pipeline = ImagesPipeline(self.tempdir, crawler=crawler) def tearDown(self): rmtree(self.tempdir) @@ -123,8 +125,8 @@ class ImagesPipelineTestCase(unittest.TestCase): ): return f"thumb/{thumb_id}/{item.get('path')}" - thumb_path = CustomImagesPipeline.from_settings( - Settings({"IMAGES_STORE": self.tempdir}) + thumb_path = CustomImagesPipeline.from_crawler( + get_crawler(None, {"IMAGES_STORE": self.tempdir}) ).thumb_path item = {"path": "path-to-store-file"} request = Request("http://example.com") @@ -218,8 +220,8 @@ class ImagesPipelineTestCaseFieldsMixin: def test_item_fields_default(self): url = "http://www.example.com/images/1.jpg" item = self.item_class(name="item1", image_urls=[url]) - pipeline = ImagesPipeline.from_settings( - Settings({"IMAGES_STORE": "s3://example/images/"}) + pipeline = ImagesPipeline.from_crawler( + get_crawler(None, {"IMAGES_STORE": "s3://example/images/"}) ) requests = list(pipeline.get_media_requests(item, None)) self.assertEqual(requests[0].url, url) @@ -232,13 +234,14 @@ class ImagesPipelineTestCaseFieldsMixin: def test_item_fields_override_settings(self): url = "http://www.example.com/images/1.jpg" item = self.item_class(name="item1", custom_image_urls=[url]) - pipeline = ImagesPipeline.from_settings( - Settings( + pipeline = ImagesPipeline.from_crawler( + get_crawler( + None, { "IMAGES_STORE": "s3://example/images/", "IMAGES_URLS_FIELD": "custom_image_urls", "IMAGES_RESULT_FIELD": "custom_images", - } + }, ) ) requests = list(pipeline.get_media_requests(item, None)) @@ -390,8 +393,10 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): """ custom_settings = self._generate_fake_settings() default_settings = Settings() - default_sts_pipe = ImagesPipeline(self.tempdir, settings=default_settings) - user_sts_pipe = ImagesPipeline.from_settings(Settings(custom_settings)) + default_sts_pipe = ImagesPipeline( + self.tempdir, settings=default_settings, crawler=get_crawler(None) # TODO + ) + user_sts_pipe = ImagesPipeline.from_crawler(get_crawler(None, custom_settings)) for pipe_attr, settings_attr in self.img_cls_attribute_names: expected_default_value = self.default_pipeline_settings.get(pipe_attr) custom_value = custom_settings.get(settings_attr) @@ -407,7 +412,9 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): from class attributes. """ pipeline_cls = self._generate_fake_pipeline_subclass() - pipeline = pipeline_cls.from_settings(Settings({"IMAGES_STORE": self.tempdir})) + pipeline = pipeline_cls.from_crawler( + get_crawler(None, {"IMAGES_STORE": self.tempdir}) + ) for pipe_attr, settings_attr in self.img_cls_attribute_names: # Instance attribute (lowercase) must be equal to class attribute (uppercase). attr_value = getattr(pipeline, pipe_attr.lower()) @@ -421,7 +428,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): """ pipeline_cls = self._generate_fake_pipeline_subclass() settings = self._generate_fake_settings() - pipeline = pipeline_cls.from_settings(Settings(settings)) + pipeline = pipeline_cls.from_crawler(get_crawler(None, settings)) for pipe_attr, settings_attr in self.img_cls_attribute_names: # Instance attribute (lowercase) must be equal to # value defined in settings. @@ -439,8 +446,8 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): class UserDefinedImagePipeline(ImagesPipeline): pass - user_pipeline = UserDefinedImagePipeline.from_settings( - Settings({"IMAGES_STORE": self.tempdir}) + user_pipeline = UserDefinedImagePipeline.from_crawler( + get_crawler(None, {"IMAGES_STORE": self.tempdir}) ) for pipe_attr, settings_attr in self.img_cls_attribute_names: # Values from settings for custom pipeline should be set on pipeline instance. @@ -458,7 +465,9 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): prefix = UserDefinedImagePipeline.__name__.upper() settings = self._generate_fake_settings(prefix=prefix) - user_pipeline = UserDefinedImagePipeline.from_settings(Settings(settings)) + user_pipeline = UserDefinedImagePipeline.from_crawler( + get_crawler(None, settings) + ) for pipe_attr, settings_attr in self.img_cls_attribute_names: # Values from settings for custom pipeline should be set on pipeline instance. custom_value = settings.get(prefix + "_" + settings_attr) @@ -473,7 +482,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): pipeline_cls = self._generate_fake_pipeline_subclass() prefix = pipeline_cls.__name__.upper() settings = self._generate_fake_settings(prefix=prefix) - user_pipeline = pipeline_cls.from_settings(Settings(settings)) + user_pipeline = pipeline_cls.from_crawler(get_crawler(None, settings)) for pipe_attr, settings_attr in self.img_cls_attribute_names: custom_value = settings.get(prefix + "_" + settings_attr) self.assertNotEqual(custom_value, self.default_pipeline_settings[pipe_attr]) @@ -484,8 +493,8 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): DEFAULT_IMAGES_URLS_FIELD = "something" DEFAULT_IMAGES_RESULT_FIELD = "something_else" - pipeline = UserDefinedImagePipeline.from_settings( - Settings({"IMAGES_STORE": self.tempdir}) + pipeline = UserDefinedImagePipeline.from_crawler( + get_crawler(None, {"IMAGES_STORE": self.tempdir}) ) self.assertEqual( pipeline.images_result_field, @@ -506,7 +515,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): class UserPipe(ImagesPipeline): pass - pipeline_cls = UserPipe.from_settings(Settings(settings)) + pipeline_cls = UserPipe.from_crawler(get_crawler(None, settings)) for pipe_attr, settings_attr in self.img_cls_attribute_names: expected_value = settings.get(settings_attr) diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index 920b4246e..a825de92a 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -2,7 +2,6 @@ from __future__ import annotations import warnings -import pytest from testfixtures import LogCapture from twisted.internet import reactor from twisted.internet.defer import Deferred, inlineCallbacks @@ -14,7 +13,6 @@ from scrapy.http import Request, Response from scrapy.http.request import NO_CALLBACK from scrapy.pipelines.files import FileException from scrapy.pipelines.media import MediaPipeline -from scrapy.settings import Settings from scrapy.spiders import Spider from scrapy.utils.log import failure_to_exc_info from scrapy.utils.signal import disconnect_all @@ -178,8 +176,8 @@ class BaseMediaPipelineTestCase(unittest.TestCase): class MockedMediaPipeline(UserDefinedPipeline): - def __init__(self, *args, **kwargs): - super().__init__(*args, **kwargs) + def __init__(self, *args, crawler=None, **kwargs): + super().__init__(*args, crawler=crawler, **kwargs) self._mockcalled = [] def download(self, request, info): @@ -380,7 +378,8 @@ class MediaPipelineTestCase(BaseMediaPipelineTestCase): class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase): def _assert_request_no3xx(self, pipeline_class, settings): - pipe = pipeline_class(settings=Settings(settings)) + crawler = get_crawler(None, settings) + pipe = pipeline_class(settings=settings, crawler=crawler) # TODO request = Request("http://url") pipe._modify_media_request(request) @@ -417,7 +416,7 @@ class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase): class BuildFromCrawlerTestCase(unittest.TestCase): def setUp(self): - self.crawler = get_crawler(Spider, {"FILES_STORE": "/foo"}) + self.crawler = get_crawler(None, {"FILES_STORE": "/foo"}) def test_simple(self): class Pipeline(UserDefinedPipeline): @@ -429,8 +428,23 @@ class BuildFromCrawlerTestCase(unittest.TestCase): assert pipe._fingerprinter self.assertEqual(len(w), 0) + def test_has_old_init(self): + class Pipeline(UserDefinedPipeline): + def __init__(self): + super().__init__() + self._init_called = True + + with warnings.catch_warnings(record=True) as w: + pipe = Pipeline.from_crawler(self.crawler) + assert pipe.crawler == self.crawler + assert pipe._fingerprinter + self.assertEqual(len(w), 2) + assert pipe._init_called + def test_has_from_settings(self): class Pipeline(UserDefinedPipeline): + _from_settings_called = False + @classmethod def from_settings(cls, settings): o = cls() @@ -441,11 +455,13 @@ class BuildFromCrawlerTestCase(unittest.TestCase): pipe = Pipeline.from_crawler(self.crawler) assert pipe.crawler == self.crawler assert pipe._fingerprinter - self.assertEqual(len(w), 0) + self.assertEqual(len(w), 1) assert pipe._from_settings_called def test_has_from_settings_and_init(self): class Pipeline(UserDefinedPipeline): + _from_settings_called = False + def __init__(self, store_uri, settings): super().__init__() self._init_called = True @@ -461,31 +477,28 @@ class BuildFromCrawlerTestCase(unittest.TestCase): pipe = Pipeline.from_crawler(self.crawler) assert pipe.crawler == self.crawler assert pipe._fingerprinter - self.assertEqual(len(w), 0) + self.assertEqual(len(w), 1) assert pipe._from_settings_called assert pipe._init_called - @pytest.mark.xfail( - reason="No way to override MediaPipeline.from_crawler having non-trivial __init__" - ) def test_has_from_crawler_and_init(self): class Pipeline(UserDefinedPipeline): - def __init__(self, store_uri, settings): - super().__init__() + _from_crawler_called = False + + def __init__(self, store_uri, settings, *, crawler): + super().__init__(crawler=crawler) self._init_called = True @classmethod def from_crawler(cls, crawler): settings = crawler.settings store_uri = settings["FILES_STORE"] - # you can either call super().from_crawler() or cls.__init__() but you need both - o = cls(store_uri, settings=settings) + o = cls(store_uri, settings=settings, crawler=crawler) o._from_crawler_called = True return o with warnings.catch_warnings(record=True) as w: pipe = Pipeline.from_crawler(self.crawler) - # this and the next assert will fail as super().from_crawler() wasn't called assert pipe.crawler == self.crawler assert pipe._fingerprinter self.assertEqual(len(w), 0) @@ -494,6 +507,8 @@ class BuildFromCrawlerTestCase(unittest.TestCase): def test_has_from_crawler(self): class Pipeline(UserDefinedPipeline): + _from_crawler_called = False + @classmethod def from_crawler(cls, crawler): settings = crawler.settings