open-feature / go-sdk-contrib

Community maintained OpenFeature Providers and Hooks for Go
https://openfeature.dev
Apache License 2.0
42 stars 38 forks source link

from-env unexpected matching #47

Closed seankhliao closed 1 year ago

seankhliao commented 1 year ago

See https://go.dev/play/p/p51mlqZ0-MK

copy of test case ```go package main import ( "context" "encoding/json" "os" "testing" fromenv "github.com/open-feature/go-sdk-contrib/providers/from-env/pkg" "github.com/open-feature/go-sdk/pkg/openfeature" ) func TestFlagResolit(t *testing.T) { var ffProvider fromenv.FromEnvProvider openfeature.SetProvider(&ffProvider) ffClient := openfeature.NewClient("client-1") type testCase struct { stored fromenv.StoredFlag flag string fallback string evalCtx openfeature.EvaluationContext targetingKey string attrs map[string]any wantValue string wantReason openfeature.Reason } tcs := []testCase{ { stored: fromenv.StoredFlag{ DefaultVariant: "var-0", Variants: []fromenv.Variant{ { Name: "var-0", Value: "val-0", }, { Name: "var-1", Value: "val-1", }, { Name: "var-2", Value: "val-2", Criteria: []fromenv.Criteria{ { Key: "crit-2", Value: "crit-val-2", }, }, }, }, }, flag: "flag-a", fallback: "fall-0", evalCtx: openfeature.NewEvaluationContext("", map[string]any{ "crit-2": "crit-val-2", }), wantValue: "val-2", wantReason: openfeature.TargetingMatchReason, }, { stored: fromenv.StoredFlag{ DefaultVariant: "var-0", Variants: []fromenv.Variant{ { Name: "var-0", Value: "val-0", }, { Name: "var-1", Value: "val-1", }, { Name: "var-2", Value: "val-2", Criteria: []fromenv.Criteria{ { Key: "crit-2", Value: "crit-val-2", }, }, }, }, }, flag: "flag-b", fallback: "fall-0", wantValue: "val-0", wantReason: openfeature.DefaultReason, }, } for _, tc := range tcs { t.Run(tc.flag, func(t *testing.T) { flagb, _ := json.Marshal(tc.stored) os.Setenv(tc.flag, string(flagb)) ctx := context.Background() details, _ := ffClient.StringValueDetails(ctx, tc.flag, tc.fallback, tc.evalCtx) if details.Value != tc.wantValue { t.Errorf("value(%s) = %q, want = %q", tc.flag, details.Value, tc.wantValue) } if details.Reason != tc.wantReason { t.Errorf("reason(%s) = %s, want = %s", tc.flag, details.Reason, tc.wantReason) } }) } } ```

It doesn't appear to match based solely on criteria (test flag-a)

Hitting the default variant returns a reason of TARGETING_MATCH rather than DEFAULT

james-milligan commented 1 year ago

If I've understood this correctly you are expecting the return value of val-2 in the test for flag-a, however, the provider evaluates all variants in order starting at index 0 so the first TARGETING_MATCH will be var-0 (there are no criteria, so 'all' criteria are matched and the variant is returned). If you reverse the order of the variants the test will pass, or, add a criteria to var-1 so that it doesn't match on all evaluations.

Variants: []fromenv.Variant{
    {
        Name:  "var-2",
        Value: "val-2",
        Criteria: []fromenv.Criteria{
            {
                Key:   "crit-2",
                Value: "crit-val-2",
            },
        },
    },
    {
        Name:  "var-1",
        Value: "val-1",
    },
},

Similar behaviour is seen in the test for flag-b, however a bug is also causing further issues, which I will be opening an issue for shortly.