Fix FETCH response bugs: literal ordering and comma-separated sequence sets

internal/server/message/fetch.go

@@ -14,6 +14,7 @@ import (
 	"raven/internal/delivery/parser"
 	"raven/internal/models"
 	"raven/internal/server/response"
+	"raven/internal/server/utils"
 )
 
 // ===== FETCH =====
@@ -90,80 +91,24 @@ func HandleFetch(deps ServerDeps, conn net.Conn, tag string, parts []string, sta
 		items = strings.Trim(items, "()")
 	}
 
-	var rows *sql.Rows
-
-	// Support for sequence ranges (e.g., 1:2, 2:4, 1:*, *)
-	seqRange := strings.Split(sequence, ":")
-	var start, end int
-	var useRange bool
-
-	if len(seqRange) == 2 {
-		useRange = true
-		if seqRange[0] == "*" {
-			start = -1 // will handle below
-		} else {
-			start, err = strconv.Atoi(seqRange[0])
-			if err != nil || start < 1 {
-				deps.SendResponse(conn, fmt.Sprintf("%s BAD Invalid sequence number", tag))
-				return
-			}
-		}
-		if seqRange[1] == "*" {
-			// Get max count for end using new schema
-			end, _ = db.GetMessageCountPerUser(targetDB, state.SelectedMailboxID)
-		} else {
-			end, err = strconv.Atoi(seqRange[1])
-			if err != nil || end < 1 {
-				deps.SendResponse(conn, fmt.Sprintf("%s BAD Invalid sequence number", tag))
-				return
-			}
-		}
-		if start == -1 {
-			start = end
-		}
-		if end < start {
-			end = start
-		}
-		// Query message_mailbox for messages in selected mailbox using new schema
-		query := `SELECT mm.message_id, mm.uid, mm.flags
-		          FROM message_mailbox mm
-		          WHERE mm.mailbox_id = ?
-		          ORDER BY mm.uid ASC LIMIT ? OFFSET ?`
-		rows, err = targetDB.Query(query, state.SelectedMailboxID, end-start+1, start-1)
-	} else if sequence == "1:*" || sequence == "*" {
-		query := `SELECT mm.message_id, mm.uid, mm.flags
-		          FROM message_mailbox mm
-		          WHERE mm.mailbox_id = ?
-		          ORDER BY mm.uid ASC`
-		rows, err = targetDB.Query(query, state.SelectedMailboxID)
-	} else {
-		msgNum, parseErr := strconv.Atoi(sequence)
-		if parseErr != nil {
-			deps.SendResponse(conn, fmt.Sprintf("%s BAD Invalid sequence number", tag))
-			return
-		}
-		query := `SELECT mm.message_id, mm.uid, mm.flags
-		          FROM message_mailbox mm
-		          WHERE mm.mailbox_id = ?
-		          ORDER BY mm.uid ASC LIMIT 1 OFFSET ?`
-		rows, err = targetDB.Query(query, state.SelectedMailboxID, msgNum-1)
-	}
-
-	if err != nil {
-		deps.SendResponse(conn, fmt.Sprintf("%s NO Database error", tag))
+	// Support sequence sets: single numbers, ranges (1:5, 1:*, *), and comma-separated
+	// combinations of both (1,3,5 / 1:3,7,9:*)
+	seqNums := utils.ParseSequenceSetWithDB(sequence, state.SelectedMailboxID, targetDB)
+	if len(seqNums) == 0 {
+		deps.SendResponse(conn, fmt.Sprintf("%s BAD Invalid sequence number", tag))
 		return
 	}
-	defer func() { _ = rows.Close() }()
 
-	seqNum := 1
-	if useRange {
-		seqNum = start
-	}
-	for rows.Next() {
+	for _, seqNum := range seqNums {
 		var messageID int64
 		var uid int64
 		var flagsStr sql.NullString
-		if err := rows.Scan(&messageID, &uid, &flagsStr); err != nil {
+		query := `SELECT mm.message_id, mm.uid, mm.flags
+		          FROM message_mailbox mm
+		          WHERE mm.mailbox_id = ?
+		          ORDER BY mm.uid ASC LIMIT 1 OFFSET ?`
+		err := targetDB.QueryRow(query, state.SelectedMailboxID, seqNum-1).Scan(&messageID, &uid, &flagsStr)
+		if err != nil {
 			continue
 		}
 
@@ -174,7 +119,6 @@ func HandleFetch(deps ServerDeps, conn net.Conn, tag string, parts []string, sta
 
 		// Process this message
 		processFetchForMessage(deps, conn, messageID, uid, seqNum, flags, items, state)
-		seqNum++
 	}
 
 	deps.SendResponse(conn, fmt.Sprintf("%s OK FETCH completed", tag))
@@ -209,7 +153,6 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 
 	itemsUpper := strings.ToUpper(items)
 	responseParts := []string{}
-	var literalData string // Store literal data separately
 
 	if strings.Contains(itemsUpper, "UID") {
 		responseParts = append(responseParts, fmt.Sprintf("UID %d", uid))
@@ -403,16 +346,12 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 					if payload == "" {
 						responseParts = append(responseParts, fmt.Sprintf("BODY[%s] NIL", sectionSpec))
 					} else {
-						if literalData != "" {
-							literalData += " "
-						}
 						// Include partial start position in response if this was a partial fetch
+						name := fmt.Sprintf("BODY[%s]", sectionSpec)
 						if partialStartPos >= 0 {
-							responseParts = append(responseParts, fmt.Sprintf("BODY[%s]<%d>", sectionSpec, partialStartPos))
-						} else {
-							responseParts = append(responseParts, fmt.Sprintf("BODY[%s]", sectionSpec))
+							name = fmt.Sprintf("BODY[%s]<%d>", sectionSpec, partialStartPos)
 						}
-						literalData += fmt.Sprintf("{%d}\r\n%s", len(payload), payload)
+						responseParts = append(responseParts, fmt.Sprintf("%s {%d}\r\n%s", name, len(payload), payload))
 					}
 				}
 			}
@@ -497,8 +436,7 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 		headersStr += "\r\n" // Final blank line
 		// Match the exact format the client requested
 		fieldList := strings.Join(requestedHeaders, " ")
-		responseParts = append(responseParts, fmt.Sprintf("BODY[HEADER.FIELDS (%s)]", fieldList))
-		literalData = fmt.Sprintf("{%d}\r\n%s", len(headersStr), headersStr)
+		responseParts = append(responseParts, fmt.Sprintf("BODY[HEADER.FIELDS (%s)] {%d}\r\n%s", fieldList, len(headersStr), headersStr))
 	}
 
 	// Handle BODY.PEEK[TEXT] or BODY[TEXT] - message body only (can be combined with other parts)
@@ -531,11 +469,7 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 			}
 		}
 
