diff --git a/docs/topics/media-pipeline.rst b/docs/topics/media-pipeline.rst index bfc405d9e..f258ff748 100644 --- a/docs/topics/media-pipeline.rst +++ b/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 = - # example: - MEDIA_HTTPSTATUS_LIST = [303, 404] # will not go through pipelines Extending the Media Pipelines ============================= diff --git a/scrapy/pipelines/media.py b/scrapy/pipelines/media.py index 0712101b0..02daf8d2c 100644 --- a/scrapy/pipelines/media.py +++ b/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 diff --git a/tests/test_pipeline_media.py b/tests/test_pipeline_media.py index f1b8076fd..4797956a0 100644 --- a/tests/test_pipeline_media.py +++ b/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):