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.
2026-07-11 02:29:17 -05:00
parent e575066ea9
commit a54d2d2756
2 changed files with 129 additions and 8 deletions
@@ -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)
@@ -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) }()