Handle NoResultFound when querying for a file, message, or reply. Ref…

…actor FileWidget to accept a File instead of a file uuid, avoiding potentially-null database query during widget construction.

client/securedrop_client/gui/widgets.py

@@ -2240,7 +2240,7 @@ class FileWidget(QWidget):
 
     def __init__(
         self,
-        file_uuid: str,
+        file: File,
         controller: Controller,
         file_download_started: pyqtBoundSignal,
         file_ready_signal: pyqtBoundSignal,
@@ -2255,8 +2255,8 @@ def __init__(
 
         self.controller = controller
 
-        self.file = self.controller.get_file(file_uuid)
-        self.uuid = file_uuid
+        self.file = file
+        self.uuid = file.uuid
         self.index = index
         self.downloading = False
 
@@ -2494,16 +2494,20 @@ def _on_left_click(self) -> None:
         of the file distinguishes which function in the logic layer to call.
         """
         # update state
-        self.file = self.controller.get_file(self.uuid)
-
-        if self.file.is_decrypted:
-            # Open the already downloaded and decrypted file.
-            self.controller.on_file_open(self.file)
-        elif not self.downloading:
-            if self.controller.api:
-                self.start_button_animation()
-            # Download the file.
-            self.controller.on_submission_download(File, self.uuid)
+        file = self.controller.get_file(self.uuid)
+        if file is None:
+            # the record was deleted from the database, so delete the widget.
+            self.deleteLater()
+        else:
+            self.file = file
+            if self.file.is_decrypted:
+                # Open the already downloaded and decrypted file.
+                self.controller.on_file_open(self.file)
+            elif not self.downloading:
+                if self.controller.api:
+                    self.start_button_animation()
+                # Download the file.
+                self.controller.on_submission_download(File, self.uuid)
 
     def start_button_animation(self) -> None:
         """
@@ -2531,8 +2535,12 @@ def stop_button_animation(self) -> None:
         Stops the download animation and restores the button to its default state.
         """
         self.download_animation.stop()
-        self.file = self.controller.get_file(self.file.uuid)
-        self._set_file_state()
+        file = self.controller.get_file(self.file.uuid)
+        if file is None:
+            self.deleteLater()
+        else:
+            self.file = file
+            self._set_file_state()
 
 
 class ConversationScrollArea(QScrollArea):
@@ -2860,9 +2868,13 @@ def add_file(self, file: File, index: int) -> None:
         """
         Add a file from the source.
         """
+        # A refactor now passes the File object directly into the FileWidget constructor,
+        # instead of doing a database query (controller.get_file) in the constructor.
+        # If we encounter any issues with FileWidget rendering updated information, then the
+        # reference could be refreshed here before the widget is created.
         logger.debug(f"Adding file for {file.uuid}")
         conversation_item = FileWidget(
-            file.uuid,
+            file,
             self.controller,
             self.controller.file_download_started,
             self.controller.file_ready,

client/securedrop_client/logic.py

@@ -854,8 +854,12 @@ def on_message_download_success(self, uuid: str) -> None:
         Called when a message has downloaded.
         """
         self.session.commit()  # Needed to flush stale data.
-        message = storage.get_message(self.session, uuid)
-        self.message_ready.emit(message.source.uuid, message.uuid, message.content)
+        try:
+            message = storage.get_message(self.session, uuid)
+            self.message_ready.emit(message.source.uuid, message.uuid, message.content)
+        except storage.SDDatabaseError as e:
+            logger.error("Failed to find message uuid in database")
+            logger.debug(f"Message {uuid} missing from database: {e}")
 
     def on_message_download_failure(self, exception: DownloadException) -> None:
         """
@@ -870,9 +874,12 @@ def on_message_download_failure(self, exception: DownloadException) -> None:
         try:
             message = storage.get_message(self.session, exception.uuid)
             self.message_download_failed.emit(message.source.uuid, message.uuid, str(message))
-        except Exception as e:
-            logger.error("Could not emit message_download_failed")
-            logger.debug(f"Could not emit message_download_failed: {e}")
+        except storage.SDDatabaseError as e:
+            # Not necessarily an error; the message may have been deleted
+            logger.warning(
+                "Message download failure for uuid not in database (likely deleted record)."
+            )
+            logger.debug(f"Message {exception.uuid} missing from database: {e}")
 
     def download_new_replies(self) -> None:
         replies = storage.find_new_replies(self.session)
@@ -889,8 +896,14 @@ def on_reply_download_success(self, uuid: str) -> None:
         Called when a reply has downloaded.
         """
         self.session.commit()  # Needed to flush stale data.
-        reply = storage.get_reply(self.session, uuid)
-        self.reply_ready.emit(reply.source.uuid, reply.uuid, reply.content)
+        try:
+            reply = storage.get_reply(self.session, uuid)
+            self.reply_ready.emit(reply.source.uuid, reply.uuid, reply.content)
+        except storage.SDDatabaseError as e:
+            # This shouldn't happen; if it's been downloaded successfully it should
+            # be in the database.
+            logger.error("Failed to find reply uuid")
+            logger.debug(f"Reply {uuid} missing from database: {e}")
 
     def on_reply_download_failure(self, exception: DownloadException) -> None:
         """
@@ -905,9 +918,12 @@ def on_reply_download_failure(self, exception: DownloadException) -> None:
         try:
             reply = storage.get_reply(self.session, exception.uuid)
             self.reply_download_failed.emit(reply.source.uuid, reply.uuid, str(reply))
-        except Exception as e:
-            logger.error("Could not emit reply_download_failed")
-            logger.debug(f"Could not emit reply_download_failed: {e}")
+        except storage.SDDatabaseError as e:
+            # If the reply was deleted from the database, this is not an error
+            logger.warning(
+                "Reply download failure for uuid not in database (likely deleted record)."
+            )
+            logger.debug(f"Reply {exception.uuid} not found in database: {e}")
 
     def downloaded_file_exists(self, file: db.File, silence_errors: bool = False) -> bool:
         """
@@ -963,7 +979,15 @@ def on_file_download_success(self, uuid: str) -> None:
         Called when a file has downloaded.
         """
         self.session.commit()
-        file_obj = storage.get_file(self.session, uuid)
+        try:
+            file_obj = storage.get_file(self.session, uuid)
+
+        except storage.SDDatabaseError as e:
+            # This shouldn't happen, it's been downloaded.
+            logger.error("Failed to find file uuid in database")
+            logger.debug(f"File {uuid} missing from database: {e}")
+            return
+
         file_obj.download_error = None
         storage.update_file_size(uuid, self.data_dir, self.session)
         self._state.record_file_download(state.FileId(uuid))
@@ -981,8 +1005,21 @@ def on_file_download_failure(self, exception: Exception) -> None:
         else:
             if isinstance(exception, DownloadDecryptionException):
                 logger.error("Failed to decrypt %s", exception.uuid)
-                f = self.get_file(exception.uuid)
+                try:
+                    f = storage.get_file(self.session, exception.uuid)
+                except storage.SDDatabaseError as e:
+                    # This isn't necessarily an error; the file may have been deleted
+                    # at the server and has been removed from the database.
+                    # Log it, but don't panic
+                    logger.warning(
+                        "File download failure for uuid not in database (likely deleted record)."
+                    )
+                    logger.debug(f"File {exception.uuid} not found in database: {e}")
+                    return
+
                 self.file_missing.emit(f.source.uuid, f.uuid, str(f))
+            # TODO: This is not a great status message. There could be multiple downloads
+            # happening at once (which one failed?) and trying again isn't always the right call.
             self.gui.update_error_status(_("The file download failed. Please try again."))
 
     def on_delete_conversation_success(self, uuid: str) -> None:
@@ -1123,8 +1160,20 @@ def on_reply_failure(self, exception: SendReplyJobError | SendReplyJobTimeoutErr
         if isinstance(exception, SendReplyJobError):
             self.reply_failed.emit(exception.reply_uuid)
 
-    def get_file(self, file_uuid: str) -> db.File:
-        file = storage.get_file(self.session, file_uuid)
+    def get_file(self, file_uuid: str) -> db.File | None:
+        """
+        Wraps storage.py get_file and returns None if no record
+        is available. GUI caller can use this method; internally, prioritize
+        storage.get_file.
+        """
+        try:
+            file = storage.get_file(self.session, file_uuid)
+        except storage.SDDatabaseError as e:
+            # Not necessarily an error
+            logger.warning("get_file for uuid not in database")
+            logger.debug(f"File {file_uuid} not found in database: {e}")
+            return None
+
         self.session.refresh(file)
         return file
 

client/securedrop_client/storage.py

@@ -66,6 +66,12 @@
 ).match
 
 
+class SDDatabaseError(Exception):
+    """
+    Handle SQLAlchemy exceptions and return relevant information to controller.
+    """
+
+
 def get_local_sources(session: Session) -> list[Source]:
     """
     Return all source objects from the local database, newest first.
@@ -505,6 +511,9 @@ def __update_submissions(
                 f"Tried to delete submission {deleted_submission.uuid}, but "
                 "it was already deleted locally."
             )
+        except SQLAlchemyError as e:
+            logger.error("Error deleting single record")
+            logger.debug(f"{e}")
     session.commit()
 
     # Check if we left any empty directories when deleting file submissions
@@ -1056,15 +1065,36 @@ def source_exists(session: Session, source_uuid: str) -> bool:
 
 
 def get_file(session: Session, uuid: str) -> File:
-    return session.query(File).filter_by(uuid=uuid).one()
+    """
+    Get File object by uuid. Raise SDDatabaseError if no match found.
+    """
+    try:
+        return session.query(File).filter_by(uuid=uuid).one()
+    except NoResultFound as e:
+        # May or may not be an error; it's up to the caller to decide
+        raise SDDatabaseError() from e
 
 
 def get_message(session: Session, uuid: str) -> Message:
-    return session.query(Message).filter_by(uuid=uuid).one()
+    """
+    Get Message object by uuid. Raise SDDatabaseError if no match found.
+    """
+    try:
+        return session.query(Message).filter_by(uuid=uuid).one()
+    except NoResultFound as e:
+        # May or may not be an error; it's up to the caller to decide
+        raise SDDatabaseError() from e
 
 
 def get_reply(session: Session, uuid: str) -> Reply:
-    return session.query(Reply).filter_by(uuid=uuid).one()
+    """
+    Get Reply object by uuid. Raise SDDatabaseError if no match found.
+    """
+    try:
+        return session.query(Reply).filter_by(uuid=uuid).one()
+    except NoResultFound as e:
+        # May or may not be an error; it's up to the caller to decide
+        raise SDDatabaseError() from e
 
 
 def mark_all_pending_drafts_as_failed(session: Session) -> list[DraftReply]: