From 94d7697d08a07cede4a548ef998792d00f8954a0 Mon Sep 17 00:00:00 2001 From: James Milligan <75740990+james-milligan@users.noreply.github.com> Date: Mon, 3 Oct 2022 16:20:50 +0100 Subject: [PATCH] fix: updated merge functionality (#182) Signed-off-by: James-Milligan - 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 --- Makefile | 1 + pkg/eval/ievaluator.go | 2 +- pkg/eval/json_evaluator.go | 5 +- pkg/eval/json_evaluator_model.go | 10 +++- pkg/eval/json_evaluator_test.go | 54 ++++++++++++------- .../mock/ievaluator.go} | 14 ++--- pkg/runtime/runtime.go | 2 +- pkg/service/connect_service_test.go | 23 ++++---- pkg/sync/filepath_sync.go | 4 ++ pkg/sync/http_sync.go | 4 ++ pkg/sync/isync.go | 1 + pkg/sync/kubernetes/kubernetes_sync.go | 4 ++ 12 files changed, 81 insertions(+), 43 deletions(-) rename pkg/{service/ievaluator_mock_test.go => eval/mock/ievaluator.go} (93%) diff --git a/Makefile b/Makefile index 735d35138..a625da405 100644 --- a/Makefile +++ b/Makefile @@ -36,3 +36,4 @@ install-mockgen: go install github.com/golang/mock/mockgen@v1.6.0 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 index 3be3cfb7b..e2e8111d4 100644 --- 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 index 1346b4b8f..9c2d1e90d 100644 --- 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 index f51d4d3a2..2a98eb07d 100644 --- 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 index a267b6766..a5ba1b83c 100644 --- 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 b/pkg/eval/mock/ievaluator.go similarity index 93% rename from pkg/service/ievaluator_mock_test.go rename to pkg/eval/mock/ievaluator.go index cb805260f..34664d432 100644 --- a/pkg/service/ievaluator_mock_test.go +++ b/pkg/eval/mock/ievaluator.go @@ -1,8 +1,8 @@ // Code generated by MockGen. DO NOT EDIT. -// Source: ievaluator.go +// Source: pkg/eval/ievaluator.go -// Package service_test is a generated GoMock package. -package service_test +// Package evalmock is a generated GoMock package. +package evalmock import ( reflect "reflect" @@ -135,15 +135,15 @@ func (mr *MockIEvaluatorMockRecorder) ResolveStringValue(flagKey, context interf } // SetState mocks base method. -func (m *MockIEvaluator) SetState(state string) error { +func (m *MockIEvaluator) SetState(source, state string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetState", state) + ret := m.ctrl.Call(m, "SetState", source, state) ret0, _ := ret[0].(error) return ret0 } // SetState indicates an expected call of SetState. -func (mr *MockIEvaluatorMockRecorder) SetState(state interface{}) *gomock.Call { +func (mr *MockIEvaluatorMockRecorder) SetState(source, state interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetState", reflect.TypeOf((*MockIEvaluator)(nil).SetState), state) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetState", reflect.TypeOf((*MockIEvaluator)(nil).SetState), source, state) } diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 55ac5b607..6692d1520 100644 --- 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 index a4d3f500b..da15ba782 100644 --- 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 index de0a2a95d..c02f3c670 100644 --- 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 index 48243661c..1f9dd8f19 100644 --- 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 index 19b38ee9c..5a478c9df 100644 --- 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 } diff --git a/pkg/sync/kubernetes/kubernetes_sync.go b/pkg/sync/kubernetes/kubernetes_sync.go index 027c8abc5..c690797ce 100644 --- a/pkg/sync/kubernetes/kubernetes_sync.go +++ b/pkg/sync/kubernetes/kubernetes_sync.go @@ -28,6 +28,10 @@ type Sync struct { client *featureflagconfiguration.FFCClient } +func (k *Sync) Source() string { + return k.ProviderArgs[featureFlagConfigurationName] +} + func (k *Sync) Fetch(ctx context.Context) (string, error) { if k.ProviderArgs[featureFlagConfigurationName] == "" { k.Logger.Info("No target feature flag configuration set")