perf: when reading models, skip model validation if the model ID is i… (

#1754)

pkg/typesystem/resolver.go

@@ -9,32 +9,39 @@ import (
 	"github.com/karlseguin/ccache/v3"
 	"github.com/oklog/ulid/v2"
 	openfgav1 "github.com/openfga/api/proto/openfga/v1"
+	"go.opentelemetry.io/otel/attribute"
+	"go.opentelemetry.io/otel/trace"
 	"golang.org/x/sync/singleflight"
 
 	"github.com/openfga/openfga/pkg/storage"
 )
 
+// TODO there is a duplicate cache of models elsewhere: https://github.com/openfga/openfga/issues/1045
+
 const (
 	typesystemCacheTTL = 168 * time.Hour // 7 days.
 )
 
-// TypesystemResolverFunc is a function type that implementations
-// can use to provide lookup and resolution of a Typesystem.
 type TypesystemResolverFunc func(ctx context.Context, storeID, modelID string) (*TypeSystem, error)
 
-// MemoizedTypesystemResolverFunc returns a TypesystemResolverFunc that fetches the provided authorization
-// model (if provided) or looks up the latest authorization model. It then constructs a TypeSystem from
-// the resolved model, and memoizes the type-system resolution. If another lookup of the same model occurs,
-// the earlier constructed TypeSystem will be used.
+// MemoizedTypesystemResolverFunc does several things.
+//
+// If given a model ID: validates the model ID, and tries to fetch it from the cache.
+// If not found in the cache, fetches from the datastore, validates it, stores in cache, and returns it.
 //
-// The memoized resolver function is designed for concurrent use.
+// If not given a model ID: fetches the latest model ID from the datastore, then sees if the model ID is in the cache.
+// If it is, returns it. Else, validates it and returns it.
 func MemoizedTypesystemResolverFunc(datastore storage.AuthorizationModelReadBackend) (TypesystemResolverFunc, func()) {
 	lookupGroup := singleflight.Group{}
 
+	// cache holds models that have already been validated.
 	cache := ccache.New(ccache.Configure[*TypeSystem]())
 
 	return func(ctx context.Context, storeID, modelID string) (*TypeSystem, error) {
-		ctx, span := tracer.Start(ctx, "MemoizedTypesystemResolverFunc")
+		ctx, span := tracer.Start(ctx, "MemoizedTypesystemResolverFunc", trace.WithAttributes(
+			attribute.String("store_id", storeID),
+			attribute.String("authorization_model_id", modelID),
+		))
 		defer span.End()
 
 		var err error
@@ -45,10 +52,10 @@ func MemoizedTypesystemResolverFunc(datastore storage.AuthorizationModelReadBack
 			}
 		}
 
-		var v interface{}
+		var model *openfgav1.AuthorizationModel
 		var key string
 		if modelID == "" {
-			v, err, _ = lookupGroup.Do(fmt.Sprintf("FindLatestAuthorizationModel:%s", storeID), func() (interface{}, error) {
+			v, err, _ := lookupGroup.Do(fmt.Sprintf("FindLatestAuthorizationModel:%s", storeID), func() (interface{}, error) {
 				return datastore.FindLatestAuthorizationModel(ctx, storeID)
 			})
 			if err != nil {
@@ -59,16 +66,18 @@ func MemoizedTypesystemResolverFunc(datastore storage.AuthorizationModelReadBack
 				return nil, fmt.Errorf("failed to FindLatestAuthorizationModel: %w", err)
 			}
 
-			model := v.(*openfgav1.AuthorizationModel)
-			key = fmt.Sprintf("%s/%s", storeID, model.GetId())
-		} else {
-			key = fmt.Sprintf("%s/%s", storeID, modelID)
-			item := cache.Get(key)
-			if item != nil {
-				return item.Value(), nil
-			}
+			model = v.(*openfgav1.AuthorizationModel)
+			modelID = model.GetId()
+		}
 
-			v, err, _ = lookupGroup.Do(fmt.Sprintf("ReadAuthorizationModel:%s/%s", storeID, modelID), func() (interface{}, error) {
+		key = fmt.Sprintf("%s/%s", storeID, modelID)
+		item := cache.Get(key)
+		if item != nil {
+			return item.Value(), nil
+		}
+
+		if model == nil {
+			v, err, _ := lookupGroup.Do(fmt.Sprintf("ReadAuthorizationModel:%s/%s", storeID, modelID), func() (interface{}, error) {
 				return datastore.ReadAuthorizationModel(ctx, storeID, modelID)
 			})
 			if err != nil {
@@ -78,9 +87,9 @@ func MemoizedTypesystemResolverFunc(datastore storage.AuthorizationModelReadBack
 
 				return nil, fmt.Errorf("failed to ReadAuthorizationModel: %w", err)
 			}
-		}
 
-		model := v.(*openfgav1.AuthorizationModel)
+			model = v.(*openfgav1.AuthorizationModel)
+		}
 
 		typesys, err := NewAndValidate(ctx, model)
 		if err != nil {