deltachat-core-rust icon indicating copy to clipboard operation
deltachat-core-rust copied to clipboard

feat: Remove "jobs" from imap_markseen if folder doesn't exist (#5870)

Open iequidoo opened this issue 1 year ago • 5 comments

Close #5870. I think a test isn't really necessary, though can add if someone thinks differently

iequidoo avatar Aug 15 '24 22:08 iequidoo

If the folder does not appear in the list, it does not necessarily mean it does not exist. Seems very dangerous to remove all data about DeltaChat folder simply because it does not appear in the list, it may still be possible to create and select it.

Dovecot explicitly has this setting: https://doc.dovecot.org/configuration_manual/namespace/#core_setting-namespace/list I am pretty sure there are servers that hide everything but INBOX yet it is possible to create INBOX.DeltaChat and select it.

link2xt avatar Aug 17 '24 15:08 link2xt

I think more robust approach to do this is not when we LIST folders, but when we actually try to use database record. E.g. if we want to store \Seen flag and fail to select the folder, it is a permanent error, so we need to remove the "job" from imap_markseen that wants to store seen flag and don't try again.

link2xt avatar Aug 17 '24 15:08 link2xt

I think more robust approach to do this is not when we LIST folders, but when we actually try to use database record. E.g. if we want to store \Seen flag and fail to select the folder, it is a permanent error, so we need to remove the "job" from imap_markseen that wants to store seen flag and don't try again.

It was the first thing i wanted to implement, but the problem is that we can't differ temporary errors from permanent ones, all we have is anyhow errors. Don't we want to add C-style error codes someday? Another problem is that this way only imap_markseen can be cleaned up, not imap/imap_sync. But if folders may be hidden by the server, apparently we have no choice, so i implemented this now.

iequidoo avatar Aug 18 '24 21:08 iequidoo

If needed self.select_with_uidvalidity can return a custom error type built with thiserror to distinguish between network errors and IMAP errors.

Actually this is not needed. select_with_uidvalidity should be split into function selecting the folder and doing UIDvalidity stuff. Then instead of calling select_with_uidvalidity markseen task should call select_folder and ignore only Err(Error::NoFolder) by matching on it like select_or_create_folder does. Then make sure to handle UIDvalidity by delegating to function factored out of select_with_uidvalidity. We don't want to create folder if it does not exist.

link2xt avatar Aug 19 '24 20:08 link2xt

I added a create param to select_with_uidvalidity(), there are only 3 places where we want to create the folder. Can split it into two separate functions, if you find that more readable.

iequidoo avatar Aug 30 '24 15:08 iequidoo