From 83afa4d57c7e2dd9a03abf65f58b3e0348e35be3 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 01:11:48 -0500 Subject: [PATCH 01/15] basic scopes for cache invalidation #44 --- app/rest/api/rest.go | 23 ++++++------ app/rest/api/rest_test.go | 2 +- app/rest/cache.go | 73 +++++++++++++++++++++++++++++++++++++-- app/rest/cache_test.go | 49 ++++++++++++++++++++++++++ 4 files changed, 132 insertions(+), 15 deletions(-) diff --git a/app/rest/api/rest.go b/app/rest/api/rest.go index 5ca7d7a4..431cfc47 100644 --- a/app/rest/api/rest.go +++ b/app/rest/api/rest.go @@ -212,7 +212,8 @@ func (s *Rest) createCommentCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't load created comment") return } - s.Cache.Flush() // reset all caches + s.Cache.Flush(comment.Locator.URL, "last", comment.User.ID) + render.Status(r, http.StatusCreated) render.JSON(w, r, &finalComment) } @@ -293,7 +294,7 @@ func (s *Rest) updateCommentCtrl(w http.ResponseWriter, r *http.Request) { return } - s.Cache.Flush() // reset all caches + s.Cache.Flush(locator.URL, "last", user.ID) render.JSON(w, r, res) } @@ -307,7 +308,7 @@ func (s *Rest) findCommentsCtrl(w http.ResponseWriter, r *http.Request) { } log.Printf("[DEBUG] get comments for %+v, sort %s, format %s", locator, sort, r.URL.Query().Get("format")) - data, err := s.Cache.Get(rest.URLKey(r), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), locator.SiteID, locator.URL), 4*time.Hour, func() ([]byte, error) { comments, e := s.DataService.Find(locator, sort) if e != nil { return nil, e @@ -332,16 +333,16 @@ func (s *Rest) findCommentsCtrl(w http.ResponseWriter, r *http.Request) { // GET /last/{limit}?site=siteID - last comments for the siteID, across all posts, sorted by time func (s *Rest) lastCommentsCtrl(w http.ResponseWriter, r *http.Request) { - - log.Printf("[DEBUG] get last comments for %s", r.URL.Query().Get("site")) + siteID := r.URL.Query().Get("site") + log.Printf("[DEBUG] get last comments for %s", siteID) limit, err := strconv.Atoi(chi.URLParam(r, "limit")) if err != nil { limit = 0 } - data, err := s.Cache.Get(rest.URLKey(r), 4*time.Hour, func() ([]byte, error) { - comments, e := s.DataService.Last(r.URL.Query().Get("site"), limit) + data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), "last", siteID), 4*time.Hour, func() ([]byte, error) { + comments, e := s.DataService.Last(siteID, limit) if e != nil { return nil, e } @@ -403,7 +404,7 @@ func (s *Rest) findUserCommentsCtrl(w http.ResponseWriter, r *http.Request) { log.Printf("[DEBUG] get comments for userID %s, %s", userID, siteID) - data, err := s.Cache.Get(rest.URLKey(r), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), userID, siteID), 4*time.Hour, func() ([]byte, error) { comments, count, e := s.DataService.User(siteID, userID, limit) if e != nil { return nil, e @@ -492,7 +493,7 @@ func (s *Rest) countMultiCtrl(w http.ResponseWriter, r *http.Request) { } sha := base64.URLEncoding.EncodeToString(hasher.Sum(nil)) - data, err := s.Cache.Get(sha, 8*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(rest.CacheKey(sha, siteID), 8*time.Hour, func() ([]byte, error) { counts, e := s.DataService.Counts(siteID, posts) if e != nil { return nil, e @@ -520,7 +521,7 @@ func (s *Rest) listCtrl(w http.ResponseWriter, r *http.Request) { skip = v } - data, err := s.Cache.Get(rest.URLKey(r), 8*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), siteID), 8*time.Hour, func() ([]byte, error) { posts, e := s.DataService.List(siteID, limit, skip) if e != nil { return nil, e @@ -554,7 +555,7 @@ func (s *Rest) voteCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't vote for comment") return } - s.Cache.Flush() + s.Cache.Flush(locator.URL) render.JSON(w, r, JSON{"id": comment.ID, "score": comment.Score}) } diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index b71bb03c..228bae07 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -567,4 +567,4 @@ func (mc *mockCache) Get(key string, ttl time.Duration, fn func() ([]byte, error return fn() } -func (mc *mockCache) Flush() {} +func (mc *mockCache) Flush(scopes ...string) {} diff --git a/app/rest/cache.go b/app/rest/cache.go index 294612f4..992c4b24 100644 --- a/app/rest/cache.go +++ b/app/rest/cache.go @@ -4,15 +4,33 @@ import ( "log" "net/http" "strings" + "sync" "time" "github.com/patrickmn/go-cache" + "github.com/pkg/errors" ) // LoadingCache defines interface for caching type LoadingCache interface { Get(key string, ttl time.Duration, fn func() ([]byte, error)) (data []byte, err error) - Flush() + Flush(scopes ...string) +} + +func CacheKey(key string, scopes ...string) string { + return strings.Join(scopes, "$$") + "@@" + key +} +func parseKey(fullKey string) (key string, scopes []string, err error) { + elems := strings.Split(fullKey, "@@") + if len(elems) != 2 { + return "", nil, errors.Errorf("can't parse cache key %s", key) + } + scopes = strings.Split(elems[0], "$$") + if len(scopes) == 1 && scopes[0] == "" { + scopes = []string{} + } + key = elems[1] + return key, scopes, nil } // loadingCache implements LoadingCache interface on top of cache.Cache (go-cache) @@ -23,6 +41,9 @@ type loadingCache struct { cleanupInterval time.Duration maxKeys int maxValueSize int + + activeKeys map[string]struct{} + lock sync.Mutex } // NewLoadingCache makes loadingCache implementation @@ -33,6 +54,7 @@ func NewLoadingCache(options ...CacheOption) LoadingCache { postFlushFn: func() {}, maxKeys: 0, maxValueSize: 0, + activeKeys: map[string]struct{}{}, } for _, opt := range options { if err := opt(&res); err != nil { @@ -40,6 +62,11 @@ func NewLoadingCache(options ...CacheOption) LoadingCache { } } res.bytesCache = cache.New(res.defaultExpiration, res.cleanupInterval) + + res.bytesCache.OnEvicted(func(key string, _ interface{}) { + res.withLock(func() { delete(res.activeKeys, key) }) + }) + log.Printf("[DEBUG] create cache with cleanupInterval=%s, maxKeys=%d, maxValueSize=%d", res.cleanupInterval, res.maxKeys, res.maxValueSize) @@ -57,13 +84,53 @@ func (lc *loadingCache) Get(key string, ttl time.Duration, fn func() ([]byte, er } if lc.allowed(data) { lc.bytesCache.Set(key, data, ttl) + lc.withLock(func() { lc.activeKeys[key] = struct{}{} }) } return data, nil } +func (lc *loadingCache) withLock(fn func()) { + lc.lock.Lock() + fn() + lc.lock.Unlock() +} + // Flush clears cache and calls postFlushFn async -func (lc *loadingCache) Flush() { - lc.bytesCache.Flush() +func (lc *loadingCache) Flush(scopes ...string) { + + if len(scopes) == 0 { + lc.bytesCache.Flush() + } + + inScope := func(fullKey string) bool { + for _, s := range scopes { + _, keyScopes, err := parseKey(fullKey) + if err != nil { + return false + } + for _, ks := range keyScopes { + if ks == s { + return true + } + } + } + return false + } + + if len(scopes) > 0 { + matchedKeys := []string{} + lc.withLock(func() { + for k := range lc.activeKeys { + if inScope(k) { + matchedKeys = append(matchedKeys, k) + } + } + }) + for _, mkey := range matchedKeys { + lc.bytesCache.Delete(mkey) + } + } + if lc.postFlushFn != nil { go lc.postFlushFn() } diff --git a/app/rest/cache_test.go b/app/rest/cache_test.go index 2e36ec53..917af2ab 100644 --- a/app/rest/cache_test.go +++ b/app/rest/cache_test.go @@ -171,3 +171,52 @@ func TestLoadingCache_Parallel(t *testing.T) { wg.Wait() assert.Equal(t, int32(0), atomic.LoadInt32(&coldCalls)) } + +func TestLoadingCache_Scopes(t *testing.T) { + lc := NewLoadingCache(CleanupInterval(time.Second)) + + res, err := lc.Get(CacheKey("key", "s1", "s2"), time.Minute, func() ([]byte, error) { + return []byte("value"), nil + }) + assert.Nil(t, err) + assert.Equal(t, "value", string(res)) + + res, err = lc.Get(CacheKey("key2", "s2"), time.Minute, func() ([]byte, error) { + return []byte("value2"), nil + }) + assert.Nil(t, err) + assert.Equal(t, "value2", string(res)) + + lc.Flush("s1") + lc.Get(CacheKey("key2", "s2"), time.Minute, func() ([]byte, error) { + assert.Fail(t, "should stay") + return nil, nil + }) + + res, err = lc.Get(CacheKey("key", "s1", "s2"), time.Minute, func() ([]byte, error) { + return []byte("value-upd"), nil + }) + assert.Equal(t, "value-upd", string(res), "was deleted, update") +} + +func TestLoadingCache_Keys(t *testing.T) { + tbl := []struct { + key string + scopes []string + full string + }{ + {"key1", []string{"s1"}, "s1@@key1"}, + {"key2", []string{"s11", "s2"}, "s11$$s2@@key2"}, + {"key3", []string{}, "@@key3"}, + } + + for n, tt := range tbl { + full := CacheKey(tt.key, tt.scopes...) + assert.Equal(t, tt.full, full, "making key, #%d", n) + + k, s, e := parseKey(full) + assert.Nil(t, e) + assert.Equal(t, tt.scopes, s) + assert.Equal(t, tt.key, k) + } +} From d8fee735f1f3622c7f991de961d2a40019d34938 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 01:15:00 -0500 Subject: [PATCH 02/15] lint:missing comment for pub func --- app/rest/cache.go | 1 + 1 file changed, 1 insertion(+) diff --git a/app/rest/cache.go b/app/rest/cache.go index 992c4b24..c279fea0 100644 --- a/app/rest/cache.go +++ b/app/rest/cache.go @@ -17,6 +17,7 @@ type LoadingCache interface { Flush(scopes ...string) } +// CacheKey makes full key from primary key ans scopes func CacheKey(key string, scopes ...string) string { return strings.Join(scopes, "$$") + "@@" + key } From 08be684d76f919a6dbb8b55085dbc91e53ddc776 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 02:08:54 -0500 Subject: [PATCH 03/15] cache to separate package --- app/main.go | 6 +++--- app/rest/api/admin.go | 3 ++- app/rest/api/import.go | 3 ++- app/rest/api/rest.go | 15 +++++++------ app/rest/api/rss.go | 10 +++++---- app/rest/{ => cache}/cache.go | 34 +++++++++++++++++------------- app/rest/{ => cache}/cache_test.go | 15 +++++++------ 7 files changed, 48 insertions(+), 38 deletions(-) rename app/rest/{ => cache}/cache.go (84%) rename app/rest/{ => cache}/cache_test.go (93%) diff --git a/app/main.go b/app/main.go index db2d60ab..bcdcb07b 100644 --- a/app/main.go +++ b/app/main.go @@ -19,9 +19,9 @@ import ( "github.com/umputun/remark/app/store/service" "github.com/umputun/remark/app/migrator" - "github.com/umputun/remark/app/rest" "github.com/umputun/remark/app/rest/api" "github.com/umputun/remark/app/rest/auth" + "github.com/umputun/remark/app/rest/cache" "github.com/umputun/remark/app/rest/proxy" ) @@ -114,8 +114,8 @@ func New(opts Opts) (*Application, error) { MaxCommentSize: opts.MaxCommentSize, } - cache := rest.NewLoadingCache(rest.MaxValSize(opts.MaxCachedValue), rest.MaxKeys(opts.MaxCachedItems), - rest.PostFlushFn(postFlushFn(opts.Sites, opts.Port))) + cache := cache.NewLoadingCache(cache.MaxValSize(opts.MaxCachedValue), cache.MaxKeys(opts.MaxCachedItems), + cache.PostFlushFn(postFlushFn(opts.Sites, opts.Port))) jwtService := auth.NewJWT(opts.SecretKey, strings.HasPrefix(opts.RemarkURL, "https://"), 7*24*time.Hour) diff --git a/app/rest/api/admin.go b/app/rest/api/admin.go index 1f55943d..e622c1a5 100644 --- a/app/rest/api/admin.go +++ b/app/rest/api/admin.go @@ -13,6 +13,7 @@ import ( "github.com/umputun/remark/app/migrator" "github.com/umputun/remark/app/rest" + "github.com/umputun/remark/app/rest/cache" "github.com/umputun/remark/app/store" "github.com/umputun/remark/app/store/service" ) @@ -21,7 +22,7 @@ import ( type admin struct { dataService service.DataStore exporter migrator.Exporter - cache rest.LoadingCache + cache cache.LoadingCache defAvatarURL string } diff --git a/app/rest/api/import.go b/app/rest/api/import.go index 551b7ef2..b3ff12f9 100644 --- a/app/rest/api/import.go +++ b/app/rest/api/import.go @@ -17,12 +17,13 @@ import ( "github.com/umputun/remark/app/migrator" "github.com/umputun/remark/app/rest" + "github.com/umputun/remark/app/rest/cache" ) // Import rest runs on unexposed port and available for local requests only type Import struct { Version string - Cache rest.LoadingCache + Cache cache.LoadingCache NativeImporter migrator.Importer DisqusImporter migrator.Importer SecretKey string diff --git a/app/rest/api/rest.go b/app/rest/api/rest.go index 431cfc47..9cc4d76d 100644 --- a/app/rest/api/rest.go +++ b/app/rest/api/rest.go @@ -25,6 +25,7 @@ import ( "github.com/umputun/remark/app/migrator" "github.com/umputun/remark/app/rest" "github.com/umputun/remark/app/rest/auth" + "github.com/umputun/remark/app/rest/cache" "github.com/umputun/remark/app/rest/proxy" "github.com/umputun/remark/app/store" "github.com/umputun/remark/app/store/service" @@ -36,7 +37,7 @@ type Rest struct { DataService service.DataStore Authenticator auth.Authenticator Exporter migrator.Exporter - Cache rest.LoadingCache + Cache cache.LoadingCache AvatarProxy *proxy.Avatar ImageProxy *proxy.Image WebRoot string @@ -308,7 +309,7 @@ func (s *Rest) findCommentsCtrl(w http.ResponseWriter, r *http.Request) { } log.Printf("[DEBUG] get comments for %+v, sort %s, format %s", locator, sort, r.URL.Query().Get("format")) - data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), locator.SiteID, locator.URL), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), locator.SiteID, locator.URL), 4*time.Hour, func() ([]byte, error) { comments, e := s.DataService.Find(locator, sort) if e != nil { return nil, e @@ -341,7 +342,7 @@ func (s *Rest) lastCommentsCtrl(w http.ResponseWriter, r *http.Request) { limit = 0 } - data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), "last", siteID), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), "last", siteID), 4*time.Hour, func() ([]byte, error) { comments, e := s.DataService.Last(siteID, limit) if e != nil { return nil, e @@ -404,7 +405,7 @@ func (s *Rest) findUserCommentsCtrl(w http.ResponseWriter, r *http.Request) { log.Printf("[DEBUG] get comments for userID %s, %s", userID, siteID) - data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), userID, siteID), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), userID, siteID), 4*time.Hour, func() ([]byte, error) { comments, count, e := s.DataService.User(siteID, userID, limit) if e != nil { return nil, e @@ -485,7 +486,7 @@ func (s *Rest) countMultiCtrl(w http.ResponseWriter, r *http.Request) { } // key could be long for multiple posts, make it sha1 - key := rest.URLKey(r) + strings.Join(posts, ",") + key := cache.URLKey(r) + strings.Join(posts, ",") hasher := sha1.New() if _, err := hasher.Write([]byte(key)); err != nil { rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't make sha1 for list of urls") @@ -493,7 +494,7 @@ func (s *Rest) countMultiCtrl(w http.ResponseWriter, r *http.Request) { } sha := base64.URLEncoding.EncodeToString(hasher.Sum(nil)) - data, err := s.Cache.Get(rest.CacheKey(sha, siteID), 8*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(sha, siteID), 8*time.Hour, func() ([]byte, error) { counts, e := s.DataService.Counts(siteID, posts) if e != nil { return nil, e @@ -521,7 +522,7 @@ func (s *Rest) listCtrl(w http.ResponseWriter, r *http.Request) { skip = v } - data, err := s.Cache.Get(rest.CacheKey(rest.URLKey(r), siteID), 8*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), siteID), 8*time.Hour, func() ([]byte, error) { posts, e := s.DataService.List(siteID, limit, skip) if e != nil { return nil, e diff --git a/app/rest/api/rss.go b/app/rest/api/rss.go index e7533b87..d5779776 100644 --- a/app/rest/api/rss.go +++ b/app/rest/api/rss.go @@ -11,6 +11,7 @@ import ( "github.com/gorilla/feeds" "github.com/umputun/remark/app/rest" + "github.com/umputun/remark/app/rest/cache" "github.com/umputun/remark/app/store" ) @@ -32,7 +33,7 @@ func (s *Rest) rssPostCommentsCtrl(w http.ResponseWriter, r *http.Request) { sort := "-time" log.Printf("[DEBUG] get rss for post %+v", locator) - data, err := s.Cache.Get(rest.URLKey(r), 4*time.Hour, func() ([]byte, error) { + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), locator.SiteID, locator.URL), 4*time.Hour, func() ([]byte, error) { comments, e := s.DataService.Find(locator, sort) if e != nil { return nil, e @@ -61,10 +62,11 @@ func (s *Rest) rssPostCommentsCtrl(w http.ResponseWriter, r *http.Request) { // GET /rss/site?site=siteID func (s *Rest) rssSiteCommentsCtrl(w http.ResponseWriter, r *http.Request) { - log.Printf("[DEBUG] get rss for site %s", r.URL.Query().Get("site")) + siteID := r.URL.Query().Get("site") + log.Printf("[DEBUG] get rss for site %s", siteID) - data, err := s.Cache.Get(rest.URLKey(r), 4*time.Hour, func() ([]byte, error) { - comments, e := s.DataService.Last(r.URL.Query().Get("site"), maxRssItems) + data, err := s.Cache.Get(cache.Key(cache.URLKey(r), siteID), 4*time.Hour, func() ([]byte, error) { + comments, e := s.DataService.Last(siteID, maxRssItems) if e != nil { return nil, e } diff --git a/app/rest/cache.go b/app/rest/cache/cache.go similarity index 84% rename from app/rest/cache.go rename to app/rest/cache/cache.go index c279fea0..ec00761c 100644 --- a/app/rest/cache.go +++ b/app/rest/cache/cache.go @@ -1,4 +1,4 @@ -package rest +package cache import ( "log" @@ -9,6 +9,7 @@ import ( "github.com/patrickmn/go-cache" "github.com/pkg/errors" + "github.com/umputun/remark/app/rest" ) // LoadingCache defines interface for caching @@ -17,8 +18,8 @@ type LoadingCache interface { Flush(scopes ...string) } -// CacheKey makes full key from primary key ans scopes -func CacheKey(key string, scopes ...string) string { +// Key makes full key from primary key ans scopes +func Key(key string, scopes ...string) string { return strings.Join(scopes, "$$") + "@@" + key } func parseKey(fullKey string) (key string, scopes []string, err error) { @@ -101,8 +102,11 @@ func (lc *loadingCache) Flush(scopes ...string) { if len(scopes) == 0 { lc.bytesCache.Flush() + go lc.postFlushFn() + return } + // check if fullKey has matching scopes inScope := func(fullKey string) bool { for _, s := range scopes { _, keyScopes, err := parseKey(fullKey) @@ -118,18 +122,18 @@ func (lc *loadingCache) Flush(scopes ...string) { return false } - if len(scopes) > 0 { - matchedKeys := []string{} - lc.withLock(func() { - for k := range lc.activeKeys { - if inScope(k) { - matchedKeys = append(matchedKeys, k) - } + // all matchedKeys should be collected first + // we can't delete it from locked section, it will lock on eviction callback + matchedKeys := []string{} + lc.withLock(func() { + for k := range lc.activeKeys { + if inScope(k) { + matchedKeys = append(matchedKeys, k) } - }) - for _, mkey := range matchedKeys { - lc.bytesCache.Delete(mkey) } + }) + for _, mkey := range matchedKeys { + lc.bytesCache.Delete(mkey) } if lc.postFlushFn != nil { @@ -188,8 +192,8 @@ func PostFlushFn(postFlushFn func()) CacheOption { // admins will have different keys in order to prevent leak of admin-only data to regular users func URLKey(r *http.Request) string { adminPrefix := "admin!!" - key := strings.TrimPrefix(r.URL.String(), adminPrefix) // prevents attach with fake url to get admin view - if user, err := GetUserInfo(r); err == nil && user.Admin { // make separate cache key for admins + key := strings.TrimPrefix(r.URL.String(), adminPrefix) // prevents attach with fake url to get admin view + if user, err := rest.GetUserInfo(r); err == nil && user.Admin { // make separate cache key for admins key = adminPrefix + key } return key diff --git a/app/rest/cache_test.go b/app/rest/cache/cache_test.go similarity index 93% rename from app/rest/cache_test.go rename to app/rest/cache/cache_test.go index 917af2ab..507d31fc 100644 --- a/app/rest/cache_test.go +++ b/app/rest/cache/cache_test.go @@ -1,4 +1,4 @@ -package rest +package cache import ( "fmt" @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/umputun/remark/app/rest" "github.com/umputun/remark/app/store" ) @@ -139,7 +140,7 @@ func TestLoadingCache_URLKey(t *testing.T) { assert.Equal(t, "http://blah/123?key=v&k2=v2", key) user := store.User{Admin: true} - r = SetUserInfo(r, user) + r = rest.SetUserInfo(r, user) key = URLKey(r) assert.Equal(t, "admin!!http://blah/123?key=v&k2=v2", key) } @@ -175,25 +176,25 @@ func TestLoadingCache_Parallel(t *testing.T) { func TestLoadingCache_Scopes(t *testing.T) { lc := NewLoadingCache(CleanupInterval(time.Second)) - res, err := lc.Get(CacheKey("key", "s1", "s2"), time.Minute, func() ([]byte, error) { + res, err := lc.Get(Key("key", "s1", "s2"), time.Minute, func() ([]byte, error) { return []byte("value"), nil }) assert.Nil(t, err) assert.Equal(t, "value", string(res)) - res, err = lc.Get(CacheKey("key2", "s2"), time.Minute, func() ([]byte, error) { + res, err = lc.Get(Key("key2", "s2"), time.Minute, func() ([]byte, error) { return []byte("value2"), nil }) assert.Nil(t, err) assert.Equal(t, "value2", string(res)) lc.Flush("s1") - lc.Get(CacheKey("key2", "s2"), time.Minute, func() ([]byte, error) { + lc.Get(Key("key2", "s2"), time.Minute, func() ([]byte, error) { assert.Fail(t, "should stay") return nil, nil }) - res, err = lc.Get(CacheKey("key", "s1", "s2"), time.Minute, func() ([]byte, error) { + res, err = lc.Get(Key("key", "s1", "s2"), time.Minute, func() ([]byte, error) { return []byte("value-upd"), nil }) assert.Equal(t, "value-upd", string(res), "was deleted, update") @@ -211,7 +212,7 @@ func TestLoadingCache_Keys(t *testing.T) { } for n, tt := range tbl { - full := CacheKey(tt.key, tt.scopes...) + full := Key(tt.key, tt.scopes...) assert.Equal(t, tt.full, full, "making key, #%d", n) k, s, e := parseKey(full) From 3b5b16a1e361fe0966ccd46715d98d2c36ba9d6e Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 02:21:13 -0500 Subject: [PATCH 04/15] lint: cach option --- app/rest/cache/cache.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/rest/cache/cache.go b/app/rest/cache/cache.go index ec00761c..5a426577 100644 --- a/app/rest/cache/cache.go +++ b/app/rest/cache/cache.go @@ -49,7 +49,7 @@ type loadingCache struct { } // NewLoadingCache makes loadingCache implementation -func NewLoadingCache(options ...CacheOption) LoadingCache { +func NewLoadingCache(options ...Option) LoadingCache { res := loadingCache{ defaultExpiration: time.Hour, cleanupInterval: 5 * time.Minute, @@ -151,12 +151,12 @@ func (lc *loadingCache) allowed(data []byte) bool { return true } -// CacheOption func type -type CacheOption func(lc *loadingCache) error +// Option func type +type Option func(lc *loadingCache) error // MaxValSize functional option defines the largest value's size allowed to be cached // By default it is 0, which means unlimited. -func MaxValSize(max int) CacheOption { +func MaxValSize(max int) Option { return func(lc *loadingCache) error { lc.maxValueSize = max return nil @@ -165,7 +165,7 @@ func MaxValSize(max int) CacheOption { // MaxKeys functional option defines how many keys to keep. // By default it is 0, which means unlimited. -func MaxKeys(max int) CacheOption { +func MaxKeys(max int) Option { return func(lc *loadingCache) error { lc.maxKeys = max return nil @@ -173,7 +173,7 @@ func MaxKeys(max int) CacheOption { } // CleanupInterval functional option defines how often cleanup loop activated. -func CleanupInterval(interval time.Duration) CacheOption { +func CleanupInterval(interval time.Duration) Option { return func(lc *loadingCache) error { lc.cleanupInterval = interval return nil @@ -181,7 +181,7 @@ func CleanupInterval(interval time.Duration) CacheOption { } // PostFlushFn functional option defines how callback function called after each Flush. -func PostFlushFn(postFlushFn func()) CacheOption { +func PostFlushFn(postFlushFn func()) Option { return func(lc *loadingCache) error { lc.postFlushFn = postFlushFn return nil From c3d5136a0eee3ab9f2758ba3827352b3392bb78c Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 10:28:17 -0500 Subject: [PATCH 05/15] try codecov.io --- .travis.yml | 1 + Dockerfile | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 9cb6efc1..99526ac4 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,6 +5,7 @@ install: script: - docker build --build-arg COVERALLS_TOKEN=$COVERALLS_TOKEN + --build-arg CODECOV_TOKEN=$CODECOV_TOKEN --build-arg CI=$CI --build-arg TRAVIS=$TRAVIS --build-arg TRAVIS_BRANCH=$TRAVIS_BRANCH diff --git a/Dockerfile b/Dockerfile index a2a84df5..eaae4397 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,7 @@ FROM umputun/baseimage:buildgo-latest as build-backend ARG COVERALLS_TOKEN +ARG CODECOV_TOKEN ARG CI ARG TRAVIS ARG TRAVIS_BRANCH @@ -27,10 +28,15 @@ RUN gometalinter --disable-all --deadline=300s --vendor --enable=vet --enable=ve RUN mkdir -p target && /script/coverage.sh -RUN if [ "x$COVERALLS_TOKEN" = "x" ] ; then \ +RUN if [ -z "$COVERALLS_TOKEN" ] ; then \ echo coverall not enabled ; \ else goveralls -coverprofile=.cover/cover.out -service=travis-ci -repotoken $COVERALLS_TOKEN; fi +RUN if [ -z "$CODECOV_TOKEN" ] ; then \ + echo codecov not enabled ; \ + else curl -s https://codecov.io/bash -o codecov && \ + bash codecov -f .cover/cover.out -X fix; fi + RUN go build -o remark -ldflags "-X main.revision=$(git rev-parse --abbrev-ref HEAD)-$(git describe --abbrev=7 --always --tags)-$(date +%Y%m%d-%H:%M:%S) -s -w" ./app From 147561bc3005258a55875d83a46e112a84f31062 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 13:48:31 -0500 Subject: [PATCH 06/15] error path tests --- app/rest/proxy/avatar.go | 11 +++++------ app/rest/proxy/avatar_test.go | 31 ++++++++++++++++++++++++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/app/rest/proxy/avatar.go b/app/rest/proxy/avatar.go index 62234684..89c2dcf6 100644 --- a/app/rest/proxy/avatar.go +++ b/app/rest/proxy/avatar.go @@ -14,7 +14,6 @@ import ( "time" "github.com/go-chi/chi" - "github.com/go-chi/render" "github.com/pkg/errors" "github.com/umputun/remark/app/rest" @@ -54,6 +53,10 @@ func (p *Avatar) Put(u store.User) (avatarURL string, err error) { } }() + if resp.StatusCode != http.StatusOK { + return "", errors.Errorf("failed to get avatar from the orig, status %s", resp.Status) + } + // get ID and location of locally cached avatar encID := store.EncodeID(u.ID) location := p.location(encID) // location adds partition to path @@ -122,11 +125,7 @@ func (p *Avatar) Routes(middlewares ...func(http.Handler) http.Handler) (string, if fi, e := fh.Stat(); e == nil { w.Header().Set("Content-Length", strconv.Itoa(int(fi.Size()))) } - - // write all headers - if status, ok := r.Context().Value(render.StatusCtxKey).(int); ok { - w.WriteHeader(status) - } + w.WriteHeader(http.StatusOK) if _, err = io.Copy(w, fh); err != nil { log.Printf("[WARN] can't send response to %s, %s", r.RemoteAddr, err) } diff --git a/app/rest/proxy/avatar_test.go b/app/rest/proxy/avatar_test.go index 655b6928..47ed547f 100644 --- a/app/rest/proxy/avatar_test.go +++ b/app/rest/proxy/avatar_test.go @@ -4,16 +4,18 @@ import ( "bytes" "fmt" "io" + "log" "net/http" "net/http/httptest" "os" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/umputun/remark/app/store" ) -func TestPut(t *testing.T) { +func TestAvatar_Put(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/pic.png" { @@ -46,18 +48,36 @@ func TestPut(t *testing.T) { assert.Equal(t, int64(21), fi.Size()) } -func TestPutNoAvatar(t *testing.T) { +func TestAvatar_PutFailed(t *testing.T) { + + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + log.Print("request: ", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + })) + defer ts.Close() + p := Avatar{StorePath: "/tmp/avatars.test", RoutePath: "/avatar"} u := store.User{ID: "user1", Name: "user1 name"} _, err := p.Put(u) - assert.Error(t, err) + assert.EqualError(t, err, "no picture for user1") + + u = store.User{ID: "user1", Name: "user1 name", Picture: "http://127.0.0.1/avater/pic"} + _, err = p.Put(u) + require.Error(t, err) + assert.Contains(t, err.Error(), "connect: connection refused") + + u = store.User{ID: "user1", Name: "user1 name", Picture: ts.URL + "/avatar/pic"} + _, err = p.Put(u) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to get avatar from the orig") } -func TestRoutes(t *testing.T) { +func TestAvatar_Routes(t *testing.T) { ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/pic.png" { w.Header().Set("Content-Type", "image/*") + w.Header().Set("Custom-Header", "xyz") fmt.Fprint(w, "some picture bin data") return } @@ -87,6 +107,7 @@ func TestRoutes(t *testing.T) { assert.Equal(t, []string{"image/*"}, rr.HeaderMap["Content-Type"]) assert.Equal(t, []string{"21"}, rr.HeaderMap["Content-Length"]) + assert.Equal(t, []string(nil), rr.HeaderMap["Custom-Header"], "strip all custom headers") assert.NotNil(t, rr.HeaderMap["Etag"]) bb := bytes.Buffer{} @@ -96,7 +117,7 @@ func TestRoutes(t *testing.T) { assert.Equal(t, "some picture bin data", bb.String()) } -func TestLocation(t *testing.T) { +func TestAvatar_Location(t *testing.T) { p := Avatar{StorePath: "/tmp/avatars.test"} tbl := []struct { From f56611930dc36066e7950377693b3b5da38f3280 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 13:50:26 -0500 Subject: [PATCH 07/15] parse key tests --- app/rest/cache/cache_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/rest/cache/cache_test.go b/app/rest/cache/cache_test.go index 507d31fc..5c5e31aa 100644 --- a/app/rest/cache/cache_test.go +++ b/app/rest/cache/cache_test.go @@ -220,4 +220,9 @@ func TestLoadingCache_Keys(t *testing.T) { assert.Equal(t, tt.scopes, s) assert.Equal(t, tt.key, k) } + + _, _, err := parseKey("abc") + assert.Error(t, err) + _, _, err = parseKey("") + assert.Error(t, err) } From 838c1a760f3cd899339194d46bbff148741e0913 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 14:08:27 -0500 Subject: [PATCH 08/15] simplify rss headers --- app/rest/api/rss.go | 12 ++++-------- app/rest/api/rss_test.go | 6 ++++++ app/rest/cache/cache.go | 6 ++++-- 3 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/rest/api/rss.go b/app/rest/api/rss.go index d5779776..430f75c1 100644 --- a/app/rest/api/rss.go +++ b/app/rest/api/rss.go @@ -7,7 +7,6 @@ import ( "time" "github.com/go-chi/chi" - "github.com/go-chi/render" "github.com/gorilla/feeds" "github.com/umputun/remark/app/rest" @@ -52,9 +51,8 @@ func (s *Rest) rssPostCommentsCtrl(w http.ResponseWriter, r *http.Request) { } w.Header().Set("Content-Type", "application/xml; charset=utf-8") - if status, ok := r.Context().Value(render.StatusCtxKey).(int); ok { - w.WriteHeader(status) - } + w.WriteHeader(http.StatusOK) + if _, err := w.Write(data); err != nil { log.Printf("[WARN] failed to send response to %s, %s", r.RemoteAddr, err) } @@ -80,14 +78,12 @@ func (s *Rest) rssSiteCommentsCtrl(w http.ResponseWriter, r *http.Request) { }) if err != nil { - rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't get last comments") + rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't get last comments") return } w.Header().Set("Content-Type", "application/xml; charset=utf-8") - if status, ok := r.Context().Value(render.StatusCtxKey).(int); ok { - w.WriteHeader(status) - } + w.WriteHeader(http.StatusOK) if _, err := w.Write(data); err != nil { log.Printf("[WARN] failed to send response to %s, %s", r.RemoteAddr, err) } diff --git a/app/rest/api/rss_test.go b/app/rest/api/rss_test.go index 9e5302b3..afc2c566 100644 --- a/app/rest/api/rss_test.go +++ b/app/rest/api/rss_test.go @@ -47,6 +47,9 @@ func TestServer_RssPost(t *testing.T) { expected, res = cleanRssFormatting(expected, res) assert.Equal(t, expected, res) + + res, code = get(t, ts.URL+"/api/v1/rss/post?site=radio-t-bad&url=https://radio-t.com/blah1") + assert.Equal(t, 400, code) } func TestServer_RssSite(t *testing.T) { @@ -98,6 +101,9 @@ func TestServer_RssSite(t *testing.T) { expected, res = cleanRssFormatting(expected, res) assert.Equal(t, expected, res) + + _, code = get(t, ts.URL+"/api/v1/rss/site?site=bad-radio-t") + assert.Equal(t, 400, code) } func TestServer_RssWithReply(t *testing.T) { diff --git a/app/rest/cache/cache.go b/app/rest/cache/cache.go index 5a426577..c32ce04b 100644 --- a/app/rest/cache/cache.go +++ b/app/rest/cache/cache.go @@ -18,10 +18,11 @@ type LoadingCache interface { Flush(scopes ...string) } -// Key makes full key from primary key ans scopes +// Key makes full key from primary key and scopes func Key(key string, scopes ...string) string { return strings.Join(scopes, "$$") + "@@" + key } + func parseKey(fullKey string) (key string, scopes []string, err error) { elems := strings.Split(fullKey, "@@") if len(elems) != 2 { @@ -44,7 +45,7 @@ type loadingCache struct { maxKeys int maxValueSize int - activeKeys map[string]struct{} + activeKeys map[string]struct{} // keep all current cached keys lock sync.Mutex } @@ -65,6 +66,7 @@ func NewLoadingCache(options ...Option) LoadingCache { } res.bytesCache = cache.New(res.defaultExpiration, res.cleanupInterval) + // OnEvicted called automatically for expired and manually deleted res.bytesCache.OnEvicted(func(key string, _ interface{}) { res.withLock(func() { delete(res.activeKeys, key) }) }) From 2c4c1998cd682740fb9bb57394ff8d439878cc08 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 14:20:42 -0500 Subject: [PATCH 09/15] missed scopes for admin and import --- app/rest/api/admin.go | 6 +++--- app/rest/api/import.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/rest/api/admin.go b/app/rest/api/admin.go index e622c1a5..bf93c870 100644 --- a/app/rest/api/admin.go +++ b/app/rest/api/admin.go @@ -50,7 +50,7 @@ func (a *admin) deleteCommentCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't delete comment") return } - a.cache.Flush() + a.cache.Flush(locator.SiteID, locator.URL) render.Status(r, http.StatusOK) render.JSON(w, r, JSON{"id": id, "locator": locator}) } @@ -65,7 +65,7 @@ func (a *admin) setBlockCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't set blocking status") return } - a.cache.Flush() + a.cache.Flush(siteID, userID) render.JSON(w, r, JSON{"user_id": userID, "site_id": siteID, "block": blockStatus}) } @@ -91,7 +91,7 @@ func (a *admin) setPinCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "can't set pin status") return } - a.cache.Flush() + a.cache.Flush(locator.URL) render.JSON(w, r, JSON{"id": commentID, "locator": locator, "pin": pinStatus}) } diff --git a/app/rest/api/import.go b/app/rest/api/import.go index b3ff12f9..e7679072 100644 --- a/app/rest/api/import.go +++ b/app/rest/api/import.go @@ -93,7 +93,7 @@ func (s *Import) importCtrl(w http.ResponseWriter, r *http.Request) { rest.SendErrorJSON(w, r, http.StatusBadRequest, err, "import failed") return } - s.Cache.Flush() + s.Cache.Flush(siteID) render.Status(r, http.StatusCreated) render.JSON(w, r, JSON{"status": "ok", "size": size}) From 232f0df03f63c6e54659ce0eda661f38b7cc849e Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 14:39:24 -0500 Subject: [PATCH 10/15] fix export file with gz close --- app/rest/api/admin.go | 4 +++- app/rest/api/admin_test.go | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/app/rest/api/admin.go b/app/rest/api/admin.go index bf93c870..6a6a2604 100644 --- a/app/rest/api/admin.go +++ b/app/rest/api/admin.go @@ -105,7 +105,9 @@ func (a *admin) exportCtrl(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/gzip") w.Header().Set("Content-Disposition", "attachment;filename="+exportFile) w.WriteHeader(http.StatusOK) - writer = gzip.NewWriter(w) + gzWriter := gzip.NewWriter(w) + defer gzWriter.Close() + writer = gzWriter } if _, err := a.exporter.Export(writer, siteID); err != nil { diff --git a/app/rest/api/admin_test.go b/app/rest/api/admin_test.go index a9c66188..c8be5690 100644 --- a/app/rest/api/admin_test.go +++ b/app/rest/api/admin_test.go @@ -1,12 +1,14 @@ package api import ( + "compress/gzip" "encoding/json" "fmt" "io/ioutil" "net/http" "strings" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -175,7 +177,7 @@ func TestAdmin_BlockedList(t *testing.T) { assert.Equal(t, "user2", users[1].ID) } -func TestAdmin_Export(t *testing.T) { +func TestAdmin_ExportStream(t *testing.T) { srv, ts := prep(t) assert.NotNil(t, srv) defer cleanup(ts) @@ -194,3 +196,35 @@ func TestAdmin_Export(t *testing.T) { assert.Equal(t, 2, strings.Count(body, "\"text\"")) t.Logf("%s", body) } + +func TestAdmin_ExportFile(t *testing.T) { + srv, ts := prep(t) + assert.NotNil(t, srv) + defer cleanup(ts) + + c1 := store.Comment{Text: "test test #1", + Locator: store.Locator{SiteID: "radio-t", URL: "https://radio-t.com/blah1"}} + c2 := store.Comment{Text: "test test #2", ParentID: "p1", + Locator: store.Locator{SiteID: "radio-t", URL: "https://radio-t.com/blah2"}} + + addComment(t, c1, ts) + addComment(t, c2, ts) + + client := &http.Client{Timeout: 5 * time.Second} + req, err := http.NewRequest("GET", ts.URL+"/api/v1/admin/export?site=radio-t&mode=file", nil) + require.Nil(t, err) + withBasicAuth(req, "dev", "password") + resp, err := client.Do(req) + require.Nil(t, err) + + assert.Equal(t, 200, resp.StatusCode) + assert.Equal(t, "application/gzip", resp.Header.Get("Content-Type")) + + ungzReader, err := gzip.NewReader(resp.Body) + assert.NoError(t, err) + ungzBody, err := ioutil.ReadAll(ungzReader) + assert.NoError(t, err) + assert.Equal(t, 2, strings.Count(string(ungzBody), "\n")) + assert.Equal(t, 2, strings.Count(string(ungzBody), "\"text\"")) + t.Logf("%s", string(ungzBody)) +} From 5280478c32a0e4ca62f71c36e49e36cf8b847251 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 14:42:56 -0500 Subject: [PATCH 11/15] lint: err check on writer close --- app/rest/api/admin.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/rest/api/admin.go b/app/rest/api/admin.go index 6a6a2604..5aeaa680 100644 --- a/app/rest/api/admin.go +++ b/app/rest/api/admin.go @@ -106,7 +106,11 @@ func (a *admin) exportCtrl(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Disposition", "attachment;filename="+exportFile) w.WriteHeader(http.StatusOK) gzWriter := gzip.NewWriter(w) - defer gzWriter.Close() + defer func() { + if e := gzWriter.Close(); e != nil { + log.Printf("[WARN] can't close gzip writer, %s", e) + } + }() writer = gzWriter } From 22c866120ff1e216960f30c9a6040f43e146eafa Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 15:00:50 -0500 Subject: [PATCH 12/15] find test with tree resp --- app/rest/api/rest_test.go | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/app/rest/api/rest_test.go b/app/rest/api/rest_test.go index 228bae07..673cf82d 100644 --- a/app/rest/api/rest_test.go +++ b/app/rest/api/rest_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "github.com/umputun/remark/app/migrator" + "github.com/umputun/remark/app/rest" "github.com/umputun/remark/app/rest/auth" "github.com/umputun/remark/app/rest/proxy" "github.com/umputun/remark/app/store" @@ -160,13 +161,14 @@ func TestServer_Find(t *testing.T) { _, code := get(t, ts.URL+"/api/v1/find?site=radio-t&url=https://radio-t.com/blah1") assert.Equal(t, 400, code, "nothing in") - c1 := store.Comment{Text: "test test #1", ParentID: "p1", + c1 := store.Comment{Text: "test test #1", ParentID: "", Locator: store.Locator{SiteID: "radio-t", URL: "https://radio-t.com/blah1"}} - c2 := store.Comment{Text: "test test #2", ParentID: "p1", - Locator: store.Locator{SiteID: "radio-t", URL: "https://radio-t.com/blah1"}} - id1 := addComment(t, c1, ts) + + c2 := store.Comment{Text: "test test #2", ParentID: id1, + Locator: store.Locator{SiteID: "radio-t", URL: "https://radio-t.com/blah1"}} id2 := addComment(t, c2, ts) + assert.NotEqual(t, id1, id2) // get sorted by +time @@ -187,6 +189,15 @@ func TestServer_Find(t *testing.T) { assert.Equal(t, 2, len(comments), "should have 2 comments") assert.Equal(t, id1, comments[1].ID) assert.Equal(t, id2, comments[0].ID) + + // get in tree mode + tree := rest.Tree{} + res, code = get(t, ts.URL+"/api/v1/find?site=radio-t&url=https://radio-t.com/blah1&format=tree") + assert.Equal(t, 200, code) + err = json.Unmarshal([]byte(res), &tree) + assert.Nil(t, err) + assert.Equal(t, 1, len(tree.Nodes)) + assert.Equal(t, 1, len(tree.Nodes[0].Replies)) } func TestServer_Update(t *testing.T) { From 98211229d37bcf69e44d1cb42f2789fffd5e67bf Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 17:55:45 -0500 Subject: [PATCH 13/15] decent tests for scoped flush --- app/rest/cache/cache.go | 53 ++------------------------------- app/rest/cache/cache_test.go | 50 +++++++++++++++++++++++++++++++ app/rest/cache/options.go | 57 ++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 51 deletions(-) create mode 100644 app/rest/cache/options.go diff --git a/app/rest/cache/cache.go b/app/rest/cache/cache.go index c32ce04b..c335e4d6 100644 --- a/app/rest/cache/cache.go +++ b/app/rest/cache/cache.go @@ -2,14 +2,12 @@ package cache import ( "log" - "net/http" "strings" "sync" "time" "github.com/patrickmn/go-cache" "github.com/pkg/errors" - "github.com/umputun/remark/app/rest" ) // LoadingCache defines interface for caching @@ -50,7 +48,7 @@ type loadingCache struct { } // NewLoadingCache makes loadingCache implementation -func NewLoadingCache(options ...Option) LoadingCache { +func NewLoadingCache(options ...Option) *loadingCache { res := loadingCache{ defaultExpiration: time.Hour, cleanupInterval: 5 * time.Minute, @@ -104,6 +102,7 @@ func (lc *loadingCache) Flush(scopes ...string) { if len(scopes) == 0 { lc.bytesCache.Flush() + lc.withLock(func() { lc.activeKeys = map[string]struct{}{} }) go lc.postFlushFn() return } @@ -152,51 +151,3 @@ func (lc *loadingCache) allowed(data []byte) bool { } return true } - -// Option func type -type Option func(lc *loadingCache) error - -// MaxValSize functional option defines the largest value's size allowed to be cached -// By default it is 0, which means unlimited. -func MaxValSize(max int) Option { - return func(lc *loadingCache) error { - lc.maxValueSize = max - return nil - } -} - -// MaxKeys functional option defines how many keys to keep. -// By default it is 0, which means unlimited. -func MaxKeys(max int) Option { - return func(lc *loadingCache) error { - lc.maxKeys = max - return nil - } -} - -// CleanupInterval functional option defines how often cleanup loop activated. -func CleanupInterval(interval time.Duration) Option { - return func(lc *loadingCache) error { - lc.cleanupInterval = interval - return nil - } -} - -// PostFlushFn functional option defines how callback function called after each Flush. -func PostFlushFn(postFlushFn func()) Option { - return func(lc *loadingCache) error { - lc.postFlushFn = postFlushFn - return nil - } -} - -// URLKey gets url from request to use it as cache key -// admins will have different keys in order to prevent leak of admin-only data to regular users -func URLKey(r *http.Request) string { - adminPrefix := "admin!!" - key := strings.TrimPrefix(r.URL.String(), adminPrefix) // prevents attach with fake url to get admin view - if user, err := rest.GetUserInfo(r); err == nil && user.Admin { // make separate cache key for admins - key = adminPrefix + key - } - return key -} diff --git a/app/rest/cache/cache_test.go b/app/rest/cache/cache_test.go index 5c5e31aa..4352c237 100644 --- a/app/rest/cache/cache_test.go +++ b/app/rest/cache/cache_test.go @@ -188,7 +188,10 @@ func TestLoadingCache_Scopes(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "value2", string(res)) + assert.Equal(t, 2, len(lc.activeKeys)) lc.Flush("s1") + assert.Equal(t, 1, len(lc.activeKeys)) + lc.Get(Key("key2", "s2"), time.Minute, func() ([]byte, error) { assert.Fail(t, "should stay") return nil, nil @@ -200,6 +203,53 @@ func TestLoadingCache_Scopes(t *testing.T) { assert.Equal(t, "value-upd", string(res), "was deleted, update") } +func TestLoadingCache_Flush(t *testing.T) { + lc := NewLoadingCache(CleanupInterval(time.Second)) + + addToCache := func(key string, scopes ...string) { + res, err := lc.Get(key, time.Minute, func() ([]byte, error) { + return []byte("value" + key), nil + }) + require.Nil(t, err) + require.Equal(t, "value"+key, string(res)) + } + + init := func() { + lc.Flush() + addToCache(Key("key1", "s1", "s2")) + addToCache(Key("key2", "s1", "s2", "s3")) + addToCache(Key("key3", "s1", "s2", "s3")) + addToCache(Key("key4", "s2", "s3")) + addToCache(Key("key5", "s2")) + addToCache(Key("key6")) + addToCache(Key("key7", "s4", "s3")) + require.Equal(t, 7, len(lc.activeKeys), "cache init") + } + + tbl := []struct { + scopes []string + left int + msg string + }{ + {[]string{}, 0, "full flush, no scopes"}, + {[]string{"s0"}, 7, "flush wrong scope"}, + {[]string{"s1"}, 4, "flush s1 scope"}, + {[]string{"s2", "s1"}, 2, "flush s2+s1 scope"}, + {[]string{"s1", "s2"}, 2, "flush s1+s2 scope"}, + {[]string{"s1", "s2", "s4"}, 1, "flush s1+s2+s4 scope"}, + {[]string{"s1", "s2", "s3"}, 1, "flush s1+s2+s3 scope"}, + {[]string{"s1", "s2", "ss"}, 2, "flush s1+s2+wrong scope"}, + } + + for i, tt := range tbl { + init() + lc.Flush(tt.scopes...) + assert.Equal(t, tt.left, len(lc.activeKeys), "keys size, %s #%d", tt.msg, i) + assert.Equal(t, tt.left, len(lc.bytesCache.Items()), "items size, %s #%d", tt.msg, i) + + } +} + func TestLoadingCache_Keys(t *testing.T) { tbl := []struct { key string diff --git a/app/rest/cache/options.go b/app/rest/cache/options.go new file mode 100644 index 00000000..8fda85f6 --- /dev/null +++ b/app/rest/cache/options.go @@ -0,0 +1,57 @@ +package cache + +import ( + "net/http" + "strings" + "time" + + "github.com/umputun/remark/app/rest" +) + +// Option func type +type Option func(lc *loadingCache) error + +// MaxValSize functional option defines the largest value's size allowed to be cached +// By default it is 0, which means unlimited. +func MaxValSize(max int) Option { + return func(lc *loadingCache) error { + lc.maxValueSize = max + return nil + } +} + +// MaxKeys functional option defines how many keys to keep. +// By default it is 0, which means unlimited. +func MaxKeys(max int) Option { + return func(lc *loadingCache) error { + lc.maxKeys = max + return nil + } +} + +// CleanupInterval functional option defines how often cleanup loop activated. +func CleanupInterval(interval time.Duration) Option { + return func(lc *loadingCache) error { + lc.cleanupInterval = interval + return nil + } +} + +// PostFlushFn functional option defines how callback function called after each Flush. +func PostFlushFn(postFlushFn func()) Option { + return func(lc *loadingCache) error { + lc.postFlushFn = postFlushFn + return nil + } +} + +// URLKey gets url from request to use it as cache key +// admins will have different keys in order to prevent leak of admin-only data to regular users +func URLKey(r *http.Request) string { + adminPrefix := "admin!!" + key := strings.TrimPrefix(r.URL.String(), adminPrefix) // prevents attach with fake url to get admin view + if user, err := rest.GetUserInfo(r); err == nil && user.Admin { // make separate cache key for admins + key = adminPrefix + key + } + return key +} From 72a2ecde499126713676b2a1d5471a35a7c349b2 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 17:58:44 -0500 Subject: [PATCH 14/15] pick random port for failed rss test --- app/rest/proxy/avatar_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/rest/proxy/avatar_test.go b/app/rest/proxy/avatar_test.go index 47ed547f..cbb7850c 100644 --- a/app/rest/proxy/avatar_test.go +++ b/app/rest/proxy/avatar_test.go @@ -61,7 +61,7 @@ func TestAvatar_PutFailed(t *testing.T) { _, err := p.Put(u) assert.EqualError(t, err, "no picture for user1") - u = store.User{ID: "user1", Name: "user1 name", Picture: "http://127.0.0.1/avater/pic"} + u = store.User{ID: "user1", Name: "user1 name", Picture: "http://127.0.0.1:12345/avater/pic"} _, err = p.Put(u) require.Error(t, err) assert.Contains(t, err.Error(), "connect: connection refused") From 2fa8153d19f639dc4940da0af8ec8f9c7168cd97 Mon Sep 17 00:00:00 2001 From: Umputun Date: Fri, 25 May 2018 18:02:28 -0500 Subject: [PATCH 15/15] hide loadingCache implementation to private --- app/rest/cache/cache.go | 2 +- app/rest/cache/cache_test.go | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/rest/cache/cache.go b/app/rest/cache/cache.go index c335e4d6..625f7f38 100644 --- a/app/rest/cache/cache.go +++ b/app/rest/cache/cache.go @@ -48,7 +48,7 @@ type loadingCache struct { } // NewLoadingCache makes loadingCache implementation -func NewLoadingCache(options ...Option) *loadingCache { +func NewLoadingCache(options ...Option) LoadingCache { res := loadingCache{ defaultExpiration: time.Hour, cleanupInterval: 5 * time.Minute, diff --git a/app/rest/cache/cache_test.go b/app/rest/cache/cache_test.go index 4352c237..c9ec7391 100644 --- a/app/rest/cache/cache_test.go +++ b/app/rest/cache/cache_test.go @@ -188,9 +188,9 @@ func TestLoadingCache_Scopes(t *testing.T) { assert.Nil(t, err) assert.Equal(t, "value2", string(res)) - assert.Equal(t, 2, len(lc.activeKeys)) + assert.Equal(t, 2, len(lc.(*loadingCache).activeKeys)) lc.Flush("s1") - assert.Equal(t, 1, len(lc.activeKeys)) + assert.Equal(t, 1, len(lc.(*loadingCache).activeKeys)) lc.Get(Key("key2", "s2"), time.Minute, func() ([]byte, error) { assert.Fail(t, "should stay") @@ -223,7 +223,7 @@ func TestLoadingCache_Flush(t *testing.T) { addToCache(Key("key5", "s2")) addToCache(Key("key6")) addToCache(Key("key7", "s4", "s3")) - require.Equal(t, 7, len(lc.activeKeys), "cache init") + require.Equal(t, 7, len(lc.(*loadingCache).activeKeys), "cache init") } tbl := []struct { @@ -244,8 +244,8 @@ func TestLoadingCache_Flush(t *testing.T) { for i, tt := range tbl { init() lc.Flush(tt.scopes...) - assert.Equal(t, tt.left, len(lc.activeKeys), "keys size, %s #%d", tt.msg, i) - assert.Equal(t, tt.left, len(lc.bytesCache.Items()), "items size, %s #%d", tt.msg, i) + assert.Equal(t, tt.left, len(lc.(*loadingCache).activeKeys), "keys size, %s #%d", tt.msg, i) + assert.Equal(t, tt.left, len(lc.(*loadingCache).bytesCache.Items()), "items size, %s #%d", tt.msg, i) } }