Skip to content

Commit

Permalink
chore: do not save update metadata
Browse files Browse the repository at this point in the history
The metadata returned is not valid and should not be stored for cache
  • Loading branch information
adriantam committed Aug 4, 2023
1 parent 63bf484 commit 67c6b3b
Show file tree
Hide file tree
Showing 6 changed files with 40 additions and 23 deletions.
38 changes: 31 additions & 7 deletions internal/graph/cached_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ import (
)

const (
defaultMaxCacheSize = 10000
defaultCacheTTL = 10 * time.Second
defaultMaxCacheSize = 10000
defaultCacheTTL = 10 * time.Second
defaultResolveNodeLimit = 25
)

var (
Expand All @@ -33,12 +34,34 @@ var (
})
)

// CachedResolveCheckResponse is very similar to ResolveCheckResponse except we
// do not store the ResolutionData
type CachedResolveCheckResponse struct {
Allowed bool
}

func (c *CachedResolveCheckResponse) convertToResolveCheckResponse() *ResolveCheckResponse {
return &ResolveCheckResponse{
Allowed: c.Allowed,
ResolutionMetadata: &ResolutionMetadata{
Depth: defaultResolveNodeLimit,
DatastoreQueryCount: 0,
},
}
}

func newCachedResolveCheckResponse(r *ResolveCheckResponse) *CachedResolveCheckResponse {
return &CachedResolveCheckResponse{
Allowed: r.Allowed,
}
}

