From 0de26f62c8f57ed07ca32851fd210339df2ec1d9 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 4 Nov 2023 15:51:00 +0100 Subject: [PATCH 1/2] combine multiple post info in DataStore.Info instead of returning first Previously, only the first one was returned for site-wide requests, and now all returned information will be correctly aggregated, and the PostInfo.URL and PostInfo.ReadOnly parameters will be dropped. --- backend/app/store/comment.go | 5 +++-- backend/app/store/service/service.go | 18 +++++++++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/backend/app/store/comment.go b/backend/app/store/comment.go index ef026a4d6f..877f942a27 100644 --- a/backend/app/store/comment.go +++ b/backend/app/store/comment.go @@ -45,9 +45,9 @@ type Edit struct { // PostInfo holds summary for given post url type PostInfo struct { - URL string `json:"url"` + URL string `json:"url,omitempty"` // can be attached to site-wide comments but won't be set then Count int `json:"count"` - ReadOnly bool `json:"read_only,omitempty" bson:"read_only,omitempty"` + ReadOnly bool `json:"read_only,omitempty" bson:"read_only,omitempty"` // can be attached to site-wide comments but won't be set then FirstTS time.Time `json:"first_time,omitempty" bson:"first_time,omitempty"` LastTS time.Time `json:"last_time,omitempty" bson:"last_time,omitempty"` } @@ -98,6 +98,7 @@ func (c *Comment) SetDeleted(mode DeleteMode) { c.Text = "" c.Orig = "" c.Score = 0 + c.Controversy = 0 c.Votes = map[string]bool{} c.VotedIPs = make(map[string]VotedIPInfo) c.Edit = nil diff --git a/backend/app/store/service/service.go b/backend/app/store/service/service.go index 5503712b52..964e9a307d 100644 --- a/backend/app/store/service/service.go +++ b/backend/app/store/service/service.go @@ -754,7 +754,23 @@ func (s *DataStore) Info(locator store.Locator, readonlyAge int) (store.PostInfo if len(res) == 0 { return store.PostInfo{}, fmt.Errorf("post %+v not found", locator) } - return res[0], nil + // URL request + if locator.URL != "" { + return res[0], nil + } + // site-wide request which returned multiple store.PostInfo, so that URL and ReadOnly flags don't make sense + var info store.PostInfo + for _, i := range res { + info.Count += i.Count + if info.FirstTS.IsZero() || i.FirstTS.Before(info.FirstTS) { + info.FirstTS = i.FirstTS + } + if info.LastTS.IsZero() || i.LastTS.After(info.LastTS) { + info.LastTS = i.LastTS + } + } + return info, nil + } // Delete comment by id From 517c5c7d1382a151dd77a6a97893813be854a0c7 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Sat, 4 Nov 2023 17:12:29 +0100 Subject: [PATCH 2/2] add tests for admin Store and DataService --- backend/app/store/admin/admin.go | 3 + backend/app/store/admin/admin_mock.go | 256 ++++++++++++++++++++++ backend/app/store/admin/admin_test.go | 14 ++ backend/app/store/service/service_test.go | 189 +++++++++++++++- 4 files changed, 461 insertions(+), 1 deletion(-) create mode 100644 backend/app/store/admin/admin_mock.go diff --git a/backend/app/store/admin/admin.go b/backend/app/store/admin/admin.go index 2697bdf948..8961f431fa 100644 --- a/backend/app/store/admin/admin.go +++ b/backend/app/store/admin/admin.go @@ -8,6 +8,9 @@ import ( log "github.com/go-pkgz/lgr" ) +// NOTE: matryer/moq should be installed globally and works with `go generate ./...` +//go:generate moq --out admin_mock.go . Store + // Store defines interface returning admins info for given site type Store interface { Key(siteID string) (key string, err error) diff --git a/backend/app/store/admin/admin_mock.go b/backend/app/store/admin/admin_mock.go new file mode 100644 index 0000000000..496836c51b --- /dev/null +++ b/backend/app/store/admin/admin_mock.go @@ -0,0 +1,256 @@ +// Code generated by moq; DO NOT EDIT. +// github.com/matryer/moq + +package admin + +import ( + "sync" +) + +// Ensure, that StoreMock does implement Store. +// If this is not the case, regenerate this file with moq. +var _ Store = &StoreMock{} + +// StoreMock is a mock implementation of Store. +// +// func TestSomethingThatUsesStore(t *testing.T) { +// +// // make and configure a mocked Store +// mockedStore := &StoreMock{ +// AdminsFunc: func(siteID string) ([]string, error) { +// panic("mock out the Admins method") +// }, +// EmailFunc: func(siteID string) (string, error) { +// panic("mock out the Email method") +// }, +// EnabledFunc: func(siteID string) (bool, error) { +// panic("mock out the Enabled method") +// }, +// KeyFunc: func(siteID string) (string, error) { +// panic("mock out the Key method") +// }, +// OnEventFunc: func(siteID string, et EventType) error { +// panic("mock out the OnEvent method") +// }, +// } +// +// // use mockedStore in code that requires Store +// // and then make assertions. +// +// } +type StoreMock struct { + // AdminsFunc mocks the Admins method. + AdminsFunc func(siteID string) ([]string, error) + + // EmailFunc mocks the Email method. + EmailFunc func(siteID string) (string, error) + + // EnabledFunc mocks the Enabled method. + EnabledFunc func(siteID string) (bool, error) + + // KeyFunc mocks the Key method. + KeyFunc func(siteID string) (string, error) + + // OnEventFunc mocks the OnEvent method. + OnEventFunc func(siteID string, et EventType) error + + // calls tracks calls to the methods. + calls struct { + // Admins holds details about calls to the Admins method. + Admins []struct { + // SiteID is the siteID argument value. + SiteID string + } + // Email holds details about calls to the Email method. + Email []struct { + // SiteID is the siteID argument value. + SiteID string + } + // Enabled holds details about calls to the Enabled method. + Enabled []struct { + // SiteID is the siteID argument value. + SiteID string + } + // Key holds details about calls to the Key method. + Key []struct { + // SiteID is the siteID argument value. + SiteID string + } + // OnEvent holds details about calls to the OnEvent method. + OnEvent []struct { + // SiteID is the siteID argument value. + SiteID string + // Et is the et argument value. + Et EventType + } + } + lockAdmins sync.RWMutex + lockEmail sync.RWMutex + lockEnabled sync.RWMutex + lockKey sync.RWMutex + lockOnEvent sync.RWMutex +} + +// Admins calls AdminsFunc. +func (mock *StoreMock) Admins(siteID string) ([]string, error) { + if mock.AdminsFunc == nil { + panic("StoreMock.AdminsFunc: method is nil but Store.Admins was just called") + } + callInfo := struct { + SiteID string + }{ + SiteID: siteID, + } + mock.lockAdmins.Lock() + mock.calls.Admins = append(mock.calls.Admins, callInfo) + mock.lockAdmins.Unlock() + return mock.AdminsFunc(siteID) +} + +// AdminsCalls gets all the calls that were made to Admins. +// Check the length with: +// +// len(mockedStore.AdminsCalls()) +func (mock *StoreMock) AdminsCalls() []struct { + SiteID string +} { + var calls []struct { + SiteID string + } + mock.lockAdmins.RLock() + calls = mock.calls.Admins + mock.lockAdmins.RUnlock() + return calls +} + +// Email calls EmailFunc. +func (mock *StoreMock) Email(siteID string) (string, error) { + if mock.EmailFunc == nil { + panic("StoreMock.EmailFunc: method is nil but Store.Email was just called") + } + callInfo := struct { + SiteID string + }{ + SiteID: siteID, + } + mock.lockEmail.Lock() + mock.calls.Email = append(mock.calls.Email, callInfo) + mock.lockEmail.Unlock() + return mock.EmailFunc(siteID) +} + +// EmailCalls gets all the calls that were made to Email. +// Check the length with: +// +// len(mockedStore.EmailCalls()) +func (mock *StoreMock) EmailCalls() []struct { + SiteID string +} { + var calls []struct { + SiteID string + } + mock.lockEmail.RLock() + calls = mock.calls.Email + mock.lockEmail.RUnlock() + return calls +} + +// Enabled calls EnabledFunc. +func (mock *StoreMock) Enabled(siteID string) (bool, error) { + if mock.EnabledFunc == nil { + panic("StoreMock.EnabledFunc: method is nil but Store.Enabled was just called") + } + callInfo := struct { + SiteID string + }{ + SiteID: siteID, + } + mock.lockEnabled.Lock() + mock.calls.Enabled = append(mock.calls.Enabled, callInfo) + mock.lockEnabled.Unlock() + return mock.EnabledFunc(siteID) +} + +// EnabledCalls gets all the calls that were made to Enabled. +// Check the length with: +// +// len(mockedStore.EnabledCalls()) +func (mock *StoreMock) EnabledCalls() []struct { + SiteID string +} { + var calls []struct { + SiteID string + } + mock.lockEnabled.RLock() + calls = mock.calls.Enabled + mock.lockEnabled.RUnlock() + return calls +} + +// Key calls KeyFunc. +func (mock *StoreMock) Key(siteID string) (string, error) { + if mock.KeyFunc == nil { + panic("StoreMock.KeyFunc: method is nil but Store.Key was just called") + } + callInfo := struct { + SiteID string + }{ + SiteID: siteID, + } + mock.lockKey.Lock() + mock.calls.Key = append(mock.calls.Key, callInfo) + mock.lockKey.Unlock() + return mock.KeyFunc(siteID) +} + +// KeyCalls gets all the calls that were made to Key. +// Check the length with: +// +// len(mockedStore.KeyCalls()) +func (mock *StoreMock) KeyCalls() []struct { + SiteID string +} { + var calls []struct { + SiteID string + } + mock.lockKey.RLock() + calls = mock.calls.Key + mock.lockKey.RUnlock() + return calls +} + +// OnEvent calls OnEventFunc. +func (mock *StoreMock) OnEvent(siteID string, et EventType) error { + if mock.OnEventFunc == nil { + panic("StoreMock.OnEventFunc: method is nil but Store.OnEvent was just called") + } + callInfo := struct { + SiteID string + Et EventType + }{ + SiteID: siteID, + Et: et, + } + mock.lockOnEvent.Lock() + mock.calls.OnEvent = append(mock.calls.OnEvent, callInfo) + mock.lockOnEvent.Unlock() + return mock.OnEventFunc(siteID, et) +} + +// OnEventCalls gets all the calls that were made to OnEvent. +// Check the length with: +// +// len(mockedStore.OnEventCalls()) +func (mock *StoreMock) OnEventCalls() []struct { + SiteID string + Et EventType +} { + var calls []struct { + SiteID string + Et EventType + } + mock.lockOnEvent.RLock() + calls = mock.calls.OnEvent + mock.lockOnEvent.RUnlock() + return calls +} diff --git a/backend/app/store/admin/admin_test.go b/backend/app/store/admin/admin_test.go index 1809c1a199..3d6925e8ba 100644 --- a/backend/app/store/admin/admin_test.go +++ b/backend/app/store/admin/admin_test.go @@ -6,6 +6,20 @@ import ( "github.com/stretchr/testify/assert" ) +func TestStaticStore_StoreWithoutSites(t *testing.T) { + var ks Store = NewStaticKeyStore("key123") + enabled, err := ks.Enabled("any") + assert.NoError(t, err) + assert.True(t, enabled, "on empty store all sites are enabled") + assert.NoError(t, ks.OnEvent("test", EvCreate), "static store does nothing OnEvent") + + // empty key + ks = NewStaticKeyStore("") + key, err := ks.Key("any") + assert.Error(t, err, "empty key") + assert.Empty(t, key) +} + func TestStaticStore_Get(t *testing.T) { var ks Store = NewStaticStore("key123", []string{"s1", "s2", "s3"}, []string{"123", "xyz"}, "aa@example.com") diff --git a/backend/app/store/service/service_test.go b/backend/app/store/service/service_test.go index e86f765a49..195b6b3c9d 100644 --- a/backend/app/store/service/service_test.go +++ b/backend/app/store/service/service_test.go @@ -2,6 +2,7 @@ package service import ( "context" + "errors" "fmt" "math/rand" "net/http" @@ -171,6 +172,50 @@ func TestService_CreateFromPartialWithTitle(t *testing.T) { assert.Equal(t, "post blah", res.PostTitle, "keep comment title") } +func TestService_Put(t *testing.T) { + ks := admin.NewStaticKeyStore("secret 123") + eng, teardown := prepStoreEngine(t) + defer teardown() + b := DataStore{Engine: eng, AdminStore: ks} + + comment := store.Comment{ + ID: "c-1", + ParentID: "id-1", + Text: "test text", + User: store.User{ID: "user2", Name: "user name 2"}, + Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, + Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), + } + _, err := b.Create(comment) + require.NoError(t, err) + + // create new comment with everything different to replace the first one with fields below + updatedComment := store.Comment{ + ID: "c-1", + ParentID: "id-new", + Text: "new text", + User: store.User{ID: "user3", Name: "user name 3"}, + Locator: store.Locator{URL: "https://example.com", SiteID: "example"}, + Timestamp: time.Date(2018, 12, 20, 15, 18, 22, 0, time.Local), + } + + err = b.Put(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, updatedComment) + require.NoError(t, err) + + // request with wrong user, should not affect the comment user + got, err := b.Get(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "c-1", store.User{ID: "user1", Name: "user name 1"}) + require.NoError(t, err) + assert.Equal(t, "c-1", got.ID) + assert.Equal(t, "new text", got.Text) + assert.Equal(t, "id-1", got.ParentID, "should be unaltered") + assert.Equal(t, "user2", got.User.ID, "should be unaltered") + assert.Equal(t, "user name 2", got.User.Name, "should be unaltered") + assert.Equal(t, "https://radio-t.com", got.Locator.URL, "should be unaltered") + assert.Equal(t, "radio-t", got.Locator.SiteID, "should be unaltered") + assert.Equal(t, time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), got.Timestamp, "should be unaltered") + +} + func TestService_SetTitle(t *testing.T) { var titleEnable int32 tss := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -545,6 +590,111 @@ func TestService_VoteControversy(t *testing.T) { assert.InDelta(t, 1.73, res[0].Controversy, 0.01) } +func TestService_RestrictedWords(t *testing.T) { + ks := admin.NewStaticKeyStore("secret 123") + eng, teardown := prepStoreEngine(t) + defer teardown() + restictedWordLister := StaticRestrictedWordsLister{Words: []string{"restricted"}} + b := DataStore{Engine: eng, AdminStore: ks, RestrictedWordsMatcher: NewRestrictedWordsMatcher(restictedWordLister)} + + // test creating a comment with restricted words which should fail with appropriate error + reply := store.Comment{ + ID: "c-1", + ParentID: "id-1", + Text: "restricted word", + Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), + Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, + User: store.User{ID: "user2", Name: "user name 2"}, + } + id, err := b.Create(reply) + assert.EqualError(t, err, ErrRestrictedWordsFound.Error(), "should fail with RestrictedWordError") + assert.Empty(t, id) +} + +func TestDataStore_AdminStoreErrors(t *testing.T) { + badKey := true + badEnabled := true + as := admin.StoreMock{ + OnEventFunc: func(siteID string, et admin.EventType) error { return errors.New("err") }, + KeyFunc: func(siteID string) (string, error) { + if badKey { + return "", errors.New("mock key err") + } + return "secret", nil + }, + EnabledFunc: func(siteID string) (bool, error) { + if badEnabled { + return false, errors.New("mock enabled err") + } + return true, nil + }, + AdminsFunc: func(siteID string) ([]string, error) { return nil, errors.New("mock admins err") }, + } + eng, teardown := prepStoreEngine(t) + defer teardown() + b := DataStore{Engine: eng, AdminStore: &as, MaxVotes: -1} + comment := store.Comment{ + ID: "c-1", + ParentID: "id-1", + Text: "restricted word", + Timestamp: time.Date(2017, 12, 20, 15, 18, 22, 0, time.Local), + Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, + User: store.User{ID: "user2", Name: "user name 2"}, + } + + // Key call error + id, err := b.Create(comment) + assert.ErrorContainsf(t, err, "mock key err", "should fail with mock error") + assert.Empty(t, id) + assert.Equal(t, len(as.KeyCalls()), 1) + assert.Equal(t, len(as.EnabledCalls()), 0) + assert.Equal(t, len(as.OnEventCalls()), 0) + + // Enabled call error + badKey = false + id, err = b.Create(comment) + assert.ErrorContains(t, err, "mock enabled err", "should fail with mock error") + assert.Empty(t, id) + assert.Equal(t, len(as.KeyCalls()), 2) + assert.Equal(t, len(as.EnabledCalls()), 1) + assert.Equal(t, len(as.OnEventCalls()), 0) + + // only OnEvent error + badEnabled = false + id, err = b.Create(comment) + assert.NoError(t, err, "OnEvent error should be just logged") + assert.Equal(t, id, "c-1") + assert.Equal(t, len(as.KeyCalls()), 3) + assert.Equal(t, len(as.EnabledCalls()), 2) + assert.Equal(t, len(as.OnEventCalls()), 1) + + // OnEvent error on Vote call + _, err = b.Vote(VoteReq{Locator: store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, CommentID: "c-1", + UserID: "user4", Val: true}) + assert.NoError(t, err, "OnEvent error should be just logged") + assert.Equal(t, len(as.KeyCalls()), 4) + assert.Equal(t, len(as.EnabledCalls()), 3) + assert.Equal(t, len(as.OnEventCalls()), 2) + + // Admins error + isAdmin := b.IsAdmin("radio-t", "user2") + assert.False(t, isAdmin) + assert.Equal(t, len(as.AdminsCalls()), 1) + assert.Equal(t, len(as.OnEventCalls()), 2) + + // OnEvent error on EditComment call + _, err = b.EditComment(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "c-1", EditRequest{Text: "new text"}) + assert.NoError(t, err, "OnEvent error should be just logged") + + // OnEvent error on Delete call + err = b.Delete(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "c-1", store.SoftDelete) + assert.NoError(t, err, "OnEvent error should be just logged") + + // OnEvent error on EditComment Delete call + _, err = b.EditComment(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, "c-1", EditRequest{Delete: true}) + assert.NoError(t, err, "OnEvent error should be just logged") +} + func TestService_VoteSameIP(t *testing.T) { eng, teardown := prepStoreEngine(t) defer teardown() @@ -1185,18 +1335,55 @@ func TestService_Info(t *testing.T) { b := DataStore{Engine: eng, EditDuration: 100 * time.Millisecond, AdminStore: admin.NewStaticStore("secret 123", nil, []string{"user2"}, "user@email.com")} - info, err := b.Info(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, 0) + // add one more comment for another URL to test site-wide Info request + comment := store.Comment{ + ID: "123456xyz", + Text: `some text, link`, + Timestamp: time.Date(2018, 12, 20, 15, 18, 22, 0, time.Local), + Locator: store.Locator{URL: "https://radio-t.com/another", SiteID: "radio-t"}, + User: store.User{ID: "user2", Name: "user name"}, + } + _, err := b.Create(comment) + require.NoError(t, err) + + // get non-existing URL info + info, err := b.Info(store.Locator{URL: "bad", SiteID: "radio-t"}, 0) + assert.Error(t, err) + assert.Empty(t, info) + + // get non-existing site info + info, err = b.Info(store.Locator{SiteID: "bad"}, 0) + assert.Error(t, err) + assert.Empty(t, info) + + // test two initially created comments and store first comment FirstTS + info, err = b.Info(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, 0) require.NoError(t, err) assert.Equal(t, "https://radio-t.com", info.URL) assert.Equal(t, 2, info.Count) assert.False(t, info.ReadOnly) assert.True(t, info.LastTS.After(info.FirstTS)) + firstTS := info.FirstTS time.Sleep(1 * time.Second) // make post RO in 1sec info, err = b.Info(store.Locator{URL: "https://radio-t.com", SiteID: "radio-t"}, 1) require.NoError(t, err) assert.Equal(t, "https://radio-t.com", info.URL) assert.True(t, info.ReadOnly) + + // get last created comment LastTS + info, err = b.Info(store.Locator{URL: "https://radio-t.com/another", SiteID: "radio-t"}, 0) + require.NoError(t, err) + lastTS := info.LastTS + + // site-level request + info, err = b.Info(store.Locator{SiteID: "radio-t"}, 1) + require.NoError(t, err) + assert.Equal(t, 3, info.Count) + assert.Empty(t, info.URL, "site-level request should not set URL") + assert.False(t, info.ReadOnly, "site-level request should not set ReadOnly") + assert.Equal(t, firstTS, info.FirstTS, "site-level request should have FirstTS from the first post") + assert.Equal(t, lastTS, info.LastTS, "site-level request should have LastTS from the last post") } func TestService_Delete(t *testing.T) {