-		if literalData != "" {
-			literalData += " "
-		}
-		responseParts = append(responseParts, "BODY[TEXT]")
-		literalData += fmt.Sprintf("{%d}\r\n%s", len(body), body)
+		responseParts = append(responseParts, fmt.Sprintf("BODY[TEXT] {%d}\r\n%s", len(body), body))
 	}
 
 	// Handle BODY.PEEK[HEADER] or BODY[HEADER] - all headers (check it's not HEADER.FIELDS)
@@ -547,11 +481,7 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 		if headerEnd != -1 {
 			headers = msg[:headerEnd+2] // include last CRLF
 		}
-		if literalData != "" {
-			literalData += " "
-		}
-		responseParts = append(responseParts, "BODY[HEADER]")
-		literalData += fmt.Sprintf("{%d}\r\n%s", len(headers), headers)
+		responseParts = append(responseParts, fmt.Sprintf("BODY[HEADER] {%d}\r\n%s", len(headers), headers))
 	}
 
 	// Handle RFC822.HEADER - return only the header portion
@@ -562,11 +492,7 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 		if headerEnd != -1 {
 			headers = msg[:headerEnd+2] // include last CRLF
 		}
-		if literalData != "" {
-			literalData += " "
-		}
-		responseParts = append(responseParts, "RFC822.HEADER")
-		literalData += fmt.Sprintf("{%d}\r\n%s", len(headers), headers)
+		responseParts = append(responseParts, fmt.Sprintf("RFC822.HEADER {%d}\r\n%s", len(headers), headers))
 	}
 
 	// Handle RFC822.TEXT - body text only (excluding headers)
@@ -577,11 +503,7 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 		if headerEnd != -1 {
 			body = msg[headerEnd+4:] // skip the double CRLF
 		}
-		if literalData != "" {
-			literalData += " "
-		}
-		responseParts = append(responseParts, "RFC822.TEXT")
-		literalData += fmt.Sprintf("{%d}\r\n%s", len(body), body)
+		responseParts = append(responseParts, fmt.Sprintf("RFC822.TEXT {%d}\r\n%s", len(body), body))
 	}
 
 	// Handle BODY[] / BODY.PEEK[] / RFC822 / RFC822.PEEK - full message
