Warning raised during deletion

[heartsucker]

A warning is raised about potentially un-deleted rows when deleting a source.

STR

1. Boot SD dev container
2. Boot client
3. Login
4. Delete source via the UI (click in menu, confirm yes in modal)

Expected

There are not any warnings issued.

Actual

/home/heartsucker/.local/share/virtualenvs/securedrop-client-GExBtLKv/lib/python3.5/site-packages/sqlalchemy/orm/persistence.py:1066: SAWarning: DELETE statement on table 'messages' expected to delete 2 row(s); 0 were matched.  Please set confirm_deleted_rows=False within the mapper configuration to prevent this warning.
  (table.description, expected, rows_matched)
/home/heartsucker/.local/share/virtualenvs/securedrop-client-GExBtLKv/lib/python3.5/site-packages/sqlalchemy/orm/persistence.py:1066: SAWarning: DELETE statement on table 'replies' expected to delete 2 row(s); 0 were matched.  Please set confirm_deleted_rows=False within the mapper configuration to prevent this warning.
  (table.description, expected, rows_matched)

Notes

The rows are correctly deleted from the database, but what is likely happening is the deletion is happening in such a way that SQLAlchemy assumes that it is correctly tracking both Python objects and their related database objects and some code violates these assumptions. SQLAlchemy thinks something went wrong and throws this warning.

[deeplow]

I did a little digging and found the bug. It has to do with double deletion of database entries.

Note: all of the code referenced refers to the file securedrop_client/storage.py

Bug explanation

The bug is in update_local_storage(). Bellow is a copy of the current version.

def update_local_storage(session: Session,
                         remote_sources: List[SDKSource],
                         remote_submissions: List[SDKSubmission],
                         remote_replies: List[SDKReply],
                         data_dir: str) -> None:

    local_sources = get_local_sources(session)
    local_files = get_local_files(session)
    local_messages = get_local_messages(session)
    local_replies = get_local_replies(session)

    remote_messages = [x for x in remote_submissions if x.filename.endswith('msg.gpg')]
    remote_files = [x for x in remote_submissions if not x.filename.endswith('msg.gpg')]

    update_sources(remote_sources, local_sources, session, data_dir)
    update_files(remote_files, local_files, session, data_dir)
    update_messages(remote_messages, local_messages, session, data_dir)
    update_replies(remote_replies, local_replies, session, data_dir)

When a source is deleted all of the files, messages and replies are deleted from the db as well since they are deleted in cascade with the source according to securedrop_client/db.py.

But because local_{sources,files,messages,replies} are all fetched before the execution of the update_* functions they will try to delete lines in the table that were already deleted, thus the warning SAWarning: DELETE statement on table ....

Solution approach

Either call the get_local_* within each corresponding update_* function like so:

def update_local_storage(session: Session,
                         remote_sources: List[SDKSource],
                         remote_submissions: List[SDKSubmission],
                         remote_replies: List[SDKReply],
                         data_dir: str) -> None:

    remote_messages = [x for x in remote_submissions if x.filename.endswith('msg.gpg')]
    remote_files = [x for x in remote_submissions if not x.filename.endswith('msg.gpg')]

    update_sources(remote_sources, get_local_sources(session), session, data_dir)
    update_files(remote_files, get_local_files(session), session, data_dir)
    update_messages(remote_messages, get_local_messages(session), session, data_dir)
    update_replies(remote_replies, get_local_replies(session), session, data_dir)

Or alternatively remove the local_* argument from each of the update_* function and call the respective get_local_* within the update_* functions from the already passed session parameter.

Comments

I'll gladly do a PR for this. Just tell me the solution approach you prefer.

[redshiftzero]

good debugging! I like the first solution (calling get_local_* at the same time we call update_*) - let's also add a comment in the code above the update_* calls briefly explaining this situation for posterity