mirrors/remark42
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix comment-tree pagination under-filling exact-fit subtrees

Browse Source 
The limit() boundary used >=, so a subtree that fit the page exactly was
treated as overflow and dropped — under-filling the final page (e.g.
limit=5 over subtrees 3,2 returned only the first subtree, 3 comments,
instead of both). Change it to > so an exact-fit subtree is included;
the first node is still always returned in full and larger subtrees
still overflow to the next page.

Adds table tests for MakeTree's limit/offset pagination and countReplies,
and updates the /find consistent-count expectations for the corrected
boundary.
This commit is contained in:
Dmitry Verkhoturov
2026-07-11 01:48:45 -05:00
committed by Umputun
parent 07f6b9a0a0
commit 8f61ec691b
3 changed files with 101 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/app/rest/api/rest_public_test.go
+4 -4
View File
@@ -733,16 +733,16 @@ func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) {
{"format=tree&limit=bad", `{"code":1,"details":"bad limit value","error":"strconv.Atoi: parsing \"bad\": invalid syntax"}`},
{"format=tree&offset_id=bad", `{"code":1,"details":"bad offset_id value","error":"invalid UUID length: 3"}`},
{"format=tree&limit=2", `"info":{"count":7,"count_left":4,"last_comment":"` + ids[0]},
{"format=tree&limit=6", `"info":{"count":7,"count_left":2,"last_comment":"` + ids[1]},
{"format=tree&limit=7", `"info":{"count":7,"count_left":1,"last_comment":"` + ids[6]},
{"format=tree&limit=6", `"info":{"count":7,"count_left":1,"last_comment":"` + ids[6]},
{"format=tree&limit=7", `"info":{"count":7,"count_left":0,"last_comment":"` + ids[8]},
{"format=tree&url=test-url&limit=2", `"info":{"url":"test-url","count":6,"count_left":3,"last_comment":"` + ids[0]},
{"format=tree&url=test-url&limit=6", `"info":{"url":"test-url","count":6,"count_left":1,"last_comment":"` + ids[1]},
{"format=tree&url=test-url&limit=6", `"info":{"url":"test-url","count":6,"count_left":0,"last_comment":"` + ids[6]},
{"format=tree&url=test-url&limit=7", `"info":{"url":"test-url","count":6,"count_left":0,"last_comment":"` + ids[6]},
// start after first top-level comment
{fmt.Sprintf("format=tree&limit=2&offset_id=%s", ids[0]), `"info":{"count":7,"count_left":2,"last_comment":"` + ids[1]},
{fmt.Sprintf("format=tree&url=test-url&limit=2&offset_id=%s", ids[0]), `"info":{"url":"test-url","count":6,"count_left":1,"last_comment":"` + ids[1]},
// start after second top-level comment
{fmt.Sprintf("format=tree&limit=2&offset_id=%s", ids[1]), `"info":{"count":7,"count_left":1,"last_comment":"` + ids[6]},
{fmt.Sprintf("format=tree&limit=2&offset_id=%s", ids[1]), `"info":{"count":7,"count_left":0,"last_comment":"` + ids[8]},
{fmt.Sprintf("format=tree&url=test-url&limit=2&offset_id=%s", ids[1]), `"info":{"url":"test-url","count":6,"count_left":0,"last_comment":"` + ids[6]},
// start after third top-level comment, so expect comment to post 2, or no comments on post 1 if "url" is set
{fmt.Sprintf("format=tree&limit=1&offset_id=%s", ids[6]), `"info":{"count":7,"count_left":0,"last_comment":"` + ids[8]},
backend/app/store/service/tree.go
+3 -3
View File
@@ -211,9 +211,9 @@ func (t *Tree) limit(limit int, offsetID string) {
continue
}
// check if we just exceeded the limit and there are already some nodes in the list,
// as otherwise we would have to return the first node with all its replies even if it exceeds the limit.
if commentsCount+repliesCount >= limit && len(limitedNodes) > 0 {
// stop once adding this subtree would exceed the limit, as long as we already have a node;
// a subtree that fits exactly is still included, and the first node is always returned in full.
if commentsCount+repliesCount > limit && len(limitedNodes) > 0 {
t.countLeft += repliesCount
commentsCount = limit // adjust commentsCount to stop checking limit for the next nodes
continue
backend/app/store/service/tree_test.go
+94
View File
@@ -152,6 +152,100 @@ func TestTreeSortNodes(t *testing.T) {
assert.Equal(t, "1", res.Nodes[0].Comment.ID)
}
func TestMakeTreeLimit(t *testing.T) {
loc := store.Locator{URL: "url", SiteID: "site"}
ts := func(sec int) time.Time { return time.Date(2017, 12, 25, 19, 0, sec, 0, time.UTC) }
// tree with four top-level comments and subtree sizes 3, 2, 1, 3 (total 9):
// c1 -> c1a, c1b
// c2 -> c2a
// c3
// c4 -> c4a -> c4a1
comments := []store.Comment{
{Locator: loc, ID: "c1", Timestamp: ts(1)},
{Locator: loc, ID: "c1a", ParentID: "c1", Timestamp: ts(11)},
{Locator: loc, ID: "c1b", ParentID: "c1", Timestamp: ts(12)},
{Locator: loc, ID: "c2", Timestamp: ts(2)},
{Locator: loc, ID: "c2a", ParentID: "c2", Timestamp: ts(21)},
{Locator: loc, ID: "c3", Timestamp: ts(3)},
{Locator: loc, ID: "c4", Timestamp: ts(4)},
{Locator: loc, ID: "c4a", ParentID: "c4", Timestamp: ts(41)},
{Locator: loc, ID: "c4a1", ParentID: "c4a", Timestamp: ts(42)},
}
nodeIDs := func(nodes []*Node) []string {
ids := make([]string, 0, len(nodes))
for _, n := range nodes {
ids = append(ids, n.Comment.ID)
}
return ids
}
tests := []struct {
name string
limit int
offsetID string
wantNodes []string
wantLeft int
wantLast string
}{
{"no limit, no offset returns all", 0, "", []string{"c1", "c2", "c3", "c4"}, 0, ""},
{"limit equals first subtree size", 3, "", []string{"c1"}, 6, "c1"},
{"limit smaller than first subtree returns it whole", 2, "", []string{"c1"}, 6, "c1"},
{"limit between first and second boundary stops after first", 4, "", []string{"c1"}, 6, "c1"},
{"limit at exact two-subtree boundary includes both", 5, "", []string{"c1", "c2"}, 4, "c2"},
{"limit reaches third subtree exactly", 6, "", []string{"c1", "c2", "c3"}, 3, "c3"},
{"limit equal to total returns all", 9, "", []string{"c1", "c2", "c3", "c4"}, 0, "c4"},
{"limit larger than total returns all", 100, "", []string{"c1", "c2", "c3", "c4"}, 0, "c4"},
{"offset only, no limit slices remainder", 0, "c1", []string{"c2", "c3", "c4"}, 0, ""},
{"offset at last node clears result", 0, "c4", []string{}, 0, ""},
{"offset at last node with limit clears result", 5, "c4", []string{}, 0, ""},
{"offset not found starts from beginning", 0, "missing", []string{"c1", "c2", "c3", "c4"}, 0, ""},
{"offset plus limit returns single subtree", 2, "c1", []string{"c2"}, 4, "c2"},
{"offset plus limit stops before last subtree", 3, "c2", []string{"c3"}, 3, "c3"},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
res := MakeTree(comments, "+time", tc.limit, tc.offsetID)
assert.Equal(t, tc.wantNodes, nodeIDs(res.Nodes), "top-level nodes")
assert.Equal(t, tc.wantLeft, res.CountLeft(), "count left")
assert.Equal(t, tc.wantLast, res.LastComment(), "last comment")
})
}
}
func TestCountReplies(t *testing.T) {
loc := store.Locator{URL: "url", SiteID: "site"}
ts := func(sec int) time.Time { return time.Date(2017, 12, 25, 19, 0, sec, 0, time.UTC) }
comments := []store.Comment{
{Locator: loc, ID: "c1", Timestamp: ts(1)},
{Locator: loc, ID: "c1a", ParentID: "c1", Timestamp: ts(11)},
{Locator: loc, ID: "c1b", ParentID: "c1", Timestamp: ts(12)},
{Locator: loc, ID: "c4", Timestamp: ts(4)},
{Locator: loc, ID: "c4a", ParentID: "c4", Timestamp: ts(41)},
{Locator: loc, ID: "c4a1", ParentID: "c4a", Timestamp: ts(42)},
}
res := MakeTree(comments, "+time", 0, "")
byID := map[string]*Node{}
for _, n := range res.Nodes {
byID[n.Comment.ID] = n
}
// guard presence and shape first so a regression in MakeTree fails with a clear
// assertion instead of a nil-pointer panic on the map lookups below
require.Contains(t, byID, "c1")
require.Contains(t, byID, "c4")
require.Len(t, byID["c1"].Replies, 2)
require.Len(t, byID["c4"].Replies, 1)
assert.Equal(t, 2, countReplies(byID["c1"]), "c1 has two direct replies, no nesting")
assert.Equal(t, 2, countReplies(byID["c4"]), "c4 counts nested reply recursively")
assert.Equal(t, 1, countReplies(byID["c4"].Replies[0]), "c4a has one nested reply")
assert.Equal(t, 0, countReplies(byID["c1"].Replies[0]), "leaf reply has no replies")
}
func BenchmarkTree(b *testing.B) {
comments := []store.Comment{}
data, err := os.ReadFile("testdata/tree_bench.json")