fix(GODT-2218): Fix invalid UID/Seq ID Sequence range

Ensure that sequence set intervals with request interval "N:*" in a mailbox with N-1 messages behaves correctly. Refactor some of the Snapshot to make it easier to test.
ProtonMail · Dec 20, 2022 · b04fb94 · b04fb94
1 parent 76e4e7e
commit b04fb94
Show file tree

Hide file tree

Showing 4 changed files with 292 additions and 164 deletions.
diff --git a/benchmarks/gluon_bench/imap_benchmarks/append.go b/benchmarks/gluon_bench/imap_benchmarks/append.go
@@ -39,7 +39,8 @@ func (a *Append) Setup(ctx context.Context, addr net.Addr) error {
 }
 
 func (a *Append) TearDown(ctx context.Context, addr net.Addr) error {
-	return a.cleanupWithAddr(addr)
+	//return a.cleanupWithAddr(addr)
+	return nil
 }
 
 func (a *Append) Run(ctx context.Context, addr net.Addr) error {

diff --git a/internal/state/snapshot.go b/internal/state/snapshot.go
@@ -2,8 +2,6 @@ package state
 
 import (
 	"context"
-	"fmt"
-	"strconv"
 	"strings"
 
 	"github.com/ProtonMail/gluon/imap"
@@ -171,47 +169,7 @@ func (snap *snapshot) resolveSeqInterval(seq *proto.SequenceSet) ([]SeqInterval,
 		return nil, err
 	}
 
-	res := make([]SeqInterval, 0, len(seqSet))
-
-	for _, seqRange := range seqSet {
-		switch len(seqRange) {
-		case 1:
-			seq, err := snap.resolveSeq(seqRange[0])
-			if err != nil {
-				return nil, err
-			}
-
-			res = append(res, SeqInterval{
-				begin: seq,
-				end:   seq,
-			})
-
-		case 2:
-			begin, err := snap.resolveSeq(seqRange[0])
-			if err != nil {
-				return nil, err
-			}
-
-			end, err := snap.resolveSeq(seqRange[1])
-			if err != nil {
-				return nil, err
-			}
-
-			if begin > end {
-				begin, end = end, begin
-			}
-
-			res = append(res, SeqInterval{
-				begin: begin,
-				end:   end,
-			})
-
-		default:
-			return nil, fmt.Errorf("bad sequence range")
-		}
-	}
-
-	return res, nil
+	return snap.messages.resolveSeqInterval(seqSet)
 }
 
 func (snap *snapshot) resolveUIDInterval(seq *proto.SequenceSet) ([]UIDInterval, error) {
@@ -220,104 +178,25 @@ func (snap *snapshot) resolveUIDInterval(seq *proto.SequenceSet) ([]UIDInterval,
 		return nil, err
 	}
 
-	res := make([]UIDInterval, 0, len(seqSet))
-
-	for _, uidRange := range seqSet {
-		switch len(uidRange) {
-		case 1:
-			uid, err := snap.resolveUID(uidRange[0])
-			if err != nil {
-				return nil, err
-			}
-
-			res = append(res, UIDInterval{
-				begin: uid,
-				end:   uid,
-			})
-
-		case 2:
-			begin, err := snap.resolveUID(uidRange[0])
-			if err != nil {
-				return nil, err
-			}
-
-			end, err := snap.resolveUID(uidRange[1])
-			if err != nil {
-				return nil, err
-			}
-
-			if begin > end {
-				begin, end = end, begin
-			}
-
-			res = append(res, UIDInterval{
-				begin: begin,
-				end:   end,
-			})
-
-		default:
-			return nil, fmt.Errorf("bad sequence range")
-		}
-	}
-
-	return res, nil
+	return snap.messages.resolveUIDInterval(seqSet)
 }
 
 func (snap *snapshot) getMessagesInSeqRange(seq *proto.SequenceSet) ([]snapMsgWithSeq, error) {
-	var res []snapMsgWithSeq
-
-	intervals, err := snap.resolveSeqInterval(seq)
+	seqSet, err := toSeqSet(seq)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, seqRange := range intervals {
-		if seqRange.begin == seqRange.end {
-			msg, ok := snap.messages.getWithSeqID(seqRange.begin)
-			if !ok {
-				return nil, ErrNoSuchMessage
-			}
-
-			res = append(res, msg)
-		} else {
-			if !snap.messages.existsWithSeqID(seqRange.begin) || !snap.messages.existsWithSeqID(seqRange.end) {
-				return nil, ErrNoSuchMessage
-			}
-
-			res = append(res, snap.messages.seqRange(seqRange.begin, seqRange.end)...)
-		}
-	}
-
-	return res, nil
+	return snap.messages.getMessagesInSeqRange(seqSet)
 }
 
 func (snap *snapshot) getMessagesInUIDRange(seq *proto.SequenceSet) ([]snapMsgWithSeq, error) {
-	var res []snapMsgWithSeq
-
-	// If there are no messages in the mailbox, we still resolve without error.
-	if snap.messages.len() == 0 {
-		return nil, nil
-	}
-
-	intervals, err := snap.resolveUIDInterval(seq)
+	seqSet, err := toSeqSet(seq)
 	if err != nil {
 		return nil, err
 	}
 
-	for _, uidRange := range intervals {
-		if uidRange.begin == uidRange.end {
-			msg, ok := snap.messages.getWithUID(uidRange.begin)
-			if !ok {
-				continue
-			}
-
-			res = append(res, msg)
-		} else {
-			res = append(res, snap.messages.uidRange(uidRange.begin, uidRange.end)...)
-		}
-	}
-
-	return res, nil
+	return snap.messages.getMessagesInUIDRange(seqSet)
 }
 
 func (snap *snapshot) firstMessageWithFlag(flag string) (snapMsgWithSeq, bool) {
@@ -421,39 +300,3 @@ func (snap *snapshot) updateMessageRemoteID(internalID imap.InternalMessageID, r
 
 	return nil
 }
-
-// resolveSeq converts a textual sequence number into an integer.
-// According to RFC 3501, the definition of seq-number, page 89, for message sequence numbers
-// - No sequence number is valid if mailbox is empty, not even "*"
-// - "*" is converted to the number of messages in the mailbox
-// - when used in a range, the order of the indexes in irrelevant.
-func (snap *snapshot) resolveSeq(number string) (imap.SeqID, error) {
-	if number == "*" {
-		return imap.SeqID(snap.messages.len()), nil
-	}
-
-	num, err := strconv.ParseUint(number, 10, 32)
-	if err != nil {
-		return 0, fmt.Errorf("failed to parse sequence number: %w", err)
-	}
-
-	return imap.SeqID(num), nil
-}
-
-// resolveUID converts a textual message UID into an integer.
-func (snap *snapshot) resolveUID(number string) (imap.UID, error) {
-	if snap.messages.len() == 0 {
-		return 0, ErrNoSuchMessage
-	}
-
-	if number == "*" {
-		return snap.messages.last().UID, nil
-	}
-
-	num, err := strconv.ParseUint(number, 10, 32)
-	if err != nil {
-		return 0, fmt.Errorf("failed to parse UID number: %w", err)
-	}
-
-	return imap.UID(num), nil
-}