From 68f0b2a3eb5cc3333ea3ce1e469bd28a105e0acc Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Tue, 3 Nov 2026 15:32:35 -0500 Subject: [PATCH] feat(tickets)!: remove the deprecated top-level totalMessageCount The ticket detail response carried a second message count, deprecated since v2.3.0 in favor of ticket.totalMessageCount. It cost a COUNT query per request and disagreed with ticket.totalMessageCount on threads with hidden messages. GetTicketFirstMessages now returns only the messages. BREAKING CHANGE: GetTicketResponse drops the top-level totalMessageCount. Closes #301 --- contract/battery.go | 2 +- contract/fake_datastore_test.go | 7 ++--- .../tickets/get_by_id_happy.golden.json | 5 ++- .../tickets/get_by_ref_happy.golden.json | 3 +- datastores/datastore.go | 2 +- datastores/tickets.go | 21 ++++--------- datastores/tickets_harness_test.go | 31 ++++++++----------- openapi/openapi.yaml | 7 +---- rest/fake_tickets_test.go | 7 ++--- rest/rest_test.go | 2 +- rest/tickets.go | 14 +++------ rest/tickets_test.go | 8 ++--- types/tickets.go | 11 ++----- 13 files changed, 44 insertions(+), 76 deletions(-) diff --git a/contract/battery.go b/contract/battery.go index 9a3017b..823160d 100644 --- a/contract/battery.go +++ b/contract/battery.go @@ -193,7 +193,7 @@ func Cases() []Case { // --- Tickets: get by id / ref --------------------------------------- {Name: "tickets/get_by_id_happy", Method: "GET", Path: "/api/v1/tickets/42", Auth: AuthReadTickets, - Notes: "Ticket + firstMessages + deprecated top-level totalMessageCount duplicating ticket.totalMessageCount."}, + Notes: "Ticket + firstMessages."}, {Name: "tickets/get_by_id_not_found", Method: "GET", Path: "/api/v1/tickets/9999", Auth: AuthReadTickets, Notes: "NotFound names the numeric id."}, {Name: "tickets/get_by_id_parse_error", Method: "GET", Path: "/api/v1/tickets/abc", Auth: AuthReadTickets, diff --git a/contract/fake_datastore_test.go b/contract/fake_datastore_test.go index 2780f8d..0bb01ff 100644 --- a/contract/fake_datastore_test.go +++ b/contract/fake_datastore_test.go @@ -674,16 +674,15 @@ func (recordingDatastore) GetTicketByRef(_ context.Context, _ datastores.TicketR return nil, gorm.ErrRecordNotFound } -func (recordingDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, _ bool) ([]*types.Message, uint32, error) { +func (recordingDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, _ bool) ([]*types.Message, error) { if ticketID != 42 { - return []*types.Message{}, 0, nil + return []*types.Message{}, nil } msgs := seedMessages42() - total := uint32(len(msgs)) if n < len(msgs) { msgs = msgs[:n] } - return msgs, total, nil + return msgs, nil } func (recordingDatastore) ListTicketMessages(_ context.Context, ticketID uint32, afterCursor string, perPage uint32, _ bool) ([]*types.Message, string, bool, error) { diff --git a/contract/goldens/tickets/get_by_id_happy.golden.json b/contract/goldens/tickets/get_by_id_happy.golden.json index b30a6ae..445d54d 100644 --- a/contract/goldens/tickets/get_by_id_happy.golden.json +++ b/contract/goldens/tickets/get_by_id_happy.golden.json @@ -78,15 +78,14 @@ "ticketRef": "MF1UI9HE", "title": "Enlistment — John.Doe", "totalMessageCount": 3 - }, - "totalMessageCount": 3 + } }, "case": "tickets/get_by_id_happy", "header": { "Content-Type": "application/json" }, "method": "GET", - "notes": "Ticket + firstMessages + deprecated top-level totalMessageCount duplicating ticket.totalMessageCount.", + "notes": "Ticket + firstMessages.", "path": "/api/v1/tickets/42", "status": 200 } diff --git a/contract/goldens/tickets/get_by_ref_happy.golden.json b/contract/goldens/tickets/get_by_ref_happy.golden.json index dbaba38..0e01d67 100644 --- a/contract/goldens/tickets/get_by_ref_happy.golden.json +++ b/contract/goldens/tickets/get_by_ref_happy.golden.json @@ -78,8 +78,7 @@ "ticketRef": "MF1UI9HE", "title": "Enlistment — John.Doe", "totalMessageCount": 3 - }, - "totalMessageCount": 3 + } }, "case": "tickets/get_by_ref_happy", "header": { diff --git a/datastores/datastore.go b/datastores/datastore.go index ffec075..08cefe8 100644 --- a/datastores/datastore.go +++ b/datastores/datastore.go @@ -101,7 +101,7 @@ type Datastore interface { ListTickets(ctx context.Context, rc TicketReferenceCache, filter *ListTicketsFilter) (tickets []*types.Ticket, nextCursor string, hasMore bool, err error) GetTicket(ctx context.Context, rc TicketReferenceCache, ticketID uint32, forumBaseURL string) (*types.Ticket, error) GetTicketByRef(ctx context.Context, rc TicketReferenceCache, ref string, forumBaseURL string) (*types.Ticket, error) - GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) (msgs []*types.Message, totalCount uint32, err error) + GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) ListTicketMessages(ctx context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) (msgs []*types.Message, nextCursor string, hasMore bool, err error) ListCategories(ctx context.Context, rc TicketReferenceCache) ([]*types.Category, error) } diff --git a/datastores/tickets.go b/datastores/tickets.go index 32da89d..274bf3d 100644 --- a/datastores/tickets.go +++ b/datastores/tickets.go @@ -392,30 +392,21 @@ func (ds *Mysql) GetTicket(ctx context.Context, rc TicketReferenceCache, ticketI return generateTicketProto(&row, rc, strings.TrimRight(base, "/")), nil } -func (ds *Mysql) GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) { - var total int64 - q := ds.Db.WithContext(ctx).Model(&xenforo.TicketMessage{}).Where("ticket_id = ?", ticketID) - if !includeHidden { - q = q.Where("message_state = ?", "visible") - } - if tx := q.Count(&total); tx.Error != nil { - return nil, 0, tx.Error - } - +func (ds *Mysql) GetTicketFirstMessages(ctx context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) { var rows []xenforo.TicketMessage - q2 := ds.Db.WithContext(ctx).Where("ticket_id = ?", ticketID) + q := ds.Db.WithContext(ctx).Where("ticket_id = ?", ticketID) if !includeHidden { - q2 = q2.Where("message_state = ?", "visible") + q = q.Where("message_state = ?", "visible") } - tx := q2.Order("position ASC").Limit(n).Find(&rows) + tx := q.Order("position ASC").Limit(n).Find(&rows) if tx.Error != nil { - return nil, 0, tx.Error + return nil, tx.Error } out := make([]*types.Message, 0, len(rows)) for i := range rows { out = append(out, messageToProto(&rows[i])) } - return out, uint32(total), nil + return out, nil } func (ds *Mysql) ListTicketMessages(ctx context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error) { diff --git a/datastores/tickets_harness_test.go b/datastores/tickets_harness_test.go index 7dcf14e..b3776d4 100644 --- a/datastores/tickets_harness_test.go +++ b/datastores/tickets_harness_test.go @@ -541,19 +541,15 @@ func TestGetTicketByRef(t *testing.T) { } } -// First-messages preview: hidden replies are excluded from both the -// page and the total unless requested; the page is position-ordered -// and capped at n while the total counts the whole ticket. +// First-messages preview: hidden replies are excluded unless requested; +// the page is position-ordered and capped at n. func TestGetTicketFirstMessages(t *testing.T) { ds, _ := openTicketsHarness(t) - msgs, total, err := ds.GetTicketFirstMessages(context.Background(), 1, 10, false) + msgs, err := ds.GetTicketFirstMessages(context.Background(), 1, 10, false) if err != nil { t.Fatalf("GetTicketFirstMessages(visible): %v", err) } - if total != 2 { - t.Errorf("visible total = %d, want 2 (hidden internal note excluded)", total) - } if len(msgs) != 2 || msgs[0].Position != 0 || msgs[1].Position != 2 { t.Fatalf("visible positions = %v, want [0 2]", messagePositions(msgs)) } @@ -561,22 +557,21 @@ func TestGetTicketFirstMessages(t *testing.T) { t.Errorf("starter message = %q by %q", msgs[0].Message, msgs[0].Username) } - msgs, total, err = ds.GetTicketFirstMessages(context.Background(), 1, 10, true) + msgs, err = ds.GetTicketFirstMessages(context.Background(), 1, 10, true) if err != nil { t.Fatalf("GetTicketFirstMessages(include hidden): %v", err) } - if total != 3 || len(msgs) != 3 || msgs[1].MessageState != "hidden" { - t.Errorf("with hidden: total=%d positions=%v state[1]=%q, want 3/[0 1 2]/hidden", - total, messagePositions(msgs), msgs[1].MessageState) + if len(msgs) != 3 || msgs[1].MessageState != "hidden" { + t.Errorf("with hidden: positions=%v state[1]=%q, want [0 1 2]/hidden", + messagePositions(msgs), msgs[1].MessageState) } - // n caps the page, not the total. - msgs, total, err = ds.GetTicketFirstMessages(context.Background(), 1, 1, false) + msgs, err = ds.GetTicketFirstMessages(context.Background(), 1, 1, false) if err != nil { t.Fatalf("GetTicketFirstMessages(n=1): %v", err) } - if len(msgs) != 1 || total != 2 { - t.Errorf("n=1: got %d messages, total %d; want 1 message, total 2", len(msgs), total) + if len(msgs) != 1 { + t.Errorf("n=1: got %d messages, want 1", len(msgs)) } } @@ -677,12 +672,12 @@ func TestListTicketMessages_CursorWalksAcrossHiddenGap(t *testing.T) { func TestTicketMessages_NonexistentTicketIsEmptyNotError(t *testing.T) { ds, _ := openTicketsHarness(t) - msgs, total, err := ds.GetTicketFirstMessages(context.Background(), 9999, 10, true) + msgs, err := ds.GetTicketFirstMessages(context.Background(), 9999, 10, true) if err != nil { t.Fatalf("GetTicketFirstMessages(9999): %v", err) } - if len(msgs) != 0 || total != 0 { - t.Errorf("nonexistent ticket: got %d messages, total %d; want 0/0", len(msgs), total) + if len(msgs) != 0 { + t.Errorf("nonexistent ticket: got %d messages, want 0", len(msgs)) } listed, next, more, err := ds.ListTicketMessages(context.Background(), 9999, "", 10, true) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 55190ff..148777a 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -1915,12 +1915,7 @@ components: type: array items: $ref: '#/components/schemas/Message' - totalMessageCount: - type: integer - minimum: 0 - deprecated: true - description: 'DEPRECATED: use ticket.totalMessageCount. Removal target: v2.3.0.' - required: [ticket, firstMessages, totalMessageCount] + required: [ticket, firstMessages] additionalProperties: false ListTicketsResponse: diff --git a/rest/fake_tickets_test.go b/rest/fake_tickets_test.go index 8bc7391..763ef2b 100644 --- a/rest/fake_tickets_test.go +++ b/rest/fake_tickets_test.go @@ -314,19 +314,18 @@ func (f *fakeDatastore) GetTicketByRef(_ context.Context, rc datastores.TicketRe return nil, gorm.ErrRecordNotFound } -func (f *fakeDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) { +func (f *fakeDatastore) GetTicketFirstMessages(_ context.Context, ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) { if f.getTicketFirstMessages != nil { return f.getTicketFirstMessages(ticketID, n, includeHidden) } if ticketID != 42 { - return []*types.Message{}, 0, nil + return []*types.Message{}, nil } msgs := seedMessages42() - total := uint32(len(msgs)) if n < len(msgs) { msgs = msgs[:n] } - return msgs, total, nil + return msgs, nil } func (f *fakeDatastore) ListTicketMessages(_ context.Context, ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error) { diff --git a/rest/rest_test.go b/rest/rest_test.go index 8cac8ed..0df3fa5 100644 --- a/rest/rest_test.go +++ b/rest/rest_test.go @@ -61,7 +61,7 @@ type fakeDatastore struct { listTickets func(*datastores.ListTicketsFilter) ([]*types.Ticket, string, bool, error) getTicket func(ticketID uint32) (*types.Ticket, error) getTicketByRef func(ref string) (*types.Ticket, error) - getTicketFirstMessages func(ticketID uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) + getTicketFirstMessages func(ticketID uint32, n int, includeHidden bool) ([]*types.Message, error) listTicketMessages func(ticketID uint32, afterCursor string, perPage uint32, includeHidden bool) ([]*types.Message, string, bool, error) listCategories func() ([]*types.Category, error) diff --git a/rest/tickets.go b/rest/tickets.go index f7bd856..d2eba58 100644 --- a/rest/tickets.go +++ b/rest/tickets.go @@ -114,11 +114,8 @@ func getTicketByRef(ds datastores.Datastore, rc datastores.TicketReferenceCache) } // writeTicketResponse assembles the shared GetTicketResponse envelope (the -// by-id and by-ref bindings return the same shape): the ticket, its first -// messages, and the deprecated top-level total that mirrors -// ticket.totalMessageCount but is computed independently (visible-message -// COUNT vs replyCount+1 — they diverge on hidden messages; see -// types.GetTicketResponse). +// by-id and by-ref bindings return the same shape): the ticket and its first +// messages. func writeTicketResponse(w http.ResponseWriter, r *http.Request, ds datastores.Datastore, ticket *types.Ticket) { if ticket == nil { // A (nil, nil) datastore return is a bug — fail loudly rather than @@ -126,7 +123,7 @@ func writeTicketResponse(w http.ResponseWriter, r *http.Request, ds datastores.D writeError(w, r, codeInternal, "fetch ticket: nil ticket") return } - msgs, total, err := ds.GetTicketFirstMessages(r.Context(), ticket.TicketId, firstMessagesCount, false) + msgs, err := ds.GetTicketFirstMessages(r.Context(), ticket.TicketId, firstMessagesCount, false) if err != nil { writeError(w, r, codeInternal, "fetch ticket messages: %v", err) return @@ -135,9 +132,8 @@ func writeTicketResponse(w http.ResponseWriter, r *http.Request, ds datastores.D msgs = []*types.Message{} } writeJSON(w, r, types.GetTicketResponse{ - Ticket: ticket, - FirstMessages: msgs, - TotalMessageCount: total, + Ticket: ticket, + FirstMessages: msgs, }) } diff --git a/rest/tickets_test.go b/rest/tickets_test.go index d8dee75..5a96ed1 100644 --- a/rest/tickets_test.go +++ b/rest/tickets_test.go @@ -59,8 +59,8 @@ func TestNewStack_NilTicketWithNilErrorIsInternalJSON(t *testing.T) { // An outage on the first-messages fetch (after the ticket resolved) keeps its // own frozen message string: "fetch ticket messages: %v". func TestNewStack_GetTicketFirstMessagesOutageIsInternalJSON(t *testing.T) { - h := rest.New(&fakeDatastore{getTicketFirstMessages: func(uint32, int, bool) ([]*types.Message, uint32, error) { - return nil, 0, io.ErrUnexpectedEOF + h := rest.New(&fakeDatastore{getTicketFirstMessages: func(uint32, int, bool) ([]*types.Message, error) { + return nil, io.ErrUnexpectedEOF }}, &stubReferenceCache{}) rr := ticketsGet(t, h, "/api/v1/tickets/42") @@ -543,9 +543,9 @@ func TestNewStack_GetTicketByRefDatastoreOutageIsInternalJSON(t *testing.T) { func TestNewStack_FirstMessagesCountAndVisibilityFrozen(t *testing.T) { for _, path := range []string{"/api/v1/tickets/42", "/api/v1/tickets/ref/MF1UI9HE"} { gotN, gotHidden, called := 0, true, false - h := rest.New(&fakeDatastore{getTicketFirstMessages: func(_ uint32, n int, includeHidden bool) ([]*types.Message, uint32, error) { + h := rest.New(&fakeDatastore{getTicketFirstMessages: func(_ uint32, n int, includeHidden bool) ([]*types.Message, error) { gotN, gotHidden, called = n, includeHidden, true - return []*types.Message{}, 0, nil + return []*types.Message{}, nil }}, &stubReferenceCache{}) rr := ticketsGet(t, h, path) diff --git a/types/tickets.go b/types/tickets.go index 4e297b6..a51d627 100644 --- a/types/tickets.go +++ b/types/tickets.go @@ -68,15 +68,10 @@ type Message struct { // GetTicketResponse is the envelope shared by GET /api/v1/tickets/{ticketId} // and GET /api/v1/tickets/ref/{ticketRef}: the ticket plus its first -// messages (oldest first). The top-level TotalMessageCount mirrors the old -// surface's duplicate of ticket.totalMessageCount — deprecated there, frozen -// here — but the two are computed INDEPENDENTLY (ticket.totalMessageCount is -// replyCount+1; this one is a COUNT of visible message rows) and may diverge -// for threads with hidden messages. +// messages (oldest first). type GetTicketResponse struct { - Ticket *Ticket `json:"ticket"` - FirstMessages []*Message `json:"firstMessages"` - TotalMessageCount uint32 `json:"totalMessageCount"` + Ticket *Ticket `json:"ticket"` + FirstMessages []*Message `json:"firstMessages"` } // ListTicketsResponse is the GET /api/v1/tickets envelope: one page of