diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 5f133fbde..7c7db387e 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -21,7 +21,7 @@ from w3lib.url import file_uri_to_path from scrapy import signals from scrapy.utils.ftp import ftp_makedirs_cwd from scrapy.exceptions import NotConfigured -from scrapy.utils.misc import load_object +from scrapy.utils.misc import create_instance, load_object from scrapy.utils.log import failure_to_exc_info from scrapy.utils.python import without_none_values from scrapy.utils.boto import is_botocore @@ -93,12 +93,29 @@ class FileFeedStorage(object): class S3FeedStorage(BlockingFeedStorage): - def __init__(self, uri): - from scrapy.conf import settings + def __init__(self, uri, access_key=None, secret_key=None): + # BEGIN Backwards compatibility for initialising without keys (and + # without using from_crawler) + no_defaults = access_key is None and secret_key is None + if no_defaults: + from scrapy.conf import settings + if 'AWS_ACCESS_KEY_ID' in settings or 'AWS_SECRET_ACCESS_KEY' in settings: + import warnings + from scrapy.exceptions import ScrapyDeprecationWarning + warnings.warn( + "Initialising `scrapy.extensions.feedexport.S3FeedStorage` " + "without AWS keys is deprecated. Please supply credentials or " + "use the `from_crawler()` constructor.", + category=ScrapyDeprecationWarning, + stacklevel=2 + ) + access_key = settings['AWS_ACCESS_KEY_ID'] + secret_key = settings['AWS_SECRET_ACCESS_KEY'] + # END Backwards compatibility u = urlparse(uri) self.bucketname = u.hostname - self.access_key = u.username or settings['AWS_ACCESS_KEY_ID'] - self.secret_key = u.password or settings['AWS_SECRET_ACCESS_KEY'] + self.access_key = u.username or access_key + self.secret_key = u.password or secret_key self.is_botocore = is_botocore() self.keyname = u.path[1:] # remove first "/" if self.is_botocore: @@ -111,6 +128,11 @@ class S3FeedStorage(BlockingFeedStorage): import boto self.connect_s3 = boto.connect_s3 + @classmethod + def from_crawler(cls, crawler, uri): + return cls(uri, crawler.settings['AWS_ACCESS_KEY_ID'], + crawler.settings['AWS_SECRET_ACCESS_KEY']) + def _store_in_thread(self, file): file.seek(0) if self.is_botocore: @@ -181,6 +203,7 @@ class FeedExporter(object): @classmethod def from_crawler(cls, crawler): o = cls(crawler.settings) + o.crawler = crawler crawler.signals.connect(o.open_spider, signals.spider_opened) crawler.signals.connect(o.close_spider, signals.spider_closed) crawler.signals.connect(o.item_scraped, signals.item_scraped) @@ -253,11 +276,16 @@ class FeedExporter(object): logger.error("Unknown feed storage scheme: %(scheme)s", {'scheme': scheme}) + def _get_instance(self, objcls, *args, **kwargs): + return create_instance( + objcls, self.settings, getattr(self, 'crawler', None), + *args, **kwargs) + def _get_exporter(self, *args, **kwargs): - return self.exporters[self.format](*args, **kwargs) + return self._get_instance(self.exporters[self.format], *args, **kwargs) def _get_storage(self, uri): - return self.storages[urlparse(uri).scheme](uri) + return self._get_instance(self.storages[urlparse(uri).scheme], uri) def _get_uri_params(self, spider): params = {} diff --git a/scrapy/middleware.py b/scrapy/middleware.py index be36f977e..f2240984c 100644 --- a/scrapy/middleware.py +++ b/scrapy/middleware.py @@ -3,7 +3,7 @@ import logging import pprint from scrapy.exceptions import NotConfigured -from scrapy.utils.misc import load_object +from scrapy.utils.misc import create_instance, load_object from scrapy.utils.defer import process_parallel, process_chain, process_chain_both logger = logging.getLogger(__name__) @@ -32,12 +32,7 @@ class MiddlewareManager(object): for clspath in mwlist: try: mwcls = load_object(clspath) - if crawler and hasattr(mwcls, 'from_crawler'): - mw = mwcls.from_crawler(crawler) - elif hasattr(mwcls, 'from_settings'): - mw = mwcls.from_settings(settings) - else: - mw = mwcls() + mw = create_instance(mwcls, settings, crawler) middlewares.append(mw) enabled.append(clspath) except NotConfigured as e: diff --git a/scrapy/utils/misc.py b/scrapy/utils/misc.py index 35f855007..5ccfdcd72 100644 --- a/scrapy/utils/misc.py +++ b/scrapy/utils/misc.py @@ -117,3 +117,28 @@ def md5sum(file): def rel_has_nofollow(rel): """Return True if link rel attribute has nofollow type""" return True if rel is not None and 'nofollow' in rel.split() else False + + +def create_instance(objcls, settings, crawler, *args, **kwargs): + """Construct a class instance using its ``from_crawler`` or + ``from_settings`` constructors, if available. + + At least one of ``settings`` and ``crawler`` needs to be different from + ``None``. If ``settings `` is ``None``, ``crawler.settings`` will be used. + If ``crawler`` is ``None``, only the ``from_settings`` constructor will be + tried. + + ``*args`` and ``**kwargs`` are forwarded to the constructors. + + Raises ``ValueError`` if both ``settings`` and ``crawler`` are ``None``. + """ + if settings is None: + if crawler is None: + raise ValueError("Specifiy at least one of settings and crawler.") + settings = crawler.settings + if crawler and hasattr(objcls, 'from_crawler'): + return objcls.from_crawler(crawler, *args, **kwargs) + elif hasattr(objcls, 'from_settings'): + return objcls.from_settings(settings, *args, **kwargs) + else: + return objcls(*args, **kwargs) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index 0d9f1e83c..2b57449d9 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -6,16 +6,19 @@ from io import BytesIO import tempfile import shutil from six.moves.urllib.parse import urlparse +import warnings from zope.interface.verify import verifyObject from twisted.trial import unittest from twisted.internet import defer from scrapy.crawler import CrawlerRunner from scrapy.settings import Settings +from tests import mock from tests.mockserver import MockServer from w3lib.url import path_to_file_uri import scrapy +from scrapy.exporters import CsvItemExporter from scrapy.extensions.feedexport import ( IFeedStorage, FileFeedStorage, FTPFeedStorage, S3FeedStorage, StdoutFeedStorage, @@ -130,13 +133,49 @@ class BlockingFeedStorageTest(unittest.TestCase): class S3FeedStorageTest(unittest.TestCase): + @mock.patch('scrapy.conf.settings', new={'AWS_ACCESS_KEY_ID': 'conf_key', + 'AWS_SECRET_ACCESS_KEY': 'conf_secret'}, create=True) + def test_parse_credentials(self): + try: + import boto + except ImportError: + raise unittest.SkipTest("S3FeedStorage requires boto") + aws_credentials = {'AWS_ACCESS_KEY_ID': 'settings_key', + 'AWS_SECRET_ACCESS_KEY': 'settings_secret'} + crawler = get_crawler(settings_dict=aws_credentials) + # Instantiate with crawler + storage = S3FeedStorage.from_crawler(crawler, + 's3://mybucket/export.csv') + self.assertEqual(storage.access_key, 'settings_key') + self.assertEqual(storage.secret_key, 'settings_secret') + # Instantiate directly + storage = S3FeedStorage('s3://mybucket/export.csv', + aws_credentials['AWS_ACCESS_KEY_ID'], + aws_credentials['AWS_SECRET_ACCESS_KEY']) + self.assertEqual(storage.access_key, 'settings_key') + self.assertEqual(storage.secret_key, 'settings_secret') + # URI priority > settings priority + storage = S3FeedStorage('s3://uri_key:uri_secret@mybucket/export.csv', + aws_credentials['AWS_ACCESS_KEY_ID'], + aws_credentials['AWS_SECRET_ACCESS_KEY']) + self.assertEqual(storage.access_key, 'uri_key') + self.assertEqual(storage.secret_key, 'uri_secret') + # Backwards compatibility for initialising without settings + with warnings.catch_warnings(record=True) as w: + storage = S3FeedStorage('s3://mybucket/export.csv') + self.assertEqual(storage.access_key, 'conf_key') + self.assertEqual(storage.secret_key, 'conf_secret') + self.assertTrue('without AWS keys' in str(w[-1].message)) + @defer.inlineCallbacks def test_store(self): assert_aws_environ() uri = os.environ.get('S3_TEST_FILE_URI') if not uri: raise unittest.SkipTest("No S3 URI available for testing") - storage = S3FeedStorage(uri) + access_key = os.environ.get('AWS_ACCESS_KEY_ID') + secret_key = os.environ.get('AWS_SECRET_ACCESS_KEY') + storage = S3FeedStorage(uri, access_key, secret_key) verifyObject(IFeedStorage, storage) file = storage.open(scrapy.Spider("default")) expected_content = b"content: \xe2\x98\x83" @@ -159,6 +198,23 @@ class StdoutFeedStorageTest(unittest.TestCase): self.assertEqual(out.getvalue(), b"content") +class FromCrawlerMixin(object): + init_with_crawler = False + + @classmethod + def from_crawler(cls, crawler, *args, **kwargs): + cls.init_with_crawler = True + return cls(*args, **kwargs) + + +class FromCrawlerCsvItemExporter(CsvItemExporter, FromCrawlerMixin): + pass + + +class FromCrawlerFileFeedStorage(FileFeedStorage, FromCrawlerMixin): + pass + + class FeedExportTest(unittest.TestCase): class MyItem(scrapy.Item): @@ -598,3 +654,15 @@ class FeedExportTest(unittest.TestCase): data = yield self.exported_data(items, settings) print(row['format'], row['indent']) self.assertEqual(row['expected'], data) + + @defer.inlineCallbacks + def test_init_exporters_storages_with_crawler(self): + settings = { + 'FEED_EXPORTERS': {'csv': 'tests.test_feedexport.' + 'FromCrawlerCsvItemExporter'}, + 'FEED_STORAGES': {'file': 'tests.test_feedexport.' + 'FromCrawlerFileFeedStorage'}, + } + yield self.exported_data({}, settings) + self.assertTrue(FromCrawlerCsvItemExporter.init_with_crawler) + self.assertTrue(FromCrawlerFileFeedStorage.init_with_crawler) diff --git a/tests/test_utils_misc/__init__.py b/tests/test_utils_misc/__init__.py index 832253aa4..fcb7772ab 100644 --- a/tests/test_utils_misc/__init__.py +++ b/tests/test_utils_misc/__init__.py @@ -3,7 +3,9 @@ import os import unittest from scrapy.item import Item, Field -from scrapy.utils.misc import load_object, arg_to_iter, walk_modules +from scrapy.utils.misc import arg_to_iter, create_instance, load_object, walk_modules + +from tests import mock __doctests__ = ['scrapy.utils.misc'] @@ -74,5 +76,59 @@ class UtilsMiscTestCase(unittest.TestCase): self.assertEqual(list(arg_to_iter({'a':1})), [{'a': 1}]) self.assertEqual(list(arg_to_iter(TestItem(name="john"))), [TestItem(name="john")]) + def test_create_instance(self): + settings = mock.MagicMock() + crawler = mock.MagicMock(spec_set=['settings']) + args = (True, 100.) + kwargs = {'key': 'val'} + + def _test_with_settings(mock, settings): + create_instance(mock, settings, None, *args, **kwargs) + if hasattr(mock, 'from_crawler'): + self.assertEqual(mock.from_crawler.call_count, 0) + 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) + + def _test_with_crawler(mock, settings, crawler): + create_instance(mock, settings, crawler, *args, **kwargs) + if hasattr(mock, 'from_crawler'): + mock.from_crawler.assert_called_once_with(crawler, *args, + **kwargs) + if hasattr(mock, 'from_settings'): + self.assertEqual(mock.from_settings.call_count, 0) + self.assertEqual(mock.call_count, 0) + elif 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 four mocks: + # 1. with no alternative constructors + # 2. with from_settings() constructor + # 3. with from_crawler() constructor + # 4. with from_settings() and from_crawler() constructor + spec_sets = ([], ['from_settings'], ['from_crawler'], + ['from_settings', 'from_crawler']) + for specs in spec_sets: + m = mock.MagicMock(spec_set=specs) + _test_with_settings(m, settings) + m.reset_mock() + _test_with_crawler(m, settings, crawler) + + # Check adoption of crawler settings + m = mock.MagicMock(spec_set=['from_settings']) + create_instance(m, None, crawler, *args, **kwargs) + m.from_settings.assert_called_once_with(crawler.settings, *args, + **kwargs) + + with self.assertRaises(ValueError): + create_instance(m, None, None) + if __name__ == "__main__": unittest.main()