Skip to content
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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iequidoo
Copy link
Collaborator

@iequidoo iequidoo commented Aug 15, 2024

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

@iequidoo iequidoo marked this pull request as ready for review August 15, 2024 22:35
@iequidoo iequidoo requested review from Hocuri and link2xt August 15, 2024 22:35
@link2xt
Copy link
Collaborator

link2xt commented Aug 17, 2024

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
Copy link
Collaborator

link2xt commented Aug 17, 2024

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.

@iequidoo iequidoo marked this pull request as draft August 17, 2024 16:34
@iequidoo iequidoo force-pushed the iequidoo/missing-folders branch from cb5e50a to a490f75 Compare August 18, 2024 21:13
@iequidoo iequidoo changed the title feat: Clean up imap, imap_sync tables for disappeared folders (#5870) feat: Remove "jobs" from imap_markseen if folder couldn't be selected (#5870) Aug 18, 2024
@iequidoo
Copy link
Collaborator Author

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 iequidoo marked this pull request as ready for review August 18, 2024 21:42
@link2xt
Copy link
Collaborator

link2xt commented Aug 19, 2024

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.

@iequidoo iequidoo marked this pull request as draft August 19, 2024 21:19
@iequidoo iequidoo force-pushed the iequidoo/missing-folders branch from a490f75 to 0a2d52d Compare August 30, 2024 02:13
@iequidoo iequidoo changed the title feat: Remove "jobs" from imap_markseen if folder couldn't be selected (#5870) feat: Remove "jobs" from imap_markseen if folder doesn't exist (#5870) Aug 30, 2024
@iequidoo iequidoo marked this pull request as ready for review August 30, 2024 02:26
@iequidoo
Copy link
Collaborator Author

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 iequidoo force-pushed the iequidoo/missing-folders branch from 0a2d52d to 98d38b4 Compare October 6, 2024 21:53
@iequidoo iequidoo force-pushed the iequidoo/missing-folders branch from 98d38b4 to 79d1cc6 Compare December 22, 2024 20:59
Add a `create` param to `select_with_uidvalidity()` instead of always trying to create the folder
and return `Ok(false)` from it if the folder doesn't exist and shouldn't be created, and handle this
in `store_seen_flags_on_imap()` by just removing "jobs" from the `imap_markseen` table. Also don't
create the folder in other code paths where it's not necessary.
@iequidoo iequidoo force-pushed the iequidoo/missing-folders branch from 79d1cc6 to 160cf9f Compare December 22, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log: Couldn't select folder "DeltaChat"
2 participants