diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e75bca6..aa20796 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -15,7 +15,7 @@ jobs: name: Checkout client specifications with: repository: Unleash/client-specification - ref: refs/tags/v4.1.0 + ref: refs/tags/v4.3.1 path: testdata/client-specification - uses: actions/setup-go@v2 name: Setup go diff --git a/api/feature.go b/api/feature.go index 0d6a76a..ccdc700 100644 --- a/api/feature.go +++ b/api/feature.go @@ -19,8 +19,8 @@ type FeatureResponse struct { } type Segment struct { - Id int `json:"id"` - Constraints []Constraint `json:"constraints"` + Id int `json:"id"` + Constraints []Constraint `json:"constraints"` } type Feature struct { @@ -63,16 +63,16 @@ func (fr FeatureResponse) SegmentsMap() map[int][]Constraint { segments[segment.Id] = segment.Constraints } - return segments; + return segments } // Get variant for a given feature which is considered as enabled -func (f Feature) GetVariant(ctx *context.Context) *Variant { - if f.Enabled && len(f.Variants) > 0 { - v := f.getOverrideVariant(ctx) +func (vc VariantCollection) GetVariant(ctx *context.Context) *Variant { + if len(vc.Variants) > 0 { + v := vc.getOverrideVariant(ctx) var variant *Variant if v == nil { - variant = f.getVariantFromWeights(ctx) + variant = vc.getVariantFromWeights(ctx) } else { variant = &v.Variant } @@ -82,19 +82,19 @@ func (f Feature) GetVariant(ctx *context.Context) *Variant { return DISABLED_VARIANT } -func (f Feature) getVariantFromWeights(ctx *context.Context) *Variant { +func (vc VariantCollection) getVariantFromWeights(ctx *context.Context) *Variant { totalWeight := 0 - for _, variant := range f.Variants { + for _, variant := range vc.Variants { totalWeight += variant.Weight } if totalWeight == 0 { return DISABLED_VARIANT } - stickiness := f.Variants[0].Stickiness + stickiness := vc.Variants[0].Stickiness - target := getNormalizedNumber(getSeed(ctx, stickiness), f.Name, totalWeight) + target := getNormalizedNumber(getSeed(ctx, stickiness), vc.GroupId, totalWeight) counter := uint32(0) - for _, variant := range f.Variants { + for _, variant := range vc.Variants { counter += uint32(variant.Weight) if counter >= target { @@ -104,8 +104,8 @@ func (f Feature) getVariantFromWeights(ctx *context.Context) *Variant { return DISABLED_VARIANT } -func (f Feature) getOverrideVariant(ctx *context.Context) *VariantInternal { - for _, variant := range f.Variants { +func (vc VariantCollection) getOverrideVariant(ctx *context.Context) *VariantInternal { + for _, variant := range vc.Variants { for _, override := range variant.Overrides { if override.matchValue(ctx) { variant.Overrides = nil diff --git a/api/strategy.go b/api/strategy.go index b4db4d5..d30b673 100644 --- a/api/strategy.go +++ b/api/strategy.go @@ -19,6 +19,9 @@ type Strategy struct { Parameters ParameterMap `json:"parameters"` Segments []int `json:"segments"` + + // Variants for a strategy + Variants []VariantInternal `json:"variants"` } type ParameterDescription struct { @@ -33,3 +36,8 @@ type StrategyDescription struct { Description string `json:"description"` Parameters []ParameterDescription `json:"parameters"` } + +type StrategyResult struct { + Enabled bool + Variant *Variant +} diff --git a/api/variant.go b/api/variant.go index b37bfdf..187fac3 100644 --- a/api/variant.go +++ b/api/variant.go @@ -41,6 +41,13 @@ type VariantInternal struct { Overrides []Override `json:"overrides"` } +type VariantCollection struct { + // groupId to evaluate the variant + GroupId string + // variants for a feature toggle or feature strategy + Variants []VariantInternal +} + func (o Override) getIdentifier(ctx *context.Context) string { var value string switch o.ContextName { diff --git a/api/variant_test.go b/api/variant_test.go index 078dbb4..47c6761 100644 --- a/api/variant_test.go +++ b/api/variant_test.go @@ -105,21 +105,12 @@ func (suite *VariantTestSuite) TestGetVariantWhenFeatureHasNoVariant() { Enabled: true, } mockContext := &context.Context{} - suite.Equal(DISABLED_VARIANT, mockFeature.GetVariant(mockContext), "Should return default variant") -} + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) -func (suite *VariantTestSuite) TestGetVariantWhenFeatureIsNotEnabled() { - mockFeature := Feature{ - Name: "test.variants", - Enabled: false, - Variants: suite.VariantWithOverride, - } - mockContext := &context.Context{ - UserId: "1", - SessionId: "ABCDE", - RemoteAddress: "127.0.0.1", - } - suite.Equal(DISABLED_VARIANT, mockFeature.GetVariant(mockContext), "Should return default variant") + suite.Equal(DISABLED_VARIANT, variantSetup, "Should return default variant") } func (suite *VariantTestSuite) TestGetVariant_OverrideOnUserId() { @@ -137,9 +128,13 @@ func (suite *VariantTestSuite) TestGetVariant_OverrideOnUserId() { Type: "string", Value: "Test 1", } - suite.Equal("VarA", mockFeature.GetVariant(mockContext).Name, "Should return VarA") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") - suite.Equal(expectedPayload, mockFeature.GetVariant(mockContext).Payload, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarA", variantSetup.Name, "Should return VarA") + suite.Equal(true, variantSetup.Enabled, "Should be equal") + suite.Equal(expectedPayload, variantSetup.Payload, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_OverrideOnRemoteAddress() { @@ -156,9 +151,13 @@ func (suite *VariantTestSuite) TestGetVariant_OverrideOnRemoteAddress() { Type: "string", Value: "Test 2", } - suite.Equal("VarB", mockFeature.GetVariant(mockContext).Name, "Should return VarB") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") - suite.Equal(expectedPayload, mockFeature.GetVariant(mockContext).Payload, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarB", variantSetup.Name, "Should return VarB") + suite.Equal(true, variantSetup.Enabled, "Should be equal") + suite.Equal(expectedPayload, variantSetup.Payload, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_OverrideOnSessionId() { @@ -176,9 +175,13 @@ func (suite *VariantTestSuite) TestGetVariant_OverrideOnSessionId() { Type: "string", Value: "Test 1", } - suite.Equal("VarA", mockFeature.GetVariant(mockContext).Name, "Should return VarA") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") - suite.Equal(expectedPayload, mockFeature.GetVariant(mockContext).Payload, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarA", variantSetup.Name, "Should return VarA") + suite.Equal(true, variantSetup.Enabled, "Should be equal") + suite.Equal(expectedPayload, variantSetup.Payload, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_OverrideOnCustomProperties() { @@ -196,9 +199,13 @@ func (suite *VariantTestSuite) TestGetVariant_OverrideOnCustomProperties() { Type: "string", Value: "Test 3", } - suite.Equal("VarC", mockFeature.GetVariant(mockContext).Name, "Should return VarC") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") - suite.Equal(expectedPayload, mockFeature.GetVariant(mockContext).Payload, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarC", variantSetup.Name, "Should return VarC") + suite.Equal(true, variantSetup.Enabled, "Should be equal") + suite.Equal(expectedPayload, variantSetup.Payload, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarD() { @@ -210,8 +217,12 @@ func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarD() { mockContext := &context.Context{ UserId: "123", } - suite.Equal("VarD", mockFeature.GetVariant(mockContext).Name, "Should return VarD") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarD", variantSetup.Name, "Should return VarD") + suite.Equal(true, variantSetup.Enabled, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarE() { @@ -223,8 +234,12 @@ func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarE() { mockContext := &context.Context{ UserId: "163", } - suite.Equal("VarE", mockFeature.GetVariant(mockContext).Name, "Should return VarE") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarE", variantSetup.Name, "Should return VarE") + suite.Equal(true, variantSetup.Enabled, "Should be equal") } func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarF() { @@ -236,8 +251,12 @@ func (suite *VariantTestSuite) TestGetVariant_ShouldReturnVarF() { mockContext := &context.Context{ UserId: "40", } - suite.Equal("VarF", mockFeature.GetVariant(mockContext).Name, "Should return VarF") - suite.Equal(true, mockFeature.GetVariant(mockContext).Enabled, "Should be equal") + variantSetup := VariantCollection{ + GroupId: mockFeature.Name, + Variants: mockFeature.Variants, + }.GetVariant(mockContext) + suite.Equal("VarF", variantSetup.Name, "Should return VarF") + suite.Equal(true, variantSetup.Enabled, "Should be equal") } func TestVariantSuite(t *testing.T) { diff --git a/client.go b/client.go index bf5b690..711cb1f 100644 --- a/client.go +++ b/client.go @@ -243,12 +243,12 @@ func (uc *Client) IsEnabled(feature string, options ...FeatureOption) (enabled b uc.metrics.count(feature, enabled) }() - return uc.isEnabled(feature, options...) + return uc.isEnabled(feature, options...).Enabled } // isEnabled abstracts away the details of checking if a toggle is turned on or off // without metrics -func (uc *Client) isEnabled(feature string, options ...FeatureOption) (enabled bool) { +func (uc *Client) isEnabled(feature string, options ...FeatureOption) api.StrategyResult { var opts featureOption for _, o := range options { o(&opts) @@ -268,19 +268,29 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) (enabled b if f == nil { if opts.fallbackFunc != nil { - return opts.fallbackFunc(feature, ctx) + return api.StrategyResult{ + Enabled: opts.fallbackFunc(feature, ctx), + } } else if opts.fallback != nil { - return *opts.fallback + return api.StrategyResult{ + Enabled: *opts.fallback, + } + } + return api.StrategyResult{ + Enabled: false, } - return false } if !f.Enabled { - return false + return api.StrategyResult{ + Enabled: false, + } } if len(f.Strategies) == 0 { - return f.Enabled + return api.StrategyResult{ + Enabled: f.Enabled, + } } for _, s := range f.Strategies { @@ -294,7 +304,9 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) (enabled b if err != nil { uc.errors <- err - return false + return api.StrategyResult{ + Enabled: false, + } } allConstraints := make([]api.Constraint, 0) @@ -304,11 +316,33 @@ func (uc *Client) isEnabled(feature string, options ...FeatureOption) (enabled b if ok, err := constraints.Check(ctx, allConstraints); err != nil { uc.errors <- err } else if ok && foundStrategy.IsEnabled(s.Parameters, ctx) { - return true + if s.Variants != nil && len(s.Variants) > 0 { + groupIdValue := s.Parameters[strategy.ParamGroupId] + groupId, ok := groupIdValue.(string) + if !ok { + return api.StrategyResult{ + Enabled: false, + } + } + + return api.StrategyResult{ + Enabled: true, + Variant: api.VariantCollection{ + GroupId: groupId, + Variants: s.Variants, + }.GetVariant(ctx), + } + } else { + return api.StrategyResult{ + Enabled: true, + } + } } } - return false + return api.StrategyResult{ + Enabled: false, + } } // GetVariant queries a variant as the specified feature is enabled. @@ -335,14 +369,16 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt ctx = ctx.Override(*opts.ctx) } + var strategyResult api.StrategyResult + if opts.resolver != nil { - if !uc.isEnabled(feature, WithContext(*ctx), WithResolver(opts.resolver)) { - return defaultVariant - } + strategyResult = uc.isEnabled(feature, WithContext(*ctx), WithResolver(opts.resolver)) } else { - if !uc.isEnabled(feature, WithContext(*ctx)) { - return defaultVariant - } + strategyResult = uc.isEnabled(feature, WithContext(*ctx)) + } + + if !strategyResult.Enabled { + return defaultVariant } var f *api.Feature @@ -365,11 +401,18 @@ func (uc *Client) getVariantWithoutMetrics(feature string, options ...VariantOpt return defaultVariant } + if strategyResult.Variant != nil { + return strategyResult.Variant + } + if len(f.Variants) == 0 { return defaultVariant } - return f.GetVariant(ctx) + return api.VariantCollection{ + GroupId: f.Name, + Variants: f.Variants, + }.GetVariant(ctx) } // Close stops the client from syncing data from the server. diff --git a/client_test.go b/client_test.go index 77bc4a3..766e0e4 100644 --- a/client_test.go +++ b/client_test.go @@ -892,3 +892,188 @@ func TestClient_VariantShouldFailWhenSegmentConstraintsDontMatch(t *testing.T) { assert.True(gock.IsDone(), "there should be no more mocks") } + +func TestClient_ShouldFavorStrategyVariantOverFeatureVariant(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + Reply(200) + + features := []api.Feature{ + { + Name: "feature-x", + Description: "feature-desc", + Enabled: true, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "feature-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "default", + Constraints: []api.Constraint{}, + Parameters: map[string]interface{}{ + "groupId": "strategyVariantName", + }, + Variants: []api.VariantInternal{ + { + Variant: api.Variant{ + Name: "strategyVariantName", + Payload: api.Payload{ + Type: "string", + Value: "strategyVariantValue", + }, + }, + Weight: 1000, + }, + }, + }, + }, + Variants: []api.VariantInternal{ + { + Variant: api.Variant{ + Name: "willBeIgnored", + Payload: api.Payload{ + Type: "string", + Value: "willBeIgnored", + }, + Enabled: true, + }, + Weight: 100, + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnCount", mock.AnythingOfType("string"), mock.AnythingOfType("bool")).Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")) + mockListener.On("OnError", mock.AnythingOfType("*errors.errorString")) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + + assert.NoError(err) + + client.WaitForReady() + + strategyVariant := client.GetVariant("feature-x") + + assert.True(strategyVariant.Enabled) + + assert.Equal("strategyVariantName", strategyVariant.Name) + + assert.True(gock.IsDone(), "there should be no more mocks") +} + +func TestClient_ShouldReturnOldVariantForNonMatchingStrategyVariant(t *testing.T) { + assert := assert.New(t) + defer gock.OffAll() + + gock.New(mockerServer). + Post("/client/register"). + Reply(200) + + features := []api.Feature{ + { + Name: "feature-x", + Description: "feature-desc", + Enabled: true, + CreatedAt: time.Date(1974, time.May, 19, 1, 2, 3, 4, time.UTC), + Strategy: "feature-strategy", + Strategies: []api.Strategy{ + { + Id: 1, + Name: "flexibleRollout", + Constraints: []api.Constraint{}, + Parameters: map[string]interface{}{ + "rollout": 0, + "stickiness": "default", + }, + Variants: []api.VariantInternal{ + { + Variant: api.Variant{ + Name: "strategyVariantName", + Payload: api.Payload{ + Type: "string", + Value: "strategyVariantValue", + }, + Enabled: true, + }, + Weight: 1000, + }, + }, + }, + { + Id: 2, + Name: "flexibleRollout", + Constraints: []api.Constraint{}, + Parameters: map[string]interface{}{ + "rollout": 100, + "stickiness": "default", + }, + }, + }, + Variants: []api.VariantInternal{ + { + Variant: api.Variant{ + Name: "willBeSelected", + Payload: api.Payload{ + Type: "string", + Value: "willBeSelected", + }, + Enabled: true, + }, + Weight: 100, + }, + }, + }, + } + + gock.New(mockerServer). + Get("/client/features"). + Reply(200). + JSON(api.FeatureResponse{ + Features: features, + Segments: []api.Segment{}, + }) + + mockListener := &MockedListener{} + mockListener.On("OnReady").Return() + mockListener.On("OnCount", mock.AnythingOfType("string"), mock.AnythingOfType("bool")).Return() + mockListener.On("OnRegistered", mock.AnythingOfType("ClientData")) + mockListener.On("OnError", mock.AnythingOfType("*errors.errorString")) + + client, err := NewClient( + WithUrl(mockerServer), + WithAppName(mockAppName), + WithInstanceId(mockInstanceId), + WithListener(mockListener), + ) + + assert.NoError(err) + + client.WaitForReady() + + strategyVariant := client.GetVariant("feature-x") + + assert.True(strategyVariant.Enabled) + + assert.Equal("willBeSelected", strategyVariant.Name) + + assert.True(gock.IsDone(), "there should be no more mocks") +}