Refactor without MEDIA_HTTPSTATUS_LIST setting

docs/topics/media-pipeline.rst

@@ -322,21 +322,18 @@ By default, there are no size constraints, so all images are processed.
 
 .. _topics-media-pipeline-override:
 
-Allowing redirection and handling various http statuses
--------------------------------------------------------
+Allowing redirections
+---------------------
 
 .. setting:: MEDIA_ALLOW_REDIRECTS
-.. setting:: MEDIA_HTTPSTATUS_LIST
 
-By default media pipelines ignore redirects. To allow redirecting(all 300 codes) set:
+By default media pipelines ignore redirects, i.e. an HTTP redirection
+to a media file URL request will mean the media download is considered failed.
+
+To handle media redirections, set this settings to ``True``:
 
     MEDIA_ALLOW_REDIRECTS = True
 
-To only allow handling only specific codes set (default: any code):
-
-    MEDIA_HTTPSTATUS_LIST = <LIST>
-    # example:
-    MEDIA_HTTPSTATUS_LIST = [303, 404]  # will not go through pipelines
 
 Extending the Media Pipelines
 =============================

scrapy/pipelines/media.py

@@ -6,8 +6,8 @@ from collections import defaultdict
 from twisted.internet.defer import Deferred, DeferredList
 from twisted.python.failure import Failure
 
-from scrapy.downloadermiddlewares.redirect import RedirectMiddleware
 from scrapy.settings import Settings
+from scrapy.utils.datatypes import SequenceExclude
 from scrapy.utils.defer import mustbe_deferred, defer_result
 from scrapy.utils.request import request_fingerprint
 from scrapy.utils.misc import arg_to_iter
@@ -29,6 +29,7 @@ class MediaPipeline(object):
 
     def __init__(self, download_func=None, settings=None):
         self.download_func = download_func
+
         if isinstance(settings, dict) or settings is None:
             settings = Settings(settings)
         resolve = functools.partial(self._key_for_pipe,
@@ -36,20 +37,12 @@ class MediaPipeline(object):
         self.allow_redirects = settings.getbool(
             resolve('MEDIA_ALLOW_REDIRECTS'), False
         )
-        self.handle_httpstatus_list = settings.getlist(
-            resolve('MEDIA_HTTPSTATUS_LIST'), []
-        )
+        self._handle_statuses(self.allow_redirects)
 
-        self.httpstatus_list = []
-        if self.handle_httpstatus_list:
-            self.httpstatus_list = self.handle_httpstatus_list
-        if self.allow_redirects:
-            if not self.httpstatus_list:
-                self.httpstatus_list = [i for i in range(1000)
-                                   if i not in RedirectMiddleware.allowed_status]
-            else:
-                self.httpstatus_list = [i for i in self.httpstatus_list
-                                   if i not in RedirectMiddleware.allowed_status]
+    def _handle_statuses(self, allow_redirects):
+        self.handle_httpstatus_list = None
+        if allow_redirects:
+            self.handle_httpstatus_list = SequenceExclude(range(300, 400))
 
     def _key_for_pipe(self, key, base_class_name=None,
                       settings=None):
@@ -117,8 +110,8 @@ class MediaPipeline(object):
         return dfd.addBoth(lambda _: wad)  # it must return wad at last
 
     def _modify_media_request(self, request):
-        if self.httpstatus_list:
-            request.meta['handle_httpstatus_list'] = self.httpstatus_list
+        if self.handle_httpstatus_list:
+            request.meta['handle_httpstatus_list'] = self.handle_httpstatus_list
         else:
             request.meta['handle_httpstatus_all'] = True
         return request

tests/test_pipeline_media.py

@@ -24,11 +24,12 @@ def _mocked_download_func(request, info):
 class BaseMediaPipelineTestCase(unittest.TestCase):
 
     pipeline_class = MediaPipeline
+    settings = None
 
     def setUp(self):
         self.spider = Spider('media.com')
         self.pipe = self.pipeline_class(download_func=_mocked_download_func,
-                                        settings=Settings())
+                                        settings=Settings(self.settings))
         self.pipe.open_spider(self.spider)
         self.info = self.pipe.spiderinfo
 
@@ -89,17 +90,37 @@ class BaseMediaPipelineTestCase(unittest.TestCase):
         request = Request('http://url')
         assert self.pipe._modify_media_request(request).meta == {'handle_httpstatus_all': True}
 
-        request = Request('http://url')
-        self.pipe.handle_httpstatus_list = list(range(100))
-        assert self.pipe._modify_media_request(request).meta == {'handle_httpstatus_list': list(range(100))}
-        self.pipe.handle_httpstatus_list = None
 
+class MediaPipelineAllowRedirectsTestCase(BaseMediaPipelineTestCase):
+
+    pipeline_class = MediaPipeline
+    settings = {
+        'MEDIA_ALLOW_REDIRECTS': True
+    }
+
+    def test_modify_media_request(self):
         request = Request('http://url')
-        self.pipe.allow_redirects = True
-        correct = {'handle_httpstatus_list': [i for i in range(1000)
-                                              if i not in RedirectMiddleware.allowed_status]}
-        assert self.pipe._modify_media_request(request).meta == correct
-        self.pipe.allow_redirects = False
+        meta = self.pipe._modify_media_request(request).meta
+        self.assertIn('handle_httpstatus_list', meta)
+        for status, check in [
+                (200, True),
+
+                # These are the status codes we want
+                # the downloader to handle itself
+                (301, False),
+                (302, False),
+                (302, False),
+                (307, False),
+                (308, False),
+
+                # we still want to get 4xx and 5xx
+                (400, True),
+                (404, True),
+                (500, True)]:
+            if check:
+                self.assertIn(status, meta['handle_httpstatus_list'])
+            else:
+                self.assertNotIn(status, meta['handle_httpstatus_list'])
 
 
 class MockedMediaPipeline(MediaPipeline):