// CachedCheckResolver implements the CheckResolver interface in way that attempts to resolve
// Check sub-problems via prior computations before delegating the request to some underlying
// CheckResolver.
type CachedCheckResolver struct {
delegate CheckResolver
cache *ccache.Cache[*ResolveCheckResponse]
cache *ccache.Cache[*CachedResolveCheckResponse]
maxCacheSize int64
cacheTTL time.Duration
logger logger.Logger
Expand Down Expand Up @@ -66,7 +89,7 @@ func WithCacheTTL(ttl time.Duration) CachedCheckResolverOpt {
}

// WithExistingCache sets the cache to the provided cache
func WithExistingCache(cache *ccache.Cache[*ResolveCheckResponse]) CachedCheckResolverOpt {
func WithExistingCache(cache *ccache.Cache[*CachedResolveCheckResponse]) CachedCheckResolverOpt {
return func(ccr *CachedCheckResolver) {
ccr.cache = cache
}
Expand All @@ -83,6 +106,7 @@ func WithLogger(logger logger.Logger) CachedCheckResolverOpt {
// but before delegating the query to the delegate a cache-key lookup is made to see if the Check sub-problem
// has already recently been computed. If the Check sub-problem is in the cache, then the response is returned
// immediately and no re-computation is necessary.
// NOTE: the ResolveCheck's resolution data will be set as the default values as we actually did no database lookup
func NewCachedCheckResolver(delegate CheckResolver, opts ...CachedCheckResolverOpt) *CachedCheckResolver {
checker := &CachedCheckResolver{
delegate: delegate,
Expand All @@ -98,7 +122,7 @@ func NewCachedCheckResolver(delegate CheckResolver, opts ...CachedCheckResolverO
if checker.cache == nil {
// this means there were no cache supplied
checker.cache = ccache.New(
ccache.Configure[*ResolveCheckResponse]().MaxSize(checker.maxCacheSize),
ccache.Configure[*CachedResolveCheckResponse]().MaxSize(checker.maxCacheSize),
)
}

Expand All @@ -121,15 +145,15 @@ func (c *CachedCheckResolver) ResolveCheck(
cachedResp := c.cache.Get(cacheKey)
if cachedResp != nil && !cachedResp.Expired() {
checkCacheHitCounter.Inc()
return cachedResp.Value(), nil
return cachedResp.Value().convertToResolveCheckResponse(), nil
}

resp, err := c.delegate.ResolveCheck(ctx, req)
if err != nil {
return nil, err
}

c.cache.Set(cacheKey, resp, c.cacheTTL)
c.cache.Set(cacheKey, newCachedResolveCheckResponse(resp), c.cacheTTL)
return resp, nil
}

Expand Down
6 changes: 3 additions & 3 deletions internal/graph/cached_resolver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func TestResolveCheckFromCache(t *testing.T) {
test.setTestExpectations(newResolver, test.req)
dut2 := NewCachedCheckResolver(newResolver, WithExistingCache(dut.cache))
actualResult, err := dut2.ResolveCheck(ctx, test.req)
require.Equal(t, result, actualResult)
require.Equal(t, result.Allowed, actualResult.Allowed)
require.Nil(t, err)

})
Expand Down Expand Up @@ -211,13 +211,13 @@ func TestResolveCheckExpired(t *testing.T) {
// expect first call to result in actual resolve call
dut := NewCachedCheckResolver(initialMockResolver, WithCacheTTL(1*time.Microsecond))
actualResult, err := dut.ResolveCheck(ctx, req)
require.Equal(t, result, actualResult)
require.Equal(t, result.Allowed, actualResult.Allowed)
require.Equal(t, nil, err)

// subsequent call would have cache timeout and result in new ResolveCheck
time.Sleep(5 * time.Microsecond)

actualResult, err = dut.ResolveCheck(ctx, req)
require.Equal(t, result, actualResult)
require.Equal(t, result.Allowed, actualResult.Allowed)
require.Nil(t, err)
}
9 changes: 1 addition & 8 deletions internal/graph/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -410,14 +410,7 @@ func (c *LocalChecker) SetDelegate(delegate CheckResolver) {
// was constructed with.
func (c *LocalChecker) dispatch(ctx context.Context, req *ResolveCheckRequest) CheckHandlerFunc {
return func(ctx context.Context) (*ResolveCheckResponse, error) {
resp, err := c.delegate.ResolveCheck(ctx, req)
if err != nil {
return nil, err
}

return &ResolveCheckResponse{
Allowed: resp.Allowed,
}, nil
return c.delegate.ResolveCheck(ctx, req)
}
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/server/commands/list_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type ListObjectsQuery struct {

// configurations for caching results
checkQueryCacheTTL time.Duration
checkCache *ccache.Cache[*graph.ResolveCheckResponse] // checkCache has to be shared across requests
checkCache *ccache.Cache[*graph.CachedResolveCheckResponse] // checkCache has to be shared across requests
}

type ListObjectsQueryOption func(d *ListObjectsQuery)
Expand Down Expand Up @@ -113,7 +113,7 @@ func WithCheckQueryCacheTTL(ttl time.Duration) ListObjectsQueryOption {
}

// WithCheckCache sets the cache used for resolve check results
func WithCheckCache(checkCache *ccache.Cache[*graph.ResolveCheckResponse]) ListObjectsQueryOption {
func WithCheckCache(checkCache *ccache.Cache[*graph.CachedResolveCheckResponse]) ListObjectsQueryOption {
return func(d *ListObjectsQuery) {
d.checkCache = checkCache
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ type Server struct {
checkQueryCacheEnabled bool
checkQueryCacheLimit uint32
checkQueryCacheTTL time.Duration
checkCache *ccache.Cache[*graph.ResolveCheckResponse] // checkCache has to be shared across requests
checkCache *ccache.Cache[*graph.CachedResolveCheckResponse] // checkCache has to be shared across requests
}

type OpenFGAServiceV1Option func(s *Server)
Expand Down Expand Up @@ -232,7 +232,7 @@ func NewServerWithOpts(opts ...OpenFGAServiceV1Option) (*Server, error) {

if slices.Contains(s.experimentals, ExperimentalCheckQueryCache) && s.checkQueryCacheEnabled {
s.checkCache = ccache.New(
ccache.Configure[*graph.ResolveCheckResponse]().MaxSize(int64(s.checkQueryCacheLimit)),
ccache.Configure[*graph.CachedResolveCheckResponse]().MaxSize(int64(s.checkQueryCacheLimit)),
)
}

Check warning on line 237 in pkg/server/server.go

View check run for this annotation

Codecov / codecov/patch

pkg/server/server.go#L234-L237

Added lines #L234 - L237 were not covered by tests

Expand Down
2 changes: 1 addition & 1 deletion pkg/server/test/list_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func TestListObjectsRespectsMaxResults(t *testing.T, ds storage.OpenFGADatastore

if test.useCheckCache {
checkCache := ccache.New(
ccache.Configure[*graph.ResolveCheckResponse]().MaxSize(100),
ccache.Configure[*graph.CachedResolveCheckResponse]().MaxSize(100),
)
opts = append(opts,
commands.WithCheckCache(checkCache),
Expand Down

0 comments on commit 67c6b3b

Please sign in to comment.