Repository: scrapy/scrapy
Pull request #5758 (2022-12-12 17:09:41)

Finish exporting for each start exporting
Figured I'd take a crack at #5537 since it's been stale for a while.

This patch moves the call to `slot.finish_exporting()` up to before the conditional that checks if `slot.item_count > 0` so that if an exception occurs when an exporter's `export_item()` method is called, `slot.finish_exporting()` will still be called (as discussed in the issue).

I've included 4 new tests to that each check that:
1. if `slot.start_exporting()` was called that there is a call to `slot.finish_exporting()` after
2. `slot.finish_exporting()` is not called before a call to `slot.start_exporting()`. I.e. they assert that `slot.finish_exporting()` isn't called twice in a row without a call to `slot.start_exporting()` in between and that `slot.finish_exporting()` isn't called before an initial call to `slot.start_exporting`.

`test_start_finish_exporting_items_exception` fails before the patch and passes after the patch. The other 3 tests pass before and after.

Fixes https://github.com/scrapy/scrapy/issues/5537


Patch:
diff --git a/scrapy/extensions/feedexport.py b/scrapy/extensions/feedexport.py
index 0aa27e417..c3382d9d8 100644
--- a/scrapy/extensions/feedexport.py
+++ b/scrapy/extensions/feedexport.py
@@ -350,11 +350,11 @@ class FeedExporter:
         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)
-        slot.finish_exporting()
         logmsg = f"{slot.format} feed ({slot.itemcount} items) in: {slot.uri}"
         d = defer.maybeDeferred(slot.storage.store, slot.file)
 


Test patch:
diff --git a/tests/test_feedexport.py b/tests/test_feedexport.py
index 97c3a74b3..feaba5dab 100644
--- a/tests/test_feedexport.py
+++ b/tests/test_feedexport.py
@@ -33,8 +33,9 @@ from zope.interface.verify import verifyObject
 
 import scrapy
 from scrapy.exceptions import NotConfigured, ScrapyDeprecationWarning
-from scrapy.exporters import CsvItemExporter
+from scrapy.exporters import CsvItemExporter, JsonItemExporter
 from scrapy.extensions.feedexport import (
+    _FeedSlot,
     BlockingFeedStorage,
     FeedExporter,
     FileFeedStorage,
@@ -680,6 +681,47 @@ class FeedExportTestBase(ABC, unittest.TestCase):
         return result
 
 
+class InstrumentedFeedSlot(_FeedSlot):
+    """Instrumented _FeedSlot subclass for keeping track of calls to
+    start_exporting and finish_exporting."""
+    def start_exporting(self):
+        self.update_listener('start')
+        super().start_exporting()
+
+    def finish_exporting(self):
+        self.update_listener('finish')
+        super().finish_exporting()
+
+    @classmethod
+    def subscribe__listener(cls, listener):
+        cls.update_listener = listener.update
+
+
+class IsExportingListener:
+    """When subscribed to InstrumentedFeedSlot, keeps track of when
+    a call to start_exporting has been made without a closing call to
+    finish_exporting and when a call to finish_exporting has been made
+    before a call to start_exporting."""
+    def __init__(self):
+        self.start_without_finish = False
+        self.finish_without_start = False
+
+    def update(self, method):
+        if method == 'start':
+            self.start_without_finish = True
+        elif method == 'finish':
+            if self.start_without_finish:
+                self.start_without_finish = False
+            else:
+                self.finish_before_start = True
+
+
+class ExceptionJsonItemExporter(JsonItemExporter):
+    """JsonItemExporter that throws an exception every time export_item is called."""
+    def export_item(self, _):
+        raise Exception('foo')
+
+
 class FeedExportTest(FeedExportTestBase):
     __test__ = True
 
@@ -890,6 +932,88 @@ class FeedExportTest(FeedExportTestBase):
             data = yield self.exported_no_data(settings)
             self.assertEqual(b'', data[fmt])
 
+    @defer.inlineCallbacks
+    def test_start_finish_exporting_items(self):
+        items = [
+            self.MyItem({'foo': 'bar1', 'egg': 'spam1'}),
+        ]
+        settings = {
+            'FEEDS': {
+                self._random_temp_filename(): {'format': 'json'},
+            },
+            'FEED_EXPORT_INDENT': None,
+        }
+
+        listener = IsExportingListener()
+        InstrumentedFeedSlot.subscribe__listener(listener)
+
+        with mock.patch('scrapy.extensions.feedexport._FeedSlot',
+                        InstrumentedFeedSlot):
+            _ = yield self.exported_data(items, settings)
+            self.assertFalse(listener.start_without_finish)
+            self.assertFalse(listener.finish_without_start)
+
+    @defer.inlineCallbacks
+    def test_start_finish_exporting_no_items(self):
+        items = []
+        settings = {
+            'FEEDS': {
+                self._random_temp_filename(): {'format': 'json'},
+            },
+            'FEED_EXPORT_INDENT': None,
+        }
+
+        listener = IsExportingListener()
+        InstrumentedFeedSlot.subscribe__listener(listener)
+
+        with mock.patch('scrapy.extensions.feedexport._FeedSlot',
+                        InstrumentedFeedSlot):
+            _ = yield self.exported_data(items, settings)
+            self.assertFalse(listener.start_without_finish)
+            self.assertFalse(listener.finish_without_start)
+
+    @defer.inlineCallbacks
+    def test_start_finish_exporting_items_exception(self):
+        items = [
+            self.MyItem({'foo': 'bar1', 'egg': 'spam1'}),
+        ]
+        settings = {
+            'FEEDS': {
+                self._random_temp_filename(): {'format': 'json'},
+            },
+            'FEED_EXPORTERS': {'json': ExceptionJsonItemExporter},
+            'FEED_EXPORT_INDENT': None,
+        }
+
+        listener = IsExportingListener()
+        InstrumentedFeedSlot.subscribe__listener(listener)
+
+        with mock.patch('scrapy.extensions.feedexport._FeedSlot',
+                        InstrumentedFeedSlot):
+            _ = yield self.exported_data(items, settings)
+            self.assertFalse(listener.start_without_finish)
+            self.assertFalse(listener.finish_without_start)
+
+    @defer.inlineCallbacks
+    def test_start_finish_exporting_no_items_exception(self):
+        items = []
+        settings = {
+            'FEEDS': {
+                self._random_temp_filename(): {'format': 'json'},
+            },
+            'FEED_EXPORTERS': {'json': ExceptionJsonItemExporter},
+            'FEED_EXPORT_INDENT': None,
+        }
+
+        listener = IsExportingListener()
+        InstrumentedFeedSlot.subscribe__listener(listener)
+
+        with mock.patch('scrapy.extensions.feedexport._FeedSlot',
+                        InstrumentedFeedSlot):
+            _ = yield self.exported_data(items, settings)
+            self.assertFalse(listener.start_without_finish)
+            self.assertFalse(listener.finish_without_start)
+
     @defer.inlineCallbacks
     def test_export_no_items_store_empty(self):
         formats = (