From f3d44ea03bae4ee3ad4f8503fd1218ad44cba8f3 Mon Sep 17 00:00:00 2001 From: Thomas Poignant Date: Wed, 18 Sep 2024 19:19:10 +0200 Subject: [PATCH] fix: evaluationContextEnrichement with key env overriden by empty string (#2367) * fix: evaluationContextEnrichement with key env overriden by empty string Signed-off-by: Thomas Poignant * Adding documentation to explain how the special case env is working Signed-off-by: Thomas Poignant * Fixing the issue for allFlag too Signed-off-by: Thomas Poignant --------- Signed-off-by: Thomas Poignant --- variation.go | 5 +- variation_all_flags.go | 4 +- variation_test.go | 74 ++++++++++++++++++++++++- website/docs/go_module/configuration.md | 32 +++++------ 4 files changed, 95 insertions(+), 20 deletions(-) diff --git a/variation.go b/variation.go index 88250526d2c..3b84fc1f0d3 100644 --- a/variation.go +++ b/variation.go @@ -277,6 +277,7 @@ func notifyVariation[T model.JSONType]( // getVariation is the internal generic func that handle the logic of a variation the result will always // contain a valid model.VariationResult +// nolint:funlen func getVariation[T model.JSONType]( g *GoFeatureFlag, flagKey string, evaluationCtx ffcontext.Context, sdkDefaultValue T, expectedType string, ) (model.VariationResult[T], error) { @@ -322,7 +323,9 @@ func getVariation[T model.JSONType]( DefaultSdkValue: sdkDefaultValue, EvaluationContextEnrichment: maps.Clone(g.config.EvaluationContextEnrichment), } - flagCtx.AddIntoEvaluationContextEnrichment("env", g.config.Environment) + if g.config.Environment != "" { + flagCtx.AddIntoEvaluationContextEnrichment("env", g.config.Environment) + } flagValue, resolutionDetails := f.Value(flagKey, evaluationCtx, flagCtx) var convertedValue interface{} diff --git a/variation_all_flags.go b/variation_all_flags.go index 6fd48d7841d..16b63a62a0a 100644 --- a/variation_all_flags.go +++ b/variation_all_flags.go @@ -34,7 +34,9 @@ func (g *GoFeatureFlag) GetFlagStates(evaluationCtx ffcontext.Context, flagsToEv EvaluationContextEnrichment: g.config.EvaluationContextEnrichment, DefaultSdkValue: nil, } - flagCtx.AddIntoEvaluationContextEnrichment("env", g.config.Environment) + if g.config.Environment != "" { + flagCtx.AddIntoEvaluationContextEnrichment("env", g.config.Environment) + } // Evaluate only the flags in flagsToEvaluate if len(flagsToEvaluate) != 0 { diff --git a/variation_test.go b/variation_test.go index 60ede7eb10c..4126688254b 100644 --- a/variation_test.go +++ b/variation_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/thejerf/slogassert" "github.com/thomaspoignant/go-feature-flag/exporter" "github.com/thomaspoignant/go-feature-flag/exporter/logsexporter" @@ -13,11 +14,13 @@ import ( "github.com/thomaspoignant/go-feature-flag/internal/dto" "github.com/thomaspoignant/go-feature-flag/internal/flag" "github.com/thomaspoignant/go-feature-flag/model" + "github.com/thomaspoignant/go-feature-flag/retriever/fileretriever" "github.com/thomaspoignant/go-feature-flag/testutils" "github.com/thomaspoignant/go-feature-flag/testutils/flagv1" "github.com/thomaspoignant/go-feature-flag/testutils/testconvert" "github.com/thomaspoignant/go-feature-flag/utils/fflog" "log/slog" + "os" "runtime" "strings" "testing" @@ -3889,11 +3892,11 @@ func Test_constructMetadataParallel(t *testing.T) { type args struct { resolutionDetails flag.ResolutionDetails } - tests := []struct { + var tests []struct { name string args args wantEvaluatedRuleName string - }{} + } runtime.GOMAXPROCS(runtime.NumCPU()) @@ -3925,3 +3928,70 @@ func Test_constructMetadataParallel(t *testing.T) { }) } } + +func Test_OverrideContextEnrichmentWithEnvironment(t *testing.T) { + tempFile, err := os.CreateTemp("", "") + require.NoError(t, err) + defer tempFile.Close() + + err = os.WriteFile(tempFile.Name(), []byte(` +flag1: + variations: + enabled: true + disabled: false + targeting: + - query: env eq "staging" + variation: enabled + defaultRule: + variation: disabled + +`), 0644) + require.NoError(t, err) + + goff, err := New(Config{ + PollingInterval: 500 * time.Millisecond, + Retriever: &fileretriever.Retriever{Path: tempFile.Name()}, + EvaluationContextEnrichment: map[string]interface{}{ + "env": "staging", + }, + }) + require.NoError(t, err) + + res, err1 := goff.BoolVariation("flag1", ffcontext.NewEvaluationContextBuilder("my-key").Build(), false) + assert.True(t, res) + assert.NoError(t, err1) + allFlags := goff.AllFlagsState(ffcontext.NewEvaluationContextBuilder("my-key").Build()) + assert.Equal(t, true, allFlags.GetFlags()["flag1"].Value) + + goff2, err2 := New(Config{ + PollingInterval: 500 * time.Millisecond, + Retriever: &fileretriever.Retriever{Path: tempFile.Name()}, + Environment: "staging", + EvaluationContextEnrichment: map[string]interface{}{ + "env": "staging", + }, + }) + require.NoError(t, err2) + res2, err3 := goff2.BoolVariation("flag1", ffcontext.NewEvaluationContextBuilder("my-key").Build(), false) + assert.True(t, res2) + assert.NoError(t, err3) + allFlags2 := goff2.AllFlagsState(ffcontext.NewEvaluationContextBuilder("my-key").Build()) + assert.Equal(t, true, allFlags2.GetFlags()["flag1"].Value) + + // Explicit environment should override the environment from the enrichment + goff3, err4 := New(Config{ + PollingInterval: 500 * time.Millisecond, + Retriever: &fileretriever.Retriever{Path: tempFile.Name()}, + Environment: "staging", + EvaluationContextEnrichment: map[string]interface{}{ + "env": "prod", + }, + }) + require.NoError(t, err4) + res3, err5 := goff3.BoolVariation("flag1", ffcontext.NewEvaluationContextBuilder("my-key").Build(), false) + assert.True(t, res3) + assert.NoError(t, err5) + + allFlags3 := goff3.AllFlagsState(ffcontext.NewEvaluationContextBuilder("my-key").Build()) + assert.Equal(t, true, allFlags3.GetFlags()["flag1"].Value) +} diff --git a/website/docs/go_module/configuration.md b/website/docs/go_module/configuration.md index 57b54591f66..579c23c7cd5 100644 --- a/website/docs/go_module/configuration.md +++ b/website/docs/go_module/configuration.md @@ -11,22 +11,22 @@ During the initialization you must give a [`ffclient.Config{}`](https://pkg.go.d ## Configuration fields -| Field | Description | -|-----------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `Retriever` | The configuration retriever you want to use to get your flag file
*See [Store your flag file](./store_file/index.mdx) for the configuration details*.

*This field is optional if `Retrievers`* is configured. | -| `Retrievers` | `Retrievers` is exactly the same thing as `Retriever` but you can configure more than 1 source for your flags.
All flags are retrieved in parallel, but we are applying them in the order you provided them _(it means that a flag can be overridden by another flag)_.
*See [Store your flag file](./store_file/index.mdx) for the configuration details*.

*This field is optional if `Retrievers`* is configured. | -| `Context` | *(optional)*
The context used by the retriever.
Default: **`context.Background()`** | -| `Environment` | *(optional)*
The environment the app is running under, can be checked in feature flag rules.
Default: `""`
*Check [**"environments"** section](../configure_flag/flag_format/#environments) to understand how to use this parameter.* | -| `DataExporter` | *(optional)*
DataExporter defines the method for exporting data on the usage of your flags.
*see [export data section](data_collection/index.md) for more details*. | -| `FileFormat` | *(optional)*
Format of your configuration file. Available formats are `yaml`, `toml` and `json`, if you omit the field it will try to unmarshal the file as a `yaml` file.
Default: **`YAML`** | -| `LeveledLogger` | *(optional)*
LeveledLogger is used to log what `go-feature-flag` is doing.
It should be a `slog` instance.
If no logger is provided the module will not log anything.
Default: **No log** | -| `Notifiers` | *(optional)*
List of notifiers to call when your flag file has been changed.
*See [notifiers section](./notifier/index.md) for more details*. | -| `PollingInterval` | (optional) Duration to wait before refreshing the flags.
The minimum polling interval is 1 second.
Default: **60 * time.Second** | -| `EnablePollingJitter` | (optional) Set to true if you want to avoid having true periodicity when retrieving your flags. It is useful to avoid having spike on your flag configuration storage in case your application is starting multiple instance at the same time.
We ensure a deviation that is maximum ±10% of your polling interval.
Default: **false** | -| `StartWithRetrieverError` | *(optional)* If **true**, the SDK will start even if we did not get any flags from the retriever. It will serve only default values until the retriever returns the flags.
The init method will not return any error if the flag file is unreachable.
Default: **false** | -| `Offline` | *(optional)* If **true**, the SDK will not try to retrieve the flag file and will not export any data. No notifications will be sent either.
Default: **false** | -| `EvaluationContextEnrichment` | *(optional)* It is a free `map[string]interface{}` field that will be merged with the evaluation context sent during the evaluations. It is useful to add common attributes to all the evaluation, such as a server version, environment, ...
All those fields will be included in the custom attributes of the evaluation context.
If in the evaluation context you have a field with the same name, it will be overriden by the `evaluationContextEnrichment`.
Default: **nil** | -| `PersistentFlagConfigurationFile` | *(optional)* If set GO Feature Flag will store the flags configuration in this file to be able to serve the flags even if none of the retrievers is available during starting time.
By default, the flag configuration is not persisted and stays on the retriever system. By setting a file here, you ensure that GO Feature Flag will always start with a configuration but which can be out-dated.

_(example: `/tmp/goff_persist_conf.yaml`)_ | +| Field | Description | +|-----------------------------------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `Retriever` | The configuration retriever you want to use to get your flag file
*See [Store your flag file](./store_file/index.mdx) for the configuration details*.

*This field is optional if `Retrievers`* is configured. | +| `Retrievers` | `Retrievers` is exactly the same thing as `Retriever` but you can configure more than 1 source for your flags.
All flags are retrieved in parallel, but we are applying them in the order you provided them _(it means that a flag can be overridden by another flag)_.
*See [Store your flag file](./store_file/index.mdx) for the configuration details*.

*This field is optional if `Retrievers`* is configured. | +| `Context` | *(optional)*
The context used by the retriever.
Default: **`context.Background()`** | +| `Environment` | *(optional)*
The environment the app is running under, can be checked in feature flag rules.
Default: `""`
*Check [**"environments"** section](../configure_flag/flag_format/#environments) to understand how to use this parameter.* | +| `DataExporter` | *(optional)*
DataExporter defines the method for exporting data on the usage of your flags.
*see [export data section](data_collection/index.md) for more details*. | +| `FileFormat` | *(optional)*
Format of your configuration file. Available formats are `yaml`, `toml` and `json`, if you omit the field it will try to unmarshal the file as a `yaml` file.
Default: **`YAML`** | +| `LeveledLogger` | *(optional)*
LeveledLogger is used to log what `go-feature-flag` is doing.
It should be a `slog` instance.
If no logger is provided the module will not log anything.
Default: **No log** | +| `Notifiers` | *(optional)*
List of notifiers to call when your flag file has been changed.
*See [notifiers section](./notifier/index.md) for more details*. | +| `PollingInterval` | (optional) Duration to wait before refreshing the flags.
The minimum polling interval is 1 second.
Default: **60 * time.Second** | +| `EnablePollingJitter` | (optional) Set to true if you want to avoid having true periodicity when retrieving your flags. It is useful to avoid having spike on your flag configuration storage in case your application is starting multiple instance at the same time.
We ensure a deviation that is maximum ±10% of your polling interval.
Default: **false** | +| `StartWithRetrieverError` | *(optional)* If **true**, the SDK will start even if we did not get any flags from the retriever. It will serve only default values until the retriever returns the flags.
The init method will not return any error if the flag file is unreachable.
Default: **false** | +| `Offline` | *(optional)* If **true**, the SDK will not try to retrieve the flag file and will not export any data. No notifications will be sent either.
Default: **false** | +| `EvaluationContextEnrichment` |

*(optional)* It is a free `map[string]interface{}` field that will be merged with the evaluation context sent during the evaluations. It is useful to add common attributes to all the evaluation, such as a server version, environment, ...

All those fields will be included in the custom attributes of the evaluation context.

_If in the evaluation context you have a field with the same name, it will be overridden by the `evaluationContextEnrichment`._

_If you have a key `env` in your `EvaluationContextEnrichment` and you also have the `Environment` set in your configuration, the `env` key from `EvaluationContextEnrichment` will be ignored._

Default: **nil** | +| `PersistentFlagConfigurationFile` | *(optional)* If set GO Feature Flag will store the flags configuration in this file to be able to serve the flags even if none of the retrievers is available during starting time.
By default, the flag configuration is not persisted and stays on the retriever system. By setting a file here, you ensure that GO Feature Flag will always start with a configuration but which can be out-dated.

_(example: `/tmp/goff_persist_conf.yaml`)_ | ## Example ```go