From fa4d0cdfe5df7165c220e2c17c2de14262fc713e Mon Sep 17 00:00:00 2001 From: Pawel Miech Date: Mon, 20 Jun 2016 12:39:09 +0200 Subject: [PATCH] [FilesPipeline, ImagesPipeline] fix for cls attrs with DEFAULT prefix some class attributes for ImagePipeline and FilesPipeline had DEFAULT prefix. These attributes should be preserved as well, if users subclasses define values for DEFAULT_ attribute this value should be preserved. --- scrapy/pipelines/files.py | 8 ++++++-- scrapy/pipelines/images.py | 15 +++++++++++---- tests/test_pipeline_files.py | 29 ++++++++++++++++++++++------- tests/test_pipeline_images.py | 13 +++++++++++++ 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/scrapy/pipelines/files.py b/scrapy/pipelines/files.py index b9c43dc3b..73eda5f34 100644 --- a/scrapy/pipelines/files.py +++ b/scrapy/pipelines/files.py @@ -235,11 +235,15 @@ class FilesPipeline(MediaPipeline): self.expires = settings.getint( self._key_for_pipe('FILES_EXPIRES', cls_name), self.EXPIRES ) + if not hasattr(self, "FILES_URLS_FIELD"): + self.FILES_URLS_FIELD = self.DEFAULT_FILES_URLS_FIELD + if not hasattr(self, "FILES_RESULT_FIELD"): + self.FILES_RESULT_FIELD = self.DEFAULT_FILES_RESULT_FIELD self.files_urls_field = settings.get( - self._key_for_pipe('FILES_URLS_FIELD', cls_name), self.DEFAULT_FILES_URLS_FIELD + self._key_for_pipe('FILES_URLS_FIELD', cls_name), self.FILES_URLS_FIELD ) self.files_result_field = settings.get( - self._key_for_pipe('FILES_RESULT_FIELD', cls_name), self.DEFAULT_FILES_RESULT_FIELD + self._key_for_pipe('FILES_RESULT_FIELD', cls_name), self.FILES_RESULT_FIELD ) super(FilesPipeline, self).__init__(download_func=download_func) diff --git a/scrapy/pipelines/images.py b/scrapy/pipelines/images.py index de616211e..73377e2c2 100644 --- a/scrapy/pipelines/images.py +++ b/scrapy/pipelines/images.py @@ -44,8 +44,8 @@ class ImagesPipeline(FilesPipeline): MIN_HEIGHT = 0 EXPIRES = 0 THUMBS = {} - IMAGES_URLS_FIELD = 'image_urls' - IMAGES_RESULT_FIELD = 'images' + DEFAULT_IMAGES_URLS_FIELD = 'image_urls' + DEFAULT_IMAGES_RESULT_FIELD = 'images' def __init__(self, store_uri, download_func=None, settings=None): super(ImagesPipeline, self).__init__(store_uri, settings=settings, download_func=download_func) @@ -57,11 +57,18 @@ class ImagesPipeline(FilesPipeline): self.expires = settings.getint( self._key_for_pipe('IMAGES_EXPIRES', cls_name), self.EXPIRES ) + if not hasattr(self, "IMAGES_RESULT_FIELD"): + self.IMAGES_RESULT_FIELD = self.DEFAULT_IMAGES_RESULT_FIELD + if not hasattr(self, "IMAGES_URLS_FIELD"): + self.IMAGES_URLS_FIELD = self.DEFAULT_IMAGES_URLS_FIELD + + default_images_urls_field = getattr(self, "IMAGES_URLS_FIELD", "DEFAULT_IMAGES_URLS_FIELD") self.images_urls_field = settings.get( - self._key_for_pipe('IMAGES_URLS_FIELD', cls_name), self.IMAGES_URLS_FIELD + self._key_for_pipe('IMAGES_URLS_FIELD', cls_name), default_images_urls_field ) + default_images_result_field = getattr(self, "IMAGES_RESULT_FIELD", "DEFAULT_IMAGES_RESULT_FIELD") self.images_result_field = settings.get( - self._key_for_pipe('IMAGES_RESULT_FIELD', cls_name), self.IMAGES_RESULT_FIELD + self._key_for_pipe('IMAGES_RESULT_FIELD', cls_name), default_images_result_field ) self.min_width = settings.getint( self._key_for_pipe('IMAGES_MIN_WIDTH', cls_name), self.MIN_WIDTH diff --git a/tests/test_pipeline_files.py b/tests/test_pipeline_files.py index fd54b7229..bda2a2199 100644 --- a/tests/test_pipeline_files.py +++ b/tests/test_pipeline_files.py @@ -187,13 +187,13 @@ class FilesPipelineTestCaseFields(unittest.TestCase): class FilesPipelineTestCaseCustomSettings(unittest.TestCase): default_cls_settings = { "EXPIRES": 90, - "DEFAULT_FILES_URLS_FIELD": "file_urls", - "DEFAULT_FILES_RESULT_FIELD": "files" + "FILES_URLS_FIELD": "file_urls", + "FILES_RESULT_FIELD": "files" } file_cls_attr_settings_map = { ("EXPIRES", "FILES_EXPIRES", "expires"), - ("DEFAULT_FILES_URLS_FIELD", "FILES_URLS_FIELD", "files_urls_field"), - ("DEFAULT_FILES_RESULT_FIELD", "FILES_RESULT_FIELD", "files_result_field") + ("FILES_URLS_FIELD", "FILES_URLS_FIELD", "files_urls_field"), + ("FILES_RESULT_FIELD", "FILES_RESULT_FIELD", "files_result_field") } def setUp(self): @@ -221,9 +221,9 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): def _generate_fake_pipeline(self): class UserDefinedFilePipeline(FilesPipeline): - FILES_EXPIRES = random.randint(1001, 2000) - DEFAULT_FILES_URLS_FIELD = "alfa" - DEFAULT_FILES_RESULT_FIELD = "beta" + EXPIRES = 1001 + FILES_URLS_FIELD = "alfa" + FILES_RESULT_FIELD = "beta" return UserDefinedFilePipeline @@ -239,6 +239,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): default_value = self.default_cls_settings[pipe_attr] self.assertEqual(getattr(one_pipeline, pipe_attr), default_value) custom_value = custom_settings[settings_attr] + self.assertNotEqual(default_value, custom_value) self.assertEqual(getattr(another_pipeline, pipe_ins_attr), custom_value) def test_subclass_attributes_preserved_if_no_settings(self): @@ -248,6 +249,8 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): pipe_cls = self._generate_fake_pipeline() pipe = pipe_cls.from_settings(Settings({"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]) self.assertEqual(getattr(pipe, pipe_ins_attr), getattr(pipe, pipe_attr)) def test_subclass_attrs_preserved_custom_settings(self): @@ -260,6 +263,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): pipeline = pipeline_cls.from_settings(Settings(settings)) for pipe_attr, settings_attr, pipe_ins_attr in self.file_cls_attr_settings_map: value = getattr(pipeline, pipe_ins_attr) + self.assertNotEqual(value, self.default_cls_settings[pipe_attr]) self.assertEqual(value, getattr(pipeline, pipe_attr)) def test_no_custom_settings_for_subclasses(self): @@ -290,6 +294,7 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): 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) + self.assertNotEqual(custom_value, self.default_cls_settings[pipe_attr]) self.assertEqual(getattr(user_pipeline, pipe_inst_attr), custom_value) def test_custom_settings_and_class_attrs_for_subclasses(self): @@ -303,8 +308,18 @@ class FilesPipelineTestCaseCustomSettings(unittest.TestCase): user_pipeline = pipeline_cls.from_settings(Settings(settings)) for pipe_cls_attr, settings_attr, pipe_inst_attr in self.file_cls_attr_settings_map: custom_value = settings.get(prefix + "_" + settings_attr) + self.assertNotEqual(custom_value, self.default_cls_settings[pipe_cls_attr]) self.assertEqual(getattr(user_pipeline, pipe_inst_attr), custom_value) + def test_cls_attrs_with_DEFAULT_prefix(self): + class UserDefinedFilesPipeline(FilesPipeline): + DEFAULT_FILES_RESULT_FIELD = "this" + DEFAULT_FILES_URLS_FIELD = "that" + + pipeline = UserDefinedFilesPipeline.from_settings(Settings({"FILES_STORE": self.tempdir})) + self.assertEqual(pipeline.files_result_field, "this") + self.assertEqual(pipeline.files_urls_field, "that") + class TestS3FilesStore(unittest.TestCase): @defer.inlineCallbacks diff --git a/tests/test_pipeline_images.py b/tests/test_pipeline_images.py index a2dd5aa28..6ccd9791e 100644 --- a/tests/test_pipeline_images.py +++ b/tests/test_pipeline_images.py @@ -304,6 +304,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): 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()) + self.assertNotEqual(attr_value, self.default_pipeline_settings[pipe_attr]) self.assertEqual(attr_value, getattr(pipeline, pipe_attr)) def test_subclass_attrs_preserved_custom_settings(self): @@ -317,6 +318,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): for pipe_attr, settings_attr in self.img_cls_attribute_names: # Instance attribute (lowercase) must be equal to class attribute (uppercase). value = getattr(pipeline, pipe_attr.lower()) + self.assertNotEqual(value, self.default_pipeline_settings[pipe_attr]) self.assertEqual(value, getattr(pipeline, pipe_attr)) def test_no_custom_settings_for_subclasses(self): @@ -347,6 +349,7 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): 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) + self.assertNotEqual(custom_value, self.default_pipeline_settings[pipe_attr]) self.assertEqual(getattr(user_pipeline, pipe_attr.lower()), custom_value) def test_custom_settings_and_class_attrs_for_subclasses(self): @@ -360,8 +363,18 @@ class ImagesPipelineTestCaseCustomSettings(unittest.TestCase): user_pipeline = pipeline_cls.from_settings(Settings(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]) self.assertEqual(getattr(user_pipeline, pipe_attr.lower()), custom_value) + def test_cls_attrs_with_DEFAULT_prefix(self): + class UserDefinedImagePipeline(ImagesPipeline): + DEFAULT_IMAGES_URLS_FIELD = "something" + DEFAULT_IMAGES_RESULT_FIELD = "something_else" + + pipeline = UserDefinedImagePipeline.from_settings(Settings({"IMAGES_STORE": self.tempdir})) + self.assertEqual(pipeline.images_result_field, "something_else") + self.assertEqual(pipeline.images_urls_field, "something") + def _create_image(format, *a, **kw): buf = TemporaryFile()