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

@@ -416,29 +416,33 @@ 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 {
 		return strategyResult.Variant
 	}
 
 	if len(f.Variants) == 0 {
-		return disabledVariantFeatureEnabled
+		return getFallbackVariant(true)
 	}
 
 	return api.VariantCollection{

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")
+}

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