diff --git a/internal/server/message/fetch.go b/internal/server/message/fetch.go index 5d11780..0697184 100644 --- a/internal/server/message/fetch.go +++ b/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)) diff --git a/internal/server/message/fetch_test.go b/internal/server/message/fetch_test.go index 8f0565b..b5df075 100644 --- a/internal/server/message/fetch_test.go +++ b/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) diff --git a/internal/server/uid/uid_test.go b/internal/server/uid/uid_test.go index 84832c7..18fa2e5 100644 --- a/internal/server/uid/uid_test.go +++ b/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) + } +}