-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve error-handling for deleted files, messages, and replies #2231
base: main
Are you sure you want to change the base?
Conversation
…actor FileWidget to accept a File instead of a file uuid, avoiding potentially-null database query during widget construction.
""" | ||
Handle SQLAlchemy exceptions and return relevant information to controller. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@legoktm : I know we talked about not adding another Exception class, and using the existing ones. However, I feel like there's a case to be made here for a generic exception class that wraps SQLAlchemy exceptions, for 2 reasons:
1: far too many parts of the client are aware of sqlalchemy exceptions, including downloads.py, crypto.py, etc (see #2222); for errorhandling consistency and to avoid uncaught exceptions leading to app crashes, I would prefer storage.py to anticipate all sqlalchemy errors and raise something db-agnostic to its callers; 2: I don't think we have a great answer in the code already (ie it's not a DownloadException etc).
For the purposes of this PR, I don't catch all SQLAlchemy exceptions and raise this one, but I'm proposing that for #2222. I also think it will be useful in that PR to be able to distinguish between "an 'anticipatable' sqlalchemyexception in our case (ie NoResultFound when searching for one()
record would be anticipated under the sync/delete race condition) vs an unexpected SQLAlchemy exception.
I would welcome your thoughts though if you think there is a different/preferable way of doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that we're losing information here, we're turning a very specific NoResultFound
error into a generic SDDatabaseError
, which (in the future) could be any error, like db corruption or something else. And those types of different underlying errors should probably be handled differently; i.e. instead of having log messages like "likely deleted record", we should just know it absolutely is a deleted record and not something else.
IMO instead of using exceptions, I think we'd benefit from making the functions like:
def get_file(session: Session, uuid: str) -> File | None:
try:
return session.query(File).filter_by(uuid=uuid).one()
except NoResultFound as e:
return None
which is basically what you did in logic.get_file() :)
I think this addresses your concerns as the caller isn't aware of SQLAlchemy errors, it just knows there's a possibility that get_file
won't return a file. The main advantage is now that it's encoded in the type system, mypy can now flag all the places this condition isn't being checked instead of us needing to manually remember to check the exception.
8e0544e
to
e378b79
Compare
e378b79
to
66d8605
Compare
(Adding this to the 0.14.0 milestone even though it isn't strictly in the "multi-delete" category because it's a bugfix, does related to delete actions, and will be released with 0.14.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments
""" | ||
Handle SQLAlchemy exceptions and return relevant information to controller. | ||
""" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that we're losing information here, we're turning a very specific NoResultFound
error into a generic SDDatabaseError
, which (in the future) could be any error, like db corruption or something else. And those types of different underlying errors should probably be handled differently; i.e. instead of having log messages like "likely deleted record", we should just know it absolutely is a deleted record and not something else.
IMO instead of using exceptions, I think we'd benefit from making the functions like:
def get_file(session: Session, uuid: str) -> File | None:
try:
return session.query(File).filter_by(uuid=uuid).one()
except NoResultFound as e:
return None
which is basically what you did in logic.get_file() :)
I think this addresses your concerns as the caller isn't aware of SQLAlchemy errors, it just knows there's a possibility that get_file
won't return a file. The main advantage is now that it's encoded in the type system, mypy can now flag all the places this condition isn't being checked instead of us needing to manually remember to check the exception.
self.file_missing.emit(f.source.uuid, f.uuid, str(f)) | ||
# TODO: This is not a great status message. There could be multiple downloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also file a bug for this :)
|
||
except storage.SDDatabaseError as e: | ||
# This shouldn't happen, it's been downloaded. | ||
logger.error("Failed to find file uuid in database") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's have this explain your comment? e.g. "Failed to find file that was just downloaded in the database."
@@ -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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly a general note, I don't think describing the history is necessary, we will find that context in the git history. Just the second-half of the comment is needed.
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow what this is for, there's already a clause for NoResultFound?
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was it intentional to lose this generic exception handling?
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here, encode the comment in the log message, e.g. "Failed to find reply that was just downloaded"
Status
Ready for review
Description
Fixes #2217 by ensuring that database queries expecting exactly one File, Message, or Reply are error-handled. Note: This is a deliberately limited-scope fix; a broader discussion about database error-handling improvements will happen in #2222
Test Plan
Checklist
If these changes modify code paths involving cryptography, the opening of files in VMs or network (via the RPC service) traffic, Qubes testing in the staging environment is required. For fine tuning of the graphical user interface, testing in any environment in Qubes is required. Please check as applicable:
If these changes add or remove files other than client code, the AppArmor profile may need to be updated. Please check as applicable:
If these changes modify the database schema, you should include a database migration. Please check as applicable:
main
and confirmed that the migration is self-contained and applies cleanlymain
and would like the reviewer to do so