From f146511928c2dce5637ebaac70d196841e68e4ed Mon Sep 17 00:00:00 2001
From: eikek <eike.kettner@posteo.de>
Date: Sat, 4 Mar 2023 22:01:56 +0100
Subject: [PATCH] Improve error reporting when a file cannot be stored

Fixes logging the error (the effect was not evaluated before) and also
distinguishes this case from having no files in the request.

Closes: #1976
---
 .../scala/docspell/backend/ops/OUpload.scala  | 46 ++++++++++---------
 .../restserver/conv/Conversions.scala         |  5 ++
 2 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/modules/backend/src/main/scala/docspell/backend/ops/OUpload.scala b/modules/backend/src/main/scala/docspell/backend/ops/OUpload.scala
index d4f9377c..5c0d7a2a 100644
--- a/modules/backend/src/main/scala/docspell/backend/ops/OUpload.scala
+++ b/modules/backend/src/main/scala/docspell/backend/ops/OUpload.scala
@@ -107,6 +107,11 @@ object OUpload {
     case object NoCollective extends UploadResult
 
     def noCollective: UploadResult = NoCollective
+
+    /** A file could not be stored due to error in the file backend. */
+    case class StoreFailure(cause: Throwable) extends UploadResult
+
+    def storeFailure(cause: Throwable): UploadResult = StoreFailure(cause)
   }
 
   private def right[F[_]: Functor, A](a: F[A]): EitherT[F, UploadResult, A] =
@@ -129,7 +134,7 @@ object OUpload {
           _ <- checkExistingItem(itemId, collectiveId)
           coll <- OptionT(store.transact(RCollective.findById(collectiveId)))
             .toRight(UploadResult.noCollective)
-          files <- right(data.files.traverse(saveFile(coll.id)).map(_.flatten))
+          files <- data.files.traverse(saveFile(coll.id))
           _ <- checkFileList(files)
           lang <- data.meta.language match {
             case Some(lang) => right(lang.pure[F])
@@ -204,28 +209,27 @@ object OUpload {
       /** Saves the file into the database. */
       private def saveFile(
           collectiveId: CollectiveId
-      )(file: File[F]): F[Option[ProcessItemArgs.File]] =
-        logger.info(s"Receiving file $file") *>
-          file.data
-            .through(
-              store.fileRepo.save(
-                collectiveId,
-                FileCategory.AttachmentSource,
-                MimeTypeHint(file.name, None)
-              )
-            )
-            .compile
-            .lastOrError
-            .attempt
-            .map(
-              _.fold(
-                ex => {
-                  logger.warn(ex)(s"Could not store file for processing!")
-                  None
-                },
-                id => Some(ProcessItemArgs.File(file.name, id))
+      )(file: File[F]): EitherT[F, UploadResult, ProcessItemArgs.File] =
+        for {
+          _ <- right(logger.info(s"Receiving file $file"))
+          id <- EitherT(
+            file.data
+              .through(
+                store.fileRepo.save(
+                  collectiveId,
+                  FileCategory.AttachmentSource,
+                  MimeTypeHint(file.name, None)
+                )
               )
+              .compile
+              .lastOrError
+              .attempt
+          ).leftSemiflatTap(ex =>
+            logger.warn(ex)(
+              s"Could not store file ${file.name}/${file.advertisedMime} for processing!"
             )
+          ).leftMap(UploadResult.storeFailure)
+        } yield ProcessItemArgs.File(file.name, id)
 
       private def checkExistingItem(
           itemId: Option[Ident],
diff --git a/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala b/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala
index 90a4fb30..0a14fb88 100644
--- a/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala
+++ b/modules/restserver/src/main/scala/docspell/restserver/conv/Conversions.scala
@@ -689,6 +689,11 @@ trait Conversions {
       case UploadResult.NoItem   => BasicResult(false, "The item could not be found.")
       case UploadResult.NoCollective =>
         BasicResult(false, "The collective could not be found.")
+      case UploadResult.StoreFailure(_) =>
+        BasicResult(
+          false,
+          "There were errors storing a file! See the server logs for details."
+        )
     }
 
   def basicResult(cr: PassChangeResult): BasicResult =