[DO NOT MERGE] Follow modifications done to imap-codec for Aerogramme

[superboum]

I am currently working on implementing CONDSTORE in Aerogramme. You can track my work on the dedicated merge request thread. I am opening a PR so you can see which features I needed to add so the feature works. The code is ugly and is not meant to be merged.

Relevant RFC: 4466 + RFC7162

[duesee]

Awesome! Do you want me to comment on the code already? :-)

[coveralls]

Pull Request Test Coverage Report for Build 7491122412

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.6%) to 92.77%

---

Totals
Change from base Build 7475751125:	-0.6%
Covered Lines:	9225
Relevant Lines:	9944

---

💛 - Coveralls

[superboum]

In the end, I would be interested to know what is the proper way to implement features in imap_codec for sure! But I would be more comfortable with a progressive approach. The first step, according to me, seems to make sure that I am synchronized with the branch+code you want. I have commits from you that are not in main. Should I discard them?

[duesee]

I have commits from you that are not in main. Should I discard them?

refactor!: Replace StatusV2 with Status -> already on main, discard
refactor(encoder)!: Add Fragment::AuthData -> discard (deleted from main)
feat(imap-{types,codec})!: Implement AuthenticateData::Cancel -> already on main, discard
quirk: mail.ru WIP -> already on main, discard

[superboum]

Ok so now I'm in sync with main and I have a single commit that contain only my changes: allowing SELECT/EXAMINE/STORE to have modifiers. Another FETCH modifier will come soon also ^^. Let me know if it's what you expect :-)

If everything is OK, the next step I envision to discuss with you is about your high-level guidelines to implement modifiers. Indeed, it seems that CONDSTORE introduce this novel concept of MODIFIERS in IMAP, concept that is described in a more generic way in RFC4466. So should we aim at being generic here? Or stay specific? The generic approach seems to be limited by the fact that the exact syntax of the modifiers value is not given, or at least I have not understood it. And the question following just this one: what shape has the imap-type you envision for these modifiers?

[duesee]

In the end, I would be interested to know what is the proper way to implement features in imap_codec for sure!

I don't know! 🙈 IMAP extensions are really more of "amendements"¹. It's difficult to reflect this in a library w/o breaking changes (or having a weird API).

This is our current approach:

When we add new extensions, we put them behind a feature flag starting with ext_. This feature flag has basically only the rule that everything should work when it's not used. You can use it to add variants to an enum -- even when the enum is not #[non_exhaustive]. This would normally be a breaking change but given that we document this features as experimental, we try to give us freedom here to experiment with features before we stabilize them. The expectation is that we will could have a few, say, 5 or so, ext_ features. And, the next time we do a major version, we'll "inline" the features into imap-{types,codec} (removing the feature flag.)

¹ I stole this sentence from Simon (go-imap).

[duesee]

What shape has the imap-type you envision for these modifiers?

I'm not confident in this part of the spec yet. Let me just "think aloud".

If I understand correctly, we have ...

;; recommended overarching syntax for extensions
tagged-ext          = tagged-ext-label SP tagged-ext-val

;; Is a valid RFC 3501 "atom".
tagged-ext-label    = tagged-label-fchar *tagged-label-char

tagged-label-fchar  = ALPHA / "-" / "_" / "."

tagged-label-char   = tagged-label-fchar / DIGIT / ":"


tagged-ext-val      = tagged-ext-simple / "(" [tagged-ext-comp] ")"

tagged-ext-simple   = sequence-set / number

;; Extensions that follow this general syntax should use nstring instead of astring when appropriate in the context of the extension.
;; Note that a message set or a "number" can always be represented as an "atom".
;; An URL should be represented as a "quoted" string.
tagged-ext-comp     = astring /
                      tagged-ext-comp *(SP tagged-ext-comp) /
                      "(" tagged-ext-comp ")"

... for the "generic approach", right?

And then, e.g., ...

fetch-modifiers = SP "(" fetch-modifier *(SP fetch-modifier) ")"

fetch-modifier  = fetch-modifier-name [ SP fetch-modif-params ]

fetch-modifier-name = tagged-ext-label

