imapserver: add CONDSTORE support #687
Conversation
Condstore server
Condstore test
| @@ -64,6 +97,12 @@ func (c *Conn) handleSelect(tag string, dec *imapwire.Decoder, readOnly bool) er | |||
| } | |||
| } | |||
|
|
|||
| if data.HighestModSeq > 0 && (options.CondStore || c.enabled.Has(imap.CapCondStore)) { | |||
There was a problem hiding this comment.
The RFC says:
This document adds two new response codes: HIGHESTMODSEQ and
NOMODSEQ. One of these two response codes MUST be returned in an OK
untagged response for any successful SELECT/EXAMINE command issued
after a CONDSTORE enabling command.
If we don't have a HIGHESTMODSEQ, we should send NOMODSEQ.
| @@ -64,6 +97,12 @@ func (c *Conn) handleSelect(tag string, dec *imapwire.Decoder, readOnly bool) er | |||
| } | |||
| } | |||
|
|
|||
| if data.HighestModSeq > 0 && (options.CondStore || c.enabled.Has(imap.CapCondStore)) { | |||
There was a problem hiding this comment.
options.CondStore indicates whether the server must include MODSEQ in all FETCH responses: https://datatracker.ietf.org/doc/html/rfc7162#section-3.1.8
It's unrelated to the HIGHESTMODSEQ returned in the SELECT command. In other words, we should always send HIGHESTMODSEQ, even if options.CondStore isn't enabled.
| return dec.Err() | ||
| } | ||
| } else { | ||
| return dec.Err() |
There was a problem hiding this comment.
At this point, ( did not follow the SP, so this is invalid
10000
syntax. However, we didn't use the Expect family of functions, so Err will be nil. We should probably replace dec.Special('(') with:
if !dec.ExpectSpecial('(') {
return dec.Err()
}However, it would be easier to use ExpectList.
| } | ||
|
|
||
| if strings.ToUpper(param) == "UNCHANGEDSINCE" { | ||
| if !dec.ExpectSP() || !dec.ExpectModSeq(&options.UnchangedSince) || !dec.ExpectSpecial(')') || !dec.ExpectSP() { |
There was a problem hiding this comment.
Nit: could we move the last part of the condition, if !dec.ExpectSpecial(')') || !dec.ExpectSP() {, below outside of the if strings.ToUpper(param) == "UNCHANGEDSINCE" {? It would apply to other store modifiers as well.
| @@ -102,6 +102,7 @@ func newMemClientServerPair(t *testing.T) (net.Conn, io.Closer) { | |||
| Caps: imap.CapSet{ | |||
| imap.CapIMAP4rev1: {}, | |||
| imap.CapIMAP4rev2: {}, | |||
| imap.CapCondStore: {}, // Add CONDSTORE capability | |||
There was a problem hiding this comment.
Nit: can this comment be dropped? I don't think it adds any useful info which is not already obvious with the code.
| if strings.ToUpper(param) == "CHANGEDSINCE" { | ||
| if !dec.ExpectSP() || !dec.ExpectModSeq(&options.ChangedSince) || !dec.ExpectSpecial(')') { | ||
| return dec.Err() | ||
| } |
There was a problem hiding this comment.
To ease server implementations, could we set options.ModSeq = true here as well? From the RFC:
When the CHANGEDSINCE FETCH modifier is specified, it implicitly
adds the MODSEQ FETCH message data item
| } | ||
|
|
||
| var modSeq int64 | ||
| if !dec.ExpectNumber64(&modSeq) { |
There was a problem hiding this comment.
Any reason to not use ExpectModSeq here?
| if !dec.ExpectSP() { | ||
| return dec.Err() | ||
| } | ||
| var quotedName, name string |
There was a problem hiding this comment.
I don't think we need two variables for the name here? A single should be enough?
If Quoted doesn't find a quoted string, it leaves the pointer unchanged.
| return dec.Err() | ||
| } | ||
| var typeName string | ||
| if !dec.ExpectAtom(&typeName) { |
There was a problem hiding this comment.
Nit: this can be combined with ExpectSP above.
| @@ -86,6 +86,9 @@ func (c *Conn) writeStatus(data *imap.StatusData, options *imap.StatusOptions) e | |||
| if options.NumRecent { | |||
| listEnc.Item().Atom("RECENT").SP().Number(*data.NumRecent) | |||
| } | |||
| if options.HighestModSeq { | |||
| listEnc.Item().Atom("HIGHESTMODSEQ").SP().Number64(int64(data.HighestModSeq)) | |||
There was a problem hiding this comment.
We should be able to use ModSeq instead of Number64 here. This would avoid sending a negative number if a modseq is high.
closes #82