Support defining file path based on item in media pipelines (#4686)

docs/topics/media-pipeline.rst

@@ -412,15 +412,16 @@ See here the methods that you can override in your custom Files Pipeline:
 
 .. class:: FilesPipeline
 
-   .. method:: file_path(self, request, response=None, info=None)
+   .. method:: file_path(self, request, response=None, info=None, *, item=None)
 
       This method is called once per downloaded item. It returns the
       download path of the file originating from the specified
       :class:`response <scrapy.http.Response>`.
 
       In addition to ``response``, this method receives the original
-      :class:`request <scrapy.Request>` and
-      :class:`info <scrapy.pipelines.media.MediaPipeline.SpiderInfo>`.
+      :class:`request <scrapy.Request>`,
+      :class:`info <scrapy.pipelines.media.MediaPipeline.SpiderInfo>` and 
+      :class:`item <scrapy.item.Item>`
 
       You can override this method to customize the download path of each file.
 
@@ -436,9 +437,12 @@ See here the methods that you can override in your custom Files Pipeline:
 
         class MyFilesPipeline(FilesPipeline):
 
-            def file_path(self, request, response=None, info=None):
+            def file_path(self, request, response=None, info=None, *, item=None):
                 return 'files/' + os.path.basename(urlparse(request.url).path)
 
+      Similarly, you can use the ``item`` to determine the file path based on some item 
+      property.
+      
       By default the :meth:`file_path` method returns
       ``full/<request URL hash>.<extension>``.
 
@@ -544,15 +548,16 @@ See here the methods that you can override in your custom Images Pipeline:
     The :class:`ImagesPipeline` is an extension of the :class:`FilesPipeline`,
     customizing the field names and adding custom behavior for images.
 
-   .. method:: file_path(self, request, response=None, info=None)
+   .. method:: file_path(self, request, response=None, info=None, *, item=None)
 
       This method is called once per downloaded item. It returns the
       download path of the file originating from the specified
       :class:`response <scrapy.http.Response>`.
 
       In addition to ``response``, this method receives the original
-      :class:`request <scrapy.Request>` and
-      :class:`info <scrapy.pipelines.media.MediaPipeline.SpiderInfo>`.
+      :class:`request <scrapy.Request>`,
+      :class:`info <scrapy.pipelines.media.MediaPipeline.SpiderInfo>` and 
+      :class:`item <scrapy.item.Item>`
 
       You can override this method to customize the download path of each file.
 
@@ -568,9 +573,12 @@ See here the methods that you can override in your custom Images Pipeline:
 
         class MyImagesPipeline(ImagesPipeline):
 
-            def file_path(self, request, response=None, info=None):
+            def file_path(self, request, response=None, info=None, *, item=None):
                 return 'files/' + os.path.basename(urlparse(request.url).path)
 
+      Similarly, you can use the ``item`` to determine the file path based on some item 
+      property.
+      
       By default the :meth:`file_path` method returns
       ``full/<request URL hash>.<extension>``.
 

scrapy/pipelines/files.py

@@ -409,7 +409,7 @@ class FilesPipeline(MediaPipeline):
         store_cls = self.STORE_SCHEMES[scheme]
         return store_cls(uri)
 
-    def media_to_download(self, request, info):
+    def media_to_download(self, request, info, *, item=None):
         def _onsuccess(result):
             if not result:
                 return  # returning None force download
@@ -436,7 +436,7 @@ class FilesPipeline(MediaPipeline):
             checksum = result.get('checksum', None)
             return {'url': request.url, 'path': path, 'checksum': checksum, 'status': 'uptodate'}
 
-        path = self.file_path(request, info=info)
+        path = self.file_path(request, info=info, item=item)
         dfd = defer.maybeDeferred(self.store.stat_file, path, info)
         dfd.addCallbacks(_onsuccess, lambda _: None)
         dfd.addErrback(
@@ -460,7 +460,7 @@ class FilesPipeline(MediaPipeline):
 
         raise FileException
 
-    def media_downloaded(self, response, request, info):
+    def media_downloaded(self, response, request, info, *, item=None):
         referer = referer_str(request)
 
         if response.status != 200:
@@ -492,8 +492,8 @@ class FilesPipeline(MediaPipeline):
         self.inc_stats(info.spider, status)
 
         try:
-            path = self.file_path(request, response=response, info=info)
-            checksum = self.file_downloaded(response, request, info)
+            path = self.file_path(request, response=response, info=info, item=item)
+            checksum = self.file_downloaded(response, request, info, item=item)
         except FileException as exc:
             logger.warning(
                 'File (error): Error processing file from %(request)s '
@@ -522,8 +522,8 @@ class FilesPipeline(MediaPipeline):
         urls = ItemAdapter(item).get(self.files_urls_field, [])
         return [Request(u) for u in urls]
 
-    def file_downloaded(self, response, request, info):
-        path = self.file_path(request, response=response, info=info)
+    def file_downloaded(self, response, request, info, *, item=None):
+        path = self.file_path(request, response=response, info=info, item=item)
         buf = BytesIO(response.body)
         checksum = md5sum(buf)
         buf.seek(0)
@@ -535,7 +535,7 @@ class FilesPipeline(MediaPipeline):
             ItemAdapter(item)[self.files_result_field] = [x for ok, x in results if ok]
         return item
 
-    def file_path(self, request, response=None, info=None):
+    def file_path(self, request, response=None, info=None, *, item=None):
         media_guid = hashlib.sha1(to_bytes(request.url)).hexdigest()
         media_ext = os.path.splitext(request.url)[1]
         # Handles empty and wild extensions by trying to guess the

scrapy/pipelines/images.py

@@ -103,12 +103,12 @@ class ImagesPipeline(FilesPipeline):
         store_uri = settings['IMAGES_STORE']
         return cls(store_uri, settings=settings)
 
-    def file_downloaded(self, response, request, info):
-        return self.image_downloaded(response, request, info)
+    def file_downloaded(self, response, request, info, *, item=None):
+        return self.image_downloaded(response, request, info, item=item)
 
-    def image_downloaded(self, response, request, info):
+    def image_downloaded(self, response, request, info, *, item=None):
         checksum = None
-        for path, image, buf in self.get_images(response, request, info):
+        for path, image, buf in self.get_images(response, request, info, item=item):
             if checksum is None:
                 buf.seek(0)
                 checksum = md5sum(buf)
@@ -119,8 +119,8 @@ class ImagesPipeline(FilesPipeline):
                 headers={'Content-Type': 'image/jpeg'})
         return checksum
 
-    def get_images(self, response, request, info):
-        path = self.file_path(request, response=response, info=info)
+    def get_images(self, response, request, info, *, item=None):
+        path = self.file_path(request, response=response, info=info, item=item)
         orig_image = Image.open(BytesIO(response.body))
 
         width, height = orig_image.size
@@ -166,7 +166,7 @@ class ImagesPipeline(FilesPipeline):
             ItemAdapter(item)[self.images_result_field] = [x for ok, x in results if ok]
         return item
 
-    def file_path(self, request, response=None, info=None):
+    def file_path(self, request, response=None, info=None, *, item=None):
         image_guid = hashlib.sha1(to_bytes(request.url)).hexdigest()
         return 'full/%s.jpg' % (image_guid)
 

scrapy/pipelines/media.py

@@ -1,12 +1,16 @@
 import functools
 import logging
 from collections import defaultdict
+from inspect import signature
+from warnings import warn
+
 from twisted.internet.defer import Deferred, DeferredList
 from twisted.python.failure import Failure
 
 from scrapy.settings import Settings
 from scrapy.utils.datatypes import SequenceExclude
 from scrapy.utils.defer import mustbe_deferred, defer_result
+from scrapy.utils.deprecate import ScrapyDeprecationWarning
 from scrapy.utils.request import request_fingerprint
 from scrapy.utils.misc import arg_to_iter
 from scrapy.utils.log import failure_to_exc_info
@@ -27,6 +31,7 @@ class MediaPipeline:
 
     def __init__(self, download_func=None, settings=None):
         self.download_func = download_func
+        self._expects_item = {}
 
         if isinstance(settings, dict) or settings is None:
             settings = Settings(settings)
@@ -38,6 +43,9 @@ class MediaPipeline:
         )
         self._handle_statuses(self.allow_redirects)
 
+        # Check if deprecated methods are being used and make them compatible
+        self._make_compatible()
+
     def _handle_statuses(self, allow_redirects):
         self.handle_httpstatus_list = None
         if allow_redirects:
@@ -77,11 +85,11 @@ class MediaPipeline:
     def process_item(self, item, spider):
         info = self.spiderinfo
         requests = arg_to_iter(self.get_media_requests(item, info))
-        dlist = [self._process_request(r, info) for r in requests]
+        dlist = [self._process_request(r, info, item) for r in requests]
         dfd = DeferredList(dlist, consumeErrors=1)
         return dfd.addCallback(self.item_completed, item, info)
 
-    def _process_request(self, request, info):
+    def _process_request(self, request, info, item):
         fp = request_fingerprint(request)
         cb = request.callback or (lambda _: _)
         eb = request.errback
@@ -102,34 +110,73 @@ class MediaPipeline:
 
         # Download request checking media_to_download hook output first
         info.downloading.add(fp)
-        dfd = mustbe_deferred(self.media_to_download, request, info)
-        dfd.addCallback(self._check_media_to_download, request, info)
+        dfd = mustbe_deferred(self.media_to_download, request, info, item=item)
+        dfd.addCallback(self._check_media_to_download, request, info, item=item)
         dfd.addBoth(self._cache_result_and_execute_waiters, fp, info)
         dfd.addErrback(lambda f: logger.error(
             f.value, exc_info=failure_to_exc_info(f), extra={'spider': info.spider})
         )
         return dfd.addBoth(lambda _: wad)  # it must return wad at last
 
+    def _make_compatible(self):
+        """Make overridable methods of MediaPipeline and subclasses backwards compatible"""
+        methods = [
+            "file_path", "media_to_download", "media_downloaded",
+            "file_downloaded", "image_downloaded", "get_images"
+        ]
+
+        for method_name in methods:
+            method = getattr(self, method_name, None)
+            if callable(method):
+                setattr(self, method_name, self._compatible(method))
+
+    def _compatible(self, func):
+        """Wrapper for overridable methods to allow backwards compatibility"""
+        self._check_signature(func)
+
+        @functools.wraps(func)
+        def wrapper(*args, **kwargs):
+            if self._expects_item[func.__name__]:
+                return func(*args, **kwargs)
+
+            kwargs.pop('item', None)
+            return func(*args, **kwargs)
+
+        return wrapper
+
+    def _check_signature(self, func):
+        sig = signature(func)
+        self._expects_item[func.__name__] = True
+
+        if 'item' not in sig.parameters:
+            old_params = str(sig)[1:-1]
+            new_params = old_params + ", *, item=None"
+            warn('%s(self, %s) is deprecated, '
+                 'please use %s(self, %s)'
+                 % (func.__name__, old_params, func.__name__, new_params),
+                 ScrapyDeprecationWarning, stacklevel=2)
+            self._expects_item[func.__name__] = False
+
     def _modify_media_request(self, request):
         if self.handle_httpstatus_list:
             request.meta['handle_httpstatus_list'] = self.handle_httpstatus_list
         else:
             request.meta['handle_httpstatus_all'] = True
 
-    def _check_media_to_download(self, result, request, info):
+    def _check_media_to_download(self, result, request, info, item):
         if result is not None:
             return result
         if self.download_func:
             # this ugly code was left only to support tests. TODO: remove
             dfd = mustbe_deferred(self.download_func, request, info.spider)
             dfd.addCallbacks(
-                callback=self.media_downloaded, callbackArgs=(request, info),
+                callback=self.media_downloaded, callbackArgs=(request, info), callbackKeywords={'item': item},
                 errback=self.media_failed, errbackArgs=(request, info))
         else:
             self._modify_media_request(request)
             dfd = self.crawler.engine.download(request, info.spider)
             dfd.addCallbacks(
-                callback=self.media_downloaded, callbackArgs=(request, info),
+                callback=self.media_downloaded, callbackArgs=(request, info), callbackKeywords={'item': item},
                 errback=self.media_failed, errbackArgs=(request, info))
         return dfd
 
@@ -171,7 +218,7 @@ class MediaPipeline:
             defer_result(result).chainDeferred(wad)
 
     # Overridable Interface
-    def media_to_download(self, request, info):
+    def media_to_download(self, request, info, *, item=None):
         """Check request before starting download"""
         pass
 
@@ -179,7 +226,7 @@ class MediaPipeline:
         """Returns the media requests to download"""
         pass
 
-    def media_downloaded(self, response, request, info):
+    def media_downloaded(self, response, request, info, *, item=None):
         """Handler for success downloads"""
         return response
 
@@ -199,3 +246,7 @@ class MediaPipeline:
                         extra={'spider': info.spider}
                     )
         return item
