fix: updated merge functionality (#182)

Signed-off-by: James-Milligan <[email protected]> - flag updates are handled on a per source basis, allowing deletions to be recognised - adds the Source method to the sync provider interface, returning a string
open-feature · Oct 3, 2022 · 94d7697 · 94d7697
1 parent e13caea
commit 94d7697
Show file tree

Hide file tree

Showing 12 changed files with 81 additions and 43 deletions.
diff --git a/Makefile b/Makefile
@@ -36,3 +36,4 @@ install-mockgen:
 	go install github.com/golang/mock/[email protected]
 mockgen: install-mockgen
 	mockgen -source=pkg/sync/http_sync.go -destination=pkg/sync/mock/http.go -package=syncmock
+	mockgen -source=pkg/eval/ievaluator.go -destination=pkg/eval/mock/ievaluator.go -package=evalmock
diff --git a/pkg/eval/ievaluator.go b/pkg/eval/ievaluator.go
@@ -10,7 +10,7 @@ do parsing and validation of the flag state and evaluate flags in response to ha
 */
 type IEvaluator interface {
 	GetState() (string, error)
-	SetState(state string) error
+	SetState(source string, state string) error
 
 	ResolveBooleanValue(
 		flagKey string,

diff --git a/pkg/eval/json_evaluator.go b/pkg/eval/json_evaluator.go
@@ -37,7 +37,7 @@ func (je *JSONEvaluator) GetState() (string, error) {
 	return string(data), nil
 }
 
-func (je *JSONEvaluator) SetState(state string) error {
+func (je *JSONEvaluator) SetState(source string, state string) error {
 	schemaLoader := gojsonschema.NewStringLoader(schema.FlagdDefinitions)
 	flagStringLoader := gojsonschema.NewStringLoader(state)
 	result, err := gojsonschema.Validate(schemaLoader, flagStringLoader)
@@ -64,7 +64,8 @@ func (je *JSONEvaluator) SetState(state string) error {
 	if err := validateDefaultVariants(newFlags); err != nil {
 		return err
 	}
-	je.state = je.state.Merge(newFlags)
+
+	je.state = je.state.Merge(source, newFlags)
 
 	return nil
 }

diff --git a/pkg/eval/json_evaluator_model.go b/pkg/eval/json_evaluator_model.go
@@ -12,12 +12,19 @@ type Evaluators struct {
 	Evaluators map[string]json.RawMessage `json:"$evaluators"`
 }
 
-func (f Flags) Merge(ff Flags) Flags {
+func (f Flags) Merge(source string, ff Flags) Flags {
 	result := Flags{Flags: make(map[string]Flag)}
 	for k, v := range f.Flags {
+		if v.Source == source {
+			if _, ok := ff.Flags[k]; !ok {
+				// flag has been deleted
+				continue
+			}
+		}
 		result.Flags[k] = v
 	}
 	for k, v := range ff.Flags {
+		v.Source = source
 		result.Flags[k] = v
 	}
 	return result
@@ -28,4 +35,5 @@ type Flag struct {
 	DefaultVariant string          `json:"defaultVariant"`
 	Variants       map[string]any  `json:"variants"`
 	Targeting      json.RawMessage `json:"targeting"`
+	Source         string          `json:"source"`
 }
diff --git a/pkg/eval/json_evaluator_test.go b/pkg/eval/json_evaluator_test.go
@@ -280,7 +280,7 @@ var Flags = fmt.Sprintf(`{
 
 func TestGetState_Valid_ContainsFlag(t *testing.T) {
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(ValidFlags)
+	err := evaluator.SetState("", ValidFlags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -302,7 +302,7 @@ func TestSetState_Invalid_Error(t *testing.T) {
 	evaluator := eval.JSONEvaluator{Logger: l}
 
 	// set state with an invalid flag definition
-	err := evaluator.SetState(InvalidFlags)
+	err := evaluator.SetState("", InvalidFlags)
 	if err == nil {
 		t.Fatalf("Expected error")
 	}
@@ -312,7 +312,7 @@ func TestSetState_Valid_NoError(t *testing.T) {
 	evaluator := eval.JSONEvaluator{Logger: l}
 
 	// set state with a valid flag definition
-	err := evaluator.SetState(ValidFlags)
+	err := evaluator.SetState("", ValidFlags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -334,7 +334,7 @@ func TestResolveBooleanValue(t *testing.T) {
 	}
 
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(Flags)
+	err := evaluator.SetState("", Flags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -373,7 +373,7 @@ func TestResolveStringValue(t *testing.T) {
 	}
 
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(Flags)
+	err := evaluator.SetState("", Flags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -413,7 +413,7 @@ func TestResolveFloatValue(t *testing.T) {
 	}
 
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(Flags)
+	err := evaluator.SetState("", Flags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -453,7 +453,7 @@ func TestResolveIntValue(t *testing.T) {
 	}
 
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(Flags)
+	err := evaluator.SetState("", Flags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -493,7 +493,7 @@ func TestResolveObjectValue(t *testing.T) {
 	}
 
 	evaluator := eval.JSONEvaluator{Logger: l}
-	err := evaluator.SetState(Flags)
+	err := evaluator.SetState("", Flags)
 	if err != nil {
 		t.Fatalf("Expected no error")
 	}
@@ -523,10 +523,11 @@ func TestResolveObjectValue(t *testing.T) {
 func TestMergeFlags(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
-		name    string
-		current eval.Flags
-		new     eval.Flags
-		want    eval.Flags
+		name      string
+		current   eval.Flags
+		new       eval.Flags
+		newSource string
+		want      eval.Flags
 	}{
 		{
 			name:    "both nil",
@@ -555,14 +556,26 @@ func TestMergeFlags(t *testing.T) {
 		{
 			name: "extra fields on each",
 			current: eval.Flags{Flags: map[string]eval.Flag{
-				"waka": {DefaultVariant: "off"},
+				"waka": {
+					DefaultVariant: "off",
+					Source:         "1",
+				},
 			}},
 			new: eval.Flags{Flags: map[string]eval.Flag{
-				"paka": {DefaultVariant: "on"},
+				"paka": {
+					DefaultVariant: "on",
+				},
 			}},
+			newSource: "2",
 			want: eval.Flags{Flags: map[string]eval.Flag{
-				"waka": {DefaultVariant: "off"},
-				"paka": {DefaultVariant: "on"},
+				"waka": {
+					DefaultVariant: "off",
+					Source:         "1",
+				},
+				"paka": {
+					DefaultVariant: "on",
+					Source:         "2",
+				},
 			}},
 		},
 		{
@@ -597,8 +610,8 @@ func TestMergeFlags(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			got := tt.current.Merge(tt.new)
-			require.Equal(t, got, tt.want)
+			got := tt.current.Merge(tt.newSource, tt.new)
+			require.Equal(t, tt.want, got)
 		})
 	}
 }
@@ -656,7 +669,7 @@ func TestSetState_DefaultVariantValidation(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			jsonEvaluator := eval.JSONEvaluator{}
 
-			err := jsonEvaluator.SetState(tt.jsonFlags)
+			err := jsonEvaluator.SetState("", tt.jsonFlags)
 
 			if tt.valid && err != nil {
 				t.Error(err)
@@ -714,6 +727,7 @@ func TestState_Evaluator(t *testing.T) {
 						  },
 						  "defaultVariant": "recursive",
 						  "state": "ENABLED",
+						  "source":"",
 						  "targeting": {
 							"if": [
 							  {
@@ -792,7 +806,7 @@ func TestState_Evaluator(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			jsonEvaluator := eval.JSONEvaluator{}
 
-			err := jsonEvaluator.SetState(tt.inputState)
+			err := jsonEvaluator.SetState("", tt.inputState)
 			if err != nil {
 				if !tt.expectedError {
 					t.Error(err)

diff --git a/pkg/service/ievaluator_mock_test.go → pkg/eval/mock/ievaluator.go b/pkg/service/ievaluator_mock_test.go → pkg/eval/mock/ievaluator.go
diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go
@@ -75,7 +75,7 @@ func (r *Runtime) updateState(ctx context.Context, syncr sync.ISync) error {
 	}
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	err = r.Evaluator.SetState(msg)
+	err = r.Evaluator.SetState(syncr.Source(), msg)
 	if err != nil {
 		return fmt.Errorf("set state: %w", err)
 	}

diff --git a/pkg/service/connect_service_test.go b/pkg/service/connect_service_test.go
@@ -10,6 +10,7 @@ import (
 
 	"github.com/bufbuild/connect-go"
 	"github.com/golang/mock/gomock"
+	mock "github.com/open-feature/flagd/pkg/eval/mock"
 	"github.com/open-feature/flagd/pkg/model"
 	service "github.com/open-feature/flagd/pkg/service"
 	log "github.com/sirupsen/logrus"
@@ -75,7 +76,7 @@ func TestConnectService_UnixConnection(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			ctrl := gomock.NewController(t)
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveBooleanValue(tt.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -167,7 +168,7 @@ func TestConnectService_ResolveBoolean(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveBooleanValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -214,7 +215,7 @@ func BenchmarkConnectService_ResolveBoolean(b *testing.B) {
 		},
 	}
 	for name, tt := range tests {
-		eval := NewMockIEvaluator(ctrl)
+		eval := mock.NewMockIEvaluator(ctrl)
 		eval.EXPECT().ResolveBooleanValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 			tt.evalFields.result,
 			tt.evalFields.variant,
@@ -299,7 +300,7 @@ func TestConnectService_ResolveString(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveStringValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -346,7 +347,7 @@ func BenchmarkConnectService_ResolveString(b *testing.B) {
 		},
 	}
 	for name, tt := range tests {
-		eval := NewMockIEvaluator(ctrl)
+		eval := mock.NewMockIEvaluator(ctrl)
 		eval.EXPECT().ResolveStringValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 			tt.evalFields.result,
 			tt.evalFields.variant,
@@ -431,7 +432,7 @@ func TestConnectService_ResolveFloat(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveFloatValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -478,7 +479,7 @@ func BenchmarkConnectService_ResolveFloat(b *testing.B) {
 		},
 	}
 	for name, tt := range tests {
-		eval := NewMockIEvaluator(ctrl)
+		eval := mock.NewMockIEvaluator(ctrl)
 		eval.EXPECT().ResolveFloatValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 			tt.evalFields.result,
 			tt.evalFields.variant,
@@ -563,7 +564,7 @@ func TestConnectService_ResolveInt(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveIntValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -610,7 +611,7 @@ func BenchmarkConnectService_ResolveInt(b *testing.B) {
 		},
 	}
 	for name, tt := range tests {
-		eval := NewMockIEvaluator(ctrl)
+		eval := mock.NewMockIEvaluator(ctrl)
 		eval.EXPECT().ResolveIntValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 			tt.evalFields.result,
 			tt.evalFields.variant,
@@ -699,7 +700,7 @@ func TestConnectService_ResolveObject(t *testing.T) {
 	}
 	for name, tt := range tests {
 		t.Run(name, func(t *testing.T) {
-			eval := NewMockIEvaluator(ctrl)
+			eval := mock.NewMockIEvaluator(ctrl)
 			eval.EXPECT().ResolveObjectValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 				tt.evalFields.result,
 				tt.evalFields.variant,
@@ -756,7 +757,7 @@ func BenchmarkConnectService_ResolveObject(b *testing.B) {
 		},
 	}
 	for name, tt := range tests {
-		eval := NewMockIEvaluator(ctrl)
+		eval := mock.NewMockIEvaluator(ctrl)
 		eval.EXPECT().ResolveObjectValue(tt.functionArgs.req.FlagKey, gomock.Any()).Return(
 			tt.evalFields.result,
 			tt.evalFields.variant,

diff --git a/pkg/sync/filepath_sync.go b/pkg/sync/filepath_sync.go
@@ -16,6 +16,10 @@ type FilePathSync struct {
 	ProviderArgs ProviderArgs
 }
 
+func (fs *FilePathSync) Source() string {
+	return fs.URI
+}
+
 func (fs *FilePathSync) Fetch(_ context.Context) (string, error) {
 	if fs.URI == "" {
 		return "", errors.New("no filepath string set")

diff --git a/pkg/sync/http_sync.go b/pkg/sync/http_sync.go
@@ -33,6 +33,10 @@ type Cron interface {
 	Start()
 }
 
+func (fs *HTTPSync) Source() string {
+	return fs.URI
+}
+
 func (fs *HTTPSync) fetchBodyFromURL(ctx context.Context, url string) ([]byte, error) {
 	req, err := http.NewRequestWithContext(ctx, "GET", url, bytes.NewBuffer(nil))
 	if err != nil {

diff --git a/pkg/sync/isync.go b/pkg/sync/isync.go
@@ -12,4 +12,5 @@ type ProviderArgs map[string]string
 type ISync interface {
 	Fetch(ctx context.Context) (string, error)
 	Notify(ctx context.Context, c chan<- INotify)
+	Source() string
 }