From eb04d418d9f3b9d1c48965882f86660bb8339376 Mon Sep 17 00:00:00 2001 From: Brad Rydzewski Date: Tue, 20 Oct 2015 16:45:24 -0700 Subject: [PATCH] moving caching w/ TTL to its own package --- cache/cache.go | 39 ++++++++++++++ cache/cache_test.go | 35 +++++++++++++ cache/context.go | 23 ++++++++ cache/helper.go | 75 +++++++++++++++++++++++++++ cache/helper_test.go | 56 ++++++++++++++++++++ docs/setup/bitbucket.md | 7 +++ docs/setup/mysql.md | 6 +-- drone.go | 2 + router/middleware/cache/cache.go | 49 +++-------------- router/middleware/cache/cache_test.go | 40 -------------- router/middleware/cache/perms.go | 22 ++++---- router/middleware/cache/perms_test.go | 9 ++-- router/middleware/cache/repos.go | 14 +++-- router/middleware/cache/repos_test.go | 10 ++-- 14 files changed, 275 insertions(+), 112 deletions(-) create mode 100644 cache/cache.go create mode 100644 cache/cache_test.go create mode 100644 cache/context.go create mode 100644 cache/helper.go create mode 100644 cache/helper_test.go delete mode 100644 router/middleware/cache/cache_test.go diff --git a/cache/cache.go b/cache/cache.go new file mode 100644 index 000000000..f29c219c1 --- /dev/null +++ b/cache/cache.go @@ -0,0 +1,39 @@ +package cache + +import ( + "time" + + "github.com/koding/cache" + "golang.org/x/net/context" +) + +type Cache interface { + Get(string) (interface{}, error) + Set(string, interface{}) error +} + +func Get(c context.Context, key string) (interface{}, error) { + return FromContext(c).Get(key) +} + +func Set(c context.Context, key string, value interface{}) error { + return FromContext(c).Set(key, value) +} + +// Default creates an in-memory cache with the default +// 24 hour expiration period. +func Default() Cache { + return cache.NewMemoryWithTTL(time.Hour * 24) +} + +// NewTTL returns an in-memory cache with the specified +// ttl expiration period. +func NewTTL(t time.Duration) Cache { + return cache.NewMemoryWithTTL(t) +} + +// NewTTL returns an in-memory cache with the specified +// ttl expiration period. +func NewLRU(size int) Cache { + return cache.NewLRU(size) +} diff --git a/cache/cache_test.go b/cache/cache_test.go new file mode 100644 index 000000000..f8cf7998b --- /dev/null +++ b/cache/cache_test.go @@ -0,0 +1,35 @@ +package cache + +import ( + "testing" + + // "github.com/drone/drone/model" + "github.com/franela/goblin" + "github.com/gin-gonic/gin" +) + +func TestCache(t *testing.T) { + + g := goblin.Goblin(t) + g.Describe("Cache", func() { + + var c *gin.Context + g.BeforeEach(func() { + c = new(gin.Context) + ToContext(c, Default()) + }) + + g.It("Should set and get an item", func() { + Set(c, "foo", "bar") + v, e := Get(c, "foo") + g.Assert(v).Equal("bar") + g.Assert(e == nil).IsTrue() + }) + + g.It("Should return nil when item not found", func() { + v, e := Get(c, "foo") + g.Assert(v == nil).IsTrue() + g.Assert(e == nil).IsFalse() + }) + }) +} diff --git a/cache/context.go b/cache/context.go new file mode 100644 index 000000000..5fe513dbd --- /dev/null +++ b/cache/context.go @@ -0,0 +1,23 @@ +package cache + +import ( + "golang.org/x/net/context" +) + +const key = "cache" + +// Setter defines a context that enables setting values. +type Setter interface { + Set(string, interface{}) +} + +// FromContext returns the Cache associated with this context. +func FromContext(c context.Context) Cache { + return c.Value(key).(Cache) +} + +// ToContext adds the Cache to this context if it supports +// the Setter interface. +func ToContext(c Setter, cache Cache) { + c.Set(key, cache) +} diff --git a/cache/helper.go b/cache/helper.go new file mode 100644 index 000000000..74a55b606 --- /dev/null +++ b/cache/helper.go @@ -0,0 +1,75 @@ +package cache + +import ( + "fmt" + + "github.com/drone/drone/model" + "golang.org/x/net/context" +) + +// GetRepos returns the user permissions to the named repository +// from the cache associated with the current context. +func GetPerms(c context.Context, user *model.User, owner, name string) *model.Perm { + key := fmt.Sprintf("perms:%s:%s/%s", + user.Login, + owner, + name, + ) + val, err := FromContext(c).Get(key) + if err != nil { + return nil + } + return val.(*model.Perm) +} + +// SetRepos adds the listof user permissions to the named repsotiory +// to the cache assocaited with the current context. +func SetPerms(c context.Context, user *model.User, perm *model.Perm, owner, name string) { + key := fmt.Sprintf("perms:%s:%s/%s", + user.Login, + owner, + name, + ) + FromContext(c).Set(key, perm) +} + +// GetRepos returns the list of user repositories from the cache +// associated with the current context. +func GetRepos(c context.Context, user *model.User) []*model.RepoLite { + key := fmt.Sprintf("repos:%s", + user.Login, + ) + val, err := FromContext(c).Get(key) + if err != nil { + return nil + } + return val.([]*model.RepoLite) +} + +// SetRepos adds the listof user repositories to the cache assocaited +// with the current context. +func SetRepos(c context.Context, user *model.User, repos []*model.RepoLite) { + key := fmt.Sprintf("repos:%s", + user.Login, + ) + FromContext(c).Set(key, repos) +} + +// GetSetRepos is a helper function that attempts to get the +// repository list from the cache first. If no data is in the +// cache or it is expired, it will remotely fetch the list of +// repositories and populate the cache. +// func GetSetRepos(c context.Context, user *model.User) ([]*model.RepoLite, error) { +// cache := FromContext(c).Repos() +// repos := FromContext(c).Repos().Get(user) +// if repos != nil { +// return repos, nil +// } +// var err error +// repos, err = remote.FromContext(c).Repos(user) +// if err != nil { +// return nil, err +// } +// cache.Set(user, repos) +// return repos, nil +// } diff --git a/cache/helper_test.go b/cache/helper_test.go new file mode 100644 index 000000000..3dacd9611 --- /dev/null +++ b/cache/helper_test.go @@ -0,0 +1,56 @@ +package cache + +import ( + "testing" + + "github.com/drone/drone/model" + "github.com/franela/goblin" + "github.com/gin-gonic/gin" +) + +func TestHelper(t *testing.T) { + + g := goblin.Goblin(t) + g.Describe("Cache helpers", func() { + + var c *gin.Context + g.BeforeEach(func() { + c = new(gin.Context) + ToContext(c, Default()) + }) + + g.It("Should set and get permissions", func() { + SetPerms(c, fakeUser, fakePerm, "octocat", "Spoon-Knife") + + v := GetPerms(c, fakeUser, "octocat", "Spoon-Knife") + g.Assert(v).Equal(fakePerm) + }) + + g.It("Should return nil if permissions if not found", func() { + v := GetPerms(c, fakeUser, "octocat", "Spoon-Knife") + g.Assert(v == nil).IsTrue() + }) + + g.It("Should set and get repositories", func() { + SetRepos(c, fakeUser, fakeRepos) + + v := GetRepos(c, fakeUser) + g.Assert(v).Equal(fakeRepos) + }) + + g.It("Should return nil if repositories not found", func() { + v := GetRepos(c, fakeUser) + g.Assert(v == nil).IsTrue() + }) + }) +} + +var ( + fakeUser = &model.User{Login: "octocat"} + fakePerm = &model.Perm{true, true, true} + fakeRepos = []*model.RepoLite{ + {Owner: "octocat", Name: "Hello-World"}, + {Owner: "octocat", Name: "hello-world"}, + {Owner: "octocat", Name: "Spoon-Knife"}, + } +) diff --git a/docs/setup/bitbucket.md b/docs/setup/bitbucket.md index 0a4bffa87..e1ab00d50 100644 --- a/docs/setup/bitbucket.md +++ b/docs/setup/bitbucket.md @@ -40,3 +40,10 @@ Please use `http://drone.mycompany.com/authorize` as the Authorization callback * Team Membership:Read * Repositories:Read * Webhooks:Read and Write + +## Known Issues + +This section details known issues and planned features: + +* Pull Request support +* Mercurial support \ No newline at end of file diff --git a/docs/setup/mysql.md b/docs/setup/mysql.md index a7fcde3b6..c6ffbb950 100644 --- a/docs/setup/mysql.md +++ b/docs/setup/mysql.md @@ -6,7 +6,7 @@ Drone comes with support for MySQL as an alternate database engine. To enable My ```bash DATABASE_DRIVER="mysql" -DATABASE_CONFIG="root:pa55word@tcp(localhost:3306)/drone" +DATABASE_CONFIG="root:pa55word@tcp(localhost:3306)/drone?parseTime=true" ``` ## MySQL configuration @@ -29,12 +29,12 @@ The components of this string are: This is an example connection string: ``` -root:pa55word@tcp(localhost:3306)/drone +root:pa55word@tcp(localhost:3306)/drone?parseTime=true ``` ## MySQL options -See the official [driver documentation](https://github.com/go-sql-driver/mysql#parameters) for a full list of driver options. +See the official [driver documentation](https://github.com/go-sql-driver/mysql#parameters) for a full list of driver options. Note that the `parseTime=true` is required. ## MySQL Database diff --git a/drone.go b/drone.go index 7bb3a4db8..571ee1893 100644 --- a/drone.go +++ b/drone.go @@ -6,6 +6,7 @@ import ( "github.com/drone/drone/engine" "github.com/drone/drone/remote" "github.com/drone/drone/router" + "github.com/drone/drone/router/middleware/cache" "github.com/drone/drone/router/middleware/context" "github.com/drone/drone/router/middleware/header" "github.com/drone/drone/shared/database" @@ -49,6 +50,7 @@ func main() { server_.Run( router.Load( header.Version(build), + cache.Default(), context.SetDatabase(database_), context.SetRemote(remote_), context.SetEngine(engine_), diff --git a/router/middleware/cache/cache.go b/router/middleware/cache/cache.go index ed41ee3bf..72fa0279e 100644 --- a/router/middleware/cache/cache.go +++ b/router/middleware/cache/cache.go @@ -1,49 +1,14 @@ package cache import ( - "time" - - "github.com/hashicorp/golang-lru" + "github.com/drone/drone/cache" + "github.com/gin-gonic/gin" ) -// single instance of a thread-safe lru cache -var cache *lru.Cache - -func init() { - var err error - cache, err = lru.New(2048) - if err != nil { - panic(err) +func Default() gin.HandlerFunc { + cc := cache.Default() + return func(c *gin.Context) { + cache.ToContext(c, cc) + c.Next() } } - -// item is a simple wrapper around a cacheable object -// that tracks the ttl for item expiration in the cache. -type item struct { - value interface{} - ttl time.Time -} - -// set adds the key value pair to the cache with the -// specified ttl expiration. -func set(key string, value interface{}, ttl int64) { - ttlv := time.Now().Add(time.Duration(ttl) * time.Second) - cache.Add(key, &item{value, ttlv}) -} - -// get gets the value from the cache for the given key. -// if the value does not exist, a nil value is returned. -// if the value exists, but is expired, the value is returned -// with a bool flag set to true. -func get(key string) (interface{}, bool) { - v, ok := cache.Get(key) - if !ok { - return nil, false - } - vv := v.(*item) - expired := vv.ttl.Before(time.Now()) - if expired { - cache.Remove(key) - } - return vv.value, expired -} diff --git a/router/middleware/cache/cache_test.go b/router/middleware/cache/cache_test.go deleted file mode 100644 index b426c78b4..000000000 --- a/router/middleware/cache/cache_test.go +++ /dev/null @@ -1,40 +0,0 @@ -package cache - -import ( - "testing" - - "github.com/franela/goblin" -) - -func TestCache(t *testing.T) { - - g := goblin.Goblin(t) - g.Describe("Cache", func() { - - g.BeforeEach(func() { - cache.Purge() - }) - - g.It("should set and get item", func() { - set("foo", "bar", 1000) - val, expired := get("foo") - g.Assert(val).Equal("bar") - g.Assert(expired).Equal(false) - }) - - g.It("should return nil when item not found", func() { - val, expired := get("foo") - g.Assert(val == nil).IsTrue() - g.Assert(expired).Equal(false) - }) - - g.It("should get expired item and purge", func() { - set("foo", "bar", -900) - val, expired := get("foo") - g.Assert(val).Equal("bar") - g.Assert(expired).Equal(true) - val, _ = get("foo") - g.Assert(val == nil).IsTrue() - }) - }) -} diff --git a/router/middleware/cache/perms.go b/router/middleware/cache/perms.go index 486932a45..323b952e4 100644 --- a/router/middleware/cache/perms.go +++ b/router/middleware/cache/perms.go @@ -1,8 +1,7 @@ package cache import ( - "fmt" - + "github.com/drone/drone/cache" "github.com/drone/drone/model" "github.com/gin-gonic/gin" ) @@ -24,16 +23,14 @@ func Perms(c *gin.Context) { return } - key := fmt.Sprintf("perm/%s/%s/%s", - user.(*model.User).Login, - owner, - name, - ) - // if the item already exists in the cache // we can continue the middleware chain and // exit afterwards. - v, _ := get(key) + v := cache.GetPerms(c, + user.(*model.User), + owner, + name, + ) if v != nil { c.Set("perm", v) c.Next() @@ -47,6 +44,11 @@ func Perms(c *gin.Context) { perm, ok := c.Get("perm") if ok { - set(key, perm, 86400) // 24 hours + cache.SetPerms(c, + user.(*model.User), + perm.(*model.Perm), + owner, + name, + ) } } diff --git a/router/middleware/cache/perms_test.go b/router/middleware/cache/perms_test.go index 2115b9f55..1345500fd 100644 --- a/router/middleware/cache/perms_test.go +++ b/router/middleware/cache/perms_test.go @@ -3,6 +3,7 @@ package cache import ( "testing" + "github.com/drone/drone/cache" "github.com/drone/drone/model" "github.com/franela/goblin" "github.com/gin-gonic/gin" @@ -13,12 +14,13 @@ func TestPermCache(t *testing.T) { g := goblin.Goblin(t) g.Describe("Perm Cache", func() { + var c *gin.Context g.BeforeEach(func() { - cache.Purge() + c = new(gin.Context) + cache.ToContext(c, cache.Default()) }) g.It("should skip when no user session", func() { - c := &gin.Context{} c.Params = gin.Params{ gin.Param{Key: "owner", Value: "octocat"}, gin.Param{Key: "name", Value: "hello-world"}, @@ -31,13 +33,12 @@ func TestPermCache(t *testing.T) { }) g.It("should get perms from cache", func() { - c := &gin.Context{} c.Params = gin.Params{ gin.Param{Key: "owner", Value: "octocat"}, gin.Param{Key: "name", Value: "hello-world"}, } c.Set("user", fakeUser) - set("perm/octocat/octocat/hello-world", fakePerm, 999) + cache.SetPerms(c, fakeUser, fakePerm, "octocat", "hello-world") Perms(c) diff --git a/router/middleware/cache/repos.go b/router/middleware/cache/repos.go index f4b0fc60e..69ade5b1a 100644 --- a/router/middleware/cache/repos.go +++ b/router/middleware/cache/repos.go @@ -1,8 +1,7 @@ package cache import ( - "fmt" - + "github.com/drone/drone/cache" "github.com/drone/drone/model" "github.com/gin-gonic/gin" ) @@ -18,14 +17,10 @@ func Repos(c *gin.Context) { return } - key := fmt.Sprintf("repos/%s", - user.(*model.User).Login, - ) - // if the item already exists in the cache // we can continue the middleware chain and // exit afterwards. - v, _ := get(key) + v := cache.GetRepos(c, user.(*model.User)) if v != nil { c.Set("repos", v) c.Next() @@ -39,6 +34,9 @@ func Repos(c *gin.Context) { repos, ok := c.Get("repos") if ok { - set(key, repos, 86400) // 24 hours + cache.SetRepos(c, + user.(*model.User), + repos.([]*model.RepoLite), + ) } } diff --git a/router/middleware/cache/repos_test.go b/router/middleware/cache/repos_test.go index ca7b9860a..4971362a4 100644 --- a/router/middleware/cache/repos_test.go +++ b/router/middleware/cache/repos_test.go @@ -3,6 +3,7 @@ package cache import ( "testing" + "github.com/drone/drone/cache" "github.com/drone/drone/model" "github.com/franela/goblin" "github.com/gin-gonic/gin" @@ -13,13 +14,13 @@ func TestReposCache(t *testing.T) { g := goblin.Goblin(t) g.Describe("Repo List Cache", func() { + var c *gin.Context g.BeforeEach(func() { - cache.Purge() + c = new(gin.Context) + cache.ToContext(c, cache.Default()) }) g.It("should skip when no user session", func() { - c := &gin.Context{} - Perms(c) _, ok := c.Get("perm") @@ -27,9 +28,8 @@ func TestReposCache(t *testing.T) { }) g.It("should get repos from cache", func() { - c := &gin.Context{} c.Set("user", fakeUser) - set("repos/octocat", fakeRepos, 999) + cache.SetRepos(c, fakeUser, fakeRepos) Repos(c)