From d311779887d5c8a34c8062eadd3155cb0ef4dc72 Mon Sep 17 00:00:00 2001 From: kenshi kikuchi Date: Wed, 8 Mar 2023 16:24:09 +0900 Subject: [PATCH 1/6] Fix FeedExporter + Fix FeedExporter not to export empty file + Change default value of FEED_STORE_EMPTY --- scrapy/extensions/feedexport.py | 90 +++++++++++++++++------------ scrapy/settings/default_settings.py | 2 +- tests/test_feedexport.py | 31 +++++----- 3 files changed, 70 insertions(+), 53 deletions(-) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index cd26b5778..8a60bc528 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -274,8 +274,6 @@ class FTPFeedStorage(BlockingFeedStorage): class _FeedSlot: def __init__( self, - file, - exporter, storage, uri, format, @@ -283,9 +281,14 @@ class _FeedSlot: batch_id, uri_template, filter, + feed_options, + spider, + exporters, + settings, + crawler, ): - self.file = file - self.exporter = exporter + self.file = None + self.exporter = None self.storage = storage # feed params self.batch_id = batch_id @@ -294,15 +297,44 @@ class _FeedSlot: self.uri_template = uri_template self.uri = uri self.filter = filter + # exporter params + self.feed_options = feed_options + self.spider = spider + self.exporters = exporters + self.settings = settings + self.crawler = crawler # flags self.itemcount = 0 self._exporting = False + self._fileloaded = False def start_exporting(self): + if not self._fileloaded: + self.file = self.storage.open(self.spider) + if "postprocessing" in self.feed_options: + self.file = PostProcessingManager( + self.feed_options["postprocessing"], self.file, self.feed_options + ) + self.exporter = self._get_exporter( + file=self.file, + format=self.feed_options["format"], + fields_to_export=self.feed_options["fields"], + encoding=self.feed_options["encoding"], + indent=self.feed_options["indent"], + **self.feed_options["item_export_kwargs"], + ) + self._fileloaded = True + if not self._exporting: self.exporter.start_exporting() self._exporting = True + def _get_instance(self, objcls, *args, **kwargs): + return create_instance(objcls, self.settings, self.crawler, *args, **kwargs) + + def _get_exporter(self, file, format, *args, **kwargs): + return self._get_instance(self.exporters[format], file, *args, **kwargs) + def finish_exporting(self): if self._exporting: self.exporter.finish_exporting() @@ -379,15 +411,22 @@ class FeedExporter: deferred_list = [] for slot in self.slots: d = self._close_slot(slot, spider) - deferred_list.append(d) + if d: + deferred_list.append(d) return defer.DeferredList(deferred_list) if deferred_list else None def _close_slot(self, slot, spider): - slot.finish_exporting() - if not slot.itemcount and not slot.store_empty: - # We need to call slot.storage.store nonetheless to get the file - # properly closed. - return defer.maybeDeferred(slot.storage.store, slot.file) + if slot.itemcount: + # Nomal case + slot.finish_exporting() + elif slot.store_empty and slot.batch_id == 1: + # Need Store Empty + slot.start_exporting() + slot.finish_exporting() + else: + # In this case, the file is not stored, so no processing is required. + return None + logmsg = f"{slot.format} feed ({slot.itemcount} items) in: {slot.uri}" d = defer.maybeDeferred(slot.storage.store, slot.file) @@ -423,23 +462,7 @@ class FeedExporter: :param uri_template: template of uri which contains %(batch_time)s or %(batch_id)d to create new uri """ storage = self._get_storage(uri, feed_options) - file = storage.open(spider) - if "postprocessing" in feed_options: - file = PostProcessingManager( - feed_options["postprocessing"], file, feed_options - ) - - exporter = self._get_exporter( - file=file, - format=feed_options["format"], - fields_to_export=feed_options["fields"], - encoding=feed_options["encoding"], - indent=feed_options["indent"], - **feed_options["item_export_kwargs"], - ) slot = _FeedSlot( - file=file, - exporter=exporter, storage=storage, uri=uri, format=feed_options["format"], @@ -447,9 +470,12 @@ class FeedExporter: batch_id=batch_id, uri_template=uri_template, filter=self.filters[uri_template], + feed_options=feed_options, + spider=spider, + exporters=self.exporters, + settings=self.settings, + crawler=getattr(self, "crawler", None), ) - if slot.store_empty: - slot.start_exporting() return slot def item_scraped(self, item, spider): @@ -533,14 +559,6 @@ class FeedExporter: else: 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, file, format, *args, **kwargs): - return self._get_instance(self.exporters[format], file, *args, **kwargs) - def _get_storage(self, uri, feed_options): """Fork of create_instance specific to feed storage classes diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 260ec1701..ea63d35c5 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -141,7 +141,7 @@ EXTENSIONS_BASE = { FEED_TEMPDIR = None FEEDS = {} FEED_URI_PARAMS = None # a function to extend uri arguments -FEED_STORE_EMPTY = False +FEED_STORE_EMPTY = True FEED_EXPORT_ENCODING = None FEED_EXPORT_FIELDS = None FEED_STORAGES = {} diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index eafe1b334..acdc39870 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -725,10 +725,9 @@ class FeedExportTest(FeedExportTestBase): yield crawler.crawl() for file_path, feed_options in FEEDS.items(): - if not Path(file_path).exists(): - continue - - content[feed_options["format"]] = Path(file_path).read_bytes() + content[feed_options["format"]] = ( + Path(file_path).read_bytes() if Path(file_path).exists() else None + ) finally: for file_path in FEEDS.keys(): @@ -945,9 +944,10 @@ class FeedExportTest(FeedExportTestBase): "FEEDS": { self._random_temp_filename(): {"format": fmt}, }, + "FEED_STORE_EMPTY": False, } data = yield self.exported_no_data(settings) - self.assertEqual(b"", data[fmt]) + self.assertEqual(None, data[fmt]) @defer.inlineCallbacks def test_start_finish_exporting_items(self): @@ -1057,7 +1057,6 @@ class FeedExportTest(FeedExportTestBase): self._random_temp_filename(): {"format": "csv"}, }, "FEED_STORAGES": {"file": LogOnStoreFileStorage}, - "FEED_STORE_EMPTY": False, } with LogCapture() as log: @@ -1680,10 +1679,9 @@ class FeedPostProcessedExportsTest(FeedExportTestBase): yield crawler.crawl() for file_path, feed_options in FEEDS.items(): - if not Path(file_path).exists(): - continue - - content[str(file_path)] = Path(file_path).read_bytes() + content[str(file_path)] = ( + Path(file_path).read_bytes() if Path(file_path).exists() else None + ) finally: for file_path in FEEDS.keys(): @@ -2184,6 +2182,9 @@ class BatchDeliveriesTest(FeedExportTestBase): for path, feed in FEEDS.items(): dir_name = Path(path).parent + if not dir_name.exists(): + content[feed["format"]] = [] + continue for file in sorted(dir_name.iterdir()): content[feed["format"]].append(file.read_bytes()) finally: @@ -2367,10 +2368,11 @@ class BatchDeliveriesTest(FeedExportTestBase): / self._file_mark: {"format": fmt}, }, "FEED_EXPORT_BATCH_ITEM_COUNT": 1, + "FEED_STORE_EMPTY": False, } data = yield self.exported_no_data(settings) data = dict(data) - self.assertEqual(b"", data[fmt][0]) + self.assertEqual(0, len(data[fmt])) @defer.inlineCallbacks def test_export_no_items_store_empty(self): @@ -2484,9 +2486,6 @@ class BatchDeliveriesTest(FeedExportTestBase): for expected_batch, got_batch in zip(expected, data[fmt]): self.assertEqual(expected_batch, got_batch) - @pytest.mark.skipif( - sys.platform == "win32", reason="Odd behaviour on file creation/output" - ) @defer.inlineCallbacks def test_batch_path_differ(self): """ @@ -2508,7 +2507,7 @@ class BatchDeliveriesTest(FeedExportTestBase): "FEED_EXPORT_BATCH_ITEM_COUNT": 1, } data = yield self.exported_data(items, settings) - self.assertEqual(len(items), len([_ for _ in data["json"] if _])) + self.assertEqual(len(items), len(data["json"])) @defer.inlineCallbacks def test_stats_batch_file_success(self): @@ -2595,7 +2594,7 @@ class BatchDeliveriesTest(FeedExportTestBase): crawler = get_crawler(TestSpider, settings) yield crawler.crawl() - self.assertEqual(len(CustomS3FeedStorage.stubs), len(items) + 1) + self.assertEqual(len(CustomS3FeedStorage.stubs), len(items)) for stub in CustomS3FeedStorage.stubs[:-1]: stub.assert_no_pending_responses() From c8ed793257d952a425ea1e55def4e1c3b3ca8b68 Mon Sep 17 00:00:00 2001 From: kenshi kikuchi Date: Thu, 16 Mar 2023 17:16:14 +0900 Subject: [PATCH 2/6] Fix test_export_no_items_multiple_feeds --- tests/test_feedexport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py index acdc39870..8ab546efd 100644 --- a/tests/test_feedexport.py +++ b/tests/test_feedexport.py @@ -1057,13 +1057,13 @@ class FeedExportTest(FeedExportTestBase): self._random_temp_filename(): {"format": "csv"}, }, "FEED_STORAGES": {"file": LogOnStoreFileStorage}, + "FEED_STORE_EMPTY": False, } with LogCapture() as log: yield self.exported_no_data(settings) - print(log) - self.assertEqual(str(log).count("Storage.store is called"), 3) + self.assertEqual(str(log).count("Storage.store is called"), 0) @defer.inlineCallbacks def test_export_multiple_item_classes(self): From 50801c7207e6f964c312b19c9fe0bcc2c6514064 Mon Sep 17 00:00:00 2001 From: kenshi kikuchi Date: Thu, 16 Mar 2023 17:17:20 +0900 Subject: [PATCH 3/6] Fix Docs --- docs/topics/feed-exports.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/topics/feed-exports.rst b/docs/topics/feed-exports.rst index eef0bb5ca..93d68d49d 100644 --- a/docs/topics/feed-exports.rst +++ b/docs/topics/feed-exports.rst @@ -552,9 +552,10 @@ to ``.json`` or ``.xml``. FEED_STORE_EMPTY ---------------- -Default: ``False`` +Default: ``True`` Whether to export empty feeds (i.e. feeds with no items). +If False and there is no items, no new files are created and existing files are not modified. .. setting:: FEED_STORAGES From 6ab49e954f25d491df7986065d270bb0068c7c89 Mon Sep 17 00:00:00 2001 From: namelessGonbai <43787036+namelessGonbai@users.noreply.github.com> Date: Thu, 16 Mar 2023 18:03:06 +0900 Subject: [PATCH 4/6] Update docs/topics/feed-exports.rst MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Adrián Chaves --- docs/topics/feed-exports.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/topics/feed-exports.rst b/docs/topics/feed-exports.rst index 93d68d49d..2a80daa46 100644 --- a/docs/topics/feed-exports.rst +++ b/docs/topics/feed-exports.rst @@ -555,7 +555,9 @@ FEED_STORE_EMPTY Default: ``True`` Whether to export empty feeds (i.e. feeds with no items). -If False and there is no items, no new files are created and existing files are not modified. +If ``False``, and there are no items to export, no new files are created and +existing files are not modified, even if the :ref:`overwrite feed option +` is enabled. .. setting:: FEED_STORAGES From 3f92882be4b35cf476f0c7c284e4fe1ba498e873 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Tue, 13 Jun 2023 19:13:58 +0400 Subject: [PATCH 5/6] Fix a wrong merge. --- scrapy/extensions/feedexport.py | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index de3ed093c..7e93bc366 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -492,20 +492,6 @@ class FeedExporter: :param uri_template: template of uri which contains %(batch_time)s or %(batch_id)d to create new uri """ storage = self._get_storage(uri, feed_options) - file = storage.open(spider) - if "postprocessing" in feed_options: - file = PostProcessingManager( - feed_options["postprocessing"], file, feed_options - ) - - exporter = self._get_exporter( - file=file, - format=feed_options["format"], - fields_to_export=feed_options["fields"], - encoding=feed_options["encoding"], - indent=feed_options["indent"], - **feed_options["item_export_kwargs"], - ) slot = FeedSlot( storage=storage, uri=uri, From e71d6d67e56e35642fddc226e34e4d523041ad17 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 22 Jun 2023 21:10:50 +0400 Subject: [PATCH 6/6] Apply suggestions from code review --- scrapy/extensions/feedexport.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py index 7e93bc366..d088450a7 100644 --- a/scrapy/extensions/feedexport.py +++ b/scrapy/extensions/feedexport.py @@ -439,10 +439,10 @@ class FeedExporter: return slot_.file if slot.itemcount: - # Nomal case + # Normal case slot.finish_exporting() elif slot.store_empty and slot.batch_id == 1: - # Need Store Empty + # Need to store the empty file slot.start_exporting() slot.finish_exporting() else: