Merge pull request #181 from checkr/zz/fix-segments-evaluation

Fix segments evaluation

pkg/handler/eval.go

@@ -140,13 +140,15 @@ var evalFlag = func(evalContext models.EvalContext) *models.EvalResult {
 	var sID *int64
 
 	for _, segment := range f.Segments {
-		variantID, log := evalSegment(f.ID, evalContext, segment)
+		sID = util.Int64Ptr(int64(segment.ID))
+		variantID, log, evalNextSegment := evalSegment(f.ID, evalContext, segment)
 		if evalContext.EnableDebug {
 			logs = append(logs, log)
 		}
 		if variantID != nil {
-			sID = util.Int64Ptr(int64(segment.ID))
 			vID = util.Int64Ptr(int64(*variantID))
+		}
+		if !evalNextSegment {
 			break
 		}
 	}
@@ -202,6 +204,7 @@ var evalSegment = func(
 ) (
 	vID *uint, // returns VariantID
 	log *models.SegmentDebugLog,
+	evalNextSegment bool,
 ) {
 	if len(segment.Constraints) != 0 {
 		m, ok := evalContext.EntityContext.(map[string]interface{})
@@ -210,7 +213,7 @@ var evalSegment = func(
 				Msg:       fmt.Sprintf("constraints are present in the segment_id %v, but got invalid entity_context: %s.", segment.ID, spew.Sdump(evalContext.EntityContext)),
 				SegmentID: int64(segment.ID),
 			}
-			return nil, log
+			return nil, log, false
 		}
 
 		expr := segment.SegmentEvaluation.ConditionsExpr
@@ -220,14 +223,14 @@ var evalSegment = func(
 				Msg:       err.Error(),
 				SegmentID: int64(segment.ID),
 			}
-			return nil, log
+			return nil, log, true
 		}
 		if !match {
 			log = &models.SegmentDebugLog{
 				Msg:       debugConstraintMsg(expr, m),
 				SegmentID: int64(segment.ID),
 			}
-			return nil, log
+			return nil, log, true
 		}
 	}
 
@@ -242,7 +245,9 @@ var evalSegment = func(
 		SegmentID: int64(segment.ID),
 	}
 
-	return vID, log
+	// at this point, all constraints are matched, so we shouldn't go to next segment
+	// thus setting evalNextSegment = false
+	return vID, log, false
 }
 
 func debugConstraintMsg(expr conditions.Expr, m map[string]interface{}) string {

pkg/handler/eval_test.go

@@ -16,16 +16,17 @@ import (
 func TestEvalSegment(t *testing.T) {
 	t.Run("test empty evalContext", func(t *testing.T) {
 		s := entity.GenFixtureSegment()
-		vID, log := evalSegment(100, models.EvalContext{}, s)
+		vID, log, evalNextSegment := evalSegment(100, models.EvalContext{}, s)
 
 		assert.Nil(t, vID)
 		assert.NotEmpty(t, log)
+		assert.False(t, evalNextSegment)
 	})
 
 	t.Run("test happy code path", func(t *testing.T) {
 		s := entity.GenFixtureSegment()
 		s.RolloutPercent = uint(100)
-		vID, log := evalSegment(100, models.EvalContext{
+		vID, log, evalNextSegment := evalSegment(100, models.EvalContext{
 			EnableDebug:   true,
 			EntityContext: map[string]interface{}{"dl_state": "CA"},
 			EntityID:      "entityID1",
@@ -35,12 +36,13 @@ func TestEvalSegment(t *testing.T) {
 
 		assert.NotNil(t, vID)
 		assert.NotEmpty(t, log)
+		assert.False(t, evalNextSegment)
 	})
 
 	t.Run("test constraint evaluation error", func(t *testing.T) {
 		s := entity.GenFixtureSegment()
 		s.RolloutPercent = uint(100)
-		vID, log := evalSegment(100, models.EvalContext{
+		vID, log, evalNextSegment := evalSegment(100, models.EvalContext{
 			EnableDebug:   true,
 			EntityContext: map[string]interface{}{},
 			EntityID:      "entityID1",
@@ -50,12 +52,13 @@ func TestEvalSegment(t *testing.T) {
 
 		assert.Nil(t, vID)
 		assert.NotEmpty(t, log)
+		assert.True(t, evalNextSegment)
 	})
 
 	t.Run("test constraint not match", func(t *testing.T) {
 		s := entity.GenFixtureSegment()
 		s.RolloutPercent = uint(100)
-		vID, log := evalSegment(100, models.EvalContext{
+		vID, log, evalNextSegment := evalSegment(100, models.EvalContext{
 			EnableDebug:   true,
 			EntityContext: map[string]interface{}{"dl_state": "NY"},
 			EntityID:      "entityID1",
@@ -65,12 +68,13 @@ func TestEvalSegment(t *testing.T) {
 
 		assert.Nil(t, vID)
 		assert.NotEmpty(t, log)
+		assert.True(t, evalNextSegment)
 	})
 
 	t.Run("test evalContext wrong format", func(t *testing.T) {
 		s := entity.GenFixtureSegment()
 		s.RolloutPercent = uint(100)
-		vID, log := evalSegment(100, models.EvalContext{
+		vID, log, evalNextSegment := evalSegment(100, models.EvalContext{
 			EnableDebug:   true,
 			EntityContext: nil,
 			EntityID:      "entityID1",
@@ -80,6 +84,7 @@ func TestEvalSegment(t *testing.T) {
 
 		assert.Nil(t, vID)
 		assert.NotEmpty(t, log)
+		assert.False(t, evalNextSegment)
 	})
 }
 
@@ -171,6 +176,26 @@ func TestEvalFlag(t *testing.T) {
 		assert.NotNil(t, result.VariantID)
 	})
 
+	t.Run("test multiple segments with the first segment 0% rollout", func(t *testing.T) {
+		f := entity.GenFixtureFlag()
+		f.Segments = append(f.Segments, entity.GenFixtureSegment())
+		f.Segments[0].Constraints = []entity.Constraint{}
+		f.Segments[0].RolloutPercent = uint(0)
+
+		f.PrepareEvaluation()
+		cache := &EvalCache{mapCache: map[string]*entity.Flag{"100": &f}}
+		defer gostub.StubFunc(&GetEvalCache, cache).Reset()
+		result := evalFlag(models.EvalContext{
+			EnableDebug:   true,
+			EntityContext: map[string]interface{}{"dl_state": "CA", "state": "CA", "rate": 2000},
+			EntityID:      "entityID1",
+			EntityType:    "entityType1",
+			FlagID:        int64(100),
+		})
+		assert.NotNil(t, result)
+		assert.Nil(t, result.VariantID)
+	})
+
 	t.Run("test no match path with multiple constraints", func(t *testing.T) {
 		f := entity.GenFixtureFlag()
 		f.Segments[0].Constraints = []entity.Constraint{