+
+    def file_path(self, request, response=None, info=None, *, item=None):
+        """Returns the path where downloaded media should be stored"""
+        pass

tests/test_pipeline_files.py

@@ -161,6 +161,19 @@ class FilesPipelineTestCase(unittest.TestCase):
         for p in patchers:
             p.stop()
 
+    def test_file_path_from_item(self):
+        """
+        Custom file path based on item data, overriding default implementation
+        """
+        class CustomFilesPipeline(FilesPipeline):
+            def file_path(self, request, response=None, info=None, item=None):
+                return 'full/%s' % item.get('path')
+
+        file_path = CustomFilesPipeline.from_settings(Settings({'FILES_STORE': self.tempdir})).file_path
+        item = dict(path='path-to-store-file')
+        request = Request("http://example.com")
+        self.assertEqual(file_path(request, item=item), 'full/path-to-store-file')
+
 
 class FilesPipelineTestCaseFieldsMixin:
 

tests/test_pipeline_media.py

@@ -7,7 +7,9 @@ from twisted.internet.defer import Deferred, inlineCallbacks
 from scrapy.http import Request, Response
 from scrapy.settings import Settings
 from scrapy.spiders import Spider
+from scrapy.utils.deprecate import ScrapyDeprecationWarning
 from scrapy.utils.request import request_fingerprint
