From a54d2d2756778d826aa1f219cb1b94e14080d513 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Fri, 3 Jul 2026 20:36:02 +0100 Subject: [PATCH] Cache per-user flag lookups in comment listings alterComment issued two engine.Flag calls (Blocked, then Verified) for every comment, so a listing of N comments triggered up to 2N BoltDB read transactions even when many comments shared the same author. Find, FindSince, User and Last all funnel through it. Add a userFlagCache that memoises blocked/verified results by site and user for the duration of a single listing, so repeated authors are looked up once. alterComment keeps its signature for single-comment callers (Get) by using a fresh cache; the batch paths share one. --- backend/app/store/service/service.go | 65 +++++++++++++++++--- backend/app/store/service/service_test.go | 72 +++++++++++++++++++++++ 2 files changed, 129 insertions(+), 8 deletions(-) diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index 75a9f5a6..b496fc28 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -128,6 +128,7 @@ func (s *DataStore) FindSince(locator store.Locator, sortMethod string, user sto } changedSort := false + flags := s.newUserFlagCache() // sets votes controversy for comments added prior to #274 // also sanitizes locator.URL for comments added prior to #927 for i, c := range comments { @@ -137,7 +138,7 @@ func (s *DataStore) FindSince(locator store.Locator, sortMethod string, user sto changedSort = true } } - comments[i] = s.alterComment(c, user) + comments[i] = s.alterCommentCached(c, user, flags) } // resort commits if altered @@ -1011,25 +1012,28 @@ func (s *DataStore) getScopedLocks(id string) (lock sync.Locker) { func (s *DataStore) alterComments(cc []store.Comment, user store.User) (res []store.Comment) { res = make([]store.Comment, len(cc)) + flags := s.newUserFlagCache() for i, c := range cc { - res[i] = s.alterComment(c, user) + res[i] = s.alterCommentCached(c, user, flags) } return res } func (s *DataStore) alterComment(c store.Comment, user store.User) (res store.Comment) { - blocReq := engine.FlagRequest{Flag: engine.Blocked, Locator: store.Locator{SiteID: c.Locator.SiteID}, UserID: c.User.ID} - blocked, bErr := s.Engine.Flag(blocReq) + return s.alterCommentCached(c, user, s.newUserFlagCache()) +} +// alterCommentCached is alterComment sharing a userFlagCache so that block/verified +// lookups for a user repeated across a listing hit the engine only once. +func (s *DataStore) alterCommentCached(c store.Comment, user store.User, flags *userFlagCache) (res store.Comment) { // mark user blocked - if bErr == nil && blocked { - c.User.Blocked = blocked + if flags.blocked(c.Locator.SiteID, c.User.ID) { + c.User.Blocked = true } // set verified status retroactively if !c.User.Blocked { - verifReq := engine.FlagRequest{Flag: engine.Verified, Locator: store.Locator{SiteID: c.Locator.SiteID}, UserID: c.User.ID} - c.User.Verified, _ = s.Engine.Flag(verifReq) + c.User.Verified = flags.verified(c.Locator.SiteID, c.User.ID) } // hide info from non-admins @@ -1043,6 +1047,51 @@ func (s *DataStore) alterComment(c store.Comment, user store.User) (res store.Co return c } +// userFlagCache memoises engine block/verified flag lookups by site and user within +// a single listing, avoiding two engine.Flag calls per comment for repeated users. +type userFlagCache struct { + s *DataStore + blockedM map[flagKey]bool + verifiedM map[flagKey]bool +} + +type flagKey struct { + siteID string + userID string +} + +func (s *DataStore) newUserFlagCache() *userFlagCache { + return &userFlagCache{s: s, blockedM: map[flagKey]bool{}, verifiedM: map[flagKey]bool{}} +} + +func (f *userFlagCache) blocked(siteID, userID string) bool { + key := flagKey{siteID: siteID, userID: userID} + if v, ok := f.blockedM[key]; ok { + return v + } + v, err := f.s.Engine.Flag(engine.FlagRequest{Flag: engine.Blocked, Locator: store.Locator{SiteID: siteID}, UserID: userID}) + if err != nil { + // don't cache on error so a repeated user is retried, matching the + // pre-refactor per-comment behavior; treat this comment as not blocked + return false + } + f.blockedM[key] = v + return v +} + +func (f *userFlagCache) verified(siteID, userID string) bool { + key := flagKey{siteID: siteID, userID: userID} + if v, ok := f.verifiedM[key]; ok { + return v + } + v, err := f.s.Engine.Flag(engine.FlagRequest{Flag: engine.Verified, Locator: store.Locator{SiteID: siteID}, UserID: userID}) + if err != nil { + return false // don't cache on error, retry on the next comment for this user + } + f.verifiedM[key] = v + return v +} + // prepare vote info for client view func (s *DataStore) prepVotes(c store.Comment, user store.User) store.Comment { c.Vote = 0 // default is "none" (not voted) diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index 1e4f410a..dbaf69da 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -1878,6 +1878,78 @@ func TestService_alterComment(t *testing.T) { assert.Equal(t, engine.FlagRequest{Flag: engine.Blocked, UserID: "devid"}, engineMock.FlagCalls()[0].Req) } +func TestService_alterCommentsFlagCaching(t *testing.T) { + t.Run("repeated user looked up once", func(t *testing.T) { + engineMock := engine.InterfaceMock{ + FlagFunc: func(engine.FlagRequest) (bool, error) { return false, nil }, + } + svc := DataStore{Engine: &engineMock} + + var comments []store.Comment + for i := 0; i < 5; i++ { + comments = append(comments, store.Comment{ID: fmt.Sprintf("c%d", i), + User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}) + } + svc.alterComments(comments, store.User{ID: "u1"}) + + // one Blocked + one Verified lookup for the single user, not two per comment + assert.Equal(t, 2, len(engineMock.FlagCalls()), "5 comments by one user -> 2 flag lookups") + }) + + t.Run("distinct users looked up per user", func(t *testing.T) { + engineMock := engine.InterfaceMock{ + FlagFunc: func(req engine.FlagRequest) (bool, error) { + return req.Flag == engine.Blocked && req.UserID == "blocked", nil // "blocked" user is blocked + }, + } + svc := DataStore{Engine: &engineMock} + + comments := []store.Comment{ + {ID: "c1", User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}, + {ID: "c2", User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}, + {ID: "c3", User: store.User{ID: "blocked"}, Locator: store.Locator{SiteID: "site1"}}, + {ID: "c4", User: store.User{ID: "blocked"}, Locator: store.Locator{SiteID: "site1"}}, + } + res := svc.alterComments(comments, store.User{ID: "admin", Admin: true}) + + // u1: Blocked+Verified (2); blocked user: Blocked only, Verified skipped (1) = 3 total + assert.Equal(t, 3, len(engineMock.FlagCalls()), "two distinct users -> 3 flag lookups") + assert.True(t, res[2].User.Blocked && res[3].User.Blocked, "blocked user marked blocked") + assert.False(t, res[0].User.Blocked, "u1 not blocked") + }) + + t.Run("flag read error is not cached", func(t *testing.T) { + var blockedCalls int + engineMock := engine.InterfaceMock{ + FlagFunc: func(req engine.FlagRequest) (bool, error) { + if req.Flag == engine.Blocked { + blockedCalls++ + if blockedCalls == 1 { + return false, fmt.Errorf("transient flag read error") + } + return true, nil + } + return false, nil + }, + } + svc := DataStore{Engine: &engineMock} + + comments := []store.Comment{ + {ID: "c0", User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}, + {ID: "c1", User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}, + {ID: "c2", User: store.User{ID: "u1"}, Locator: store.Locator{SiteID: "site1"}}, + } + res := svc.alterComments(comments, store.User{ID: "admin", Admin: true}) + + // the errored first lookup must not be cached, so the next comment retries and + // picks up the real blocked state; once it succeeds the result is cached + assert.False(t, res[0].User.Blocked, "errored lookup treated as not blocked") + assert.True(t, res[1].User.Blocked, "retry after error picks up blocked state") + assert.True(t, res[2].User.Blocked, "successful read is cached") + assert.Equal(t, 2, blockedCalls, "blocked retried once after the error, then cached") + }) +} + func Benchmark_ServiceCreate(b *testing.B) { dbFile := fmt.Sprintf("%s/test-remark42-%d.db", os.TempDir(), rand.Intn(9999999999)) defer func() { _ = os.Remove(dbFile) }()