diff --git a/internal/service/revisions.go b/internal/service/revisions.go index 271b7f7..a0ae39b 100644 --- a/internal/service/revisions.go +++ b/internal/service/revisions.go @@ -346,9 +346,13 @@ func (s *Service) RevertChangeSet(ctx context.Context, actorID, changeSetID int6 return cs, conflicts, nil } -// countRevisions tallies revisions by (entity type, op), matching what the -// history list read produces so both sides of the API describe a change set the -// same way. +// countRevisions tallies revisions by (entity type, op). +// +// This deliberately reproduces in Go what ListChangeSets does in SQL, because a +// change set that has just been written has no rows to GROUP BY yet — the +// alternative is a second round trip to count what we are already holding. +// The parity is a real cross-layer contract, and TestRevertResultCarriesItsCounts +// compares the two breakdowns row for row so it can't drift silently. func countRevisions(revs []domain.Revision) []domain.ChangeCount { type key struct{ entityType, op string } seen := map[key]int{} diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index 8fd8fe9..fecd22f 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -712,6 +712,11 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) { // from "the tally wasn't loaded", and the chat panel read that ambiguity the // wrong way: a successful undo reported "nothing left to undo", which is the // worst possible thing to say about an action that just worked. +// +// It also pins the cross-layer contract that fix created. countRevisions tallies +// in Go what ListChangeSets tallies in SQL, and nothing but this test stops the +// two drifting — so it compares the FULL per-(entity, op) breakdown, not just +// the totals, which would agree even if the groupings had diverged. func TestRevertResultCarriesItsCounts(t *testing.T) { s := newTestService(t, openConfig()) owner := seedUser(t, s, "a@example.com") @@ -723,35 +728,77 @@ func TestRevertResultCarriesItsCounts(t *testing.T) { if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { t.Fatalf("fill: %v", err) } - fill := history(t, s, owner, g.ID)[0] - planted := totalOf(fill.Counts) - if planted == 0 { - t.Fatal("the fill recorded no counts") + // A second, different kind of change, so the breakdown has more than one row + // to get wrong. + if _, err := s.UpdateObject(ctx, owner, bed.ID, ObjectPatch{Name: strPtr("North Bed")}, bed.Version); err != nil { + t.Fatalf("rename: %v", err) } + sets := history(t, s, owner, g.ID) + rename, fill := sets[0], sets[1] - undo, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + undoRename, conflicts, err := s.RevertChangeSet(ctx, owner, rename.ID, domain.SourceUI) if err != nil || len(conflicts) != 0 { - t.Fatalf("revert: err=%v conflicts=%+v", err, conflicts) + t.Fatalf("revert rename: err=%v conflicts=%+v", err, conflicts) } - if undo == nil { - t.Fatal("a revert that did work returned no change set") - } - if got := totalOf(undo.Counts); got != planted { - t.Errorf("revert reported %d changes, want %d — an empty tally reads as 'nothing happened'", got, planted) + undoFill, conflicts, err := s.RevertChangeSet(ctx, owner, fill.ID, domain.SourceUI) + if err != nil || len(conflicts) != 0 { + t.Fatalf("revert fill: err=%v conflicts=%+v", err, conflicts) } - // And the list read agrees with what the revert returned, so both halves of - // the API describe the same change set the same way. - listed := history(t, s, owner, g.ID)[0] - if listed.ID != undo.ID { - t.Fatalf("newest change set is %d, want the revert %d", listed.ID, undo.ID) + for _, undo := range []*domain.ChangeSet{undoRename, undoFill} { + if undo == nil { + t.Fatal("a revert that did work returned no change set") + } + if len(undo.Counts) == 0 { + t.Fatalf("change set %d came back with no tally — an empty tally reads as 'nothing happened'", undo.ID) + } } - if totalOf(listed.Counts) != totalOf(undo.Counts) { - t.Errorf("list says %d changes, revert said %d", totalOf(listed.Counts), totalOf(undo.Counts)) + // Undoing a fill deletes every plop it created, so the tallies must match. + if got, want := countsTotal(undoFill.Counts), countsTotal(fill.Counts); got != want { + t.Errorf("undo of the fill reports %d changes, want %d", got, want) + } + + // The SQL tally and the Go tally must agree, row for row. + listed := history(t, s, owner, g.ID) + byID := map[int64][]domain.ChangeCount{} + for _, cs := range listed { + byID[cs.ID] = cs.Counts + } + for _, undo := range []*domain.ChangeSet{undoRename, undoFill} { + fromSQL, ok := byID[undo.ID] + if !ok { + t.Fatalf("revert %d is missing from the history list", undo.ID) + } + if !sameCounts(undo.Counts, fromSQL) { + t.Errorf("change set %d: revert returned %+v, the list read says %+v — countRevisions and the SQL grouping have drifted", + undo.ID, undo.Counts, fromSQL) + } } } -func totalOf(counts []domain.ChangeCount) int { +// sameCounts compares two tallies regardless of order. +func sameCounts(a, b []domain.ChangeCount) bool { + if len(a) != len(b) { + return false + } + index := func(cs []domain.ChangeCount) map[string]int { + m := map[string]int{} + for _, c := range cs { + m[c.EntityType+"/"+c.Op] = c.N + } + return m + } + ia, ib := index(a), index(b) + for k, v := range ia { + if ib[k] != v { + return false + } + } + return true +} + +// countsTotal sums a tally — the server-side twin of the client's totalChanges. +func countsTotal(counts []domain.ChangeCount) int { n := 0 for _, c := range counts { n += c.N diff --git a/internal/store/revisions.go b/internal/store/revisions.go index 669ebab..d696710 100644 --- a/internal/store/revisions.go +++ b/internal/store/revisions.go @@ -119,7 +119,9 @@ func (d *DB) ListChangeSets(ctx context.Context, gardenID int64, limit, offset i return sets, nil } - // One grouped query for the whole page rather than N per-row counts. + // One grouped query for the whole page rather than N per-row counts. The + // service's countRevisions mirrors this grouping for change sets it has just + // written; TestRevertResultCarriesItsCounts holds the two in step. countRows, err := d.sql.QueryContext(ctx, `SELECT change_set_id, entity_type, op, COUNT(*) FROM revisions