Standardize database error-handling in logic.py, storage.py, downloads.py, crypto.py

[rocodes]

Description

logic.py, storage.py, downloads.py, and crypto.py all interact with the database, querying the current session for records. Currently, they are all 'aware' of sqlalchemy-specific errors, but don't implement consistent error-handling, meaning that a SqlAlchemy exception under certain circumstances (particularly a race between an operation on a File, Message, or Reply object and the deletion of that object) can crash the app (see https://github.com/freedomofpress/securedrop-client/issues/2217).

IMO:

• SQLAlchemy errors should not be exposed outside of storage.py, so that the rest of the stack can remain database-agnostic. The most basic version of this would be to handle all SqlAlchemyErrors in storage.py, wrapping in our own exception class to report the relevant information (ie, preserving the distinction between an error such as NoResultFound, which may not be an error, and a more general SqlAlchemy error, would does represent a problem)
• We can refactor some of the classes that store a reference to the session (and operate on it) in order to avoid all of the elements having to be aware of the db session, and outsource those operations to the controller
• Consolidating all the database/session operations into one place will make it easier to maintain consistent logic and to reason about database operations, and will also pave the way for performance improvements (offloading to another thread etc).

Needs a bit more detail to sketch out exactly what the implementation changes should be, but filing just to provide context for anyone looking into this more.

[cfm]

Everything labeled "database" is relevant evidence in support of this refactoring, but see especially #1440.

[rocodes]

see especially https://github.com/freedomofpress/securedrop-client/issues/1440.

Oh, hello, past me, it's so nice to see you :facepalm:

Thanks. I initially was going to close that as a dupe, but there's a lot of comment history there. I'll leave it open for now.

[rocodes]

Here's what I propose:

• #1440 includes things around transaction handling, savepoints, and better transactions, let's leave it open
• Keep this issue scoped to areas where we don't catch SQLAlchemy errors at all, and/or specifically the issue of constraining SQLAlchemy exceptions to storage.py (and db.py by extension), and leave any modifications to transactions, commits, etc out of scope of this issue.

[eloquence]

Closing all refactoring issues pertaining to the old codebase since we're focused on the Electron rewrite.