From 920199862e181ebff560f29c23bfc2985da4e845 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 15:09:44 +0200 Subject: [PATCH] mesh-mail: group-by-sender and order-by sender compare addresses case-insensitively in Postgres too, and a sender's shown name and address are its newest mail's, deterministically (issue 1230, review finding 6) --- modules/mesh-mail/cmd/mesh-mail/pgstore.go | 15 ++++-- .../mesh-mail/cmd/mesh-mail/search_test.go | 47 +++++++++++++++++++ modules/mesh-mail/cmd/mesh-mail/store.go | 10 ++-- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/modules/mesh-mail/cmd/mesh-mail/pgstore.go b/modules/mesh-mail/cmd/mesh-mail/pgstore.go index 66272106..9d0c1d6b 100644 --- a/modules/mesh-mail/cmd/mesh-mail/pgstore.go +++ b/modules/mesh-mail/cmd/mesh-mail/pgstore.go @@ -561,7 +561,7 @@ func searchWhere(q Query) (whereSQL string, args []any) { // unknown or empty answers "date" — the tool refuses an unknown order-by before the store is ever asked (AC16). var orderColumns = map[string]string{ "date": "date", - "sender": "coalesce(sender->0->>'address', '')", + "sender": "lower(coalesce(sender->0->>'address', ''))", "subject": "subject", "size": "size", } @@ -620,9 +620,16 @@ func (s *PgStore) GroupBySender(ctx context.Context, q Query) ([]SenderGroup, er args = append(args, v) return fmt.Sprintf("$%d", len(args)) } - sqlText := `select el->>'address' as address, max(el->>'name') as name, count(*) as count, max(date) as newest - from message, jsonb_array_elements(sender) el where ` + whereSQL + ` group by el->>'address' - order by count desc, address asc limit ` + param(limit) + // Review round 1 finding 6: grouped by lower(address) (not case-sensitively, as a mixed-case repeat of the + // same address would otherwise split into two lines); name and address are the newest mail's own + // (array_agg ordered by date, its first element), never max()'s lexicographically largest, which is neither. + // The outer select re-sorts by lower(address): Postgres resolves a SELECT alias used bare in order by, but + // not wrapped in a function, so the grouping is wrapped in its own subquery first. + sqlText := `select address, name, count, newest from ( + select (array_agg(el->>'address' order by date desc))[1] as address, + (array_agg(el->>'name' order by date desc))[1] as name, count(*) as count, max(date) as newest + from message, jsonb_array_elements(sender) el where ` + whereSQL + ` group by lower(el->>'address') + ) grouped order by count desc, lower(address) asc limit ` + param(limit) rows, err := s.pool.Query(ctx, sqlText, args...) if err != nil { return nil, err diff --git a/modules/mesh-mail/cmd/mesh-mail/search_test.go b/modules/mesh-mail/cmd/mesh-mail/search_test.go index f31015a4..bc8bdde8 100644 --- a/modules/mesh-mail/cmd/mesh-mail/search_test.go +++ b/modules/mesh-mail/cmd/mesh-mail/search_test.go @@ -9,6 +9,7 @@ package main import ( "context" "sort" + "strings" "testing" "time" ) @@ -529,6 +530,52 @@ func TestGroupBySenderOrdersByCountDescending(t *testing.T) { }) } +// 1230 (review round 1 finding 6): the same address, written differently across two mails, is one sender, not +// two — grouped case-insensitively, in both stores. Its displayed address and name are the newest mail's own +// (not max()'s lexicographically largest, and not whichever memStore's map iteration visited first), and +// order-by sender compares case-insensitively too. +func TestGroupBySenderAndOrderBySenderAreCaseInsensitive(t *testing.T) { + eachStore(t, func(t *testing.T, st Store) { + ctx := context.Background() + older := time.Date(2026, 10, 1, 9, 0, 0, 0, time.UTC) + newer := time.Date(2026, 10, 2, 9, 0, 0, 0, time.UTC) + for _, m := range []Message{ + {Account: "mixedcase", Folder: "INBOX", UID: 1, MessageID: "", Subject: "first", Date: older, + From: []Person{{Name: "Bob", Address: "Bob@Example.ORG"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}}, + {Account: "mixedcase", Folder: "INBOX", UID: 2, MessageID: "", Subject: "second", Date: newer, + From: []Person{{Name: "Bobby", Address: "bob@example.org"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}}, + {Account: "mixedcase", Folder: "INBOX", UID: 3, MessageID: "", Subject: "third", Date: older.Add(time.Hour), + From: []Person{{Name: "Alice", Address: "alice@example.org"}}, To: []Person{}, Flags: []string{}, Attachments: []AttachmentRef{}}, + } { + if err := st.Add(ctx, m, nil); err != nil { + t.Fatal(err) + } + } + groups, err := st.GroupBySender(ctx, Query{Accounts: []string{"mixedcase"}, Folder: "INBOX", Limit: 10}) + if err != nil { + t.Fatal(err) + } + if len(groups) != 2 { + t.Fatalf("Bob@Example.ORG and bob@example.org were not merged into one sender: %+v", groups) + } + var bob *SenderGroup + for i := range groups { + if strings.EqualFold(groups[i].Address, "bob@example.org") { + bob = &groups[i] + } + } + if bob == nil || bob.Count != 2 || bob.Address != "bob@example.org" || bob.Name != "Bobby" || !bob.Newest.Equal(newer) { + t.Fatalf("bob's merged line: %+v", bob) + } + + bySender, err := st.Search(ctx, Query{Accounts: []string{"mixedcase"}, Folder: "INBOX", OrderBy: "sender", Limit: 10}) + if err != nil { + t.Fatal(err) + } + sameOrder(t, bySender, "", "", "") // alice@... before Bob@.../bob@... (case-insensitive) + }) +} + // 1231/AC8: in-reply-to a mail I sent, combined with from (its recipient), answers the replies to it; alone, it // answers every reply in the thread's direct answer set. func TestSearchFiltersByInReplyTo(t *testing.T) { diff --git a/modules/mesh-mail/cmd/mesh-mail/store.go b/modules/mesh-mail/cmd/mesh-mail/store.go index ae44c3d4..4483dc8b 100644 --- a/modules/mesh-mail/cmd/mesh-mail/store.go +++ b/modules/mesh-mail/cmd/mesh-mail/store.go @@ -455,13 +455,17 @@ func (s *memStore) GroupBySender(_ context.Context, q Query) ([]SenderGroup, err addr := strings.ToLower(p.Address) g, ok := byAddress[addr] if !ok { - g = &SenderGroup{Address: p.Address, Name: p.Name} + g = &SenderGroup{} byAddress[addr] = g order = append(order, addr) } g.Count++ - if m.Date.After(g.Newest) { + // Review round 1 finding 6: Address and Name are the newest mail's own, never whichever message a + // Go map's (randomised) iteration order happened to visit first. + if g.Newest.IsZero() || m.Date.After(g.Newest) { g.Newest = m.Date + g.Address = p.Address + g.Name = p.Name } } } @@ -473,7 +477,7 @@ func (s *memStore) GroupBySender(_ context.Context, q Query) ([]SenderGroup, err if out[i].Count != out[j].Count { return out[i].Count > out[j].Count } - return out[i].Address < out[j].Address + return strings.ToLower(out[i].Address) < strings.ToLower(out[j].Address) }) if q.Limit > 0 && len(out) > q.Limit { out = out[:q.Limit]