imapserver: Add CONDSTORE and QRESYNC extension support #690
Conversation
dejanstrbac
changed the title
|
Ref #687 which contained the previous version of this PR. |
imapserver/select.go
Outdated
| } | ||
| } | ||
|
|
||
| shouldSendModSeqStatus := c.enabled.Has(imap.CapIMAP4rev2) || c.server.options.caps().Has(imap.CapCondStore) |
There was a problem hiding this comment.
I don't think IMAP4rev2 has CONDSTORE folded in? Is there a reason to send mod seq when it's enabled?
There was a problem hiding this comment.
IMAP4rev2 requires QRESYNC which on the other hand requires CONDSTORE. I added support in capability check for this chain
imapserver/imapmemserver/mailbox.go
Outdated
| msg.modSeq = mbox.highestModSeq | ||
| mbox.highestModSeq++ |
There was a problem hiding this comment.
Shouldn't we bump mbox.highestModSeq before setting the message's mod seq?
If we bump mbox.highestModSeq after, then it's not the highest mod seq, it's the next mod seq.
| return | ||
| } | ||
|
|
||
| if options.ChangedSince > 0 && msg.modSeq <= options.ChangedSince { |
There was a problem hiding this comment.
Shouldn't this be msg.modSeq < options.ChangedSince? In other words, we want to send the message if CHANGEDSINCE equals MODSEQ.
There was a problem hiding this comment.
The RFC specifies that the message's modSeq must be strictly greater than the CHANGEDSINCE value (we have the opposite condition)
imapserver/imapmemserver/mailbox.go
Outdated
| var modifiedSeqNums []uint32 | ||
| var modifiedMsgs []*message | ||
|
|
||
| mbox.mutex.Lock() |
There was a problem hiding this comment.
Nit: we can use mbox.forEach instead of manually locking/unlocking.
There was a problem hiding this comment.
agreed, updated
imapserver/imapmemserver/mailbox.go
Outdated
| mbox.highestModSeq++ | ||
| msg.modSeq = mbox.highestModSeq |
There was a problem hiding this comment.
Could we handle the mod seq bump inside msg.store? This would make it so we don't forget bumping it if we call this function from elsewhere.
There was a problem hiding this comment.
agreed, updated
imapserver/imapmemserver/mailbox.go
Outdated
|
|
||
| for i, seqNum := range modifiedSeqNums { | ||
| msg := modifiedMsgs[i] | ||
| mbox.Mailbox.tracker.QueueMessageFlags(seqNum, msg.uid, msg.flagList(), mbox.tracker) |
There was a problem hiding this comment.
Is there a reason to move the QueueMessageFlags call outside of the mbox.forEach loop?
msg.flagList can only be called with the mailbox mutex locked. QueueMessageFlags never blocks.
imapserver/imapmemserver/mailbox.go
Outdated
| } | ||
|
|
||
| if !flags.Silent && len(modifiedMsgs) > 0 { | ||
| for i, seqNum := range modifiedSeqNums { |
There was a problem hiding this comment.
Instead of open-coding this, could we accumulate an imap.SeqSet of modified message sequence numbers in the loop above, and pass that to mbox.Fetch?
There was a problem hiding this comment.
update the whole logic with an accumulator
dejanstrbac
changed the title
|
|
||
| if expunge { | ||
| w := &ExpungeWriter{} | ||
| w := &ExpungeWriter{conn: c} |
There was a problem hiding this comment.
@emersion this is an unrelated bug, conn was missing
There was a problem hiding this comment.
No untagged EXPUNGE responses are sent.
Maybe a comment would be warranted here.
Capabilities
Imapclientflags
|
I notice that this branch produces: I believe the following to be correct: From: https://datatracker.ietf.org/doc/html/rfc7162#section-3.2.6:
|
|
There's also an example in that section of the RFC: |
|
Could we try merging CONDSTORE first, then do QRESYNC as a second step? Both at the same time are a bit much to review, especially since commits aren't split into semantic chunks. (I'd suggest opening a new draft PR based on this one for QRESYNC, then dropping the QRESYNC stuff from this PR. I can help with that if needed.) |
4 participants
Implementing changes requested