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)
mesh/merge-gate head 920199862e18 merged into trunk 7bb588ca1bfb — pass: builds mesh-mail → novox; no bus step; every machine composes with the change as i…
mesh/repo-check head 920199862e18 merged into trunk 7bb588ca1bfb — pass: its merge-check.sh passed, each part in its toolchain: merge-check.sh (go), merge-…
mesh/delivery delivered

This commit is contained in:
jochen
2026-10-19 15:09:44 +02:00
parent 8dd2a42448
commit 920199862e
3 changed files with 65 additions and 7 deletions
+11 -4
View File
@@ -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). // unknown or empty answers "date" — the tool refuses an unknown order-by before the store is ever asked (AC16).
var orderColumns = map[string]string{ var orderColumns = map[string]string{
"date": "date", "date": "date",
"sender": "coalesce(sender->0->>'address', '')", "sender": "lower(coalesce(sender->0->>'address', ''))",
"subject": "subject", "subject": "subject",
"size": "size", "size": "size",
} }
@@ -620,9 +620,16 @@ func (s *PgStore) GroupBySender(ctx context.Context, q Query) ([]SenderGroup, er
args = append(args, v) args = append(args, v)
return fmt.Sprintf("$%d", len(args)) return fmt.Sprintf("$%d", len(args))
} }
sqlText := `select el->>'address' as address, max(el->>'name') as name, count(*) as count, max(date) as newest // Review round 1 finding 6: grouped by lower(address) (not case-sensitively, as a mixed-case repeat of the
from message, jsonb_array_elements(sender) el where ` + whereSQL + ` group by el->>'address' // same address would otherwise split into two lines); name and address are the newest mail's own
order by count desc, address asc limit ` + param(limit) // (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...) rows, err := s.pool.Query(ctx, sqlText, args...)
if err != nil { if err != nil {
return nil, err return nil, err
@@ -9,6 +9,7 @@ package main
import ( import (
"context" "context"
"sort" "sort"
"strings"
"testing" "testing"
"time" "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: "<mc1>", 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: "<mc2>", 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: "<mc3>", 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, "<mc3>", "<mc1>", "<mc2>") // 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 // 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. // answers every reply in the thread's direct answer set.
func TestSearchFiltersByInReplyTo(t *testing.T) { func TestSearchFiltersByInReplyTo(t *testing.T) {
+7 -3
View File
@@ -455,13 +455,17 @@ func (s *memStore) GroupBySender(_ context.Context, q Query) ([]SenderGroup, err
addr := strings.ToLower(p.Address) addr := strings.ToLower(p.Address)
g, ok := byAddress[addr] g, ok := byAddress[addr]
if !ok { if !ok {
g = &SenderGroup{Address: p.Address, Name: p.Name} g = &SenderGroup{}
byAddress[addr] = g byAddress[addr] = g
order = append(order, addr) order = append(order, addr)
} }
g.Count++ 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.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 { if out[i].Count != out[j].Count {
return 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 { if q.Limit > 0 && len(out) > q.Limit {
out = out[:q.Limit] out = out[:q.Limit]