From 4081ef8b67f1ab0f1b5071818e77a3dcbb6f67da Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Thu, 21 Dec 2023 08:54:52 +0100 Subject: [PATCH] fix: variant fallback usage (#171) This PR fixes a bug in how we use fallback variants in this SDK. The fallback should be used when there is no flag, when the flag doesn't have any variants, and if the flag has variants, but is disabled. However, prior to this, it was only used if the flag didn't exist. It addresses the issues in and closes #160. * fix: add tests and impl * fix: update comments * fix: minor rename * fix: use noopListener * fix: don't change the "FeatureEnabled" state of the fallback variant * Chore: remove empty struct from list * Test: ensure that the fallback variant's `FeatureEnabled` is unchanged * Docs: add notes about `FeatureEnabled` to function docs --- client.go | 20 ++-- client_test.go | 273 +++++++++++++++++++++++++++++++++++++++++++++++++ config.go | 22 +++- 3 files changed, 303 insertions(+), 12 deletions(-) diff --git a/client.go b/client.go index 0254e72..8abe334 100644 --- a/client.go +++ b/client.go @@ -416,21 +416,25 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt strategyResult, f = uc.isEnabled(feature, WithContext(*ctx)) } - if !strategyResult.Enabled { - return defaultVariant - } - - if f == nil { + getFallbackVariant := func(featureEnabled bool) *api.Variant { if opts.variantFallbackFunc != nil { return opts.variantFallbackFunc(feature, ctx) } else if opts.variantFallback != nil { return opts.variantFallback } + + if featureEnabled { + return disabledVariantFeatureEnabled + } return defaultVariant } - if !f.Enabled { - return defaultVariant + if !strategyResult.Enabled { + return getFallbackVariant(false) + } + + if f == nil || !f.Enabled { + return getFallbackVariant(false) } if strategyResult.Variant != nil { @@ -438,7 +442,7 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt } if len(f.Variants) == 0 { - return disabledVariantFeatureEnabled + return getFallbackVariant(true) } return api.VariantCollection{ diff --git a/client_test.go b/client_test.go index 8950ce6..64f50b8 100644 --- a/client_test.go +++ b/client_test.go @@ -1144,3 +1144,276 @@ func TestClient_VariantFromEnabledFeatureWithNoVariants(t *testing.T) { assert.True(gock.IsDone(), "there should be no more mocks") } + +func TestGetVariantWithFallbackVariantWhenFeatureDisabled(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-disabled" + features := []api.Feature{ + { + Name: feature, + Description: "feature-desc", + Enabled: false, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(&NoopListener{}), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + assert.False(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } + + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) + + assert.Equal(fallbackVariant, *variantWithFallbackFunc) + + assert.True(gock.IsDone(), "there should be no more mocks") +} + +func TestGetVariantWithFallbackVariantWhenFeatureEnabledButNoVariants(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-no-variants" + features := []api.Feature{ + { + Name: feature, + Description: "feature-desc", + Enabled: true, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(&NoopListener{}), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + // Because we return the fallback variant and the variant + // doesn't have a FeatureEnabled property, it will always be + // false, regardless of whether the actual feature is enabled + // or not. + assert.False(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } + + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) + + assert.Equal(fallbackVariant, *variantWithFallbackFunc) + + assert.True(gock.IsDone(), "there should be no more mocks") +} + +func TestGetVariantWithFallbackVariantWhenFeatureDoesntExist(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + feature := "feature-no-variants" + features := []api.Feature{} + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(&NoopListener{}), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariant := api.Variant{ + Name: "fallback-variant", + } + + variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant)) + + assert.False(variant.Enabled) + + assert.False(variant.FeatureEnabled) + + assert.Equal(fallbackVariant, *variant) + + fallbackFunc := func(feature string, ctx *context.Context) *api.Variant { + return &fallbackVariant + } + + variantWithFallbackFunc := client.GetVariant(feature, WithVariantFallbackFunc(fallbackFunc)) + + assert.Equal(fallbackVariant, *variantWithFallbackFunc) + + assert.True(gock.IsDone(), "there should be no more mocks") +} + +func TestGetVariant_FallbackVariantFeatureEnabledSettingIsLeftUnchanged(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + MatchHeader("UNLEASH-APPNAME", mockAppName). + MatchHeader("UNLEASH-INSTANCEID", mockInstanceId). + Reply(200) + + enabledFeatureNoVariants := "enabled-feature" + disabledFeature := "disabled-feature" + features := []api.Feature{ + { + Name: disabledFeature, + Description: "feature-desc", + Enabled: false, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + { + Name: enabledFeatureNoVariants, + Description: "feature-desc", + Enabled: true, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "default-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(&NoopListener{}), + ) + + assert.NoError(err) + client.WaitForReady() + + fallbackVariantFeatureEnabled := api.Variant{ + Name: "fallback-variant-feature-enabled", + FeatureEnabled: true, + } + fallbackVariantFeatureDisabled := api.Variant{ + Name: "fallback-variant-feature-disabled", + FeatureEnabled: false, + } + + variantForEnabledFeatureNoVariants := client.GetVariant(enabledFeatureNoVariants, WithVariantFallback(&fallbackVariantFeatureDisabled)) + + assert.False(variantForEnabledFeatureNoVariants.FeatureEnabled) + + variantForDisabledFeature := client.GetVariant(disabledFeature, WithVariantFallback(&fallbackVariantFeatureEnabled)) + + assert.True(variantForDisabledFeature.FeatureEnabled) + + assert.True(gock.IsDone(), "there should be no more mocks") +} diff --git a/config.go b/config.go index 58aaa99..9ed7ddc 100644 --- a/config.go +++ b/config.go @@ -214,16 +214,30 @@ type variantOption struct { // VariantOption provides options for querying if a variant is found or not. type VariantOption func(*variantOption) -// WithVariantFallback specifies what the value should be if the variant is not found on the -// unleash service. +// WithVariantFallback specifies what the value should be if the +// variant is not found on the unleash service. This could be because +// the feature doesn't exist, because it is disabled, or because it +// has no variants. +// +// If you specify a fallback variant, note that its `FeatureEnabled` +// field will be set to whatever you pass in or `false` by default. In +// other words, it will not reflect the feature's actual enabled +// state. func WithVariantFallback(variantFallback *api.Variant) VariantOption { return func(opts *variantOption) { opts.variantFallback = variantFallback } } -// WithVariantFallbackFunc specifies a fallback function to evaluate a variant -// is not found on the service. +// WithVariantFallbackFunc specifies a fallback function to evaluate +// to a variant when a variant is not found for a feature. This could +// be because the feature doesn't exist, because it is disabled, or +// because it has no variants. +// +// If you specify a fallback variant, note that its `FeatureEnabled` +// field will be set to whatever you pass in or `false` by default. In +// other words, it will not reflect the feature's actual enabled +// state. func WithVariantFallbackFunc(variantFallbackFunc VariantFallbackFunc) VariantOption { return func(opts *variantOption) { opts.variantFallbackFunc = variantFallbackFunc