Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: variant fallback usage #171

Merged
merged 8 commits into from
Dec 21, 2023
27 changes: 18 additions & 9 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -416,29 +416,38 @@ 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)
variant := opts.variantFallbackFunc(feature, ctx)
if variant != nil {
variant.FeatureEnabled = featureEnabled
}
return variant
} else if opts.variantFallback != nil {
opts.variantFallback.FeatureEnabled = featureEnabled
return opts.variantFallback
}

if featureEnabled {
return disabledVariantFeatureEnabled
}
return defaultVariant
}

if !f.Enabled {
return defaultVariant
Comment on lines -432 to -433
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know when we ever hit this? I couldn't find a way to trigger it. One of the tests uses a default strategy (which is always enabled), but with f.Enabled set to false. This seems to still hit the previous strategyResult.enabled, though, so I suspect this isn't used anymore, but I'll leave it in combined with missing flags regardless.

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{
Expand Down
213 changes: 213 additions & 0 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1144,3 +1144,216 @@ 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{},
})

mockListener := &MockedListener{}
mockListener.On("OnReady").Return()
mockListener.On("OnRegistered", mock.AnythingOfType("ClientData"))
mockListener.On("OnCount", feature, false).Return()
mockListener.On("OnError").Return()

thomasheartman marked this conversation as resolved.
Show resolved Hide resolved
client, err := NewClient(
WithUrl(mockerServer),
WithAppName(mockAppName),
WithInstanceId(mockInstanceId),
WithListener(mockListener),
)

assert.NoError(err)
client.WaitForReady()

fallbackVariant := api.Variant{
Name: "fallback-variant",
FeatureEnabled: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing is a bit weird here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's probably a tabs/spaces thing. Weird that my editor changed to using whatever the other is. Anyway, that line is gone now, so that's not an issue anymore.

}

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I get that the FeatureEnabled field is being changed in the previous call to GetVariant that is a bit confusing here looking at the test. I think it might be better if you did:

	fallbackVariant = api.Variant{
		Name: "fallback-variant",
    FeatureEnabled: true,
	}
	fallbackFunc := func(feature string, ctx *context.Context) *api.Variant {
		return &fallbackVariant
	}

then later did the same

	assert.False(variantWithFallbackFunc.Enabled)

	assert.False(variantWithFallbackFunc.FeatureEnabled)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed changing the feature enabled in 251d167 as per your suggestion. With that change, is there still anything here you'd like to address?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, lgtm


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{},
})

mockListener := &MockedListener{}
mockListener.On("OnReady").Return()
mockListener.On("OnRegistered", mock.AnythingOfType("ClientData"))
mockListener.On("OnCount", feature, true).Return()
mockListener.On("OnError").Return()

client, err := NewClient(
WithUrl(mockerServer),
WithAppName(mockAppName),
WithInstanceId(mockInstanceId),
WithListener(mockListener),
)

assert.NoError(err)
client.WaitForReady()

fallbackVariant := api.Variant{
Name: "fallback-variant",
FeatureEnabled: false,
}

variant := client.GetVariant(feature, WithVariantFallback(&fallbackVariant))

assert.False(variant.Enabled)

assert.True(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)

This comment was marked as resolved.


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{},
})

mockListener := &MockedListener{}
mockListener.On("OnReady").Return()
mockListener.On("OnRegistered", mock.AnythingOfType("ClientData"))
mockListener.On("OnCount", feature, false).Return()
mockListener.On("OnError").Return()

client, err := NewClient(
WithUrl(mockerServer),
WithAppName(mockAppName),
WithInstanceId(mockInstanceId),
WithListener(mockListener),
)

assert.NoError(err)
client.WaitForReady()

fallbackVariant := api.Variant{
Name: "fallback-variant",
FeatureEnabled: true,
}

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)

This comment was marked as resolved.


assert.True(gock.IsDone(), "there should be no more mocks")
}
12 changes: 8 additions & 4 deletions config.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,16 +214,20 @@ 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

This comment was marked as resolved.

// 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.
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.
func WithVariantFallbackFunc(variantFallbackFunc VariantFallbackFunc) VariantOption {
return func(opts *variantOption) {
opts.variantFallbackFunc = variantFallbackFunc
Expand Down
Loading