Handle exceptions thrown while getting message ID

[floriansnow]

Python's email library can throw both an IndexError as well as a HeaderParseError when the email has certain invalid data in the message ID. This prevents users with faulty emails from syncing. This change allows us to simply continue without a message ID as was intended in the code before anyway.

This v1.1 template stands in .github/.

This PR

Add character x [x].

• I've read the DCO.
• I've read the Coding Guidelines
• The relevant informations about the changes stands in the commit message, not here in the message of the pull request.
• Code changes follow the style of the files they change.
• Code is tested (I ran this with a local copy of the code without issues).

References

• Issue #no_space

Additional information

Closes #119.
I am wondering whether we should catch some more exceptions in similar cases and also fix things like #160 along the way.

[nickspoons]

This patch resolves #119 for me, thanks!

[DaveSophoServices]

Thank you, this patch fixed an email header for me too.

[thekix]

Hi @floriansnow ,

Thanks a lot for your patch!

I applied other patch that have conflicts with this. In this patch, msg_id is None, but in the other patch msg_id is set to "[broken message-id]"

@floriansnow Is it ok for you?

Best regards,
kix

<<<<<<< handle_exception_when_getting_msg_id
        except (HeaderParseError, IndexError):
            msg_id = None
        if not msg_id:
            msg_id = '[unknown message-id]'
=======
            if not msg_id:
                msg_id = '[unknown message-id]'
        except:
            msg_id = '[broken message-id]'
>>>>>>> master

[floriansnow]

Hi @thekix!

Thanks for letting me know!

I applied other patch that have conflicts with this. In this patch, msg_id is None, but in the other patch msg_id is set to "[broken message-id]"

In this patch here, msg_id is only temporarily None, just like in the other one. It's ultimately set to [unknown message-id]. I was trying to avoid another special case, but if you prefer setting [broken message-id] as a separate case from an unknown message ID (do we need this information?), I have no problem with it. :)

The exception handling in master seems overly broad, though. I mean catching any exception will fix the issue, but I think usually, it would be best to only catch specific exceptions which is what I did in my patch.

Feel free to close this as either patch will fix the bug; you can probably judge better if the distinction between a broken and an unknown message ID is useful and whether broad exception handling might introduce additional bugs.

[thekix]

Hi @floriansnow

Thanks a lot for your reply. Yes, I agree with you about the broad exception. About the msg_id info, with the [broken message-id] could be a good idea for debug (broken message is different than unknown and we can differentiate them).

What do you think about include this changes like:

diff --git a/offlineimap/folder/IMAP.py b/offlineimap/folder/IMAP.py
index da1ccc7..608fb5f 100644
--- a/offlineimap/folder/IMAP.py
+++ b/offlineimap/folder/IMAP.py
@@ -24,7 +24,7 @@ from offlineimap import imaputil, imaplibutil, OfflineImapError
 from offlineimap import globals
 from imaplib2 import MonthNames
 from .Base import BaseFolder
-from email.errors import NoBoundaryInMultipartDefect
+from email.errors import HeaderParseError, NoBoundaryInMultipartDefect
 
 # Globals
 CRLF = '\r\n'
@@ -662,7 +662,7 @@ class IMAPFolder(BaseFolder):
             msg_id = self.getmessageheader(msg, "message-id")
             if not msg_id:
                 msg_id = '[unknown message-id]'
-        except:
+        except (HeaderParseError, IndexError):
             msg_id = '[broken message-id]'
 
         retry_left = 2  # succeeded in APPENDING?

If you agree, because you are de author of this patch/idea, do you like tu create the new PR?

Again, thanks a lot.

Best Regards,
kix

[thekix]

Hi,

I included this change in this commit: 47f74c4

I will close this PR.

Again, thanks a lot @floriansnow
Regards,
kix