Skip to content

Commit 00afcf4

Browse files
aepfliaepfli
committed
fixup: incoporating feedback from peer review
Signed-off-by: Simon Schrottner <[email protected]>
1 parent bad30c8 commit 00afcf4

File tree

6 files changed

+182
-153
lines changed

6 files changed

+182
-153
lines changed

‎core/pkg/evaluator/ievaluator.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ IEvaluator is an extension of IResolver, allowing storage updates and retrievals
3535
*/
3636
type IEvaluator interface {
3737
GetState() (string, error)
38-
SetState(payload sync.DataSync) (map[string]interface{}, bool, error)
38+
SetState(payload sync.DataSync) (map[string]interface{}, bool, error) // TODO: here we are returning the notifications, should we maybe declare a more specific type? - we ignore this in the runtime :)
3939
IResolver
4040
}
4141

‎core/pkg/evaluator/json.go‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"encoding/json"
77
"errors"
88
"fmt"
9-
"github.com/mitchellh/mapstructure"
109
flagd_definitions "github.com/open-feature/flagd-schemas/json"
1110
"regexp"
1211
"strconv"
@@ -131,7 +130,7 @@ func (je *JSON) GetState() (string, error) {
131130
return s, nil
132131
}
133132

134-
func (je *JSON) SetState(payload sync.DataSync) (map[string]interface{}, bool, error) {
133+
func (je *JSON) SetState(payload sync.DataSync) error {
135134
_, span := je.jsonEvalTracer.Start(
136135
context.Background(),
137136
"flagSync",
@@ -144,18 +143,12 @@ func (je *JSON) SetState(payload sync.DataSync) (map[string]interface{}, bool, e
144143
if err != nil {
145144
span.SetStatus(codes.Error, "flagSync error")
146145
span.RecordError(err)
147-
return nil, false, err
146+
return err
148147
}
149148

150-
var events map[string]interface{}
151-
var reSync bool
152-
153-
events, reSync = je.store.Update(payload.Source, definition.Flags, definition.Metadata)
154-
155-
// Number of events correlates to the number of flags changed through this sync, record it
156-
span.SetAttributes(attribute.Int("feature_flag.change_count", len(events)))
149+
je.store.Update(payload.Source, definition.Flags, definition.Metadata)
157150

158-
return events, reSync, nil
151+
return nil
159152
}
160153

161154
// Resolver implementation for flagd flags. This resolver should be kept reusable, hence must interact with interfaces.
@@ -497,7 +490,7 @@ func (je *JSON) configToFlagDefinition(config string, definition *Definition) er
497490
switch v := intermediateConfig.Flags.(type) {
498491
case map[string]interface{}: // Handle ValidFlags format
499492
for k, e := range v {
500-
flag, err := convertToModelFlag(e.(map[string]interface{}))
493+
flag, err := convertToModelFlag(e)
501494
if err != nil {
502495
return fmt.Errorf("failed to process flag for key %s: %w", k, err)
503496
}
@@ -507,7 +500,7 @@ func (je *JSON) configToFlagDefinition(config string, definition *Definition) er
507500

508501
case []interface{}: // Handle ValidMapFlags format
509502
for _, value := range v {
510-
flag, err := convertToModelFlag(value.(map[string]interface{}))
503+
flag, err := convertToModelFlag(value)
511504
if err != nil {
512505
return fmt.Errorf("failed to process flag: %w", err)
513506
}
@@ -522,21 +515,28 @@ func (je *JSON) configToFlagDefinition(config string, definition *Definition) er
522515
}
523516

524517
// Refactored Helper Function to Convert interface{} to model.Flag
525-
func convertToModelFlag(data map[string]interface{}) (model.Flag, error) {
518+
func convertToModelFlag(data interface{}) (model.Flag, error) {
519+
526520
type Flag struct {
527521
Key string `json:"key,omitempty"`
528522
model.Flag
529523
}
530524

531525
var flag Flag
526+
jsonBytes, err := json.Marshal(data)
532527

533-
// Use mapstructure to decode the data into the Flag struct
534-
if err := mapstructure.Decode(data, &flag); err != nil {
535-
return flag.Flag, fmt.Errorf("failed to decode flag data: %w", err)
528+
if err != nil {
529+
return flag.Flag, fmt.Errorf("failed to marshal flag data: %w", err)
530+
}
531+
532+
if err := json.Unmarshal(jsonBytes, &flag); err != nil {
533+
return flag.Flag, fmt.Errorf("failed to unmarshal flag data: %w", err)
536534
}
537535

538-
flag.Flag.Key = flag.Key
539-
return flag.Flag, nil
536+
exportFlag := flag.Flag
537+
exportFlag.Key = flag.Key
538+
539+
return exportFlag, nil
540540
}
541541

542542
// validateDefaultVariants returns an error if any of the default variants aren't valid

‎core/pkg/evaluator/json_test.go‎

Lines changed: 26 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ var Flags = fmt.Sprintf(`{
400400

401401
func TestGetState_Valid_ContainsFlag(t *testing.T) {
402402
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
403-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: ValidFlags, Source: "testSource"})
403+
err := evaluator.SetState(sync.DataSync{FlagData: ValidFlags, Source: "testSource"})
404404
if err != nil {
405405
t.Fatalf("Expected no error")
406406
}
@@ -419,7 +419,7 @@ func TestGetState_Valid_ContainsFlag(t *testing.T) {
419419
}
420420
func TestGetState_ValidMap_ContainsFlag(t *testing.T) {
421421
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
422-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: ValidMapFlags, Source: "testSource"})
422+
err := evaluator.SetState(sync.DataSync{FlagData: ValidMapFlags, Source: "testSource"})
423423
if err != nil {
424424
t.Fatalf("Expected no error")
425425
}
@@ -441,7 +441,7 @@ func TestSetState_Invalid_Error(t *testing.T) {
441441
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
442442

443443
// set state with an invalid flag definition
444-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: InvalidFlags, Source: "testSource"})
444+
err := evaluator.SetState(sync.DataSync{FlagData: InvalidFlags, Source: "testSource"})
445445
if err != nil {
446446
t.Fatalf("unexpected error")
447447
}
@@ -451,15 +451,15 @@ func TestSetState_Valid_NoError(t *testing.T) {
451451
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
452452

453453
// set state with a valid flag definition
454-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: ValidFlags, Source: "testSource"})
454+
err := evaluator.SetState(sync.DataSync{FlagData: ValidFlags, Source: "testSource"})
455455
if err != nil {
456456
t.Fatalf("expected no error")
457457
}
458458
}
459459

460460
func TestResolveAllValues(t *testing.T) {
461461
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
462-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
462+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
463463
if err != nil {
464464
t.Fatalf("expected no error")
465465
}
@@ -528,7 +528,7 @@ func TestResolveBooleanValue(t *testing.T) {
528528
}
529529
const reqID = "default"
530530
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
531-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
531+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
532532
if err != nil {
533533
t.Fatalf("expected no error")
534534
}
@@ -563,7 +563,7 @@ func BenchmarkResolveBooleanValue(b *testing.B) {
563563
}
564564

565565
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
566-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
566+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
567567
if err != nil {
568568
b.Fatalf("expected no error")
569569
}
@@ -603,7 +603,7 @@ func TestResolveStringValue(t *testing.T) {
603603
}
604604
const reqID = "default"
605605
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
606-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
606+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
607607
if err != nil {
608608
t.Fatalf("expected no error")
609609
}
@@ -639,7 +639,7 @@ func BenchmarkResolveStringValue(b *testing.B) {
639639
}
640640

641641
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
642-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
642+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
643643
if err != nil {
644644
b.Fatalf("expected no error")
645645
}
@@ -679,7 +679,7 @@ func TestResolveFloatValue(t *testing.T) {
679679
}
680680
const reqID = "default"
681681
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
682-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
682+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
683683
if err != nil {
684684
t.Fatalf("expected no error")
685685
}
@@ -715,7 +715,7 @@ func BenchmarkResolveFloatValue(b *testing.B) {
715715
}
716716

717717
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
718-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
718+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
719719
if err != nil {
720720
b.Fatalf("expected no error")
721721
}
@@ -755,7 +755,7 @@ func TestResolveIntValue(t *testing.T) {
755755
}
756756
const reqID = "default"
757757
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
758-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
758+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
759759
if err != nil {
760760
t.Fatalf("expected no error")
761761
}
@@ -791,7 +791,7 @@ func BenchmarkResolveIntValue(b *testing.B) {
791791
}
792792

793793
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
794-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
794+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
795795
if err != nil {
796796
b.Fatalf("expected no error")
797797
}
@@ -831,7 +831,7 @@ func TestResolveObjectValue(t *testing.T) {
831831
}
832832
const reqID = "default"
833833
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
834-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
834+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
835835
if err != nil {
836836
t.Fatalf("expected no error")
837837
}
@@ -870,7 +870,7 @@ func BenchmarkResolveObjectValue(b *testing.B) {
870870
}
871871

872872
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
873-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
873+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
874874
if err != nil {
875875
b.Fatalf("expected no error")
876876
}
@@ -915,7 +915,7 @@ func TestResolveAsAnyValue(t *testing.T) {
915915
}
916916

917917
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
918-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
918+
err := evaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
919919
if err != nil {
920920
t.Fatalf("expected no error")
921921
}
@@ -951,7 +951,7 @@ func TestResolve_DefaultVariant(t *testing.T) {
951951
for _, test := range tests {
952952
t.Run("", func(t *testing.T) {
953953
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
954-
_, _, err := evaluator.SetState(sync.DataSync{FlagData: test.flags, Source: "testSource"})
954+
err := evaluator.SetState(sync.DataSync{FlagData: test.flags, Source: "testSource"})
955955

956956
if err != nil {
957957
t.Fatalf("expected no error")
@@ -1018,7 +1018,7 @@ func TestSetState_DefaultVariantValidation(t *testing.T) {
10181018
t.Run(name, func(t *testing.T) {
10191019
jsonEvaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
10201020

1021-
_, _, err := jsonEvaluator.SetState(sync.DataSync{FlagData: tt.jsonFlags, Source: "testSource"})
1021+
err := jsonEvaluator.SetState(sync.DataSync{FlagData: tt.jsonFlags, Source: "testSource"})
10221022

10231023
if tt.valid && err != nil {
10241024
t.Error(err)
@@ -1032,7 +1032,6 @@ func TestState_Evaluator(t *testing.T) {
10321032
inputState string
10331033
expectedOutputState string
10341034
expectedError bool
1035-
expectedResync bool
10361035
}{
10371036
"success": {
10381037
inputState: `
@@ -1320,24 +1319,17 @@ func TestState_Evaluator(t *testing.T) {
13201319
t.Run(name, func(t *testing.T) {
13211320
jsonEvaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
13221321

1323-
_, resync, err := jsonEvaluator.SetState(sync.DataSync{FlagData: tt.inputState, Source: "testSource"})
1322+
err := jsonEvaluator.SetState(sync.DataSync{FlagData: tt.inputState, Source: "testSource"})
13241323
if err != nil {
13251324
if !tt.expectedError {
13261325
t.Error(err)
13271326
}
1328-
if resync != tt.expectedResync {
1329-
t.Errorf("expected resync %t got %t", tt.expectedResync, resync)
1330-
}
13311327
return
13321328
} else if tt.expectedError {
13331329
t.Error("expected error, got nil")
13341330
return
13351331
}
13361332

1337-
if resync != tt.expectedResync {
1338-
t.Errorf("expected resync %t got %t", tt.expectedResync, resync)
1339-
}
1340-
13411333
got, err := jsonEvaluator.GetState()
13421334
if err != nil {
13431335
t.Error(err)
@@ -1427,7 +1419,7 @@ func TestFlagStateSafeForConcurrentReadWrites(t *testing.T) {
14271419
t.Run(name, func(t *testing.T) {
14281420
jsonEvaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
14291421

1430-
_, _, err := jsonEvaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
1422+
err := jsonEvaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
14311423
if err != nil {
14321424
t.Fatal(err)
14331425
}
@@ -1450,7 +1442,7 @@ func TestFlagStateSafeForConcurrentReadWrites(t *testing.T) {
14501442
errChan <- nil
14511443
return
14521444
default:
1453-
_, _, err := jsonEvaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
1445+
err := jsonEvaluator.SetState(sync.DataSync{FlagData: Flags, Source: "testSource"})
14541446
if err != nil {
14551447
errChan <- err
14561448
return
@@ -1492,7 +1484,7 @@ func TestFlagdAmbientProperties(t *testing.T) {
14921484
t.Run("flagKeyIsInTheContext", func(t *testing.T) {
14931485
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
14941486

1495-
_, _, err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
1487+
err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
14961488
"flags": {
14971489
"welcome-banner": {
14981490
"state": "ENABLED",
@@ -1532,7 +1524,7 @@ func TestFlagdAmbientProperties(t *testing.T) {
15321524
t.Run("timestampIsInTheContext", func(t *testing.T) {
15331525
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
15341526

1535-
_, _, err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
1527+
err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
15361528
"flags": {
15371529
"welcome-banner": {
15381530
"state": "ENABLED",
@@ -1566,7 +1558,7 @@ func TestTargetingVariantBehavior(t *testing.T) {
15661558
t.Run("missing variant error", func(t *testing.T) {
15671559
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
15681560

1569-
_, _, err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
1561+
err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
15701562
"flags": {
15711563
"missing-variant": {
15721564
"state": "ENABLED",
@@ -1594,7 +1586,7 @@ func TestTargetingVariantBehavior(t *testing.T) {
15941586
t.Run("null fallback", func(t *testing.T) {
15951587
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
15961588

1597-
_, _, err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
1589+
err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
15981590
"flags": {
15991591
"null-fallback": {
16001592
"state": "ENABLED",
@@ -1627,7 +1619,7 @@ func TestTargetingVariantBehavior(t *testing.T) {
16271619
evaluator, _ := evaluator.NewJSON(logger.NewLogger(nil, false), store.NewFlags())
16281620

16291621
//nolint:dupword
1630-
_, _, err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
1622+
err := evaluator.SetState(sync.DataSync{Source: "testSource", FlagData: `{
16311623
"flags": {
16321624
"match-boolean": {
16331625
"state": "ENABLED",

‎core/pkg/notifications/notifications.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ type Notifications map[string]any
1515
func NewFromFlags(oldFlags, newFlags map[string]model.Flag) Notifications {
1616
notifications := map[string]interface{}{}
1717

18+
// NOTE: we do not care about the events here for flagd itself, those are only needed for the provider, except for the openTelemetry information!!!!
1819
// flags removed
1920
for key := range oldFlags {
2021
if _, ok := newFlags[key]; !ok {

0 commit comments

Comments
 (0)