-
-
Notifications
You must be signed in to change notification settings - Fork 15
feat: add quirk to dedup and sort sequence sets during encoding #691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
db3f422 to
bb9e0f5
Compare
bb9e0f5 to
d69ebe4
Compare
|
Looks like the feature breaks things, I will need more time to adjust. I would like to know first if you agree with the proposition. |
|
This is going in the right direction! I think that our structured fuzzer will break due to this, but I propose we first don't care about that here. We should maybe add an issue to also account for this quirk during fuzzing but I can take care of that later. Thanks to your unit tests, I think that we can make the best of this quirk by making it a nice generic feature. Curious what you think. By the way, I'll be on vacation for a fee weeks. But I'll try to have a look here now and then. |
4351bcb to
71f5ba0
Compare
|
I pushed a new version in another commit so you can see the difference. I will squash before merging. I moved the logic into
|
|
I also added more cases to the normalization, for exemple range a:a to single a, single asterisk to range 1:asterisk (because not all IMAP server like the |
| match self { | ||
| Sequence::Single(SeqOrUid::Asterisk) => { | ||
| let begin = NonZeroU32::new(1).unwrap(); | ||
| let range = Sequence::Range(begin.into(), SeqOrUid::Asterisk); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is not right. * doesn't mean "all messages". * resolves to "the largest uid/seq" (see below). If we have, say 1337 messages, fetch * should only fetch the single message with seq 1337. This code would change it to 1:* which means 1:1337 and would fetch 1337 messages.
Reference:
seq-number = nz-number / "*"
; message sequence number (COPY, FETCH, STORE
; commands) or unique identifier (UID COPY,
; UID FETCH, UID STORE commands).
; * represents the largest number in use. In
; the case of message sequence numbers, it is
; the number of messages in a non-empty mailbox.
; In the case of unique identifiers, it is the
; unique identifier of the last message in the
; mailbox or, if the mailbox is empty, the
; mailbox's current UIDNEXT value.
; The server should respond with a tagged BAD
; response to a command that uses a message
; sequence number greater than the number of
; messages in the selected mailbox. This
; includes "*" if the selected mailbox is empty.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been so long I did not put my nose in the IMAP RFC that I mistook the meaning of *. I was convinced it had the same meaning as a range bound ..2 = *:2. Sorry for that. It makes things easier tho, let me fix that.
| let tests = vec![ | ||
| ("*", "1:*"), | ||
| ("1:*", "1:*"), | ||
| ("*:1", "*:1"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm... since we don't know what * would resolve to, it's unclear if it should be *:x or x:*. Havn't thought about this case to be honest...
| #[test] | ||
| fn normalize_sequence_set() { | ||
| let tests = [ | ||
| ("1,2,3,4", "1,2,3,4"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably, this could be 1:4? But: I think currently it is not too important to have a precise (and "stable") normalization. When these changes help you already, let's get them in! But we could open an issue to improve the normalization. Maybe I'll come back to it at some point. Sounds like a fun algorithm to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely, and I have an idea on how implement it. Let me give it a shot in another commit.
Closes #689.