1806770 - IMAP shouldn't use UID as message key.

Status: ASSIGNED

(MailNews Core :: Database, task)

Description

[Ben Campbell]

The IMAP code uses the UID from the server as the unique message key (nsIMsgDBHdr.messageKey). This is problematic for a few reasons, but the major issue is that it makes it hard to transition to a global message index (Bug 1572000).
It'd be good to move the UID into it's own field and handle the message key the same way for all protocols.

Background:
At some point in the past, local folders used the offset into the mbox file as the primary key in the msgDB. This was changed to just use a generated ID (the mork oid, I think).
But the IMAP never made the leap.

The "part 3" patch in Bug 1572000 looks like it might be a good roadmap to where the IMAP code pushes UIDs into message keys.
It landed about 7 years ago and left this comment:

// m_new_key is set in nsImapMailFolder::ParseAdoptedHeaderLine to be
// the UID of the message, so that the key can get created as UID. That of
// course is extremely confusing, and we really need to clean that up. We
// really should not conflate the meaning of envelope position, key, and
// UID.

Comment 1

[Ben Campbell]

Attachment: imap-dont-use-uid-as-msgkey.patch

I doubt there's any real incentive to address this issue as things stand - it'd be too intrusive for too little benefit. I think it's more likely that'd it'll get sorted out as a side effect of a database and/or imap overhaul.

But as part of exploratory work on Bug 1802828, I've had a crack at it. This is the section of that work that separates out msgKey and IMAP UIDs. And as such, it provides a good roadmap for what would need to be done if we ever did actually want to address this bug.

Misc comments:

• it's not a stand-alone patch. it doesn't include the database work to add an UID field to nsIMsgDBHdr (it expects a .imapUID attribute), for example.
• It does include some of my other experimental DB work, but you can ignore all that (just ignore anything in nsMsgDatabase2 and GlobalDB).
• It's likely to be incomplete. It's at the point where the IMAP is basically functional, but there are almost certainly things I've missed. Like I said, it's a roadmap, not a complete solution.
• I say UID, but the IMAP code also supports using sequence numbers for servers without UID support (but I doubt that's ever the case in the wild these days). So anywhere you see 'UID', think 'UID or sequence number'.
• It turned out to be a lot more work than I was expecting!
• I haven't updated the javascript imap implementation, but the changes to the public functions in C++ also act as a roadmap for what would need changing in JS too.

Comment 2

[Ben Campbell]

Other things to change that I probably missed in my Comment 1 patch:

Offline ops generate fake keys to avoid msgKey clashes with the UIDs that the IMAP server will eventually generate for them (See Bug 1873282 comment 28 and 29 for some analysis). That could all be avoided if/when UIDs are not used as msgKeys. Instead, fake/pseudo/provisional/offline-only messages would just have an unset UID field, until the message shows up on the IMAP side.

I'm pretty sure nsIMsgDBHdr.messageKey could be made read-only if UIDs are not used as msgKey - I think the only reason it's writable is to support IMAP. That change ought to simplify a lot of code.

Comment 3

[Ben Campbell]

Attachment: Bug 1806770 - Remove dead code cruft nsMsgOfflineOpEnumerator. r=#thunderbird-reviewers

Comment 4

[Ben Campbell]

Attachment: Bug 1806770 - Add nsIMsgHdr.uidOnServer attr. r=mkmelin

Comment 5

[Ben Campbell]

Attachment: WIP: Bug 1806770 - WIP Don't use IMAP UID as msgKey.

Comment 6

[Ben Campbell]

Attachment: WIP: Bug 1806770 - WIP Fix IMAP client folder creation: don't rely on DBs being files.

Comment 7

[Ben Campbell]

Attachment: WIP: Bug 1806770 - WIP Implement nsMsgDatabase UID functions.

Comment 8

[Ben Campbell]

Attachment: WIP: Bug 1806770 - WIP Printf debugging for offlineimapoperations.

Comment 9

[Ben Campbell]

Attachment: WIP: Bug 1806770 - WIP Use UIDs for IMAP offline move operation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e0914ed057c7
Remove dead code cruft nsMsgOfflineOpEnumerator. r=mkmelin

Comment 11

[Vineet Deo]

Attachment: Bug 1806770 - Fix whitespace linting error in mailnews/db/msgdb/src/nsMailDatabase.cpp. r=#thunderbird-reviewers

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/167ebdc3f89d
Fix whitespace linting error in mailnews/db/msgdb/src/nsMailDatabase.cpp. r=leftmostcat DONTBUILD

Comment 13

[Ben Campbell]

Attachment: Bug 1806770 - Add IMAP UID support functions to nsIMsgDatabase. r=mkmelin

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bc0d9dde985e
Add nsIMsgHdr.uidOnServer attr. r=mkmelin
https://hg.mozilla.org/comm-central/rev/a2eb59577338
Add IMAP UID support functions to nsIMsgDatabase. r=mkmelin