Enhancement: better try to catch db errors before unlink

diff --git a/src/documents/consumer.py b/src/documents/consumer.py
index 31db723d933aad87c6aea2252f85605566ff47da..e25f438dba16a8954d2312d9b7f6a356853fbf81 100644 (file)
--- a/src/documents/consumer.py
+++ b/src/documents/consumer.py
@@ -467,6 +467,8 @@ class ConsumerPlugin(
         )
         # now that everything is done, we can start to store the document
         # in the system. This will be a transaction and reasonably fast.
+        success = False
+        result = None
         try:
             with transaction.atomic():
                 # store the document.
@@ -531,7 +533,22 @@ class ConsumerPlugin(
                 # renaming logic to acquire the lock as well.
                 # This triggers things like file renaming
                 document.save()
+                success = True
 
+        except Exception as e:
+            # save the exception for later
+            try:
+                self._fail(
+                    str(e),
+                    f"The following error occurred while storing document "
+                    f"{self.filename} after parsing: {e}",
+                    exc_info=True,
+                    exception=e,
+                )
+            except Exception as fail_exc:
+                stored_exception = fail_exc
+        finally:
+            if success:
                 # Delete the file only if it was successfully consumed
                 self.log.debug(f"Deleting file {self.working_copy}")
                 self.input_doc.original_file.unlink()
@@ -549,34 +566,33 @@ class ConsumerPlugin(
                     self.log.debug(f"Deleting file {shadow_file}")
                     Path(shadow_file).unlink()
 
-        except Exception as e:
-            self._fail(
-                str(e),
-                f"The following error occurred while storing document "
-                f"{self.filename} after parsing: {e}",
-                exc_info=True,
-                exception=e,
-            )
-        finally:
-            document_parser.cleanup()
-            tempdir.cleanup()
+                self.run_post_consume_script(document)
 
-        self.run_post_consume_script(document)
+                self.log.info(f"Document {document} consumption finished")
 
-        self.log.info(f"Document {document} consumption finished")
+                self._send_progress(
+                    100,
+                    100,
+                    ProgressStatusOptions.SUCCESS,
+                    ConsumerStatusShortMessage.FINISHED,
+                    document.id,
+                )
 
-        self._send_progress(
-            100,
-            100,
-            ProgressStatusOptions.SUCCESS,
-            ConsumerStatusShortMessage.FINISHED,
-            document.id,
-        )
+                # Return the most up to date fields
+                document.refresh_from_db()
 
-        # Return the most up to date fields
-        document.refresh_from_db()
+                result = f"Success. New document id {document.pk} created"
+            elif stored_exception:
+                raise stored_exception
+            else:
+                self._fail(
+                    ConsumerStatusShortMessage.FAILED,
+                    f"Error occurred while saving {self.filename}.",
+                )
 
-        return f"Success. New document id {document.pk} created"
+            document_parser.cleanup()
+            tempdir.cleanup()
+            return result
 
     def _parse_title_placeholders(self, title: str) -> str:
         local_added = timezone.localtime(timezone.now())

diff --git a/src/documents/tests/test_consumer.py b/src/documents/tests/test_consumer.py
index f0fdc02c7ce9976e8571c9c68723e80235905cb1..bed07fc8aa026711fca4c5b3589417c2b7943925 100644 (file)
--- a/src/documents/tests/test_consumer.py
+++ b/src/documents/tests/test_consumer.py
@@ -633,6 +633,25 @@ class TestConsumer(
         # Database empty
         self.assertEqual(Document.objects.all().count(), 0)
 
+    @mock.patch("documents.consumer.ConsumerPlugin._store")
+    @mock.patch("documents.consumer.document_consumption_finished.send")
+    @mock.patch("documents.consumer.generate_unique_filename")
+    def testSaveFailsStillCaught(self, m_filename, m_signal, m_store):
+        filename = self.get_test_file()
+        m_store.return_value = None
+        m_filename.side_effect = AttributeError("BOOM")
+
+        with self.get_consumer(filename) as consumer:
+            with self.assertRaisesMessage(
+                ConsumerError,
+                "sample.pdf: The following error occurred while storing document sample.pdf after parsing: BOOM",
+            ):
+                consumer.run()
+
+        self._assert_first_last_send_progress(last_status="FAILED")
+        self.assertIsFile(filename)
+        self.assertEqual(Document.objects.count(), 0)
+
     @override_settings(FILENAME_FORMAT="{correspondent}/{title}")
     def testFilenameHandling(self):
         with self.get_consumer(