;; This non-terminal shows recommended syntax for future extensions.
fetch-modif-params  = tagged-ext-val

;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
fetch-modifier  = tagged-ext-label [ SP tagged-ext-val ]

... and ...

esearch-response  = "ESEARCH" [search-correlator] [SP "UID"] *(SP search-return-data)

;; Note that not every SEARCH return option is required to have the corresponding ESEARCH return data.
search-return-data = search-modifier-name SP search-return-value

search-modifier-name = tagged-ext-label

;; Data for the returned search option. A single "nz-number"/"number" value
;; can be returned as an atom (i.e., without quoting). A sequence-set can be
;; returned as an atom as well.
search-return-value = tagged-ext-val

;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
search-return-data = tagged-ext-label SP tagged-ext-val

... and ...

store-modifiers =  SP "(" store-modifier *(SP store-modifier)")"

store-modifier  = store-modifier-name [SP store-modif-params]

store-modifier-name = tagged-ext-label

;; This non-terminal shows recommended syntax for future extensions.
store-modif-params = tagged-ext-val

;;;;;;;;;;;; Simplified ;;;;;;;;;;;;
store-modifier  = tagged-ext-label [SP tagged-ext-val]

[superboum]

The weird part for me is:

Extensions that follow this general syntax should use nstring instead of astring when appropriate in the context of the extension.

Which means that some extensions do not follow the general syntax?! So I am not sure to understand the ins and the outs of implementing a generic approach that does not apply to all cases.

[duesee]

Which means that some extensions do not follow the general syntax?! So I am not sure to understand the ins and the outs of implementing a generic approach that does not apply to all cases.

Me neither. Accepting NString in case of AString would "hide" a possible Atom("nil")?

[duesee]

So, it seems safe to implement tagged-ext-label exactly as defined? Not sure about tagged-ext-val due to the comment? (Havn't looked to deep in your code yet.) Maybe this is as "generic" as it gets :D)

Do you have an example of an extension that uses NString instead?

Sorry, I should not edit my comments too fast.

[duesee]

Some issues that may come up when we implement generically... When you have number / atom, you cannot tell what 123 should be. Currently, we try to test that parsing is the inverse of serialization. But when we naively use e.g. an enum, someone could construct an Atom("123"), that will be parsed as Number(123) breaking the inverse-property. This could be fun :-/

(If these cases are not easy to fix, I think we should give up on the inverse-test for some cases. Can't fix the standard ...)

Does the formal syntax only try to give an "idea" of how these things may look like? Is there a case when we really expect either a number or an atom? Seems weird? I think you are right in not being too optimistic about the generic approach.

[superboum]

Maybe we should not go for the generic modifier implementation? And only try to support modifiers for a selection of IMAP extensions like condstore? It seems that this generic specification is not very useful: another problem I face is that, while number are generally u32 in IMAP, CONDSTORE MODSEQ are u64, so when you use modifiers like CHANGEDSINCE XXX or UNCHANGED SINCE XXX, XXX is a u64. Or to be more specific, they can be u63 or u64 depending on which RFC you read x)

[duesee]

{,UN}CHANGEDSINCE is a new thing from CONDSTORE, right? Or is there a case where support of an extension requires to update existing syntax from u32 to u64?

[superboum]

Thanks for your review, indeed I am not testing the client encoding part as I am not using it yet. Thanks also for the input on Option VS simply Vec<>, it makes sense :-) I think I will finish a first implementation on Aerogramme, test it, and then rewrite my code, and especially drop the generic approach of RFC4466 in favor of writing something specific to RFC7162. I will keep you updated here, it might take a week or two however :-s If it's too constraining due to your planning, feel free to pick what you think relevant in this PR and write your own one, I don't request any attribution, just happy to help (but if you really want to, you can add a suggested-by field in your commit message.)

[duesee]

Don't worry! On my side, there is no time pressure regarding extensions, and I'm very happy about your contributions! Unfortunately, I don't know CONDSTORE/QRESYNC (yet) and am probably not too much of a help, sorry! But I'll jump in as soon as you have something reviewable (or ask for a review :-))!