Repository: Unleash/unleash-client-go
Pull request #135 (2023-08-02 10:29:02)

feat: add variant strategy support

sonatype-lift[bot]: ## Sonatype Lift is retiring
Sonatype Lift will be retiring on <b>Sep 12, 2023</b>, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

<b>[📖 Read about the impacts and timeline](https://www.sonatype.com/lift-retirement)</b>

Patch:
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/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.


Test patch:
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_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")
+}