From 95703e456527c43e5f997d360194c64e3b91fad3 Mon Sep 17 00:00:00 2001 From: Ben Toogood Date: Sun, 24 May 2020 20:26:37 +0100 Subject: [PATCH] Fixes and improved test coverage --- auth/service/service.go | 38 +++++++-------------------- util/wrapper/wrapper.go | 15 ++++++++--- util/wrapper/wrapper_test.go | 51 ++++++++++++++++++++++++++++-------- 3 files changed, 61 insertions(+), 43 deletions(-) diff --git a/auth/service/service.go b/auth/service/service.go index fb5cb755..eacc2596 100644 --- a/auth/service/service.go +++ b/auth/service/service.go @@ -5,7 +5,6 @@ import ( "fmt" "sort" "strings" - "sync" "time" "github.com/micro/go-micro/v2/auth" @@ -23,9 +22,6 @@ type svc struct { auth pb.AuthService rule pb.RulesService jwt token.Provider - - rules []*pb.Rule - sync.Mutex } func (s *svc) String() string { @@ -53,8 +49,6 @@ func (s *svc) Init(opts ...auth.Option) { } func (s *svc) Options() auth.Options { - s.Lock() - defer s.Unlock() return s.options } @@ -110,9 +104,6 @@ func (s *svc) Revoke(role string, res *auth.Resource) error { // Verify an account has access to a resource func (s *svc) Verify(acc *auth.Account, res *auth.Resource) error { - // load the rules if none are loaded - s.loadRulesIfEmpty() - // set the namespace on the resource if len(res.Namespace) == 0 { res.Namespace = s.Options().Namespace @@ -230,11 +221,14 @@ func accessForRule(rule *pb.Rule, acc *auth.Account, res *auth.Resource) pb.Acce // listRules gets all the rules from the store which match the filters. // filters are namespace, type, name and then endpoint. func (s *svc) listRules(filters ...string) []*pb.Rule { - s.Lock() - defer s.Unlock() + // load rules using the client cache + allRules, err := s.loadRules() + if err != nil { + return []*pb.Rule{} + } var rules []*pb.Rule - for _, r := range s.rules { + for _, r := range allRules { if len(filters) > 0 && r.Resource.Namespace != filters[0] { continue } @@ -260,27 +254,15 @@ func (s *svc) listRules(filters ...string) []*pb.Rule { } // loadRules retrieves the rules from the auth service -func (s *svc) loadRules() { +func (s *svc) loadRules() ([]*pb.Rule, error) { rsp, err := s.rule.List(context.TODO(), &pb.ListRequest{}, client.WithCache(time.Minute)) - s.Lock() - defer s.Unlock() if err != nil { - log.Errorf("Error listing rules: %v", err) - return + log.Debugf("Error listing rules: %v", err) + return nil, err } - s.rules = rsp.Rules -} - -func (s *svc) loadRulesIfEmpty() { - s.Lock() - rules := s.rules - s.Unlock() - - if len(rules) == 0 { - s.loadRules() - } + return rsp.Rules, nil } func serializeToken(t *pb.Token) *auth.Token { diff --git a/util/wrapper/wrapper.go b/util/wrapper/wrapper.go index d261940d..ba40d7fd 100644 --- a/util/wrapper/wrapper.go +++ b/util/wrapper/wrapper.go @@ -2,6 +2,7 @@ package wrapper import ( "context" + "reflect" "strings" "time" @@ -229,7 +230,7 @@ func AuthHandler(fn func() auth.Auth) server.HandlerWrapper { } type cacheWrapper struct { - cache func() *client.Cache + cacheFn func() *client.Cache client.Client } @@ -243,7 +244,7 @@ func (c *cacheWrapper) Call(ctx context.Context, req client.Request, rsp interfa } // if the client doesn't have a cacbe setup don't continue - cache := c.cache() + cache := c.cacheFn() if cache == nil { return c.Client.Call(ctx, req, rsp, opts...) } @@ -253,9 +254,15 @@ func (c *cacheWrapper) Call(ctx context.Context, req client.Request, rsp interfa return c.Client.Call(ctx, req, rsp, opts...) } - // check to see if there is a response + // if the response is nil don't call the cache since we can't assign the response + if rsp == nil { + return c.Client.Call(ctx, req, rsp, opts...) + } + + // check to see if there is a response cached, if there is assign it if r, ok := cache.Get(ctx, &req); ok { - rsp = r + val := reflect.ValueOf(rsp).Elem() + val.Set(reflect.ValueOf(r).Elem()) return nil } diff --git a/util/wrapper/wrapper_test.go b/util/wrapper/wrapper_test.go index c3454779..d9760fba 100644 --- a/util/wrapper/wrapper_test.go +++ b/util/wrapper/wrapper_test.go @@ -2,6 +2,7 @@ package wrapper import ( "context" + "reflect" "testing" "time" @@ -56,25 +57,33 @@ func TestWrapper(t *testing.T) { type testClient struct { callCount int callRsp interface{} - cache *client.Cache client.Client } func (c *testClient) Call(ctx context.Context, req client.Request, rsp interface{}, opts ...client.CallOption) error { c.callCount++ - rsp = c.callRsp + + if c.callRsp != nil { + val := reflect.ValueOf(rsp).Elem() + val.Set(reflect.ValueOf(c.callRsp).Elem()) + } + return nil } -func (c *testClient) Options() client.Options { - return client.Options{Cache: c.cache} +type testRsp struct { + value string } + func TestCacheWrapper(t *testing.T) { req := client.NewRequest("go.micro.service.foo", "Foo.Bar", nil) t.Run("NilCache", func(t *testing.T) { cli := new(testClient) - w := CacheClient(cli) + + w := CacheClient(func() *client.Cache { + return nil + }, cli) // perfroming two requests should increment the call count by two indicating the cache wasn't // used even though the WithCache option was passed. @@ -88,7 +97,11 @@ func TestCacheWrapper(t *testing.T) { t.Run("OptionNotSet", func(t *testing.T) { cli := new(testClient) - w := CacheClient(cli) + cache := client.NewCache() + + w := CacheClient(func() *client.Cache { + return cache + }, cli) // perfroming two requests should increment the call count by two since we didn't pass the WithCache // option to Call. @@ -101,13 +114,21 @@ func TestCacheWrapper(t *testing.T) { }) t.Run("OptionSet", func(t *testing.T) { - cli := &testClient{callRsp: "foobar", cache: client.NewCache()} - w := CacheClient(cli) + val := "foo" + cli := &testClient{callRsp: &testRsp{value: val}} + cache := client.NewCache() + + w := CacheClient(func() *client.Cache { + return cache + }, cli) // perfroming two requests should increment the call count by once since the second request should - // have used the cache - err1 := w.Call(context.TODO(), req, nil, client.WithCache(time.Minute)) - err2 := w.Call(context.TODO(), req, nil, client.WithCache(time.Minute)) + // have used the cache. The correct value should be set on both responses and no errors should + // be returned. + rsp1 := &testRsp{} + rsp2 := &testRsp{} + err1 := w.Call(context.TODO(), req, rsp1, client.WithCache(time.Minute)) + err2 := w.Call(context.TODO(), req, rsp2, client.WithCache(time.Minute)) if err1 != nil { t.Errorf("Expected nil error, got %v", err1) @@ -115,6 +136,14 @@ func TestCacheWrapper(t *testing.T) { if err2 != nil { t.Errorf("Expected nil error, got %v", err2) } + + if rsp1.value != val { + t.Errorf("Expected %v to be assigned to the value, got %v", val, rsp1.value) + } + if rsp2.value != val { + t.Errorf("Expected %v to be assigned to the value, got %v", val, rsp2.value) + } + if cli.callCount != 1 { t.Errorf("Expected the client to be called 1 time, was actually called %v time(s)", cli.callCount) }