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
…ptions; readSearchKeyWithAtom should return errors instead of swallowing them; tag in search response should be quoted; replaced deprecated strings.Title
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
Capabilities
Imapclientflags
Implementing changes requested