+from scrapy.pipelines.images import ImagesPipeline
 from scrapy.pipelines.media import MediaPipeline
 from scrapy.pipelines.files import FileException
 from scrapy.utils.log import failure_to_exc_info
@@ -169,7 +171,7 @@ class MockedMediaPipeline(MediaPipeline):
         self._mockcalled.append('download')
         return super().download(request, info)
 
-    def media_to_download(self, request, info):
+    def media_to_download(self, request, info, *, item=None):
         self._mockcalled.append('media_to_download')
         if 'result' in request.meta:
             return request.meta.get('result')
@@ -179,7 +181,7 @@ class MockedMediaPipeline(MediaPipeline):
         self._mockcalled.append('get_media_requests')
         return item.get('requests')
 
-    def media_downloaded(self, response, request, info):
+    def media_downloaded(self, response, request, info, *, item=None):
         self._mockcalled.append('media_downloaded')
         return super().media_downloaded(response, request, info)
 
@@ -335,6 +337,123 @@ class MediaPipelineTestCase(BaseMediaPipelineTestCase):
             ['get_media_requests', 'media_to_download', 'item_completed'])
 
 
+class MockedMediaPipelineDeprecatedMethods(ImagesPipeline):
+
+    def __init__(self, *args, **kwargs):
+        super(MockedMediaPipelineDeprecatedMethods, self).__init__(*args, **kwargs)
+        self._mockcalled = []
+
+    def get_media_requests(self, item, info):
+        item_url = item['image_urls'][0]
+        return Request(
+            item_url,
+            meta={'response': Response(item_url, status=200, body=b'data')}
+        )
+
+    def inc_stats(self, *args, **kwargs):
+        return True
+
+    def media_to_download(self, request, info):
+        self._mockcalled.append('media_to_download')
+        return super(MockedMediaPipelineDeprecatedMethods, self).media_to_download(request, info)
+
+    def media_downloaded(self, response, request, info):
+        self._mockcalled.append('media_downloaded')
+        return super(MockedMediaPipelineDeprecatedMethods, self).media_downloaded(response, request, info)
+
+    def file_downloaded(self, response, request, info):
+        self._mockcalled.append('file_downloaded')
+        return super(MockedMediaPipelineDeprecatedMethods, self).file_downloaded(response, request, info)
+
+    def file_path(self, request, response=None, info=None):
+        self._mockcalled.append('file_path')
+        return super(MockedMediaPipelineDeprecatedMethods, self).file_path(request, response, info)
+
+    def get_images(self, response, request, info):
+        self._mockcalled.append('get_images')
+        return []
+
+    def image_downloaded(self, response, request, info):
+        self._mockcalled.append('image_downloaded')
+        return super(MockedMediaPipelineDeprecatedMethods, self).image_downloaded(response, request, info)
+
+
+class MediaPipelineDeprecatedMethodsTestCase(unittest.TestCase):
+
+    def setUp(self):
+        self.pipe = MockedMediaPipelineDeprecatedMethods(store_uri='store-uri', download_func=_mocked_download_func)
+        self.pipe.open_spider(None)
+        self.item = dict(image_urls=['http://picsum.photos/id/1014/200/300'], images=[])
+
+    def _assert_method_called_with_warnings(self, method, message, warnings):
+        self.assertIn(method, self.pipe._mockcalled)
+        warningShown = False
+        for warning in warnings:
+            if warning['message'] == message and warning['category'] == ScrapyDeprecationWarning:
+                warningShown = True
+        self.assertTrue(warningShown)
+
+    @inlineCallbacks
+    def test_media_to_download_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'media_to_download(self, request, info) is deprecated, '
+            'please use media_to_download(self, request, info, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('media_to_download', message, warnings)
+
+    @inlineCallbacks
+    def test_media_downloaded_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'media_downloaded(self, response, request, info) is deprecated, '
+            'please use media_downloaded(self, response, request, info, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('media_downloaded', message, warnings)
+
+    @inlineCallbacks
+    def test_file_downloaded_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'file_downloaded(self, response, request, info) is deprecated, '
+            'please use file_downloaded(self, response, request, info, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('file_downloaded', message, warnings)
+
+    @inlineCallbacks
+    def test_file_path_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'file_path(self, request, response=None, info=None) is deprecated, '
+            'please use file_path(self, request, response=None, info=None, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('file_path', message, warnings)
+
+    @inlineCallbacks
+    def test_get_images_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'get_images(self, response, request, info) is deprecated, '
+            'please use get_images(self, response, request, info, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('get_images', message, warnings)
+
+    @inlineCallbacks
+    def test_image_downloaded_called(self):
+        yield self.pipe.process_item(self.item, None)
+        warnings = self.flushWarnings([MediaPipeline._compatible])
+        message = (
+            'image_downloaded(self, response, request, info) is deprecated, '
+            'please use image_downloaded(self, response, request, info, *, item=None)'
+        )
+        self._assert_method_called_with_warnings('image_downloaded', message, warnings)
+
+
 class MediaPipelineAllowRedirectSettingsTestCase(unittest.TestCase):
 
     def _assert_request_no3xx(self, pipeline_class, settings):