From b473457ddff28789fee1eeb6704491b6aa3525e3 Mon Sep 17 00:00:00 2001 From: Kavindu Dodanduwa Date: Thu, 18 Apr 2024 11:59:53 -0700 Subject: [PATCH] feat: move json logic operator registration to resolver (#1291) ## This PR Attempts to isolate Resolver creation and related logic into a single component. Previously, custom Json logic operator registration was done at flagd using `WithEvaluator` option . But this adds confusion when Resolver needs to be reused in another component. Further the option was merely adding the custom operator to json logic through its global methods and did not perform anything on the evaluator . Given that Resolver is responsible for flag resolving (core logic of the flag evaluations), I have deprecated `WithEvaluator` option and added custom json logic registration as part of the resolver constructor. I have not removed `JSONEvaluatorOption` option so we have flexibility to support any future option (ex:-option to have a customized `IResolver` implemetnation ) Signed-off-by: Kavindu Dodanduwa --- core/pkg/evaluator/fractional_test.go | 18 ++-------- core/pkg/evaluator/json.go | 8 +++++ core/pkg/evaluator/legacy_fractional_test.go | 9 +---- core/pkg/evaluator/semver_test.go | 9 +---- core/pkg/evaluator/string_comparison.go | 2 +- core/pkg/evaluator/string_comparison_test.go | 21 +++-------- flagd/pkg/runtime/from_config.go | 37 +++----------------- flagd/pkg/runtime/from_config_test.go | 16 --------- 8 files changed, 21 insertions(+), 99 deletions(-) delete mode 100644 flagd/pkg/runtime/from_config_test.go diff --git a/core/pkg/evaluator/fractional_test.go b/core/pkg/evaluator/fractional_test.go index 807ba0a8..e228a0af 100644 --- a/core/pkg/evaluator/fractional_test.go +++ b/core/pkg/evaluator/fractional_test.go @@ -394,14 +394,7 @@ func TestFractionalEvaluation(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - store.NewFlags(), - WithEvaluator( - FractionEvaluationName, - NewFractional(log).Evaluate, - ), - ) + je := NewJSON(log, store.NewFlags()) je.store.Flags = tt.flags.Flags value, variant, reason, _, err := resolve[string](ctx, reqID, tt.flagKey, tt.context, je.evaluateVariant) @@ -530,14 +523,7 @@ func BenchmarkFractionalEvaluation(b *testing.B) { for name, tt := range tests { b.Run(name, func(b *testing.B) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - &store.Flags{Flags: tt.flags.Flags}, - WithEvaluator( - FractionEvaluationName, - NewFractional(log).Evaluate, - ), - ) + je := NewJSON(log, &store.Flags{Flags: tt.flags.Flags}) for i := 0; i < b.N; i++ { value, variant, reason, _, err := resolve[string]( ctx, reqID, tt.flagKey, tt.context, je.evaluateVariant) diff --git a/core/pkg/evaluator/json.go b/core/pkg/evaluator/json.go index 566f9994..c4bfa9f6 100644 --- a/core/pkg/evaluator/json.go +++ b/core/pkg/evaluator/json.go @@ -55,6 +55,7 @@ type flagdProperties struct { type variantEvaluator func(context.Context, string, string, map[string]any) ( variant string, variants map[string]interface{}, reason string, metadata map[string]interface{}, error error) +// Deprecated - this will be remove in the next release func WithEvaluator(name string, evalFunc func(interface{}, interface{}) interface{}) JSONEvaluatorOption { return func(_ *JSON) { jsonlogic.AddOperator(name, evalFunc) @@ -145,6 +146,13 @@ type Resolver struct { } func NewResolver(store store.IStore, logger *logger.Logger, jsonEvalTracer trace.Tracer) Resolver { + // register supported json logic custom operator implementations + jsonlogic.AddOperator(FractionEvaluationName, NewFractional(logger).Evaluate) + jsonlogic.AddOperator(StartsWithEvaluationName, NewStringComparisonEvaluator(logger).StartsWithEvaluation) + jsonlogic.AddOperator(EndsWithEvaluationName, NewStringComparisonEvaluator(logger).EndsWithEvaluation) + jsonlogic.AddOperator(SemVerEvaluationName, NewSemVerComparison(logger).SemVerEvaluation) + jsonlogic.AddOperator(LegacyFractionEvaluationName, NewLegacyFractional(logger).LegacyFractionalEvaluation) + return Resolver{store: store, Logger: logger, tracer: jsonEvalTracer} } diff --git a/core/pkg/evaluator/legacy_fractional_test.go b/core/pkg/evaluator/legacy_fractional_test.go index f544ccaa..04c4792b 100644 --- a/core/pkg/evaluator/legacy_fractional_test.go +++ b/core/pkg/evaluator/legacy_fractional_test.go @@ -272,14 +272,7 @@ func TestLegacyFractionalEvaluation(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - store.NewFlags(), - WithEvaluator( - "fractionalEvaluation", - NewLegacyFractional(log).LegacyFractionalEvaluation, - ), - ) + je := NewJSON(log, store.NewFlags()) je.store.Flags = tt.flags.Flags value, variant, reason, _, err := resolve[string](ctx, reqID, tt.flagKey, tt.context, je.evaluateVariant) diff --git a/core/pkg/evaluator/semver_test.go b/core/pkg/evaluator/semver_test.go index a6717c75..557072b9 100644 --- a/core/pkg/evaluator/semver_test.go +++ b/core/pkg/evaluator/semver_test.go @@ -701,14 +701,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - store.NewFlags(), - WithEvaluator( - SemVerEvaluationName, - NewSemVerComparison(log).SemVerEvaluation, - ), - ) + je := NewJSON(log, store.NewFlags()) je.store.Flags = tt.flags.Flags value, variant, reason, _, err := resolve[string](ctx, reqID, tt.flagKey, tt.context, je.evaluateVariant) diff --git a/core/pkg/evaluator/string_comparison.go b/core/pkg/evaluator/string_comparison.go index a6e49f27..d4a0d6b7 100644 --- a/core/pkg/evaluator/string_comparison.go +++ b/core/pkg/evaluator/string_comparison.go @@ -68,7 +68,7 @@ func (sce *StringComparisonEvaluator) StartsWithEvaluation(values, _ interface{} // // Note that the 'ends_with' evaluation rule must contain exactly two items, which both resolve to a // string value -func (sce StringComparisonEvaluator) EndsWithEvaluation(values, _ interface{}) interface{} { +func (sce *StringComparisonEvaluator) EndsWithEvaluation(values, _ interface{}) interface{} { propertyValue, target, err := parseStringComparisonEvaluationData(values) if err != nil { sce.Logger.Error(fmt.Sprintf("parse ends_with evaluation data: %v", err)) diff --git a/core/pkg/evaluator/string_comparison_test.go b/core/pkg/evaluator/string_comparison_test.go index 4d49d2aa..75a67865 100644 --- a/core/pkg/evaluator/string_comparison_test.go +++ b/core/pkg/evaluator/string_comparison_test.go @@ -2,6 +2,7 @@ package evaluator import ( "context" + "errors" "fmt" "testing" @@ -184,14 +185,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - store.NewFlags(), - WithEvaluator( - StartsWithEvaluationName, - NewStringComparisonEvaluator(log).StartsWithEvaluation, - ), - ) + je := NewJSON(log, store.NewFlags()) je.store.Flags = tt.flags.Flags value, variant, reason, _, err := resolve[string](ctx, reqID, tt.flagKey, tt.context, je.evaluateVariant) @@ -208,7 +202,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { t.Errorf("expected reason '%s', got '%s'", tt.expectedReason, reason) } - if err != tt.expectedError { + if !errors.Is(err, tt.expectedError) { t.Errorf("expected err '%v', got '%v'", tt.expectedError, err) } }) @@ -388,14 +382,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { for name, tt := range tests { t.Run(name, func(t *testing.T) { log := logger.NewLogger(nil, false) - je := NewJSON( - log, - store.NewFlags(), - WithEvaluator( - EndsWithEvaluationName, - NewStringComparisonEvaluator(log).EndsWithEvaluation, - ), - ) + je := NewJSON(log, store.NewFlags()) je.store.Flags = tt.flags.Flags diff --git a/flagd/pkg/runtime/from_config.go b/flagd/pkg/runtime/from_config.go index 54ca04e4..28fe4307 100644 --- a/flagd/pkg/runtime/from_config.go +++ b/flagd/pkg/runtime/from_config.go @@ -74,18 +74,18 @@ func FromConfig(logger *logger.Logger, version string, config Config) (*Runtime, } // derive evaluator - evaluator := setupJSONEvaluator(logger, s) + jsonEvaluator := evaluator.NewJSON(logger, s) // derive services // connect service connectService := flageval.NewConnectService( logger.WithFields(zap.String("component", "service")), - evaluator, + jsonEvaluator, recorder) // ofrep service - ofrepService, err := ofrep.NewOfrepService(evaluator, config.CORS, ofrep.SvcConfiguration{ + ofrepService, err := ofrep.NewOfrepService(jsonEvaluator, config.CORS, ofrep.SvcConfiguration{ Logger: logger.WithFields(zap.String("component", "OFREPService")), Port: config.OfrepServicePort, }) @@ -118,7 +118,7 @@ func FromConfig(logger *logger.Logger, version string, config Config) (*Runtime, return &Runtime{ Logger: logger.WithFields(zap.String("component", "runtime")), - Evaluator: evaluator, + Evaluator: jsonEvaluator, FlagSync: flagSyncService, OfrepService: ofrepService, Service: connectService, @@ -136,35 +136,6 @@ func FromConfig(logger *logger.Logger, version string, config Config) (*Runtime, }, nil } -func setupJSONEvaluator(logger *logger.Logger, s *store.Flags) *evaluator.JSON { - evaluator := evaluator.NewJSON( - logger, - s, - evaluator.WithEvaluator( - evaluator.FractionEvaluationName, - evaluator.NewFractional(logger).Evaluate, - ), - evaluator.WithEvaluator( - evaluator.StartsWithEvaluationName, - evaluator.NewStringComparisonEvaluator(logger).StartsWithEvaluation, - ), - evaluator.WithEvaluator( - evaluator.EndsWithEvaluationName, - evaluator.NewStringComparisonEvaluator(logger).EndsWithEvaluation, - ), - evaluator.WithEvaluator( - evaluator.SemVerEvaluationName, - evaluator.NewSemVerComparison(logger).SemVerEvaluation, - ), - // deprecated: will be removed before v1! - evaluator.WithEvaluator( - evaluator.LegacyFractionEvaluationName, - evaluator.NewLegacyFractional(logger).LegacyFractionalEvaluation, - ), - ) - return evaluator -} - // syncProvidersFromConfig is a helper to build ISync implementations from SourceConfig func syncProvidersFromConfig(logger *logger.Logger, sources []sync.SourceConfig) ([]sync.ISync, error) { builder := syncbuilder.NewSyncBuilder() diff --git a/flagd/pkg/runtime/from_config_test.go b/flagd/pkg/runtime/from_config_test.go deleted file mode 100644 index e2ad2da3..00000000 --- a/flagd/pkg/runtime/from_config_test.go +++ /dev/null @@ -1,16 +0,0 @@ -package runtime - -import ( - "testing" - - "github.com/open-feature/flagd/core/pkg/logger" - "github.com/open-feature/flagd/core/pkg/store" - "github.com/stretchr/testify/require" -) - -func Test_setupJSONEvaluator(t *testing.T) { - lg := logger.NewLogger(nil, false) - - je := setupJSONEvaluator(lg, store.NewFlags()) - require.NotNil(t, je) -}