From a43882c1209b21501803ed9d4db9f95b0b690f11 Mon Sep 17 00:00:00 2001 From: Kavindu Dodanduwa Date: Thu, 20 Jun 2024 13:10:35 -0700 Subject: [PATCH] add support to emit errors from bulk evaluator Signed-off-by: Kavindu Dodanduwa --- core/pkg/evaluator/fractional.go | 2 +- core/pkg/evaluator/ievaluator.go | 2 +- core/pkg/evaluator/json.go | 46 ++---- core/pkg/evaluator/json_test.go | 21 ++- core/pkg/evaluator/mock/ievaluator.go | 147 +++++++++++++++++- core/pkg/service/ofrep/models.go | 9 +- core/pkg/store/flags.go | 13 +- .../service/flag-evaluation/flag_evaluator.go | 8 +- .../flag-evaluation/flag_evaluator_test.go | 2 +- .../flag-evaluation/flag_evaluator_v2.go | 7 +- .../flag-evaluation/flag_evaluator_v2_test.go | 33 ++-- .../service/flag-evaluation/ofrep/handler.go | 17 +- .../flag-evaluation/ofrep/handler_test.go | 16 +- .../ofrep/ofrep_service_test.go | 4 +- .../pkg/service/flag-sync/sync-multiplexer.go | 6 +- 15 files changed, 264 insertions(+), 69 deletions(-) diff --git a/core/pkg/evaluator/fractional.go b/core/pkg/evaluator/fractional.go index 085b9d0fc..3bb36ccc2 100644 --- a/core/pkg/evaluator/fractional.go +++ b/core/pkg/evaluator/fractional.go @@ -27,7 +27,7 @@ func NewFractional(logger *logger.Logger) *Fractional { func (fe *Fractional) Evaluate(values, data any) any { valueToDistribute, feDistributions, err := parseFractionalEvaluationData(values, data) if err != nil { - fe.Logger.Error(fmt.Sprintf("parse fractional evaluation data: %v", err)) + fe.Logger.Warn(fmt.Sprintf("parse fractional evaluation data: %v", err)) return nil } diff --git a/core/pkg/evaluator/ievaluator.go b/core/pkg/evaluator/ievaluator.go index 03aebe19f..bbdd063b9 100644 --- a/core/pkg/evaluator/ievaluator.go +++ b/core/pkg/evaluator/ievaluator.go @@ -77,5 +77,5 @@ type IResolver interface { ResolveAllValues( ctx context.Context, reqID string, - context map[string]any) (values []AnyValue) + context map[string]any) (values []AnyValue, err error) } diff --git a/core/pkg/evaluator/json.go b/core/pkg/evaluator/json.go index 419161a46..655eaf75c 100644 --- a/core/pkg/evaluator/json.go +++ b/core/pkg/evaluator/json.go @@ -156,17 +156,22 @@ func NewResolver(store store.IStore, logger *logger.Logger, jsonEvalTracer trace return Resolver{store: store, Logger: logger, tracer: jsonEvalTracer} } -func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) []AnyValue { +func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) ([]AnyValue, error) { _, span := je.tracer.Start(ctx, "resolveAll") defer span.End() + var err error + allFlags, err := je.store.GetAll(ctx) + if err != nil { + return nil, fmt.Errorf("error retreiving flags from the store: %w", err) + } + values := []AnyValue{} var value interface{} var variant string var reason string var metadata map[string]interface{} - var err error - allFlags := je.store.GetAll(ctx) + for flagKey, flag := range allFlags { if flag.State == Disabled { // ignore evaluation of disabled flag @@ -176,44 +181,21 @@ func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context defaultValue := flag.Variants[flag.DefaultVariant] switch defaultValue.(type) { case bool: - value, variant, reason, metadata, err = resolve[bool]( - ctx, - reqID, - flagKey, - context, - je.evaluateVariant, - ) + value, variant, reason, metadata, err = resolve[bool](ctx, reqID, flagKey, context, je.evaluateVariant) case string: - value, variant, reason, metadata, err = resolve[string]( - ctx, - reqID, - flagKey, - context, - je.evaluateVariant, - ) + value, variant, reason, metadata, err = resolve[string](ctx, reqID, flagKey, context, je.evaluateVariant) case float64: - value, variant, reason, metadata, err = resolve[float64]( - ctx, - reqID, - flagKey, - context, - je.evaluateVariant, - ) + value, variant, reason, metadata, err = resolve[float64](ctx, reqID, flagKey, context, je.evaluateVariant) case map[string]any: - value, variant, reason, metadata, err = resolve[map[string]any]( - ctx, - reqID, - flagKey, - context, - je.evaluateVariant, - ) + value, variant, reason, metadata, err = resolve[map[string]any](ctx, reqID, flagKey, context, je.evaluateVariant) } if err != nil { je.Logger.ErrorWithID(reqID, fmt.Sprintf("bulk evaluation: key: %s returned error: %s", flagKey, err.Error())) } values = append(values, NewAnyValue(value, variant, reason, flagKey, metadata, err)) } - return values + + return values, nil } func (je *Resolver) ResolveBooleanValue( diff --git a/core/pkg/evaluator/json_test.go b/core/pkg/evaluator/json_test.go index ae5a94f56..8954a518e 100644 --- a/core/pkg/evaluator/json_test.go +++ b/core/pkg/evaluator/json_test.go @@ -335,7 +335,11 @@ func TestResolveAllValues(t *testing.T) { } const reqID = "default" for _, test := range tests { - vals := evaluator.ResolveAllValues(context.TODO(), reqID, test.context) + vals, err := evaluator.ResolveAllValues(context.TODO(), reqID, test.context) + if err != nil { + t.Error("error from resolver", err) + } + for _, val := range vals { // disabled flag must be ignored from bulk evaluation if val.FlagKey == DisabledFlag { @@ -1234,21 +1238,30 @@ func TestFlagStateSafeForConcurrentReadWrites(t *testing.T) { "Add_ResolveAllValues": { dataSyncType: sync.ADD, flagResolution: func(evaluator *evaluator.JSON) error { - evaluator.ResolveAllValues(context.TODO(), "", nil) + _, err := evaluator.ResolveAllValues(context.TODO(), "", nil) + if err != nil { + return err + } return nil }, }, "Update_ResolveAllValues": { dataSyncType: sync.UPDATE, flagResolution: func(evaluator *evaluator.JSON) error { - evaluator.ResolveAllValues(context.TODO(), "", nil) + _, err := evaluator.ResolveAllValues(context.TODO(), "", nil) + if err != nil { + return err + } return nil }, }, "Delete_ResolveAllValues": { dataSyncType: sync.DELETE, flagResolution: func(evaluator *evaluator.JSON) error { - evaluator.ResolveAllValues(context.TODO(), "", nil) + _, err := evaluator.ResolveAllValues(context.TODO(), "", nil) + if err != nil { + return err + } return nil }, }, diff --git a/core/pkg/evaluator/mock/ievaluator.go b/core/pkg/evaluator/mock/ievaluator.go index c06108d18..96536d8e3 100644 --- a/core/pkg/evaluator/mock/ievaluator.go +++ b/core/pkg/evaluator/mock/ievaluator.go @@ -57,11 +57,12 @@ func (mr *MockIEvaluatorMockRecorder) GetState() *gomock.Call { } // ResolveAllValues mocks base method. -func (m *MockIEvaluator) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) []evaluator.AnyValue { +func (m *MockIEvaluator) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) ([]evaluator.AnyValue, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "ResolveAllValues", ctx, reqID, context) ret0, _ := ret[0].([]evaluator.AnyValue) - return ret0 + ret1, _ := ret[1].(error) + return ret0, ret1 } // ResolveAllValues indicates an expected call of ResolveAllValues. @@ -189,3 +190,145 @@ func (mr *MockIEvaluatorMockRecorder) SetState(payload any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetState", reflect.TypeOf((*MockIEvaluator)(nil).SetState), payload) } + +// MockIResolver is a mock of IResolver interface. +type MockIResolver struct { + ctrl *gomock.Controller + recorder *MockIResolverMockRecorder +} + +// MockIResolverMockRecorder is the mock recorder for MockIResolver. +type MockIResolverMockRecorder struct { + mock *MockIResolver +} + +// NewMockIResolver creates a new mock instance. +func NewMockIResolver(ctrl *gomock.Controller) *MockIResolver { + mock := &MockIResolver{ctrl: ctrl} + mock.recorder = &MockIResolverMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockIResolver) EXPECT() *MockIResolverMockRecorder { + return m.recorder +} + +// ResolveAllValues mocks base method. +func (m *MockIResolver) ResolveAllValues(ctx context.Context, reqID string, context map[string]any) ([]evaluator.AnyValue, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveAllValues", ctx, reqID, context) + ret0, _ := ret[0].([]evaluator.AnyValue) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ResolveAllValues indicates an expected call of ResolveAllValues. +func (mr *MockIResolverMockRecorder) ResolveAllValues(ctx, reqID, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveAllValues", reflect.TypeOf((*MockIResolver)(nil).ResolveAllValues), ctx, reqID, context) +} + +// ResolveAsAnyValue mocks base method. +func (m *MockIResolver) ResolveAsAnyValue(ctx context.Context, reqID, flagKey string, context map[string]any) evaluator.AnyValue { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveAsAnyValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(evaluator.AnyValue) + return ret0 +} + +// ResolveAsAnyValue indicates an expected call of ResolveAsAnyValue. +func (mr *MockIResolverMockRecorder) ResolveAsAnyValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveAsAnyValue", reflect.TypeOf((*MockIResolver)(nil).ResolveAsAnyValue), ctx, reqID, flagKey, context) +} + +// ResolveBooleanValue mocks base method. +func (m *MockIResolver) ResolveBooleanValue(ctx context.Context, reqID, flagKey string, context map[string]any) (bool, string, string, map[string]any, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveBooleanValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(string) + ret3, _ := ret[3].(map[string]any) + ret4, _ := ret[4].(error) + return ret0, ret1, ret2, ret3, ret4 +} + +// ResolveBooleanValue indicates an expected call of ResolveBooleanValue. +func (mr *MockIResolverMockRecorder) ResolveBooleanValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveBooleanValue", reflect.TypeOf((*MockIResolver)(nil).ResolveBooleanValue), ctx, reqID, flagKey, context) +} + +// ResolveFloatValue mocks base method. +func (m *MockIResolver) ResolveFloatValue(ctx context.Context, reqID, flagKey string, context map[string]any) (float64, string, string, map[string]any, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveFloatValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(float64) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(string) + ret3, _ := ret[3].(map[string]any) + ret4, _ := ret[4].(error) + return ret0, ret1, ret2, ret3, ret4 +} + +// ResolveFloatValue indicates an expected call of ResolveFloatValue. +func (mr *MockIResolverMockRecorder) ResolveFloatValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveFloatValue", reflect.TypeOf((*MockIResolver)(nil).ResolveFloatValue), ctx, reqID, flagKey, context) +} + +// ResolveIntValue mocks base method. +func (m *MockIResolver) ResolveIntValue(ctx context.Context, reqID, flagKey string, context map[string]any) (int64, string, string, map[string]any, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveIntValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(string) + ret3, _ := ret[3].(map[string]any) + ret4, _ := ret[4].(error) + return ret0, ret1, ret2, ret3, ret4 +} + +// ResolveIntValue indicates an expected call of ResolveIntValue. +func (mr *MockIResolverMockRecorder) ResolveIntValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveIntValue", reflect.TypeOf((*MockIResolver)(nil).ResolveIntValue), ctx, reqID, flagKey, context) +} + +// ResolveObjectValue mocks base method. +func (m *MockIResolver) ResolveObjectValue(ctx context.Context, reqID, flagKey string, context map[string]any) (map[string]any, string, string, map[string]any, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveObjectValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(map[string]any) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(string) + ret3, _ := ret[3].(map[string]any) + ret4, _ := ret[4].(error) + return ret0, ret1, ret2, ret3, ret4 +} + +// ResolveObjectValue indicates an expected call of ResolveObjectValue. +func (mr *MockIResolverMockRecorder) ResolveObjectValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveObjectValue", reflect.TypeOf((*MockIResolver)(nil).ResolveObjectValue), ctx, reqID, flagKey, context) +} + +// ResolveStringValue mocks base method. +func (m *MockIResolver) ResolveStringValue(ctx context.Context, reqID, flagKey string, context map[string]any) (string, string, string, map[string]any, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ResolveStringValue", ctx, reqID, flagKey, context) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(string) + ret2, _ := ret[2].(string) + ret3, _ := ret[3].(map[string]any) + ret4, _ := ret[4].(error) + return ret0, ret1, ret2, ret3, ret4 +} + +// ResolveStringValue indicates an expected call of ResolveStringValue. +func (mr *MockIResolverMockRecorder) ResolveStringValue(ctx, reqID, flagKey, context any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ResolveStringValue", reflect.TypeOf((*MockIResolver)(nil).ResolveStringValue), ctx, reqID, flagKey, context) +} diff --git a/core/pkg/service/ofrep/models.go b/core/pkg/service/ofrep/models.go index 2cc0c736c..eac10b00b 100644 --- a/core/pkg/service/ofrep/models.go +++ b/core/pkg/service/ofrep/models.go @@ -73,13 +73,20 @@ func ContextErrorResponseFrom(key string) EvaluationError { } } -func BulkEvaluationContextErrorFrom() BulkEvaluationError { +func BulkEvaluationContextError() BulkEvaluationError { return BulkEvaluationError{ ErrorCode: model.InvalidContextCode, ErrorDetails: "Provider context is not valid", } } +func BulkEvaluationContextErrorFrom(code string, details string) BulkEvaluationError { + return BulkEvaluationError{ + ErrorCode: code, + ErrorDetails: details, + } +} + func EvaluationErrorResponseFrom(result evaluator.AnyValue) (int, EvaluationError) { payload := EvaluationError{ Key: result.FlagKey, diff --git a/core/pkg/store/flags.go b/core/pkg/store/flags.go index 10409ffd9..626e2940f 100644 --- a/core/pkg/store/flags.go +++ b/core/pkg/store/flags.go @@ -12,7 +12,7 @@ import ( ) type IStore interface { - GetAll(ctx context.Context) map[string]model.Flag + GetAll(ctx context.Context) (map[string]model.Flag, error) Get(ctx context.Context, key string) (model.Flag, bool) SelectorForFlag(ctx context.Context, flag model.Flag) string } @@ -90,7 +90,7 @@ func (f *Flags) String() (string, error) { } // GetAll returns a copy of the store's state (copy in order to be concurrency safe) -func (f *Flags) GetAll(_ context.Context) map[string]model.Flag { +func (f *Flags) GetAll(_ context.Context) (map[string]model.Flag, error) { f.mx.RLock() defer f.mx.RUnlock() state := make(map[string]model.Flag, len(f.Flags)) @@ -99,7 +99,7 @@ func (f *Flags) GetAll(_ context.Context) map[string]model.Flag { state[key] = flag } - return state + return state, nil } // Add new flags from source. @@ -187,7 +187,12 @@ func (f *Flags) DeleteFlags(logger *logger.Logger, source string, flags map[stri notifications := map[string]interface{}{} if len(flags) == 0 { - allFlags := f.GetAll(ctx) + allFlags, err := f.GetAll(ctx) + if err != nil { + logger.Error(fmt.Sprintf("error while retrieving flags from the store: %v", err)) + return notifications + } + for key, flag := range allFlags { if flag.Source != source { continue diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator.go b/flagd/pkg/service/flag-evaluation/flag_evaluator.go index 8d1ed8def..6db6ed4da 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator.go @@ -69,7 +69,13 @@ func (s *OldFlagEvaluationService) ResolveAll( if e := req.Msg.GetContext(); e != nil { evalCtx = e.AsMap() } - values := s.eval.ResolveAllValues(sCtx, reqID, evalCtx) + + values, err := s.eval.ResolveAllValues(sCtx, reqID, evalCtx) + if err != nil { + s.logger.WarnWithID(reqID, fmt.Sprintf("error resolving all flags: %v", err)) + return nil, fmt.Errorf("error resolving flags. Tracking ID: %s", reqID) + } + span.SetAttributes(attribute.Int("feature_flag.count", len(values))) for _, value := range values { // register the impression and reason for each flag evaluated diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator_test.go b/flagd/pkg/service/flag-evaluation/flag_evaluator_test.go index 52c3d36f1..b0b744dae 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator_test.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator_test.go @@ -118,7 +118,7 @@ func TestConnectService_ResolveAll(t *testing.T) { t.Run(name, func(t *testing.T) { eval := mock.NewMockIEvaluator(ctrl) eval.EXPECT().ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()).Return( - tt.evalRes, + tt.evalRes, nil, ).AnyTimes() metrics, exp := getMetricReader() s := NewOldFlagEvaluationService( diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go index b6eea6324..277976f91 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2.go @@ -68,7 +68,12 @@ func (s *FlagEvaluationService) ResolveAll( evalCtx = e.AsMap() } - values := s.eval.ResolveAllValues(sCtx, reqID, evalCtx) + values, err := s.eval.ResolveAllValues(sCtx, reqID, evalCtx) + if err != nil { + s.logger.WarnWithID(reqID, fmt.Sprintf("error resolving all flags: %v", err)) + return nil, fmt.Errorf("error resolving flags. Tracking ID: %s", reqID) + } + span.SetAttributes(attribute.Int("feature_flag.count", len(values))) for _, value := range values { // register the impression and reason for each flag evaluated diff --git a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2_test.go b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2_test.go index ed7e1bcb6..e77b67fa9 100644 --- a/flagd/pkg/service/flag-evaluation/flag_evaluator_v2_test.go +++ b/flagd/pkg/service/flag-evaluation/flag_evaluator_v2_test.go @@ -21,7 +21,8 @@ func TestConnectServiceV2_ResolveAll(t *testing.T) { tests := map[string]struct { req *evalV1.ResolveAllRequest evalRes []evaluator.AnyValue - wantErr error + evalErr error + wantErr bool wantRes *evalV1.ResolveAllResponse }{ "happy-path": { @@ -52,7 +53,6 @@ func TestConnectServiceV2_ResolveAll(t *testing.T) { FlagKey: "object", }, }, - wantErr: nil, wantRes: &evalV1.ResolveAllResponse{ Flags: map[string]*evalV1.AnyFlag{ "bool": { @@ -76,26 +76,37 @@ func TestConnectServiceV2_ResolveAll(t *testing.T) { }, }, }, + "resolver error": { + req: &evalV1.ResolveAllRequest{}, + evalRes: []evaluator.AnyValue{}, + evalErr: errors.New("some error from internal evaluator"), + wantErr: true, + }, } ctrl := gomock.NewController(t) for name, tt := range tests { t.Run(name, func(t *testing.T) { + // given eval := mock.NewMockIEvaluator(ctrl) eval.EXPECT().ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()).Return( - tt.evalRes, + tt.evalRes, tt.evalErr, ).AnyTimes() + metrics, exp := getMetricReader() - s := NewFlagEvaluationService( - logger.NewLogger(nil, false), - eval, - &eventingConfiguration{}, - metrics, - ) + s := NewFlagEvaluationService(logger.NewLogger(nil, false), eval, &eventingConfiguration{}, metrics) + + // when got, err := s.ResolveAll(context.Background(), connect.NewRequest(tt.req)) - if err != nil && !errors.Is(err, tt.wantErr) { - t.Errorf("ConnectService.ResolveAll() error = %v, wantErr %v", err.Error(), tt.wantErr.Error()) + + // then + if tt.wantErr { + if err == nil { + t.Error("expected error but git none") + } + return } + var data metricdata.ResourceMetrics err = exp.Collect(context.TODO(), &data) require.Nil(t, err) diff --git a/flagd/pkg/service/flag-evaluation/ofrep/handler.go b/flagd/pkg/service/flag-evaluation/ofrep/handler.go index f58ae91e2..f2bee4907 100644 --- a/flagd/pkg/service/flag-evaluation/ofrep/handler.go +++ b/flagd/pkg/service/flag-evaluation/ofrep/handler.go @@ -8,6 +8,7 @@ import ( "github.com/gorilla/mux" "github.com/open-feature/flagd/core/pkg/evaluator" "github.com/open-feature/flagd/core/pkg/logger" + "github.com/open-feature/flagd/core/pkg/model" "github.com/open-feature/flagd/core/pkg/service/ofrep" "github.com/rs/xid" ) @@ -37,6 +38,7 @@ func NewOfrepHandler(logger *logger.Logger, evaluator evaluator.IEvaluator) http func (h *handler) HandleFlagEvaluation(w http.ResponseWriter, r *http.Request) { requestID := xid.New().String() + defer h.Logger.ClearFields(requestID) // obtain flag key vars := mux.Vars(r) @@ -66,16 +68,25 @@ func (h *handler) HandleFlagEvaluation(w http.ResponseWriter, r *http.Request) { func (h *handler) HandleBulkEvaluation(w http.ResponseWriter, r *http.Request) { requestID := xid.New().String() + defer h.Logger.ClearFields(requestID) request, err := extractOfrepRequest(r) if err != nil { - h.writeJSONToResponse(http.StatusBadRequest, ofrep.BulkEvaluationContextErrorFrom(), w) + h.writeJSONToResponse(http.StatusBadRequest, ofrep.BulkEvaluationContextError(), w) return } context := flagdContext(h.Logger, requestID, request) - evaluations := h.evaluator.ResolveAllValues(r.Context(), requestID, context) - h.writeJSONToResponse(http.StatusOK, ofrep.BulkEvaluationResponseFrom(evaluations), w) + evaluations, err := h.evaluator.ResolveAllValues(r.Context(), requestID, context) + if err != nil { + h.Logger.WarnWithID(requestID, fmt.Sprintf("error from resolver: %v", err)) + + res := ofrep.BulkEvaluationContextErrorFrom(model.GeneralErrorCode, + fmt.Sprintf("Bulk evaluation failed. Tracking ID: %s", requestID)) + h.writeJSONToResponse(http.StatusInternalServerError, res, w) + } else { + h.writeJSONToResponse(http.StatusOK, ofrep.BulkEvaluationResponseFrom(evaluations), w) + } } func (h *handler) writeJSONToResponse(status int, payload interface{}, w http.ResponseWriter) { diff --git a/flagd/pkg/service/flag-evaluation/ofrep/handler_test.go b/flagd/pkg/service/flag-evaluation/ofrep/handler_test.go index 12f323e36..b48e4d87a 100644 --- a/flagd/pkg/service/flag-evaluation/ofrep/handler_test.go +++ b/flagd/pkg/service/flag-evaluation/ofrep/handler_test.go @@ -155,6 +155,7 @@ func Test_handler_HandleBulkEvaluation(t *testing.T) { method string input *bytes.Reader mockAnyResponse []evaluator.AnyValue + mockAnyError error expectedStatus int }{ @@ -179,6 +180,14 @@ func Test_handler_HandleBulkEvaluation(t *testing.T) { mockAnyResponse: []evaluator.AnyValue{genericErrorValue, flagNotFoundValue}, expectedStatus: http.StatusOK, }, + { + name: "handles internal errors and yield 500", + method: "http.MethodPost", + input: bytes.NewReader([]byte{}), + mockAnyResponse: []evaluator.AnyValue{}, + mockAnyError: errors.New("some internal error from evaluator"), + expectedStatus: http.StatusInternalServerError, + }, { name: "valid context payload", method: http.MethodPost, @@ -197,11 +206,8 @@ func Test_handler_HandleBulkEvaluation(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { eval := mock.NewMockIEvaluator(gomock.NewController(t)) - if test.mockAnyResponse != nil { - eval.EXPECT(). - ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()). - Return(test.mockAnyResponse) - } + eval.EXPECT().ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()). + Return(test.mockAnyResponse, test.mockAnyError).MinTimes(0) h := handler{Logger: log, evaluator: eval} diff --git a/flagd/pkg/service/flag-evaluation/ofrep/ofrep_service_test.go b/flagd/pkg/service/flag-evaluation/ofrep/ofrep_service_test.go index c05c09711..37479e25b 100644 --- a/flagd/pkg/service/flag-evaluation/ofrep/ofrep_service_test.go +++ b/flagd/pkg/service/flag-evaluation/ofrep/ofrep_service_test.go @@ -19,7 +19,9 @@ func Test_OfrepServiceStartStop(t *testing.T) { port := 18282 eval := mock.NewMockIEvaluator(gomock.NewController(t)) - eval.EXPECT().ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()).Return([]evaluator.AnyValue{}) + eval.EXPECT().ResolveAllValues(gomock.Any(), gomock.Any(), gomock.Any()). + Return([]evaluator.AnyValue{}, nil) + cfg := SvcConfiguration{ Logger: logger.NewLogger(nil, false), Port: uint16(port), diff --git a/flagd/pkg/service/flag-sync/sync-multiplexer.go b/flagd/pkg/service/flag-sync/sync-multiplexer.go index 6dcf7a07f..4d680712c 100644 --- a/flagd/pkg/service/flag-sync/sync-multiplexer.go +++ b/flagd/pkg/service/flag-sync/sync-multiplexer.go @@ -163,7 +163,11 @@ func (r *Multiplexer) SourcesAsMetadata() string { func (r *Multiplexer) reFill() error { clear(r.selectorFlags) - all := r.store.GetAll(context.Background()) + all, err := r.store.GetAll(context.Background()) + if err != nil { + return fmt.Errorf("error retrieving flags from the store: %w", err) + } + bytes, err := json.Marshal(map[string]interface{}{"flags": all}) if err != nil { return fmt.Errorf("error from marshallin: %w", err)