From a6f3b7ea5dd1b612e696d917d0e07e1ac7281453 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 18 Oct 2023 02:50:01 +0200 Subject: [PATCH] collect /find Info for tree and plain types consistently MakeTree calculated Info locally for historical reasons, and the results were consistent with the dataService.Info call but calculated differently. That change fixes that, ensuring that Info is requested in the same manner. --- backend/app/rest/api/rest.go | 5 ++ backend/app/rest/api/rest_public.go | 48 ++++++++++--------- backend/app/rest/api/rest_public_test.go | 14 +++--- backend/app/store/service/testdata/tree.json | 10 +--- .../app/store/service/testdata/tree_del.json | 10 +--- backend/app/store/service/tree.go | 37 ++++---------- backend/app/store/service/tree_test.go | 32 ++++++------- 7 files changed, 63 insertions(+), 93 deletions(-) diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index a2e07ce73a..dab091e3cf 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -98,6 +98,11 @@ type commentsWithInfo struct { Info store.PostInfo `json:"info,omitempty"` } +type treeWithInfo struct { + *service.Tree + Info store.PostInfo `json:"info,omitempty"` +} + // Run the lister and request's router, activate rest server func (s *Rest) Run(address string, port int) { if address == "*" { diff --git a/backend/app/rest/api/rest_public.go b/backend/app/rest/api/rest_public.go index 41d9992b39..bb7023a97c 100644 --- a/backend/app/rest/api/rest_public.go +++ b/backend/app/rest/api/rest_public.go @@ -79,34 +79,36 @@ func (s *public) findCommentsCtrl(w http.ResponseWriter, r *http.Request) { comments = []store.Comment{} // error should clear comments and continue for post info } comments = s.applyView(comments, view) + + var commentsInfo store.PostInfo + if info, ee := s.dataService.Info(locator, s.readOnlyAge); ee == nil { + commentsInfo = info + } + + if !since.IsZero() { // if since is set, number of comments can be different from total in the DB + commentsInfo.Count = 0 + for _, c := range comments { + if !c.Deleted { + commentsInfo.Count++ + } + } + } + + // post might be readonly without any comments, Info call will fail then and ReadOnly flag should be checked separately + if !commentsInfo.ReadOnly && locator.URL != "" && s.dataService.IsReadOnly(locator) { + commentsInfo.ReadOnly = true + } + var b []byte switch format { case "tree": - tree := service.MakeTree(comments, sort, s.readOnlyAge) - if tree.Nodes == nil { // eliminate json nil serialization - tree.Nodes = []*service.Node{} - } - if s.dataService.IsReadOnly(locator) { - tree.Info.ReadOnly = true + withInfo := treeWithInfo{Tree: service.MakeTree(comments, sort), Info: commentsInfo} + if withInfo.Nodes == nil { // eliminate json nil serialization + withInfo.Nodes = []*service.Node{} } - b, e = encodeJSONWithHTML(tree) + b, e = encodeJSONWithHTML(withInfo) default: - withInfo := commentsWithInfo{Comments: comments} - if info, ee := s.dataService.Info(locator, s.readOnlyAge); ee == nil { - withInfo.Info = info - } - if !since.IsZero() { // if since is set, number of comments can be different from total in the DB - withInfo.Info.Count = 0 - for _, c := range comments { - if !c.Deleted { - withInfo.Info.Count++ - } - } - } - // post might be readonly without any comments, Info call will fail then and ReadOnly flag should be checked separately - if !withInfo.Info.ReadOnly && locator.URL != "" && s.dataService.IsReadOnly(locator) { - withInfo.Info.ReadOnly = true - } + withInfo := commentsWithInfo{Comments: comments, Info: commentsInfo} b, e = encodeJSONWithHTML(withInfo) } return b, e diff --git a/backend/app/rest/api/rest_public_test.go b/backend/app/rest/api/rest_public_test.go index 3553d8d6a2..8e05e43ec7 100644 --- a/backend/app/rest/api/rest_public_test.go +++ b/backend/app/rest/api/rest_public_test.go @@ -229,7 +229,7 @@ func TestRest_Find(t *testing.T) { assert.Equal(t, id2, comments.Comments[0].ID) // get in tree mode - tree := service.Tree{} + tree := treeWithInfo{} res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree") assert.Equal(t, http.StatusOK, code) err = json.Unmarshal([]byte(res), &tree) @@ -255,7 +255,7 @@ func TestRest_FindAge(t *testing.T) { _, err = srv.DataService.Create(c2) require.NoError(t, err) - tree := service.Tree{} + tree := treeWithInfo{} res, code := get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree") assert.Equal(t, http.StatusOK, code) @@ -298,7 +298,7 @@ func TestRest_FindReadOnly(t *testing.T) { require.NoError(t, err) require.NoError(t, resp.Body.Close()) - tree := service.Tree{} + tree := treeWithInfo{} res, code := get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah1&format=tree") assert.Equal(t, http.StatusOK, code) err = json.Unmarshal([]byte(res), &tree) @@ -306,7 +306,7 @@ func TestRest_FindReadOnly(t *testing.T) { assert.Equal(t, "https://radio-t.com/blah1", tree.Info.URL) assert.True(t, tree.Info.ReadOnly, "post is ro") - tree = service.Tree{} + tree = treeWithInfo{} res, code = get(t, ts.URL+"/api/v1/find?site=remark42&url=https://radio-t.com/blah2&format=tree") assert.Equal(t, http.StatusOK, code) err = json.Unmarshal([]byte(res), &tree) @@ -658,11 +658,11 @@ func TestPublic_FindCommentsCtrl_ConsistentCount(t *testing.T) { {"url=test-url&since=" + sinceTS[1], fmt.Sprintf(`"info":{"url":"test-url","count":5,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])}, {"since=" + sinceTS[4], fmt.Sprintf(`"info":{"count":3,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[8])}, {"url=test-url&since=" + sinceTS[4], fmt.Sprintf(`"info":{"url":"test-url","count":2,"first_time":%q,"last_time":%q}`, formattedTS[0], formattedTS[7])}, - {"format=tree", `"info":{"url":"test-url","count":7`}, + {"format=tree", `"info":{"count":7`}, {"format=tree&url=test-url", `"info":{"url":"test-url","count":6`}, - {"format=tree&sort=+time", `"info":{"url":"test-url","count":7`}, + {"format=tree&sort=+time", `"info":{"count":7`}, {"format=tree&url=test-url&sort=+time", `"info":{"url":"test-url","count":6`}, - {"format=tree&sort=-score", `"info":{"url":"test-url","count":7`}, + {"format=tree&sort=-score", `"info":{"count":7`}, {"format=tree&url=test-url&sort=-score", `"info":{"url":"test-url","count":6`}, {"sort=+time", fmt.Sprintf(`"score":-25,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[8])}, {"sort=-time", fmt.Sprintf(`"score":1,"vote":0,"time":%q}],"info":{"count":7`, formattedTS[0])}, diff --git a/backend/app/store/service/testdata/tree.json b/backend/app/store/service/testdata/tree.json index b06eb1d299..911cf1a52d 100644 --- a/backend/app/store/service/testdata/tree.json +++ b/backend/app/store/service/testdata/tree.json @@ -242,11 +242,5 @@ "time": "2017-12-25T19:47:22Z" } } - ], - "info": { - "url": "url", - "count": 12, - "first_time": "2017-12-25T19:46:01Z", - "last_time": "2017-12-25T19:47:22Z" - } - } \ No newline at end of file + ] + } diff --git a/backend/app/store/service/testdata/tree_del.json b/backend/app/store/service/testdata/tree_del.json index e8a2ce9190..b464286f77 100644 --- a/backend/app/store/service/testdata/tree_del.json +++ b/backend/app/store/service/testdata/tree_del.json @@ -176,11 +176,5 @@ } }] }] - }], - "info": { - "url": "url", - "count": 8, - "first_time": "2017-12-25T19:46:01Z", - "last_time": "2017-12-25T19:47:05Z" - } - } \ No newline at end of file + }] + } diff --git a/backend/app/store/service/tree.go b/backend/app/store/service/tree.go index bd2ee60851..d6ffdc7649 100644 --- a/backend/app/store/service/tree.go +++ b/backend/app/store/service/tree.go @@ -10,8 +10,7 @@ import ( // Tree is formatter making tree from the list of comments type Tree struct { - Nodes []*Node `json:"comments"` - Info store.PostInfo `json:"info,omitempty"` + Nodes []*Node `json:"comments"` } // Node is a comment with optional replies @@ -30,22 +29,12 @@ type recurData struct { } // MakeTree gets unsorted list of comments and produces Tree -// It will make store.PostInfo by itself and will mark Info.ReadOnly based on passed readOnlyAge -// Tree maker is local and has no access to the data store. By this reason it has to make Info and won't be able -// to handle store's read-only status. This status should be set by caller. -func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree { +func MakeTree(comments []store.Comment, sortType string) *Tree { if len(comments) == 0 { return &Tree{} } - res := Tree{ - Info: store.PostInfo{ - URL: comments[0].Locator.URL, - FirstTS: comments[0].Timestamp, - LastTS: comments[0].Timestamp, - }, - } - res.Info.Count = len(res.filter(comments, func(c store.Comment) bool { return !c.Deleted })) + res := Tree{} topComments := res.filter(comments, func(c store.Comment) bool { return c.ParentID == "" }) @@ -54,23 +43,12 @@ func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree node := Node{Comment: rootComment} rd := recurData{} - commentsTree, tsModified, tsCreated := res.proc(comments, &node, &rd, rootComment.ID) - // skip deleted with no sub-comments ar all sub-comments deleted + commentsTree := res.proc(comments, &node, &rd, rootComment.ID) + // skip deleted with no sub-comments and all sub-comments deleted if rootComment.Deleted && (len(commentsTree.Replies) == 0 || !rd.visible) { continue } - commentsTree.tsModified, commentsTree.tsCreated = tsModified, tsCreated - if commentsTree.tsCreated.Before(res.Info.FirstTS) { - res.Info.FirstTS = commentsTree.tsCreated - } - if commentsTree.tsModified.After(res.Info.LastTS) { - res.Info.LastTS = commentsTree.tsModified - } - - res.Info.ReadOnly = readOnlyAge > 0 && !res.Info.FirstTS.IsZero() && - res.Info.FirstTS.AddDate(0, 0, readOnlyAge).Before(time.Now()) - res.Nodes = append(res.Nodes, commentsTree) } @@ -79,7 +57,7 @@ func MakeTree(comments []store.Comment, sortType string, readOnlyAge int) *Tree } // proc makes tree for one top-level comment recursively -func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentID string) (result *Node, modified, created time.Time) { +func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentID string) (result *Node) { if rd.tsModified.IsZero() || rd.tsCreated.IsZero() { rd.tsModified, rd.tsCreated = node.Comment.Timestamp, node.Comment.Timestamp } @@ -106,7 +84,8 @@ func (t *Tree) proc(comments []store.Comment, node *Node, rd *recurData, parentI sort.Slice(node.Replies, func(i, j int) bool { return node.Replies[i].Comment.Timestamp.Before(node.Replies[j].Comment.Timestamp) }) - return node, rd.tsModified, rd.tsCreated + node.tsModified, node.tsCreated = rd.tsModified, rd.tsCreated + return node } // filter returns comments for parentID diff --git a/backend/app/store/service/tree_test.go b/backend/app/store/service/tree_test.go index 403e6a16a7..c7e90fc751 100644 --- a/backend/app/store/service/tree_test.go +++ b/backend/app/store/service/tree_test.go @@ -37,19 +37,15 @@ func TestMakeTree(t *testing.T) { {Locator: loc, ID: "611", ParentID: "61", Deleted: true}, } - res := MakeTree(comments, "time", 0) + res := MakeTree(comments, "time") resJSON, err := json.Marshal(&res) require.NoError(t, err) expJSON := mustLoadJSONFile(t, "testdata/tree.json") assert.Equal(t, expJSON, resJSON) - assert.Equal(t, store.PostInfo{URL: "url", Count: 12, FirstTS: ts(46, 1), LastTS: ts(47, 22)}, res.Info) - res = MakeTree([]store.Comment{}, "time", 0) + res = MakeTree([]store.Comment{}, "time") assert.Equal(t, &Tree{}, res) - - res = MakeTree(comments, "time", 10) - assert.Equal(t, store.PostInfo{URL: "url", Count: 12, FirstTS: ts(46, 1), LastTS: ts(47, 22), ReadOnly: true}, res.Info) } func TestMakeEmptySubtree(t *testing.T) { @@ -79,7 +75,7 @@ func TestMakeEmptySubtree(t *testing.T) { {Locator: loc, ID: "3", Timestamp: ts(48, 1), Deleted: true}, // deleted top level } - res := MakeTree(comments, "time", 0) + res := MakeTree(comments, "time") resJSON, err := json.Marshal(&res) require.NoError(t, err) t.Log(string(resJSON)) @@ -108,50 +104,50 @@ func TestTreeSortNodes(t *testing.T) { {ID: "5", Deleted: true, Timestamp: time.Date(2017, 12, 25, 19, 47, 22, 150, time.UTC)}, } - res := MakeTree(comments, "+active", 0) + res := MakeTree(comments, "+active") assert.Equal(t, "2", res.Nodes[0].Comment.ID) t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified) - res = MakeTree(comments, "-active", 0) + res = MakeTree(comments, "-active") t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified) assert.Equal(t, "1", res.Nodes[0].Comment.ID) - res = MakeTree(comments, "+time", 0) + res = MakeTree(comments, "+time") t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified) assert.Equal(t, "1", res.Nodes[0].Comment.ID) - res = MakeTree(comments, "-time", 0) + res = MakeTree(comments, "-time") assert.Equal(t, "6", res.Nodes[0].Comment.ID) - res = MakeTree(comments, "score", 0) + res = MakeTree(comments, "score") assert.Equal(t, "4", res.Nodes[0].Comment.ID) assert.Equal(t, "3", res.Nodes[1].Comment.ID) assert.Equal(t, "6", res.Nodes[2].Comment.ID) assert.Equal(t, "1", res.Nodes[3].Comment.ID) - res = MakeTree(comments, "+score", 0) + res = MakeTree(comments, "+score") assert.Equal(t, "4", res.Nodes[0].Comment.ID) - res = MakeTree(comments, "-score", 0) + res = MakeTree(comments, "-score") assert.Equal(t, "2", res.Nodes[0].Comment.ID) assert.Equal(t, "1", res.Nodes[1].Comment.ID) assert.Equal(t, "3", res.Nodes[2].Comment.ID) assert.Equal(t, "6", res.Nodes[3].Comment.ID) - res = MakeTree(comments, "+controversy", 0) + res = MakeTree(comments, "+controversy") assert.Equal(t, "3", res.Nodes[0].Comment.ID) assert.Equal(t, "6", res.Nodes[1].Comment.ID) assert.Equal(t, "2", res.Nodes[2].Comment.ID) assert.Equal(t, "4", res.Nodes[3].Comment.ID) assert.Equal(t, "1", res.Nodes[4].Comment.ID) - res = MakeTree(comments, "-controversy", 0) + res = MakeTree(comments, "-controversy") assert.Equal(t, "1", res.Nodes[0].Comment.ID) assert.Equal(t, "4", res.Nodes[1].Comment.ID) assert.Equal(t, "2", res.Nodes[2].Comment.ID) assert.Equal(t, "3", res.Nodes[3].Comment.ID) - res = MakeTree(comments, "undefined", 0) + res = MakeTree(comments, "undefined") t.Log(res.Nodes[0].Comment.ID, res.Nodes[0].tsModified) assert.Equal(t, "1", res.Nodes[0].Comment.ID) } @@ -164,7 +160,7 @@ func BenchmarkTree(b *testing.B) { assert.NoError(b, err) for i := 0; i < b.N; i++ { - res := MakeTree(comments, "time", 0) + res := MakeTree(comments, "time") assert.NotNil(b, res) } }