Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Collect /find Info for tree and plain types consistently #1685

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Collect /find Info for tree and plain types consistently #1685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions backend/app/rest/api/rest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "*" {
Expand Down
48 changes: 25 additions & 23 deletions backend/app/rest/api/rest_public.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 7 additions & 7 deletions backend/app/rest/api/rest_public_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -298,15 +298,15 @@ 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)
require.NoError(t, err)
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)
Expand Down Expand Up @@ -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])},
Expand Down
10 changes: 2 additions & 8 deletions backend/app/store/service/testdata/tree.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
]
}
10 changes: 2 additions & 8 deletions backend/app/store/service/testdata/tree_del.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,5 @@
}
}]
}]
}],
"info": {
"url": "url",
"count": 8,
"first_time": "2017-12-25T19:46:01Z",
"last_time": "2017-12-25T19:47:05Z"
}
}
}]
}
37 changes: 8 additions & 29 deletions backend/app/store/service/tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 == "" })

Expand All @@ -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)
}

Expand All @@ -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
}
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is magic, I have no idea why it doesn't mess up tsCreated or tsModified of any of the nodes. Maybe we just don't have good enough test cases to catch it.

return node
}

// filter returns comments for parentID
Expand Down
32 changes: 14 additions & 18 deletions backend/app/store/service/tree_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
}
Expand All @@ -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)
}
}
Expand Down
Loading