@@ -590,20 +512,11 @@ func processFetchForMessage(deps ServerDeps, conn net.Conn, messageID, uid int64
 		(strings.Contains(itemsUpper, "RFC822") && !strings.Contains(itemsUpper, "RFC822.SIZE") &&
 			!strings.Contains(itemsUpper, "RFC822.HEADER") && !strings.Contains(itemsUpper, "RFC822.TEXT") && !strings.Contains(itemsUpper, "RFC822.PEEK")) {
 		msg := loadRawMsg()
-		if literalData != "" {
-			literalData += " "
-		}
-		responseParts = append(responseParts, "BODY[]")
-		literalData += fmt.Sprintf("{%d}\r\n%s", len(msg), msg)
+		responseParts = append(responseParts, fmt.Sprintf("BODY[] {%d}\r\n%s", len(msg), msg))
 	}
 
 	if len(responseParts) > 0 {
-		responseStr := fmt.Sprintf("* %d FETCH (%s", seqNum, strings.Join(responseParts, " "))
-		if literalData != "" {
-			responseStr += " " + literalData + ")"
-		} else {
-			responseStr += ")"
-		}
+		responseStr := fmt.Sprintf("* %d FETCH (%s)", seqNum, strings.Join(responseParts, " "))
 		deps.SendResponse(conn, responseStr)
 	} else {
 		deps.SendResponse(conn, fmt.Sprintf("* %d FETCH (FLAGS ())", seqNum))

internal/server/message/fetch_test.go

@@ -393,6 +393,56 @@ func TestFetchCommand_SequenceRange(t *testing.T) {
 	}
 }
 
+// TestFetchCommand_CommaSequenceSet tests FETCH with a comma-separated sequence set
+func TestFetchCommand_CommaSequenceSet(t *testing.T) {
+	srv := server.SetupTestServerSimple(t)
+	conn := server.NewMockConn()
+	database := server.GetDatabaseFromServer(srv)
+
+	userID := server.CreateTestUser(t, database, "testuser")
+	server.InsertTestMail(t, database, "testuser", "Message 1", "sender@test.com", "testuser@localhost", "INBOX")
+	server.InsertTestMail(t, database, "testuser", "Message 2", "sender@test.com", "testuser@localhost", "INBOX")
+	server.InsertTestMail(t, database, "testuser", "Message 3", "sender@test.com", "testuser@localhost", "INBOX")
+	server.InsertTestMail(t, database, "testuser", "Message 4", "sender@test.com", "testuser@localhost", "INBOX")
+	server.InsertTestMail(t, database, "testuser", "Message 5", "sender@test.com", "testuser@localhost", "INBOX")
+
+	mailboxID, _ := server.GetMailboxID(t, database, userID, "INBOX")
+
+	state := &models.ClientState{
+		Authenticated:     true,
+		UserID:            userID,
+		Username:          "testuser",
+		SelectedMailboxID: mailboxID,
+	}
+
+	// Test comma-separated sequence set 1,3,5
+	srv.HandleFetch(conn, "F014", []string{"F014", "FETCH", "1,3,5", "FLAGS"}, state)
+
+	response := conn.GetWrittenData()
+
+	if strings.Contains(response, "BAD") {
+		t.Errorf("Expected comma-separated sequence set to be accepted, got: %s", response)
+	}
+	if !strings.Contains(response, "* 1 FETCH") {
+		t.Errorf("Expected message 1 in response, got: %s", response)
+	}
+	if !strings.Contains(response, "* 3 FETCH") {
+		t.Errorf("Expected message 3 in response, got: %s", response)
+	}
+	if !strings.Contains(response, "* 5 FETCH") {
+		t.Errorf("Expected message 5 in response, got: %s", response)
+	}
+	if strings.Contains(response, "* 2 FETCH") {
+		t.Errorf("Did not expect message 2 in response, got: %s", response)
+	}
+	if strings.Contains(response, "* 4 FETCH") {
+		t.Errorf("Did not expect message 4 in response, got: %s", response)
+	}
+	if !strings.Contains(response, "F014 OK FETCH completed") {
+		t.Errorf("Expected completion, got: %s", response)
+	}
+}
+
 // TestFetchCommand_MultipleItems tests FETCH with multiple items
 func TestFetchCommand_MultipleItems(t *testing.T) {
 	srv := server.SetupTestServerSimple(t)

internal/server/uid/uid_test.go

@@ -987,3 +987,34 @@ func TestUIDSearch_DefaultBehavior(t *testing.T) {
 		t.Errorf("Expected OK response, got: %s", response)
 	}
 }
+
+// TestUIDFetch_BodySectionOrdering tests that each BODY[section]'s literal
+// appears immediately after its own item name, not batched after all item
+// names (regression test for issue #298).
+func TestUIDFetch_BodySectionOrdering(t *testing.T) {
+	srv := server.SetupTestServerSimple(t)
+	conn := server.NewMockConn()
+	database := server.GetDatabaseFromServer(srv)
+
+	userID := server.CreateTestUser(t, database, "orderuser")
+	server.InsertTestMail(t, database, "orderuser", "Test Subject", "sender@test.com", "orderuser@localhost", "INBOX")
+	inboxID, _ := server.GetMailboxID(t, database, userID, "INBOX")
+
+	state := &models.ClientState{
+		Authenticated:     true,
+		UserID:            userID,
+		SelectedMailboxID: inboxID,
+	}
+
+	srv.HandleUID(conn, "A001", []string{"UID", "UID", "FETCH", "1", "(BODY.PEEK[1] BODY.PEEK[1.MIME])"}, state)
+	response := conn.GetWrittenData()
+
+	body := "Test message body" // InsertTestMail's fixed raw body text
+	wantOrder := fmt.Sprintf("BODY[1] {%d}\r\n%s BODY[1.MIME] {", len(body), body)
+	if !strings.Contains(response, wantOrder) {
+		t.Errorf("expected BODY[1]'s literal immediately after its own name, before BODY[1.MIME]; got: %s", response)
+	}
+	if strings.Contains(response, "BODY[1] BODY[1.MIME] {") {
+		t.Errorf("names batched before literals (old bug), got: %s", response)
+	}
+}