From eda3a89b3fe3e88ed8b90d032a2f25b92a1b79ca Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Mon, 11 Nov 2024 16:44:47 +0500 Subject: [PATCH 01/10] Remove build_from_settings(). --- scrapy/middleware.py | 28 +++++++++++++++++++++---- scrapy/utils/misc.py | 25 ++--------------------- tests/test_middleware.py | 8 +++----- tests/test_utils_misc/__init__.py | 34 ------------------------------- tests/test_webclient.py | 33 ++++++++++++------------------ 5 files changed, 42 insertions(+), 86 deletions(-) diff --git a/scrapy/middleware.py b/scrapy/middleware.py index b6a427895..9e994703d 100644 --- a/scrapy/middleware.py +++ b/scrapy/middleware.py @@ -2,12 +2,13 @@ from __future__ import annotations import logging import pprint +import warnings from collections import defaultdict, deque from typing import TYPE_CHECKING, Any, TypeVar, cast -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.utils.defer import process_chain, process_parallel -from scrapy.utils.misc import build_from_crawler, build_from_settings, load_object +from scrapy.utils.misc import build_from_crawler, load_object if TYPE_CHECKING: from collections.abc import Callable, Iterable @@ -20,7 +21,7 @@ if TYPE_CHECKING: from scrapy import Spider from scrapy.crawler import Crawler - from scrapy.settings import Settings + from scrapy.settings import BaseSettings, Settings _P = ParamSpec("_P") @@ -50,8 +51,27 @@ class MiddlewareManager: def _get_mwlist_from_settings(cls, settings: Settings) -> list[Any]: raise NotImplementedError + @staticmethod + def _build_from_settings(objcls: type[_T], settings: BaseSettings) -> _T: + if hasattr(objcls, "from_settings"): + instance = objcls.from_settings(settings) # type: ignore[attr-defined] + method_name = "from_settings" + else: + instance = objcls() + method_name = "__new__" + if instance is None: + raise TypeError(f"{objcls.__qualname__}.{method_name} returned None") + return cast(_T, instance) + @classmethod def from_settings(cls, settings: Settings, crawler: Crawler | None = None) -> Self: + if crawler is None: + warnings.warn( + "Calling MiddlewareManager.from_settings() without a Crawler instance is deprecated." + " As this method will be deprecated in the future, please switch to from_crawler().", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) mwlist = cls._get_mwlist_from_settings(settings) middlewares = [] enabled = [] @@ -61,7 +81,7 @@ class MiddlewareManager: if crawler is not None: mw = build_from_crawler(mwcls, crawler) else: - mw = build_from_settings(mwcls, settings) + mw = MiddlewareManager._build_from_settings(mwcls, settings) middlewares.append(mw) enabled.append(clspath) except NotConfigured as e: diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index 12c09839f..efb475131 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -26,7 +26,6 @@ if TYPE_CHECKING: from scrapy import Spider from scrapy.crawler import Crawler - from scrapy.settings import BaseSettings _ITERABLE_SINGLE_VALUES = dict, Item, str, bytes @@ -150,7 +149,7 @@ def create_instance(objcls, settings, crawler, *args, **kwargs): """ warnings.warn( "The create_instance() function is deprecated. " - "Please use build_from_crawler() or build_from_settings() instead.", + "Please use build_from_crawler() instead.", category=ScrapyDeprecationWarning, stacklevel=2, ) @@ -176,7 +175,7 @@ def create_instance(objcls, settings, crawler, *args, **kwargs): def build_from_crawler( objcls: type[T], crawler: Crawler, /, *args: Any, **kwargs: Any ) -> T: - """Construct a class instance using its ``from_crawler`` constructor. + """Construct a class instance using its ``from_crawler`` or ``from_settings`` constructor. ``*args`` and ``**kwargs`` are forwarded to the constructor. @@ -196,26 +195,6 @@ def build_from_crawler( return cast(T, instance) -def build_from_settings( - objcls: type[T], settings: BaseSettings, /, *args: Any, **kwargs: Any -) -> T: - """Construct a class instance using its ``from_settings`` constructor. - - ``*args`` and ``**kwargs`` are forwarded to the constructor. - - Raises ``TypeError`` if the resulting instance is ``None``. - """ - if hasattr(objcls, "from_settings"): - instance = objcls.from_settings(settings, *args, **kwargs) # type: ignore[attr-defined] - method_name = "from_settings" - else: - instance = objcls(*args, **kwargs) - method_name = "__new__" - if instance is None: - raise TypeError(f"{objcls.__qualname__}.{method_name} returned None") - return cast(T, instance) - - @contextmanager def set_environ(**kwargs: str) -> Iterator[None]: """Temporarily set environment variables inside the context manager and diff --git a/tests/test_middleware.py b/tests/test_middleware.py index a42c7b3d1..3a1cf19ad 100644 --- a/tests/test_middleware.py +++ b/tests/test_middleware.py @@ -2,7 +2,7 @@ from twisted.trial import unittest from scrapy.exceptions import NotConfigured from scrapy.middleware import MiddlewareManager -from scrapy.settings import Settings +from scrapy.utils.test import get_crawler class M1: @@ -23,8 +23,6 @@ class M2: def close_spider(self, spider): pass - pass - class M3: def process(self, response, request, spider): @@ -83,7 +81,7 @@ class MiddlewareManagerTest(unittest.TestCase): self.assertEqual(mwman.middlewares, (m1, m2, m3)) def test_enabled_from_settings(self): - settings = Settings() - mwman = TestMiddlewareManager.from_settings(settings) + crawler = get_crawler() + mwman = TestMiddlewareManager.from_crawler(crawler) classes = [x.__class__ for x in mwman.middlewares] self.assertEqual(classes, [M1, M3]) diff --git a/tests/test_utils_misc/__init__.py b/tests/test_utils_misc/__init__.py index 4d8e71521..f71b2b034 100644 --- a/tests/test_utils_misc/__init__.py +++ b/tests/test_utils_misc/__init__.py @@ -10,7 +10,6 @@ from scrapy.item import Field, Item from scrapy.utils.misc import ( arg_to_iter, build_from_crawler, - build_from_settings, create_instance, load_object, rel_has_nofollow, @@ -197,39 +196,6 @@ class UtilsMiscTestCase(unittest.TestCase): with self.assertRaises(TypeError): build_from_crawler(m, crawler, *args, **kwargs) - def test_build_from_settings(self): - settings = mock.MagicMock() - args = (True, 100.0) - kwargs = {"key": "val"} - - def _test_with_settings(mock, settings): - build_from_settings(mock, settings, *args, **kwargs) - if hasattr(mock, "from_settings"): - mock.from_settings.assert_called_once_with(settings, *args, **kwargs) - self.assertEqual(mock.call_count, 0) - else: - mock.assert_called_once_with(*args, **kwargs) - - # Check usage of correct constructor using three mocks: - # 1. with no alternative constructors - # 2. with from_settings() constructor - # 3. with from_settings() and from_crawler() constructor - spec_sets = ( - ["__qualname__"], - ["__qualname__", "from_settings"], - ["__qualname__", "from_settings", "from_crawler"], - ) - for specs in spec_sets: - m = mock.MagicMock(spec_set=specs) - _test_with_settings(m, settings) - m.reset_mock() - - # Check adoption of crawler settings - m = mock.MagicMock(spec_set=["__qualname__", "from_settings"]) - m.from_settings.return_value = None - with self.assertRaises(TypeError): - build_from_settings(m, settings, *args, **kwargs) - def test_set_environ(self): assert os.environ.get("some_test_environ") is None with set_environ(some_test_environ="test_value"): diff --git a/tests/test_webclient.py b/tests/test_webclient.py index cce119001..1797d5e1f 100644 --- a/tests/test_webclient.py +++ b/tests/test_webclient.py @@ -9,25 +9,18 @@ from tempfile import mkdtemp import OpenSSL.SSL from twisted.internet import defer, reactor +from twisted.internet.defer import inlineCallbacks +from twisted.internet.testing import StringTransport +from twisted.protocols.policies import WrappingFactory from twisted.trial import unittest from twisted.web import resource, server, static, util -try: - from twisted.internet.testing import StringTransport -except ImportError: - # deprecated in Twisted 19.7.0 - # (remove once we bump our requirement past that version) - from twisted.test.proto_helpers import StringTransport - -from twisted.internet.defer import inlineCallbacks -from twisted.protocols.policies import WrappingFactory - from scrapy.core.downloader import webclient as client from scrapy.core.downloader.contextfactory import ScrapyClientContextFactory from scrapy.http import Headers, Request -from scrapy.settings import Settings -from scrapy.utils.misc import build_from_settings +from scrapy.utils.misc import build_from_crawler from scrapy.utils.python import to_bytes, to_unicode +from scrapy.utils.test import get_crawler from tests.mockserver import ( BrokenDownloadResource, ErrorResource, @@ -469,22 +462,22 @@ class WebClientCustomCiphersSSLTestCase(WebClientSSLTestCase): def testPayload(self): s = "0123456789" * 10 - settings = Settings({"DOWNLOADER_CLIENT_TLS_CIPHERS": self.custom_ciphers}) - client_context_factory = build_from_settings( - ScrapyClientContextFactory, settings + crawler = get_crawler( + settings_dict={"DOWNLOADER_CLIENT_TLS_CIPHERS": self.custom_ciphers} ) + client_context_factory = build_from_crawler(ScrapyClientContextFactory, crawler) return getPage( self.getURL("payload"), body=s, contextFactory=client_context_factory ).addCallback(self.assertEqual, to_bytes(s)) def testPayloadDisabledCipher(self): s = "0123456789" * 10 - settings = Settings( - {"DOWNLOADER_CLIENT_TLS_CIPHERS": "ECDHE-RSA-AES256-GCM-SHA384"} - ) - client_context_factory = build_from_settings( - ScrapyClientContextFactory, settings + crawler = get_crawler( + settings_dict={ + "DOWNLOADER_CLIENT_TLS_CIPHERS": "ECDHE-RSA-AES256-GCM-SHA384" + } ) + client_context_factory = build_from_crawler(ScrapyClientContextFactory, crawler) d = getPage( self.getURL("payload"), body=s, contextFactory=client_context_factory ) From 83d4939d41ab8790587f721755a74f883cc04e31 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Mon, 11 Nov 2024 22:14:42 +0500 Subject: [PATCH 02/10] Deprecate scrapy.extensions.feedexport.build_storage() and simplify _get_storage(). --- scrapy/extensions/feedexport.py | 50 ++++++++------------------------- 1 file changed, 12 insertions(+), 38 deletions(-) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 6ab88dbb4..27f0b79ae 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -62,6 +62,11 @@ def build_storage( preargs: Iterable[Any] = (), **kwargs: Any, ) -> _StorageT: + warnings.warn( + "scrapy.extensions.feedexport.build_storage() is deprecated, call the builder directly.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) kwargs["feed_options"] = feed_options return builder(*preargs, uri, *args, **kwargs) @@ -248,8 +253,7 @@ class S3FeedStorage(BlockingFeedStorage): *, feed_options: dict[str, Any] | None = None, ) -> Self: - return build_storage( - cls, + return cls( uri, access_key=crawler.settings["AWS_ACCESS_KEY_ID"], secret_key=crawler.settings["AWS_SECRET_ACCESS_KEY"], @@ -323,10 +327,9 @@ class FTPFeedStorage(BlockingFeedStorage): *, feed_options: dict[str, Any] | None = None, ) -> Self: - return build_storage( - cls, + return cls( uri, - crawler.settings.getbool("FEED_STORAGE_FTP_ACTIVE"), + use_active_mode=crawler.settings.getbool("FEED_STORAGE_FTP_ACTIVE"), feed_options=feed_options, ) @@ -407,15 +410,12 @@ class FeedSlot: self.exporter.start_exporting() self._exporting = True - def _get_instance( - self, objcls: type[BaseItemExporter], *args: Any, **kwargs: Any - ) -> BaseItemExporter: - return build_from_crawler(objcls, self.crawler, *args, **kwargs) - def _get_exporter( self, file: IO[bytes], format: str, *args: Any, **kwargs: Any ) -> BaseItemExporter: - return self._get_instance(self.exporters[format], file, *args, **kwargs) + return build_from_crawler( + self.exporters[format], self.crawler, file, *args, **kwargs + ) def finish_exporting(self) -> None: if self._exporting: @@ -692,34 +692,8 @@ class FeedExporter: def _get_storage( self, uri: str, feed_options: dict[str, Any] ) -> FeedStorageProtocol: - """Fork of create_instance specific to feed storage classes - - It supports not passing the *feed_options* parameters to classes that - do not support it, and issuing a deprecation warning instead. - """ feedcls = self.storages.get(urlparse(uri).scheme, self.storages["file"]) - crawler = getattr(self, "crawler", None) - - def build_instance( - builder: type[FeedStorageProtocol], *preargs: Any - ) -> FeedStorageProtocol: - return build_storage( - builder, uri, feed_options=feed_options, preargs=preargs - ) - - instance: FeedStorageProtocol - if crawler and hasattr(feedcls, "from_crawler"): - instance = build_instance(feedcls.from_crawler, crawler) - method_name = "from_crawler" - elif hasattr(feedcls, "from_settings"): - instance = build_instance(feedcls.from_settings, self.settings) - method_name = "from_settings" - else: - instance = build_instance(feedcls) - method_name = "__new__" - if instance is None: - raise TypeError(f"{feedcls.__qualname__}.{method_name} returned None") - return instance + return build_from_crawler(feedcls, self.crawler, uri, feed_options=feed_options) def _get_uri_params( self, From f796d8780c75543eadb6cf3689c7d0ca02896f0c Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 12 Nov 2024 21:08:04 +0500 Subject: [PATCH 03/10] Add tests for MediaPipeline.from_crawler() and related code. --- tests/test_pipeline_files.py | 62 ++++++++++++++++++++++ tests/test_pipeline_media.py | 99 ++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+) diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index 47840caaa..83eaa1fdd 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -2,6 +2,7 @@ import dataclasses import os import random import time +import warnings from datetime import datetime from io import BytesIO from pathlib import Path @@ -16,6 +17,7 @@ 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 ( @@ -687,3 +689,63 @@ def _prepare_request_object(item_url, flags=None): item_url, meta={"response": Response(item_url, status=200, body=b"data", flags=flags)}, ) + + +# this is separate from the one in test_pipeline_media.py to specifically test FilesPipeline subclasses +class BuildFromCrawlerTestCase(unittest.TestCase): + def setUp(self): + self.tempdir = mkdtemp() + self.crawler = get_crawler(Spider, {"FILES_STORE": self.tempdir}) + + def tearDown(self): + rmtree(self.tempdir) + + def test_simple(self): + class Pipeline(FilesPipeline): + pass + + 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), 0) + assert pipe.store + + def test_has_from_settings(self): + class Pipeline(FilesPipeline): + @classmethod + def from_settings(cls, settings): + o = super().from_settings(settings) + o._from_settings_called = True + return o + + 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), 0) + 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): + @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._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) + assert pipe.store + assert pipe._from_crawler_called diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index c979e45d7..920b4246e 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -1,5 +1,8 @@ from __future__ import annotations +import warnings + +import pytest from testfixtures import LogCapture from twisted.internet import reactor from twisted.internet.defer import Deferred, inlineCallbacks @@ -410,3 +413,99 @@ class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase): self._assert_request_no3xx( UserDefinedPipeline, {"USERDEFINEDPIPELINE_MEDIA_ALLOW_REDIRECTS": True} ) + + +class BuildFromCrawlerTestCase(unittest.TestCase): + def setUp(self): + self.crawler = get_crawler(Spider, {"FILES_STORE": "/foo"}) + + def test_simple(self): + class Pipeline(UserDefinedPipeline): + pass + + 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), 0) + + def test_has_from_settings(self): + class Pipeline(UserDefinedPipeline): + @classmethod + def from_settings(cls, settings): + o = cls() + o._from_settings_called = True + return o + + 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), 0) + assert pipe._from_settings_called + + def test_has_from_settings_and_init(self): + class Pipeline(UserDefinedPipeline): + def __init__(self, store_uri, settings): + super().__init__() + self._init_called = True + + @classmethod + def from_settings(cls, settings): + store_uri = settings["FILES_STORE"] + o = cls(store_uri, settings=settings) + o._from_settings_called = True + return o + + 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), 0) + 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__() + 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._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) + assert pipe._from_crawler_called + assert pipe._init_called + + def test_has_from_crawler(self): + class Pipeline(UserDefinedPipeline): + @classmethod + def from_crawler(cls, crawler): + settings = crawler.settings + o = super().from_crawler(crawler) + o._from_crawler_called = True + o.store_uri = settings["FILES_STORE"] + 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) + assert pipe._from_crawler_called From 499e7e8aa685b2c8ba60576707e6fbf8ed8180ba Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 12 Nov 2024 21:12:32 +0500 Subject: [PATCH 04/10] Add from_crawler() to components that only had from_settings(). --- docs/topics/email.rst | 16 ++++++------ docs/topics/request-response.rst | 9 +------ scrapy/core/downloader/contextfactory.py | 26 ++++++++++++++++++++ scrapy/dupefilters.py | 31 +++++++++++++++++++++--- scrapy/extensions/memusage.py | 2 +- scrapy/extensions/statsmailer.py | 2 +- scrapy/mail.py | 16 ++++++++++++ scrapy/spidermiddlewares/urllength.py | 17 ++++++++++++- tests/test_dupefilters.py | 18 -------------- tests/test_spidermiddleware_urllength.py | 7 ++---- 10 files changed, 99 insertions(+), 45 deletions(-) diff --git a/docs/topics/email.rst b/docs/topics/email.rst index d6a7ad354..8f7a2357a 100644 --- a/docs/topics/email.rst +++ b/docs/topics/email.rst @@ -27,13 +27,13 @@ the standard ``__init__`` method: mailer = MailSender() -Or you can instantiate it passing a Scrapy settings object, which will respect -the :ref:`settings `: +Or you can instantiate it passing a :class:`scrapy.Crawler` instance, which +will respect the :ref:`settings `: .. skip: start .. code-block:: python - mailer = MailSender.from_settings(settings) + mailer = MailSender.from_crawler(crawler) And here is how to use it to send an e-mail (without attachments): @@ -81,13 +81,13 @@ rest of the framework. :param smtpssl: enforce using a secure SSL connection :type smtpssl: bool - .. classmethod:: from_settings(settings) + .. classmethod:: from_crawler(crawler) - Instantiate using a Scrapy settings object, which will respect - :ref:`these Scrapy settings `. + Instantiate using a :class:`scrapy.Crawler` instance, which will + respect :ref:`these Scrapy settings `. - :param settings: the e-mail recipients - :type settings: :class:`scrapy.settings.Settings` object + :param crawler: the crawler + :type settings: :class:`scrapy.Crawler` object .. method:: send(to, subject, body, cc=None, attachs=(), mimetype='text/plain', charset=None) diff --git a/docs/topics/request-response.rst b/docs/topics/request-response.rst index 7c15b67e8..710e2e131 100644 --- a/docs/topics/request-response.rst +++ b/docs/topics/request-response.rst @@ -488,7 +488,7 @@ A request fingerprinter is a class that must implement the following method: :param request: request to fingerprint :type request: scrapy.http.Request -Additionally, it may also implement the following methods: +Additionally, it may also implement the following method: .. classmethod:: from_crawler(cls, crawler) :noindex: @@ -504,13 +504,6 @@ Additionally, it may also implement the following methods: :param crawler: crawler that uses this request fingerprinter :type crawler: :class:`~scrapy.crawler.Crawler` object -.. classmethod:: from_settings(cls, settings) - - If present, and ``from_crawler`` is not defined, this class method is called - to create a request fingerprinter instance from a - :class:`~scrapy.settings.Settings` object. It must return a new instance of - the request fingerprinter. - .. currentmodule:: scrapy.http The :meth:`fingerprint` method of the default request fingerprinter, diff --git a/scrapy/core/downloader/contextfactory.py b/scrapy/core/downloader/contextfactory.py index f80f832a7..8e17eab9a 100644 --- a/scrapy/core/downloader/contextfactory.py +++ b/scrapy/core/downloader/contextfactory.py @@ -21,6 +21,7 @@ from scrapy.core.downloader.tls import ( ScrapyClientTLSOptions, openssl_methods, ) +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.utils.misc import build_from_crawler, load_object if TYPE_CHECKING: @@ -69,6 +70,31 @@ class ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): method: int = SSL.SSLv23_METHOD, *args: Any, **kwargs: Any, + ) -> Self: + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings, method, *args, **kwargs) + + @classmethod + def from_crawler( + cls, + crawler: Crawler, + method: int = SSL.SSLv23_METHOD, + *args: Any, + **kwargs: Any, + ) -> Self: + return cls._from_settings(crawler.settings, method, *args, **kwargs) + + @classmethod + def _from_settings( + cls, + settings: BaseSettings, + method: int = SSL.SSLv23_METHOD, + *args: Any, + **kwargs: Any, ) -> Self: tls_verbose_logging: bool = settings.getbool( "DOWNLOADER_CLIENT_TLS_VERBOSE_LOGGING" diff --git a/scrapy/dupefilters.py b/scrapy/dupefilters.py index d37d2741a..7b8eea135 100644 --- a/scrapy/dupefilters.py +++ b/scrapy/dupefilters.py @@ -1,9 +1,11 @@ from __future__ import annotations import logging +import warnings from pathlib import Path from typing import TYPE_CHECKING +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.utils.job import job_dir from scrapy.utils.request import ( RequestFingerprinter, @@ -26,6 +28,15 @@ if TYPE_CHECKING: class BaseDupeFilter: @classmethod def from_settings(cls, settings: BaseSettings) -> Self: + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls() + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: return cls() def request_seen(self, request: Request) -> bool: @@ -72,17 +83,31 @@ class RFPDupeFilter(BaseDupeFilter): *, fingerprinter: RequestFingerprinterProtocol | None = None, ) -> Self: - debug = settings.getbool("DUPEFILTER_DEBUG") - return cls(job_dir(settings), debug, fingerprinter=fingerprinter) + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings, fingerprinter=fingerprinter) @classmethod def from_crawler(cls, crawler: Crawler) -> Self: assert crawler.request_fingerprinter - return cls.from_settings( + return cls._from_settings( crawler.settings, fingerprinter=crawler.request_fingerprinter, ) + @classmethod + def _from_settings( + cls, + settings: BaseSettings, + *, + fingerprinter: RequestFingerprinterProtocol | None = None, + ) -> Self: + debug = settings.getbool("DUPEFILTER_DEBUG") + return cls(job_dir(settings), debug, fingerprinter=fingerprinter) + def request_seen(self, request: Request) -> bool: fp = self.request_fingerprint(request) if fp in self.fingerprints: diff --git a/scrapy/extensions/memusage.py b/scrapy/extensions/memusage.py index 73d864d5d..d7f810107 100644 --- a/scrapy/extensions/memusage.py +++ b/scrapy/extensions/memusage.py @@ -48,7 +48,7 @@ class MemoryUsage: self.check_interval: float = crawler.settings.getfloat( "MEMUSAGE_CHECK_INTERVAL_SECONDS" ) - self.mail: MailSender = MailSender.from_settings(crawler.settings) + self.mail: MailSender = MailSender.from_crawler(crawler) crawler.signals.connect(self.engine_started, signal=signals.engine_started) crawler.signals.connect(self.engine_stopped, signal=signals.engine_stopped) diff --git a/scrapy/extensions/statsmailer.py b/scrapy/extensions/statsmailer.py index 600eebcf2..221628642 100644 --- a/scrapy/extensions/statsmailer.py +++ b/scrapy/extensions/statsmailer.py @@ -33,7 +33,7 @@ class StatsMailer: recipients: list[str] = crawler.settings.getlist("STATSMAILER_RCPTS") if not recipients: raise NotConfigured - mail: MailSender = MailSender.from_settings(crawler.settings) + mail: MailSender = MailSender.from_crawler(crawler) assert crawler.stats o = cls(crawler.stats, recipients, mail) crawler.signals.connect(o.spider_closed, signal=signals.spider_closed) diff --git a/scrapy/mail.py b/scrapy/mail.py index ce7beb773..3c40fea34 100644 --- a/scrapy/mail.py +++ b/scrapy/mail.py @@ -7,6 +7,7 @@ See documentation in docs/topics/email.rst from __future__ import annotations import logging +import warnings from email import encoders as Encoders from email.mime.base import MIMEBase from email.mime.multipart import MIMEMultipart @@ -19,6 +20,7 @@ from typing import IO, TYPE_CHECKING, Any from twisted.internet import ssl from twisted.internet.defer import Deferred +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.utils.misc import arg_to_iter from scrapy.utils.python import to_bytes @@ -32,6 +34,7 @@ if TYPE_CHECKING: # typing.Self requires Python 3.11 from typing_extensions import Self + from scrapy.crawler import Crawler from scrapy.settings import BaseSettings @@ -72,6 +75,19 @@ class MailSender: @classmethod def from_settings(cls, settings: BaseSettings) -> Self: + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings) + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: + return cls._from_settings(crawler.settings) + + @classmethod + def _from_settings(cls, settings: BaseSettings) -> Self: return cls( smtphost=settings["MAIL_HOST"], mailfrom=settings["MAIL_FROM"], diff --git a/scrapy/spidermiddlewares/urllength.py b/scrapy/spidermiddlewares/urllength.py index 191adb6cd..a1cd1bb7c 100644 --- a/scrapy/spidermiddlewares/urllength.py +++ b/scrapy/spidermiddlewares/urllength.py @@ -7,9 +7,10 @@ See documentation in docs/topics/spider-middleware.rst from __future__ import annotations import logging +import warnings from typing import TYPE_CHECKING, Any -from scrapy.exceptions import NotConfigured +from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.http import Request, Response if TYPE_CHECKING: @@ -19,6 +20,7 @@ if TYPE_CHECKING: from typing_extensions import Self from scrapy import Spider + from scrapy.crawler import Crawler from scrapy.settings import BaseSettings @@ -31,6 +33,19 @@ class UrlLengthMiddleware: @classmethod def from_settings(cls, settings: BaseSettings) -> Self: + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings) + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: + return cls._from_settings(crawler.settings) + + @classmethod + def _from_settings(cls, settings: BaseSettings) -> Self: maxlength = settings.getint("URLLENGTH_LIMIT") if not maxlength: raise NotConfigured diff --git a/tests/test_dupefilters.py b/tests/test_dupefilters.py index 9ba8bd64f..4fd648f48 100644 --- a/tests/test_dupefilters.py +++ b/tests/test_dupefilters.py @@ -33,14 +33,6 @@ class FromCrawlerRFPDupeFilter(RFPDupeFilter): return df -class FromSettingsRFPDupeFilter(RFPDupeFilter): - @classmethod - def from_settings(cls, settings, *, fingerprinter=None): - df = super().from_settings(settings, fingerprinter=fingerprinter) - df.method = "from_settings" - return df - - class DirectDupeFilter: method = "n/a" @@ -56,16 +48,6 @@ class RFPDupeFilterTest(unittest.TestCase): self.assertTrue(scheduler.df.debug) self.assertEqual(scheduler.df.method, "from_crawler") - def test_df_from_settings_scheduler(self): - settings = { - "DUPEFILTER_DEBUG": True, - "DUPEFILTER_CLASS": FromSettingsRFPDupeFilter, - } - crawler = get_crawler(settings_dict=settings) - scheduler = Scheduler.from_crawler(crawler) - self.assertTrue(scheduler.df.debug) - self.assertEqual(scheduler.df.method, "from_settings") - def test_df_direct_scheduler(self): settings = { "DUPEFILTER_CLASS": DirectDupeFilter, diff --git a/tests/test_spidermiddleware_urllength.py b/tests/test_spidermiddleware_urllength.py index 9111e4c82..1a0f2e223 100644 --- a/tests/test_spidermiddleware_urllength.py +++ b/tests/test_spidermiddleware_urllength.py @@ -3,7 +3,6 @@ from unittest import TestCase from testfixtures import LogCapture from scrapy.http import Request, Response -from scrapy.settings import Settings from scrapy.spidermiddlewares.urllength import UrlLengthMiddleware from scrapy.spiders import Spider from scrapy.utils.test import get_crawler @@ -12,12 +11,10 @@ from scrapy.utils.test import get_crawler class TestUrlLengthMiddleware(TestCase): def setUp(self): self.maxlength = 25 - settings = Settings({"URLLENGTH_LIMIT": self.maxlength}) - - crawler = get_crawler(Spider) + crawler = get_crawler(Spider, {"URLLENGTH_LIMIT": self.maxlength}) self.spider = crawler._create_spider("foo") self.stats = crawler.stats - self.mw = UrlLengthMiddleware.from_settings(settings) + self.mw = UrlLengthMiddleware.from_crawler(crawler) self.response = Response("http://scrapytest.org") self.short_url_req = Request("http://scrapytest.org/") From eda1a8a7c5b3b61aedd5fcc2e3950b7af28a2926 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 12 Nov 2024 22:57:39 +0500 Subject: [PATCH 05/10] Deprecate MiddlewareManager.from_settings(). --- scrapy/middleware.py | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/scrapy/middleware.py b/scrapy/middleware.py index 9e994703d..2b67dcd21 100644 --- a/scrapy/middleware.py +++ b/scrapy/middleware.py @@ -65,13 +65,19 @@ class MiddlewareManager: @classmethod def from_settings(cls, settings: Settings, crawler: Crawler | None = None) -> Self: - if crawler is None: - warnings.warn( - "Calling MiddlewareManager.from_settings() without a Crawler instance is deprecated." - " As this method will be deprecated in the future, please switch to from_crawler().", - category=ScrapyDeprecationWarning, - stacklevel=2, - ) + warnings.warn( + f"{cls.__name__}.from_settings() is deprecated, use from_crawler() instead.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + return cls._from_settings(settings, crawler) + + @classmethod + def from_crawler(cls, crawler: Crawler) -> Self: + return cls._from_settings(crawler.settings, crawler) + + @classmethod + def _from_settings(cls, settings: Settings, crawler: Crawler | None = None) -> Self: mwlist = cls._get_mwlist_from_settings(settings) middlewares = [] enabled = [] @@ -102,10 +108,6 @@ class MiddlewareManager: ) return cls(*middlewares) - @classmethod - def from_crawler(cls, crawler: Crawler) -> Self: - return cls.from_settings(crawler.settings, crawler) - def _add_middleware(self, mw: Any) -> None: if hasattr(mw, "open_spider"): self.methods["open_spider"].append(mw.open_spider) From 8700a5b7a92582fb1dc2a8fad7e41aa79948258b Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 12 Nov 2024 23:25:53 +0500 Subject: [PATCH 06/10] Deprecate build_from_crawler() calling from_settings(). --- scrapy/middleware.py | 8 ++++++++ scrapy/utils/misc.py | 8 ++++++++ tests/test_utils_request.py | 5 ++++- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/scrapy/middleware.py b/scrapy/middleware.py index 2b67dcd21..91411506f 100644 --- a/scrapy/middleware.py +++ b/scrapy/middleware.py @@ -54,6 +54,14 @@ class MiddlewareManager: @staticmethod def _build_from_settings(objcls: type[_T], settings: BaseSettings) -> _T: if hasattr(objcls, "from_settings"): + warnings.warn( + f"{objcls.__qualname__} has from_settings() but not from_crawler()." + " This is deprecated and calling from_settings() will be removed in a future" + " Scrapy version. You can implement a simple from_crawler() that calls" + " from_settings() with crawler.settings.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) instance = objcls.from_settings(settings) # type: ignore[attr-defined] method_name = "from_settings" else: diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index efb475131..a408a205d 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -185,6 +185,14 @@ def build_from_crawler( instance = objcls.from_crawler(crawler, *args, **kwargs) # type: ignore[attr-defined] method_name = "from_crawler" elif hasattr(objcls, "from_settings"): + warnings.warn( + f"{objcls.__qualname__} has from_settings() but not from_crawler()." + " This is deprecated and calling from_settings() will be removed in a future" + " Scrapy version. You can implement a simple from_crawler() that calls" + " from_settings() with crawler.settings.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) instance = objcls.from_settings(crawler.settings, *args, **kwargs) # type: ignore[attr-defined] method_name = "from_settings" else: diff --git a/tests/test_utils_request.py b/tests/test_utils_request.py index 965d050a4..0a3e3b00b 100644 --- a/tests/test_utils_request.py +++ b/tests/test_utils_request.py @@ -8,6 +8,7 @@ from weakref import WeakKeyDictionary import pytest +from scrapy.exceptions import ScrapyDeprecationWarning from scrapy.http import Request from scrapy.utils.python import to_bytes from scrapy.utils.request import ( @@ -384,7 +385,9 @@ class CustomRequestFingerprinterTestCase(unittest.TestCase): "REQUEST_FINGERPRINTER_CLASS": RequestFingerprinter, "FINGERPRINT": b"fingerprint", } - crawler = get_crawler(settings_dict=settings) + with warnings.catch_warnings(): + warnings.simplefilter("ignore", ScrapyDeprecationWarning) + crawler = get_crawler(settings_dict=settings) request = Request("http://www.example.com") fingerprint = crawler.request_fingerprinter.fingerprint(request) From 28fafbb8c56257eb6f09b8cbcb919483d5b30a11 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 14 Nov 2024 01:29:51 +0500 Subject: [PATCH 07/10] Modernize the media pipeline initialization API. --- scrapy/pipelines/files.py | 49 +++++++++++++++++++++-- scrapy/pipelines/images.py | 27 ++++++++++--- scrapy/pipelines/media.py | 46 +++++++++++++++++---- tests/test_pipeline_files.py | 75 +++++++++++++++++++++-------------- tests/test_pipeline_images.py | 47 +++++++++++++--------- tests/test_pipeline_media.py | 47 ++++++++++++++-------- 6 files changed, 211 insertions(+), 80 deletions(-) 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 From 6aa4d2b4ab28b8f657645de613295ea2498e8cee Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 14 Nov 2024 02:01:04 +0500 Subject: [PATCH 08/10] Prefer crawler.settings over settings in media pipelines. --- scrapy/pipelines/files.py | 18 +++++++++++++++--- scrapy/pipelines/images.py | 19 ++++++++++++++++--- scrapy/pipelines/media.py | 13 +++++++++++-- tests/test_pipeline_files.py | 2 +- tests/test_pipeline_images.py | 6 +----- tests/test_pipeline_media.py | 3 +-- 6 files changed, 45 insertions(+), 16 deletions(-) diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index f83037e6c..3b730c432 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -453,7 +453,17 @@ class FilesPipeline(MediaPipeline): if not store_uri: raise NotConfigured - if isinstance(settings, dict) or settings is None: + if crawler is not None: + if settings is not None: + warnings.warn( + f"FilesPipeline.__init__() was called with a crawler instance and a settings instance" + f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + settings = crawler.settings + elif isinstance(settings, dict) or settings is None: settings = Settings(settings) cls_name = "FilesPipeline" self.store: FilesStoreProtocol = self._get_store(store_uri) @@ -473,7 +483,9 @@ class FilesPipeline(MediaPipeline): ) super().__init__( - download_func=download_func, settings=settings, crawler=crawler + download_func=download_func, + settings=settings if not crawler else None, + crawler=crawler, ) @classmethod @@ -526,7 +538,7 @@ class FilesPipeline(MediaPipeline): store_uri = settings["FILES_STORE"] if "crawler" in get_func_args(cls.__init__): - o = cls(store_uri, settings=settings, crawler=crawler) + o = cls(store_uri, crawler=crawler) else: o = cls(store_uri, settings=settings) if crawler: diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index 71da6a196..fa26133bb 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -79,10 +79,23 @@ class ImagesPipeline(FilesPipeline): ) super().__init__( - store_uri, settings=settings, download_func=download_func, crawler=crawler + store_uri, + settings=settings if not crawler else None, + download_func=download_func, + crawler=crawler, ) - if isinstance(settings, dict) or settings is None: + if crawler is not None: + if settings is not None: + warnings.warn( + f"ImagesPipeline.__init__() was called with a crawler instance and a settings instance" + f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + settings = crawler.settings + elif isinstance(settings, dict) or settings is None: settings = Settings(settings) resolve = functools.partial( @@ -140,7 +153,7 @@ class ImagesPipeline(FilesPipeline): store_uri = settings["IMAGES_STORE"] if "crawler" in get_func_args(cls.__init__): - o = cls(store_uri, settings=settings, crawler=crawler) + o = cls(store_uri, crawler=crawler) else: o = cls(store_uri, settings=settings) if crawler: diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 99abed09e..70c52d090 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -81,7 +81,17 @@ class MediaPipeline(ABC): ): self.download_func = download_func - if isinstance(settings, dict) or settings is None: + if crawler is not None: + if settings is not None: + warnings.warn( + f"MediaPipeline.__init__() was called with a crawler instance and a settings instance" + f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", + category=ScrapyDeprecationWarning, + stacklevel=2, + ) + settings = crawler.settings + elif isinstance(settings, dict) or settings is None: settings = Settings(settings) resolve = functools.partial( self._key_for_pipe, base_class_name="MediaPipeline", settings=settings @@ -92,7 +102,6 @@ class MediaPipeline(ABC): self._handle_statuses(self.allow_redirects) if crawler: - # TODO use crawler.settings self._finish_init(crawler) self._modern_init = True else: diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index 5e94f9271..9dcb3e4d1 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -755,7 +755,7 @@ class BuildFromCrawlerTestCase(unittest.TestCase): def from_crawler(cls, crawler): settings = crawler.settings store_uri = settings["FILES_STORE"] - o = cls(store_uri, settings=settings, crawler=crawler) + o = cls(store_uri, crawler=crawler) o._from_crawler_called = True return o diff --git a/tests/test_pipeline_images.py b/tests/test_pipeline_images.py index 3f18c83f7..3ffef4102 100644 --- a/tests/test_pipeline_images.py +++ b/tests/test_pipeline_images.py @@ -13,7 +13,6 @@ from twisted.trial import unittest 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 @@ -392,10 +391,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): have different settings. """ custom_settings = self._generate_fake_settings() - default_settings = Settings() - default_sts_pipe = ImagesPipeline( - self.tempdir, settings=default_settings, crawler=get_crawler(None) # TODO - ) + default_sts_pipe = ImagesPipeline(self.tempdir, crawler=get_crawler(None)) 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) diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index a825de92a..58a2d3678 100644 --- a/tests/test_pipeline_media.py +++ b/tests/test_pipeline_media.py @@ -378,8 +378,7 @@ class MediaPipelineTestCase(BaseMediaPipelineTestCase): class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase): def _assert_request_no3xx(self, pipeline_class, settings): - crawler = get_crawler(None, settings) - pipe = pipeline_class(settings=settings, crawler=crawler) # TODO + pipe = pipeline_class(crawler=get_crawler(None, settings)) request = Request("http://url") pipe._modify_media_request(request) From 2ad5f0c12bfafc66fda6eb71790f447d2f7b8c13 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 14 Nov 2024 13:03:04 +0500 Subject: [PATCH 09/10] Extract duplicated code. --- scrapy/pipelines/files.py | 33 ++++++++++++++++++--------------- scrapy/pipelines/images.py | 35 +++-------------------------------- 2 files changed, 21 insertions(+), 47 deletions(-) diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index 3b730c432..065d822f3 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -29,7 +29,7 @@ from scrapy.exceptions import IgnoreRequest, NotConfigured, ScrapyDeprecationWar 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.settings import BaseSettings, Settings from scrapy.utils.boto import is_botocore_available from scrapy.utils.datatypes import CaseInsensitiveDict from scrapy.utils.deprecate import method_is_overridden @@ -513,6 +513,23 @@ class FilesPipeline(MediaPipeline): @classmethod def _from_settings(cls, settings: Settings, crawler: Crawler | None) -> Self: + cls._update_stores(settings) + store_uri = settings["FILES_STORE"] + if "crawler" in get_func_args(cls.__init__): + o = cls(store_uri, 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 + + @classmethod + def _update_stores(cls, settings: BaseSettings) -> None: 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"] @@ -536,20 +553,6 @@ class FilesPipeline(MediaPipeline): ftp_store.FTP_PASSWORD = settings["FTP_PASSWORD"] ftp_store.USE_ACTIVE_MODE = settings.getbool("FEED_STORAGE_FTP_ACTIVE") - store_uri = settings["FILES_STORE"] - if "crawler" in get_func_args(cls.__init__): - o = cls(store_uri, 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 scheme = "file" diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index fa26133bb..7defafb26 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -11,21 +11,14 @@ import hashlib import warnings from contextlib import suppress from io import BytesIO -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, Any from itemadapter import ItemAdapter from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning from scrapy.http import Request, Response from scrapy.http.request import NO_CALLBACK -from scrapy.pipelines.files import ( - FileException, - FilesPipeline, - FTPFilesStore, - GCSFilesStore, - S3FilesStore, - _md5sum, -) +from scrapy.pipelines.files import FileException, FilesPipeline, _md5sum from scrapy.settings import Settings from scrapy.utils.python import get_func_args, to_bytes @@ -128,29 +121,7 @@ class ImagesPipeline(FilesPipeline): @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"] - s3store.AWS_SESSION_TOKEN = settings["AWS_SESSION_TOKEN"] - s3store.AWS_ENDPOINT_URL = settings["AWS_ENDPOINT_URL"] - s3store.AWS_REGION_NAME = settings["AWS_REGION_NAME"] - s3store.AWS_USE_SSL = settings["AWS_USE_SSL"] - s3store.AWS_VERIFY = settings["AWS_VERIFY"] - s3store.POLICY = settings["IMAGES_STORE_S3_ACL"] - - gcs_store: type[GCSFilesStore] = cast( - type[GCSFilesStore], cls.STORE_SCHEMES["gs"] - ) - gcs_store.GCS_PROJECT_ID = settings["GCS_PROJECT_ID"] - gcs_store.POLICY = settings["IMAGES_STORE_GCS_ACL"] or None - - ftp_store: type[FTPFilesStore] = cast( - type[FTPFilesStore], cls.STORE_SCHEMES["ftp"] - ) - ftp_store.FTP_USERNAME = settings["FTP_USER"] - ftp_store.FTP_PASSWORD = settings["FTP_PASSWORD"] - ftp_store.USE_ACTIVE_MODE = settings.getbool("FEED_STORAGE_FTP_ACTIVE") - + cls._update_stores(settings) store_uri = settings["IMAGES_STORE"] if "crawler" in get_func_args(cls.__init__): o = cls(store_uri, crawler=crawler) From 929d665a74333434c9cede7133ea4f0707dbf9a6 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 14 Nov 2024 19:35:56 +0500 Subject: [PATCH 10/10] Address PR feedback. --- scrapy/middleware.py | 8 -------- scrapy/pipelines/files.py | 8 ++++---- scrapy/pipelines/images.py | 6 +++--- scrapy/pipelines/media.py | 8 ++++---- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/scrapy/middleware.py b/scrapy/middleware.py index 91411506f..2b67dcd21 100644 --- a/scrapy/middleware.py +++ b/scrapy/middleware.py @@ -54,14 +54,6 @@ class MiddlewareManager: @staticmethod def _build_from_settings(objcls: type[_T], settings: BaseSettings) -> _T: if hasattr(objcls, "from_settings"): - warnings.warn( - f"{objcls.__qualname__} has from_settings() but not from_crawler()." - " This is deprecated and calling from_settings() will be removed in a future" - " Scrapy version. You can implement a simple from_crawler() that calls" - " from_settings() with crawler.settings.", - category=ScrapyDeprecationWarning, - stacklevel=2, - ) instance = objcls.from_settings(settings) # type: ignore[attr-defined] method_name = "from_settings" else: diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index 065d822f3..196b54acb 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -35,7 +35,7 @@ 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 get_func_args, to_bytes +from scrapy.utils.python import get_func_args, global_object_name, to_bytes from scrapy.utils.request import referer_str if TYPE_CHECKING: @@ -457,7 +457,7 @@ class FilesPipeline(MediaPipeline): if settings is not None: warnings.warn( f"FilesPipeline.__init__() was called with a crawler instance and a settings instance" - f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" when creating {global_object_name(self.__class__)}. The settings instance will be ignored" f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", category=ScrapyDeprecationWarning, stacklevel=2, @@ -501,7 +501,7 @@ class FilesPipeline(MediaPipeline): 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"{global_object_name(cls)} 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, @@ -522,7 +522,7 @@ class FilesPipeline(MediaPipeline): if crawler: o._finish_init(crawler) warnings.warn( - f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + f"{global_object_name(cls)}.__init__() doesn't take a crawler argument." " This is deprecated and the argument will be required in future Scrapy versions.", category=ScrapyDeprecationWarning, ) diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index 7defafb26..e86e7c493 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -20,7 +20,7 @@ from scrapy.http import Request, Response from scrapy.http.request import NO_CALLBACK from scrapy.pipelines.files import FileException, FilesPipeline, _md5sum from scrapy.settings import Settings -from scrapy.utils.python import get_func_args, to_bytes +from scrapy.utils.python import get_func_args, global_object_name, to_bytes if TYPE_CHECKING: from collections.abc import Callable, Iterable @@ -82,7 +82,7 @@ class ImagesPipeline(FilesPipeline): if settings is not None: warnings.warn( f"ImagesPipeline.__init__() was called with a crawler instance and a settings instance" - f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" when creating {global_object_name(self.__class__)}. The settings instance will be ignored" f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", category=ScrapyDeprecationWarning, stacklevel=2, @@ -130,7 +130,7 @@ class ImagesPipeline(FilesPipeline): if crawler: o._finish_init(crawler) warnings.warn( - f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + f"{global_object_name(cls)}.__init__() doesn't take a crawler argument." " This is deprecated and the argument will be required in future Scrapy versions.", category=ScrapyDeprecationWarning, ) diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 70c52d090..6d7808c31 100644 --- a/scrapy/pipelines/media.py +++ b/scrapy/pipelines/media.py @@ -28,7 +28,7 @@ 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 +from scrapy.utils.python import get_func_args, global_object_name if TYPE_CHECKING: from collections.abc import Callable @@ -85,7 +85,7 @@ class MediaPipeline(ABC): if settings is not None: warnings.warn( f"MediaPipeline.__init__() was called with a crawler instance and a settings instance" - f" when creating {self.__class__.__qualname__}. The settings instance will be ignored" + f" when creating {global_object_name(self.__class__)}. The settings instance will be ignored" f" and crawler.settings will be used. The settings argument will be removed in a future Scrapy version.", category=ScrapyDeprecationWarning, stacklevel=2, @@ -107,7 +107,7 @@ class MediaPipeline(ABC): else: warnings.warn( f"MediaPipeline.__init__() was called without the crawler argument" - f" when creating {self.__class__.__qualname__}." + f" when creating {global_object_name(self.__class__)}." f" This is deprecated and the argument will be required in future Scrapy versions.", category=ScrapyDeprecationWarning, stacklevel=2, @@ -154,7 +154,7 @@ class MediaPipeline(ABC): else: pipe = cls() warnings.warn( - f"{cls.__qualname__}.__init__() doesn't take a crawler argument." + f"{global_object_name(cls)}.__init__() doesn't take a crawler argument." " This is deprecated and the argument will be required in future Scrapy versions.", category=ScrapyDeprecationWarning, )