feat!: Enable parsing of array flag configurations for flagd #1797
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for polite-licorice-3db33c canceled.
|
dosubot
bot
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @aepfli, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant internal refactoring of the flag storage mechanism, transitioning from a map-based structure to a slice-based one. This change aims to enhance the flexibility and robustness of flag management, particularly in scenarios involving multiple flag sources and flag sets. The update also simplifies the evaluator's state management interface, contributing to a cleaner and more focused API for flag updates.
Highlights
- Store Interface Refactoring: The core store interface (IStore) and its implementations have been refactored to manage flags as a slice of
model.Flaginstead of a map. This change impacts how flags are stored, retrieved, and updated internally, allowing for more flexible handling of flags, especially those associated with different flag sets. - IEvaluator Interface Simplification: The
IEvaluatorinterface has been simplified by removing theGetState()method and modifying theSetState()method signature. TheSetState()method no longer returns detailed event information or a resync boolean, streamlining its contract to simply indicate success or failure via an error. - Flag Key Handling in Store: To accommodate flags from different sources and flag sets, the internal storage now uses a compound key (
FlagSetId|Key) to uniquely identify flags. This ensures correct handling of flags that might have the sameKeybut originate from different flag sets or sources. - Test Suite Updates: Extensive updates have been made across the test suite, including
fractional_test.go,json_test.go,semver_test.go,string_comparison_test.go, andstore_test.go, to align with the new[]model.Flagstructure and the simplifiedIEvaluator.SetState()signature. This includes changes to test data structures and assertion logic.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
aepfli
force-pushed
the
refactor/change_store_interface
branch
from
6049dad to
d805690
Compare
dosubot
bot
added
size:XL
There was a problem hiding this comment.
Code Review
This pull request refactors the flag storage from a map to a slice, which is a positive change for performance and deterministic ordering. The associated test improvements, especially in TestState_Evaluator, are also commendable.
My review focuses on a few areas for improvement:
- Restoring a lost observability metric.
- Performance optimizations in the store implementation.
- A potential bug in the flag update logic.
Overall, this is a solid refactoring with significant improvements. Addressing the points in my review will make it even better.
aepfli
force-pushed
the
refactor/change_store_interface
branch
from
d805690 to
f754958
Compare
Signed-off-by: Simon Schrottner <[email protected]> diff --git c/core/pkg/evaluator/fractional_test.go i/core/pkg/evaluator/fractional_test.go index e933e86..c1dfb9a 100644 --- c/core/pkg/evaluator/fractional_test.go +++ i/core/pkg/evaluator/fractional_test.go @@ -15,11 +15,12 @@ func TestFractionalEvaluation(t *testing.T) { var sources = []string{source} ctx := context.Background() - commonFlags := map[string]model.Flag{ - "headerColor": { + commonFlags := []model.Flag{ + { + Key: "headerColor", State: "ENABLED", DefaultVariant: "red", - Variants: colorVariants, + Variants: colorVariants, Targeting: []byte(`{ "if": [ { @@ -51,10 +52,11 @@ func TestFractionalEvaluation(t *testing.T) { ] }`), }, - "customSeededHeaderColor": { + { + Key: "customSeededHeaderColor", State: "ENABLED", DefaultVariant: "red", - Variants: colorVariants, + Variants: colorVariants, Targeting: []byte(`{ "if": [ { @@ -77,7 +79,7 @@ func TestFractionalEvaluation(t *testing.T) { } tests := map[string]struct { - flags map[string]model.Flag + flags []model.Flag flagKey string context map[string]any expectedValue string @@ -166,12 +168,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "[email protected] with different flag key": { - flags: map[string]model.Flag{ - "footerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "footerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "in": ["@faas.com", { @@ -201,7 +203,7 @@ func TestFractionalEvaluation(t *testing.T) { }, null ] }`), - }, + }, }, flagKey: "footerColor", context: map[string]any{ @@ -212,12 +214,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "non even split": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "in": ["@faas.com", { @@ -243,7 +245,7 @@ func TestFractionalEvaluation(t *testing.T) { }, null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -254,12 +256,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "fallback to default variant if no email provided": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "fractional": [ {"var": "email"}, [ @@ -280,7 +282,7 @@ func TestFractionalEvaluation(t *testing.T) { ] ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{}, @@ -289,12 +291,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.DefaultReason, }, "get variant for non-percentage weight values": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "fractional": [ {"var": "email"}, [ @@ -307,7 +309,7 @@ func TestFractionalEvaluation(t *testing.T) { ] ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -318,12 +320,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "get variant for non-specified weight values": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "fractional": [ {"var": "email"}, [ @@ -334,7 +336,7 @@ func TestFractionalEvaluation(t *testing.T) { ] ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -345,12 +347,12 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "default to targetingKey if no bucket key provided": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "fractional": [ [ "blue", @@ -362,7 +364,7 @@ func TestFractionalEvaluation(t *testing.T) { ] ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -373,20 +375,20 @@ func TestFractionalEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "missing email - parser should ignore nil/missing custom variables and continue": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte( - `{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte( + `{ "fractional": [ {"var": "email"}, ["red",50], ["blue",50] ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -438,12 +440,12 @@ func BenchmarkFractionalEvaluation(b *testing.B) { var sources = []string{source} ctx := context.Background() - flags := map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags := []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "in": ["@faas.com", { @@ -473,11 +475,11 @@ func BenchmarkFractionalEvaluation(b *testing.B) { }, null ] }`), - }, + }, } tests := map[string]struct { - flags map[string]model.Flag + flags []model.Flag flagKey string context map[string]any expectedValue string diff --git c/core/pkg/evaluator/json.go i/core/pkg/evaluator/json.go index 12b862b..5f8fce1 100644 --- c/core/pkg/evaluator/json.go +++ i/core/pkg/evaluator/json.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "fmt" + "github.com/xeipuuv/gojsonschema" "regexp" "strings" "time" @@ -177,7 +178,7 @@ func (je *Resolver) ResolveAllValues(ctx context.Context, reqID string, context var reason string var metadata map[string]interface{} - for flagKey, flag := range allFlags { + for _, flag := range allFlags { if flag.State == Disabled { // ignore evaluation of disabled flag continue @@ -186,18 +187,18 @@ 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, flag.Key, 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, flag.Key, 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, flag.Key, 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, flag.Key, context, je.evaluateVariant) } if err != nil { - je.Logger.ErrorWithID(reqID, fmt.Sprintf("bulk evaluation: key: %s returned error: %s", flagKey, err.Error())) + je.Logger.ErrorWithID(reqID, fmt.Sprintf("bulk evaluation: key: %s returned error: %s", flag.Key, err.Error())) } - values = append(values, NewAnyValue(value, variant, reason, flagKey, metadata, err)) + values = append(values, NewAnyValue(value, variant, reason, flag.Key, metadata, err)) } return values, flagSetMetadata, nil @@ -453,23 +454,32 @@ func (je *JSON) configToFlagDefinition(config string, definition *Definition) er "flag definition does not conform to the schema; validation errors: %s", err), ) } - + type JsonRawDef struct { + Flags map[string]model.Flag `json:"flags"` + Metadata map[string]interface{} `json:"metadata"` + } + // Transpose evaluators and unmarshal directly into JsonDef transposedConfig, err := transposeEvaluators(config) if err != nil { return fmt.Errorf("transposing evaluators: %w", err) } - err = json.Unmarshal([]byte(transposedConfig), &definition) + var rawDef JsonRawDef + err = json.Unmarshal([]byte(transposedConfig), &rawDef) if err != nil { return fmt.Errorf("unmarshalling provided configurations: %w", err) } - + definition.Metadata = rawDef.Metadata + for s, flag := range rawDef.Flags { + flag.Key = s + definition.Flags = append(definition.Flags, flag) + } return validateDefaultVariants(definition) } // validateDefaultVariants returns an error if any of the default variants aren't valid func validateDefaultVariants(flags *Definition) error { - for name, flag := range flags.Flags { + for _, flag := range flags.Flags { // Default Variant is not provided in the config if flag.DefaultVariant == "" { continue @@ -477,7 +487,7 @@ func validateDefaultVariants(flags *Definition) error { if _, ok := flag.Variants[flag.DefaultVariant]; !ok { return fmt.Errorf( - "default variant: '%s' isn't a valid variant of flag: '%s'", flag.DefaultVariant, name, + "default variant: '%s' isn't a valid variant of flag: '%s'", flag.DefaultVariant, flag.Key, ) } } diff --git c/core/pkg/evaluator/json_model.go i/core/pkg/evaluator/json_model.go index 0f09eec..826f390 100644 --- c/core/pkg/evaluator/json_model.go +++ i/core/pkg/evaluator/json_model.go @@ -11,7 +11,7 @@ type Evaluators struct { } type Definition struct { - Flags map[string]model.Flag `json:"flags"` + Flags []model.Flag `json:"flags"` Metadata map[string]interface{} `json:"metadata"` } diff --git c/core/pkg/evaluator/semver_test.go i/core/pkg/evaluator/semver_test.go index fbc6582..52f59a9 100644 --- c/core/pkg/evaluator/semver_test.go +++ i/core/pkg/evaluator/semver_test.go @@ -321,7 +321,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { ctx := context.Background() tests := map[string]struct { - flags map[string]model.Flag + flags []model.Flag flagKey string context map[string]any expectedValue string @@ -330,12 +330,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedError error }{ "versions and operator provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.0.0", ">", "0.1.0"] @@ -343,7 +343,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -354,12 +354,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [{"var": "version"}, ">", "1.0.0"] @@ -367,7 +367,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -378,12 +378,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and operator provided - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.0.0", ">", "1.0.0"] @@ -391,7 +391,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -402,12 +402,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and major-version operator provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.2.3", "^", "1.5.6"] @@ -415,7 +415,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -426,12 +426,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and minor-version operator provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.2.3", "~", "1.2.6"] @@ -439,7 +439,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -450,12 +450,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions given as double - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [1.2, "=", "1.2"] @@ -463,7 +463,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -474,12 +474,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions given as int - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [1, "=", "v1.0.0"] @@ -487,7 +487,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -498,12 +498,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and minor-version without patch version operator provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [1.2, "=", "1.2"] @@ -511,7 +511,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -522,12 +522,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions with prefixed v operator provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [{"var": "version"}, "<", "v1.2"] @@ -535,7 +535,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -546,12 +546,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and major-version operator provided - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["2.2.3", "^", "1.2.3"] @@ -559,7 +559,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -570,12 +570,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "versions and minor-version operator provided - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.3.3", "~", "1.2.6"] @@ -583,7 +583,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -594,12 +594,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [{"var": "version"}, ">", "1.0.0"] @@ -607,7 +607,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -618,12 +618,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (not an array) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": "not an array" @@ -631,7 +631,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -642,12 +642,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (wrong number of items in array) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["not", "enough"] @@ -655,7 +655,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -666,12 +666,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (invalid property value) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["invalid", ">", "1.0.0"] @@ -679,8 +679,9 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, }, + }, + flagKey: "headerColor", context: map[string]any{ "email": "[email protected]", @@ -690,12 +691,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (invalid property type) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": [1.0, ">", "1.0.0"] @@ -703,8 +704,9 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, }, + }, + flagKey: "headerColor", context: map[string]any{ "email": "[email protected]", @@ -714,12 +716,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (invalid operator) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.0.0", "invalid", "1.0.0"] @@ -727,7 +729,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -738,12 +740,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (invalid operator type) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.0.0", 1, "1.0.0"] @@ -751,7 +753,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -762,12 +764,12 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing (invalid target version) - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "sem_ver": ["1.0.0", ">", "invalid"] @@ -775,7 +777,7 @@ func TestJSONEvaluator_semVerEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ diff --git c/core/pkg/evaluator/string_comparison_test.go i/core/pkg/evaluator/string_comparison_test.go index 3e6163c..f22466f 100644 --- c/core/pkg/evaluator/string_comparison_test.go +++ i/core/pkg/evaluator/string_comparison_test.go @@ -18,7 +18,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { ctx := context.Background() tests := map[string]struct { - flags map[string]model.Flag + flags []model.Flag flagKey string context map[string]any expectedValue string @@ -27,12 +27,12 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { expectedError error }{ "two strings provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "starts_with": ["[email protected]", "user@faas"] @@ -40,7 +40,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -51,12 +51,12 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "starts_with": [{"var": "email"}, "user@faas"] @@ -64,7 +64,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -75,12 +75,12 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "two strings provided - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "starts_with": ["[email protected]", "nope"] @@ -88,7 +88,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -99,12 +99,12 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "starts_with": [{"var": "email"}, "nope"] @@ -112,7 +112,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -123,12 +123,12 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "starts_with": "no-array" @@ -136,7 +136,7 @@ func TestJSONEvaluator_startsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -186,7 +186,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { ctx := context.Background() tests := map[string]struct { - flags map[string]model.Flag + flags []model.Flag flagKey string context map[string]any expectedValue string @@ -195,12 +195,12 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { expectedError error }{ "two strings provided - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "ends_with": ["[email protected]", "faas.com"] @@ -208,7 +208,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -219,12 +219,12 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "ends_with": [{"var": "email"}, "faas.com"] @@ -232,7 +232,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { "red", null ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -243,12 +243,12 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "two strings provided - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "ends_with": ["[email protected]", "nope"] @@ -256,7 +256,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -267,12 +267,12 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "resolve target property using nested operation - no match": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "ends_with": [{"var": "email"}, "nope"] @@ -280,7 +280,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ @@ -291,12 +291,12 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { expectedReason: model.TargetingMatchReason, }, "error during parsing - return default": { - flags: map[string]model.Flag{ - "headerColor": { - State: "ENABLED", - DefaultVariant: "red", - Variants: colorVariants, - Targeting: []byte(`{ + flags: []model.Flag{{ + Key: "headerColor", + State: "ENABLED", + DefaultVariant: "red", + Variants: colorVariants, + Targeting: []byte(`{ "if": [ { "ends_with": "no-array" @@ -304,7 +304,7 @@ func TestJSONEvaluator_endsWithEvaluation(t *testing.T) { "red", "green" ] }`), - }, + }, }, flagKey: "headerColor", context: map[string]any{ diff --git c/core/pkg/store/store.go i/core/pkg/store/store.go index 0c6bc16..404604a 100644 --- c/core/pkg/store/store.go +++ i/core/pkg/store/store.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "slices" + "sort" "github.com/hashicorp/go-memdb" "github.com/open-feature/flagd/core/pkg/logger" @@ -15,14 +16,14 @@ var noValidatedSources = []string{} type SelectorContextKey struct{} type FlagQueryResult struct { - Flags map[string]model.Flag + Flags []model.Flag } type IStore interface { Get(ctx context.Context, key string, selector *Selector) (model.Flag, model.Metadata, error) - GetAll(ctx context.Context, selector *Selector) (map[string]model.Flag, model.Metadata, error) + GetAll(ctx context.Context, selector *Selector) ([]model.Flag, model.Metadata, error) Watch(ctx context.Context, selector *Selector, watcher chan<- FlagQueryResult) - Update(source string, flags map[string]model.Flag, metadata model.Metadata) + Update(source string, flags []model.Flag, metadata model.Metadata) } var _ IStore = (*Store)(nil) @@ -192,8 +193,8 @@ func (s *Store) Get(_ context.Context, key string, selector *Selector) (model.Fl } // GetAll returns a copy of the store's state (copy in order to be concurrency safe) -func (s *Store) GetAll(ctx context.Context, selector *Selector) (map[string]model.Flag, model.Metadata, error) { - flags := make(map[string]model.Flag) +func (s *Store) GetAll(ctx context.Context, selector *Selector) ([]model.Flag, model.Metadata, error) { + var flags []model.Flag queryMeta := selector.ToMetadata() it, err := s.selectOrAll(selector) @@ -208,7 +209,7 @@ func (s *Store) GetAll(ctx context.Context, selector *Selector) (map[string]mode // Update the flag state with the provided flags. func (s *Store) Update( source string, - flags map[string]model.Flag, + flags []model.Flag, metadata model.Metadata, ) { if source == "" { @@ -225,32 +226,10 @@ func (s *Store) Update( priority = 0 } - txn := s.db.Txn(true) - defer txn.Abort() - - // get all flags for the source we are updating - selector := NewSelector(sourceIndex + "=" + source) - oldFlags, _, _ := s.GetAll(context.Background(), &selector) - - for key := range oldFlags { - if _, ok := flags[key]; !ok { - // flag has been deleted - s.logger.Debug(fmt.Sprintf("flag %s has been deleted from source %s", key, source)) - - count, err := txn.DeleteAll(flagsTable, keySourceCompoundIndex, key, source) - s.logger.Debug(fmt.Sprintf("deleted %d flags with key %s from source %s", count, key, source)) - - if err != nil { - s.logger.Error(fmt.Sprintf("error deleting flag: %s, %v", key, err)) - } - continue - } - } - - for key, newFlag := range flags { + newFlags := make(map[string]model.Flag) + for _, newFlag := range flags { s.logger.Debug(fmt.Sprintf("got metadata %v", metadata)) - newFlag.Key = key newFlag.Source = source newFlag.Priority = priority newFlag.Metadata = patchMetadata(metadata, newFlag.Metadata) @@ -263,10 +242,42 @@ func (s *Store) Update( flagSetId = setFlagSetId } newFlag.FlagSetId = flagSetId + newFlags[newFlag.FlagSetId+"|"+newFlag.Key] = newFlag + } - raw, err := txn.First(flagsTable, keySourceCompoundIndex, key, source) + txn := s.db.Txn(true) + defer txn.Abort() + + // get all flags for the source we are updating + selector := NewSelector(sourceIndex + "=" + source) + oldFlags, _, _ := s.GetAll(context.Background(), &selector) + + for _, oldFlag := range oldFlags { + if _, ok := newFlags[oldFlag.FlagSetId+"|"+oldFlag.Key]; !ok { + // flag has been deleted + s.logger.Debug(fmt.Sprintf("flag '%s' and flagSetId '%s' has been deleted from source '%s'", oldFlag.Key, oldFlag.FlagSetId, source)) + + count, err := txn.DeleteAll(flagsTable, flagSetIdKeySourceCompoundIndex, oldFlag.FlagSetId, oldFlag.Key, source) + s.logger.Debug(fmt.Sprintf( + "deleted %d flags with key '%s' and flagSetId '%s' from source '%s'", + count, + oldFlag.Key, + oldFlag.FlagSetId, + source, + )) + + if err != nil { + s.logger.Error(fmt.Sprintf("error deleting flag: %s, %v", oldFlag.Key, err)) + } + continue + } + } + + for _, newFlag := range newFlags { + + raw, err := txn.First(flagsTable, keySourceCompoundIndex, newFlag.Key, source) if err != nil { - s.logger.Error(fmt.Sprintf("unable to get flag %s from source %s: %v", key, source, err)) + s.logger.Error(fmt.Sprintf("unable to get flag %s from source %s: %v", newFlag.Key, source, err)) continue } oldFlag, ok := raw.(model.Flag) @@ -275,9 +286,9 @@ func (s *Store) Update( if oldFlag.FlagSetId != newFlag.FlagSetId { // If the flagSetId is different, we need to delete the entry, since flagSetId+key represents the primary index, and it's now been changed. // This is important especially for clients listening to flagSetId changes, as they expect the flag to be removed from the set in this case. - _, err = txn.DeleteAll(flagsTable, idIndex, oldFlag.FlagSetId, key) + _, err = txn.DeleteAll(flagsTable, idIndex, oldFlag.FlagSetId, newFlag.Key) if err != nil { - s.logger.Error(fmt.Sprintf("unable to delete flags with key %s and flagSetId %s: %v", key, oldFlag.FlagSetId, err)) + s.logger.Error(fmt.Sprintf("unable to delete flags with key %s and flagSetId %s: %v", newFlag.Key, oldFlag.FlagSetId, err)) continue } } @@ -286,7 +297,7 @@ func (s *Store) Update( s.logger.Debug(fmt.Sprintf("storing flag: %v", newFlag)) err = txn.Insert(flagsTable, newFlag) if err != nil { - s.logger.Error(fmt.Sprintf("unable to insert flag %s: %v", key, err)) + s.logger.Error(fmt.Sprintf("unable to insert flag %s: %v", newFlag.Key, err)) continue } } @@ -335,20 +346,32 @@ func (s *Store) selectOrAll(selector *Selector) (it memdb.ResultIterator, err er } // collects flags from an iterator, ensuring that only the highest priority flag is kept when there are duplicates -func (s *Store) collect(it memdb.ResultIterator) map[string]model.Flag { +func (s *Store) collect(it memdb.ResultIterator) []model.Flag { flags := make(map[string]model.Flag) for raw := it.Next(); raw != nil; raw = it.Next() { flag := raw.(model.Flag) - if existing, ok := flags[flag.Key]; ok { + + // checking for multiple flags with the same key, as they can be defined multiple times in different sources + if existing, ok := flags[flag.FlagSetId+"|"+flag.Key]; ok { if flag.Priority < existing.Priority { - s.logger.Debug(fmt.Sprintf("discarding duplicate flag %s from lower priority source %s in favor of flag from source %s", flag.Key, s.sources[flag.Priority], s.sources[existing.Priority])) + s.logger.Debug(fmt.Sprintf("discarding duplicate flag with key '%s' and flagSetId '%s' from lower priority source '%s' in favor of flag from source '%s'", flag.Key, flag.FlagSetId, s.sources[flag.Priority], s.sources[existing.Priority])) continue // we already have a higher priority flag } - s.logger.Debug(fmt.Sprintf("overwriting duplicate flag %s from lower priority source %s in favor of flag from source %s", flag.Key, s.sources[existing.Priority], s.sources[flag.Priority])) + s.logger.Debug(fmt.Sprintf("overwriting duplicate flag with key '%s' and flagSetId '%s' from lower priority source '%s' in favor of flag from source '%s'", flag.Key, flag.FlagSetId, s.sources[existing.Priority], s.sources[flag.Priority])) } - flags[flag.Key] = flag + + flags[flag.FlagSetId+"|"+flag.Key] = flag } - return flags + + flattenedFlags := make([]model.Flag, 0, len(flags)) + for _, value := range flags { + flattenedFlags = append(flattenedFlags, value) + } + // we should order to keep the same order all the time in our response + sort.Slice(flattenedFlags, func(i, j int) bool { + return fmt.Sprintf("%s|%s", flattenedFlags[i].FlagSetId, flattenedFlags[i].Key) < fmt.Sprintf("%s|%s", flattenedFlags[j].FlagSetId, flattenedFlags[j].Key) + }) + return flattenedFlags } func patchMetadata(original, patch model.Metadata) model.Metadata { diff --git c/core/pkg/store/store_test.go i/core/pkg/store/store_test.go index c6cf2dd..f482e6a 100644 --- c/core/pkg/store/store_test.go +++ i/core/pkg/store/store_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "sort" "testing" "time" @@ -21,9 +22,9 @@ func TestUpdateFlags(t *testing.T) { tests := []struct { name string setup func(t *testing.T) IStore - newFlags map[string]model.Flag + newFlags []model.Flag source string - wantFlags map[string]model.Flag + wantFlags []model.Flag setMetadata model.Metadata }{ { @@ -37,7 +38,7 @@ func TestUpdateFlags(t *testing.T) { }, source: source1, newFlags: nil, - wantFlags: map[string]model.Flag{}, + wantFlags: []model.Flag{}, }, { name: "both empty flags", @@ -49,8 +50,8 @@ func TestUpdateFlags(t *testing.T) { return s }, source: source1, - newFlags: map[string]model.Flag{}, - wantFlags: map[string]model.Flag{}, + newFlags: []model.Flag{}, + wantFlags: []model.Flag{}, }, { name: "empty new", @@ -63,7 +64,7 @@ func TestUpdateFlags(t *testing.T) { }, source: source1, newFlags: nil, - wantFlags: map[string]model.Flag{}, + wantFlags: []model.Flag{}, }, { name: "update from source 1 (old flag removed)", @@ -72,17 +73,17 @@ func TestUpdateFlags(t *testing.T) { if err != nil { t.Fatalf("NewStore failed: %v", err) } - s.Update(source1, map[string]model.Flag{ - "waka": {DefaultVariant: "off"}, + s.Update(source1, []model.Flag{ + {Key: "waka", DefaultVariant: "off"}, }, nil) return s }, - newFlags: map[string]model.Flag{ - "paka": {DefaultVariant: "on"}, + newFlags: []model.Flag{ + {Key: "paka", DefaultVariant: "on"}, }, source: source1, - wantFlags: map[string]model.Flag{ - "paka": {Key: "paka", DefaultVariant: "on", Source: source1, FlagSetId: nilFlagSetId, Priority: 0}, + wantFlags: []model.Flag{ + {Key: "paka", DefaultVariant: "on", Source: source1, FlagSetId: nilFlagSetId, Priority: 0}, }, }, { @@ -92,18 +93,18 @@ func TestUpdateFlags(t *testing.T) { if err != nil { t.Fatalf("NewStore failed: %v", err) } - s.Update(source1, map[string]model.Flag{ - "waka": {DefaultVariant: "off"}, + s.Update(source1, []model.Flag{ + {Key: "waka", DefaultVariant: "off"}, }, nil) return s }, - newFlags: map[string]model.Flag{ - "paka": {DefaultVariant: "on"}, + newFlags: []model.Flag{ + {Key: "paka", DefaultVariant: "on"}, }, source: source2, - wantFlags: map[string]model.Flag{ - "waka": {Key: "waka", DefaultVariant: "off", Source: source1, FlagSetId: nilFlagSetId, Priority: 0}, - "paka": {Key: "paka", DefaultVariant: "on", Source: source2, FlagSetId: nilFlagSetId, Priority: 1}, + wantFlags: []model.Flag{ + {Key: "waka", DefaultVariant: "off", Source: source1, FlagSetId: nilFlagSetId, Priority: 0}, + {Key: "paka", DefaultVariant: "on", Source: source2, FlagSetId: nilFlagSetId, Priority: 1}, }, }, { @@ -113,20 +114,20 @@ func TestUpdateFlags(t *testing.T) { if err != nil { t.Fatalf("NewStore failed: %v", err) } - s.Update(source1, map[string]model.Flag{}, model.Metadata{}) + s.Update(source1, []model.Flag{}, model.Metadata{}) return s }, setMetadata: model.Metadata{ "flagSetId": "topLevelSet", // top level set metadata, including flagSetId }, - newFlags: map[string]model.Flag{ - "waka": {DefaultVariant: "on"}, - "paka": {DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": "flagLevelSet"}}, // overrides set level flagSetId + newFlags: []model.Flag{ + {Key: "waka", DefaultVariant: "on"}, + {Key: "paka", DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": "flagLevelSet"}}, // overrides set level flagSetId }, source: source1, - wantFlags: map[string]model.Flag{ - "waka": {Key: "waka", DefaultVariant: "on", Source: source1, FlagSetId: "topLevelSet", Priority: 0, Metadata: model.Metadata{"flagSetId": "topLevelSet"}}, - "paka": {Key: "paka", DefaultVariant: "on", Source: source1, FlagSetId: "flagLevelSet", Priority: 0, Metadata: model.Metadata{"flagSetId": "flagLevelSet"}}, + wantFlags: []model.Flag{ + {Key: "waka", DefaultVariant: "on", Source: source1, FlagSetId: "topLevelSet", Priority: 0, Metadata: model.Metadata{"flagSetId": "topLevelSet"}}, + {Key: "paka", DefaultVariant: "on", Source: source1, FlagSetId: "flagLevelSet", Priority: 0, Metadata: model.Metadata{"flagSetId": "flagLevelSet"}}, }, }, } @@ -138,8 +139,13 @@ func TestUpdateFlags(t *testing.T) { store := tt.setup(t) store.Update(tt.source, tt.newFlags, tt.setMetadata) gotFlags, _, _ := store.GetAll(context.Background(), nil) - - require.Equal(t, tt.wantFlags, gotFlags) + sort.Slice(tt.wantFlags, func(i, j int) bool { + return tt.wantFlags[i].FlagSetId+"|"+tt.wantFlags[i].Key > tt.wantFlags[j].FlagSetId+"|"+tt.wantFlags[j].Key + }) + sort.Slice(gotFlags, func(i, j int) bool { + return gotFlags[i].FlagSetId+"|"+gotFlags[i].Key > gotFlags[j].FlagSetId+"|"+gotFlags[j].Key + }) + require.EqualValues(t, tt.wantFlags, gotFlags) }) } } @@ -206,16 +212,16 @@ func TestGet(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - sourceAFlags := map[string]model.Flag{ - "flagA": {Key: "flagA", DefaultVariant: "off"}, - "dupe": {Key: "dupe", DefaultVariant: "on"}, + sourceAFlags := []model.Flag{ + {Key: "flagA", DefaultVariant: "off"}, + {Key: "dupe", DefaultVariant: "on"}, } - sourceBFlags := map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdB}}, + sourceBFlags := []model.Flag{ + {Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdB}}, } - sourceCFlags := map[string]model.Flag{ - "flagC": {Key: "flagC", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, - "dupe": {Key: "dupe", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + sourceCFlags := []model.Flag{ + {Key: "flagC", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "dupe", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, } store, err := NewStore(logger.NewLogger(nil, false), sources) @@ -253,35 +259,36 @@ func TestGetAllNoWatcher(t *testing.T) { tests := []struct { name string selector *Selector - wantFlags map[string]model.Flag + wantFlags []model.Flag }{ { name: "nil selector", selector: nil, - wantFlags: map[string]model.Flag{ + wantFlags: []model.Flag{ // "dupe" should be overwritten by higher priority flag - "flagA": {Key: "flagA", DefaultVariant: "off", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, - "flagB": {Key: "flagB", DefaultVariant: "off", Source: sourceB, FlagSetId: flagSetIdB, Priority: 1, Metadata: model.Metadata{"flagSetId": flagSetIdB}}, - "flagC": {Key: "flagC", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, - "dupe": {Key: "dupe", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "dupe", DefaultVariant: "on", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, + {Key: "flagA", DefaultVariant: "off", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, + {Key: "flagB", DefaultVariant: "off", Source: sourceB, FlagSetId: flagSetIdB, Priority: 1, Metadata: model.Metadata{"flagSetId": flagSetIdB}}, + {Key: "flagC", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "dupe", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, }, }, { name: "source selector", selector: &sourceASelector, - wantFlags: map[string]model.Flag{ + wantFlags: []model.Flag{ // we should get the "dupe" from sourceA - "flagA": {Key: "flagA", DefaultVariant: "off", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, - "dupe": {Key: "dupe", DefaultVariant: "on", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, + {Key: "flagA", DefaultVariant: "off", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, + {Key: "dupe", DefaultVariant: "on", Source: sourceA, FlagSetId: nilFlagSetId, Priority: 0}, }, }, { name: "flagSetId selector", selector: &flagSetIdCSelector, - wantFlags: map[string]model.Flag{ + wantFlags: []model.Flag{ // we should get the "dupe" from flagSetIdC - "flagC": {Key: "flagC", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, - "dupe": {Key: "dupe", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "flagC", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "dupe", DefaultVariant: "off", Source: sourceC, FlagSetId: flagSetIdC, Priority: 2, Metadata: model.Metadata{"flagSetId": flagSetIdC}}, }, }, } @@ -291,16 +298,16 @@ func TestGetAllNoWatcher(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - sourceAFlags := map[string]model.Flag{ - "flagA": {Key: "flagA", DefaultVariant: "off"}, - "dupe": {Key: "dupe", DefaultVariant: "on"}, + sourceAFlags := []model.Flag{ + {Key: "flagA", DefaultVariant: "off"}, + {Key: "dupe", DefaultVariant: "on"}, } - sourceBFlags := map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdB}}, + sourceBFlags := []model.Flag{ + {Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdB}}, } - sourceCFlags := map[string]model.Flag{ - "flagC": {Key: "flagC", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, - "dupe": {Key: "dupe", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + sourceCFlags := []model.Flag{ + {Key: "flagC", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, + {Key: "dupe", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": flagSetIdC}}, } store, err := NewStore(logger.NewLogger(nil, false), sources) @@ -314,6 +321,12 @@ func TestGetAllNoWatcher(t *testing.T) { gotFlags, _, _ := store.GetAll(context.Background(), tt.selector) require.Equal(t, len(tt.wantFlags), len(gotFlags)) + sort.Slice(tt.wantFlags, func(i, j int) bool { + return tt.wantFlags[i].FlagSetId+"|"+tt.wantFlags[i].Key > tt.wantFlags[j].FlagSetId+"|"+tt.wantFlags[j].Key + }) + sort.Slice(gotFlags, func(i, j int) bool { + return gotFlags[i].FlagSetId+"|"+gotFlags[i].Key > gotFlags[j].FlagSetId+"|"+gotFlags[j].Key + }) require.Equal(t, tt.wantFlags, gotFlags) }) } @@ -365,14 +378,12 @@ func TestWatch(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - sourceAFlags := map[string]model.Flag{ - "flagA": {Key: "flagA", DefaultVariant: "off"}, + sourceAFlags := []model.Flag{ + {Key: "flagA", DefaultVariant: "off"}, } - sourceBFlags := map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": myFlagSetId}}, - } - sourceCFlags := map[string]model.Flag{ - "flagC": {Key: "flagC", DefaultVariant: "off"}, + sourceBFlags := []model.Flag{{Key: "flagB", DefaultVariant: "off", Metadata: model.Metadata{"flagSetId": myFlagSetId}}} + sourceCFlags := []model.Flag{ + {Key: "flagC", DefaultVariant: "off"}, } store, err := NewStore(logger.NewLogger(nil, false), sources) @@ -396,29 +407,30 @@ func TestWatch(t *testing.T) { time.Sleep(pauseTime) // changing a flag default variant should trigger an update - store.Update(sourceA, map[string]model.Flag{ - "flagA": {Key: "flagA", DefaultVariant: "on"}, + store.Update(sourceA, []model.Flag{ + {Key: "flagA", DefaultVariant: "on"}, }, model.Metadata{}) time.Sleep(pauseTime) // changing a flag default variant should trigger an update - store.Update(sourceB, map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": myFlagSetId}}, + store.Update(sourceB, []model.Flag{ + {Key: "flagB", DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": myFlagSetId}}, }, model.Metadata{}) time.Sleep(pauseTime) // removing a flag set id should trigger an update (even for flag set id selectors; it should remove the flag from the set) - store.Update(sourceB, map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "on"}, + // TODO: challenge this test and behaviour + store.Update(sourceB, []model.Flag{ + {Key: "flagB", DefaultVariant: "on"}, }, model.Metadata{}) time.Sleep(pauseTime) // adding a flag set id should trigger an update - store.Update(sourceB, map[string]model.Flag{ - "flagB": {Key: "flagB", DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": myFlagSetId}}, + store.Update(sourceB, []model.Flag{ + {Key: "flagB", DefaultVariant: "on", Metadata: model.Metadata{"flagSetId": myFlagSetId}}, }, model.Metadata{}) }() @@ -448,9 +460,9 @@ func TestQueryMetadata(t *testing.T) { otherSource := "otherSource" nonExistingFlagSetId := "nonExistingFlagSetId" var sources = []string{sourceA} - sourceAFlags := map[string]model.Flag{ - "flagA": {Key: "flagA", DefaultVariant: "off"}, - "flagB": {Key: "flagB", DefaultVariant: "on"}, + sourceAFlags := []model.Flag{ + {Key: "flagA", DefaultVariant: "off"}, + {Key: "flagB", DefaultVariant: "on"}, } store, err := NewStore(logger.NewLogger(nil, false), sources)
aepfli
force-pushed
the
refactor/change_store_interface
branch
from
f754958 to
b87ddac
Compare
Signed-off-by: Simon Schrottner <[email protected]>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the internal flag storage from a map[string]model.Flag to a []model.Flag. This is a significant change that touches many parts of the codebase, especially the store, evaluator, and tests.
The changes are well-implemented for the most part. The store logic has been updated to handle the new data structure, including de-duplication and sorting, which improves determinism. The tests have been diligently updated to reflect the new structures.
I've found one potential high-severity bug in the eventing logic where converting the flag slice to a map could lead to data loss due to duplicate keys. I've also pointed out a minor issue with an outdated comment in a test file that could cause confusion.
Overall, this is a good refactoring. Please address the identified issues.
aepfli
changed the title
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
aepfli
changed the title
Signed-off-by: Simon Schrottner <[email protected]> Co-Authored-by: Alexandra Oberaigner <[email protected]> Co-Authored-by: Guido Breitenhuber <[email protected]>
dosubot
bot
added
size:XXL
toddbaert
reviewed
Sep 9, 2025
Signed-off-by: Todd Baert <[email protected]>
toddbaert
approved these changes
Sep 9, 2025
chrfwow
reviewed
Sep 10, 2025
| // for looking up by flagSetId and key | ||
| flagSetIdKeyIndex: { | ||
| Name: flagSetIdKeyIndex, | ||
| Unique: false, |
There was a problem hiding this comment.
I'm not familiar with go-memdb, but what does the Unique mean and why do we set it to false?
There was a problem hiding this comment.
we do have now multiple possible entries in our database where the flagsetId and the key are the same, because we can get those from multiple sources - setting this to true, would limit the possibilities, and might produce wrong information
There was a problem hiding this comment.
I'm confused by this change.
I remember that we were talking about using (flagSetID, flagKey) as the composite key, and IIRC @toddbaert confirmed that if there are same composite keys from different sources, the latest one will override as the semantic is always a full set, not update.
There was a problem hiding this comment.
This discussion continues here.
tangenti
requested changes
Sep 10, 2025
| Indexes: []memdb.Indexer{ | ||
| &memdb.StringFieldIndex{Field: model.FlagSetId, Lowercase: false}, | ||
| &memdb.StringFieldIndex{Field: model.Key, Lowercase: false}, | ||
| &memdb.StringFieldIndex{Field: model.Source, Lowercase: false}, |
There was a problem hiding this comment.
Why we need to index the source?
There was a problem hiding this comment.
so here comes my assumption, lets say we do have two sources, with the same flagset "awesomeFlagSet" and the same flag, call them sourceA and sourceB. if i select based on flagset i want to get the flag from sourceB. but if i select based on source and request sourceA, i want to get the flag from sourceA - but without adding this, to the primary key, we ensure that both versions stay in the database
There was a problem hiding this comment.
I think this is a valid-enough use case that we can justify this.
We can of course NOT implement this if we want to be strict about the composite key being SOLELY the flagSet/Key.
I think before release, we should document this more thoroughly though, along with deciding on this and documenting that as well. We need documentation around how our new selector works in general. I'm happy to write these docs once we are all in agreement and have a release candidate.
There was a problem hiding this comment.
It sounds a new feature instead of a refactor? Before having the selectors for flag set IDs and memdb change, flags with the same key from different source are overriding based on the priority IIUC.
There was a problem hiding this comment.
i am not sure if this is a new feature or a undefined side-effect of the maps configuration. the maps configuration and the selector allow for different possible configuration and retrieval of flags. We never specified what happens if a flag is defined in multiple sources for the same flagset, and what is the impact for the user. -> my interpretation, we are using a database and a selector, i allow selection per flagset and per source, if i select by source, it is unexpected for me, that the flag is missing, although it is defined in the source. Maybe we should clarify what we expect in this case. Do we need to improve the ADR or create a new one? Is it worth it?
There was a problem hiding this comment.
The more I am thinking about it, i agree that the simplicity of flagset + key is desireable. but I think that we should keep this simplicity to the outside, and decouple this from our store. I already mentioned the selector edge case, but there is eg. also this one, say we have:
Source A (higher priority) has flag "new-checkout"
Source B (lower priority) also has flag "new-checkout" but with completely different targeting
Source A removes the flag
Source B wasnt updated in hours
do we want to fallback in this case to B? should be with the next update of source B suddenly reappear?
Another case, how do we handle updates when the source is asynchronous, eg. the grpc streams, and those are not happening in order, how do we define if we should overwrite the flag. The priority/source is hard to utilize, because again, maybe the priortized source removes this flag.
I do agree that this adds complexity on the implementation level, and makes the logic less clear, but i think it is easier to keep the data in memdb and handle it via logic, rather than deciding already on import how to handle this. Currently this datamodel represents the source model, it is the same as the sources, and as a user I would expect this to happen, when querying for data. As it is in sync with the data i provide. The simplicity for flagset and key is still valid to the outside, as providers only select currently based on selector (old deprecated) or flagsetid (new stylish fancy awesome) ;).
We also need to be aware that although we do not encourage people to such a setup, we should still somehow be aware of potential occurences, especially as this duplication might also be a migration scenario from one source to another.
I am not stricly opposing with just flagsetid and key as a primary key, but i think this might add more complexity or unpredicted behaviour in the future. - so how should we proceed? (if we continue with just flagsetId and key, we should definitely define our expected behaviour clearly)
There was a problem hiding this comment.
I think these are good questions, but maybe they are questions for a different PR so that we can move forward with the array support for now.
There was a problem hiding this comment.
@aepfli can we revert just the part? I think we can just remove a couple tests and the new index.
I think for now it's just easier to say the PK is flag set + key, and keep it there, with any behavior around duplicates unspecified and not a supported use case
There was a problem hiding this comment.
yes i will, sorry i was occupied with other topics and sick
i am still curious to here the opinion about all the strange cases outlined from me by @tangenti - currently it feels like we are allowing a lot of options, but only focus on some specific usecases/implementations and ignoring all other possibilities which can happen.
There was a problem hiding this comment.
i improved the tests for store_test.go, by adding another dimension in which i am also changing the order updates are happening (especially interesting for asynchronous data sources like grpc) (maybe my knowledge here is wrong, and this is not a problem) - but as soon as i am removing the source from the primary key, i am getting a lot inconsistencies.
Therefor, i just want to check with everyone, should i really revert to just flagsetid and key? - remember this is only our internal data representation.
// edit: food for thought - part of this is related with our multisources approach, removing this would be the other solution to this problem ;)
| // for looking up by flagSetId and key | ||
| flagSetIdKeyIndex: { | ||
| Name: flagSetIdKeyIndex, | ||
| Unique: false, |
There was a problem hiding this comment.
I'm confused by this change.
I remember that we were talking about using (flagSetID, flagKey) as the composite key, and IIRC @toddbaert confirmed that if there are same composite keys from different sources, the latest one will override as the semantic is always a full set, not update.
Signed-off-by: Todd Baert <[email protected]>
|
I re-ran the benchmark: 3e99ad4 (no significant deviation, looks like 15% faster/slower across various tests). I hate that we don't have automation for this; I may have mentioned before that we once did, but github actions are far too vulnerable to noisy-neighbor effects to be useful in this sort of benchmarking. |
aepfli
commented
Sep 11, 2025
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces a significant internal refactoring of the flag storage mechanism, transitioning from a map-based structure to a slice-based one. This change aims to enhance the flexibility and robustness of flag management, particularly in scenarios involving multiple flag sources and flag sets. The update also simplifies the evaluator's state management interface, contributing to a cleaner and more focused API for flag updates.
Highlights
model.Flaginstead of a map. This change impacts how flags are stored, retrieved, and updated internally, allowing for more flexible handling of flags, especially those associated with different flag sets.IEvaluatorinterface has been simplified by removing theGetState()method and modifying theSetState()method signature. TheSetState()method no longer returns detailed event information or a resync boolean, streamlining its contract to simply indicate success or failure via an error.FlagSetId|Key) to uniquely identify flags. This ensures correct handling of flags that might have the sameKeybut originate from different flag sets or sources.fractional_test.go,json_test.go,semver_test.go,string_comparison_test.go, andstore_test.go, to align with the new[]model.Flagstructure and the simplifiedIEvaluator.SetState()signature. This includes changes to test data structures and assertion logic.