From 0ec8879436cf0a1b771b837ca4e7c79001ae511e Mon Sep 17 00:00:00 2001
From: Kyle Brandt <kyle@grafana.com>
Date: Fri, 2 Apr 2021 08:46:32 -0400
Subject: [PATCH] SSE/Alerting: Fix classic condition logic and some more tests
 (#32615)

---
 pkg/expr/classic/classic.go      |  36 +++++----
 pkg/expr/classic/classic_test.go | 128 +++++++++++++++++++++++++++++++
 2 files changed, 151 insertions(+), 13 deletions(-)

diff --git a/pkg/expr/classic/classic.go b/pkg/expr/classic/classic.go
index 45f9436acc0..7b268d6ef46 100644
--- a/pkg/expr/classic/classic.go
+++ b/pkg/expr/classic/classic.go
@@ -79,6 +79,7 @@ func (ccc *ConditionsCmd) Execute(ctx context.Context, vars mathexp.Vars) (mathe
 	for i, c := range ccc.Conditions {
 		querySeriesSet := vars[c.QueryRefID]
 		nilReducedCount := 0
+		firingCount := 0
 		for _, val := range querySeriesSet.Values {
 			series, ok := val.(mathexp.Series)
 			if !ok {
@@ -105,26 +106,35 @@ func (ccc *ConditionsCmd) Execute(ctx context.Context, vars mathexp.Vars) (mathe
 					match.Labels = reducedNum.GetLabels().Copy()
 				}
 				matches = append(matches, match)
-			}
-
-			if i == 0 {
-				firing = evalRes
-				noDataFound = thisCondNoDataFound
-			}
-
-			if c.Operator == "or" {
-				firing = firing || evalRes
-				noDataFound = noDataFound || thisCondNoDataFound
-			} else {
-				firing = firing && evalRes
-				noDataFound = noDataFound && thisCondNoDataFound
+				firingCount++
 			}
 		}
+
+		thisCondFiring := firingCount > 0
+		thisCondNoData := nilReducedCount > 0
+
+		if i == 0 {
+			firing = thisCondFiring
+			noDataFound = thisCondNoData
+		}
+
+		if c.Operator == "or" {
+			firing = firing || thisCondFiring
+			noDataFound = noDataFound || thisCondNoData
+		} else {
+			firing = firing && thisCondFiring
+			noDataFound = noDataFound && thisCondNoData
+		}
+
 		if len(querySeriesSet.Values) == nilReducedCount {
 			matches = append(matches, EvalMatch{
 				Metric: "NoData",
 			})
+			noDataFound = true
 		}
+
+		firingCount = 0
+		nilReducedCount = 0
 	}
 
 	num := mathexp.NewNumber("", nil)
diff --git a/pkg/expr/classic/classic_test.go b/pkg/expr/classic/classic_test.go
index 2828cf6c998..5adb6cc2d34 100644
--- a/pkg/expr/classic/classic_test.go
+++ b/pkg/expr/classic/classic_test.go
@@ -144,6 +144,134 @@ func TestConditionsCmdExecute(t *testing.T) {
 				return v
 			},
 		},
+		{
+			name: "single query and single condition - empty series",
+			vars: mathexp.Vars{
+				"A": mathexp.Results{
+					Values: []mathexp.Value{
+						valBasedSeries(),
+					},
+				},
+			},
+			conditionsCmd: &ConditionsCmd{
+				Conditions: []condition{
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("avg"),
+						Operator:   "and",
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 34},
+					},
+				}},
+			resultNumber: func() mathexp.Number {
+				v := valBasedNumber(nil)
+				v.SetMeta([]EvalMatch{{Metric: "NoData"}})
+				return v
+			},
+		},
+		{
+			name: "single query and two conditions",
+			vars: mathexp.Vars{
+				"A": mathexp.Results{
+					Values: []mathexp.Value{
+						valBasedSeries(ptr.Float64(30), ptr.Float64(40)),
+					},
+				},
+			},
+			conditionsCmd: &ConditionsCmd{
+				Conditions: []condition{
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("max"),
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 34},
+					},
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("min"),
+						Operator:   "or",
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 12},
+					},
+				}},
+			resultNumber: func() mathexp.Number {
+				v := valBasedNumber(ptr.Float64(1))
+				v.SetMeta([]EvalMatch{{Value: ptr.Float64(40)}, {Value: ptr.Float64(30)}})
+				return v
+			},
+		},
+		{
+			name: "single query and single condition - multiple series (one true, one not == true)",
+			vars: mathexp.Vars{
+				"A": mathexp.Results{
+					Values: []mathexp.Value{
+						valBasedSeries(ptr.Float64(30), ptr.Float64(40)),
+						valBasedSeries(ptr.Float64(0), ptr.Float64(10)),
+					},
+				},
+			},
+			conditionsCmd: &ConditionsCmd{
+				Conditions: []condition{
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("avg"),
+						Operator:   "and",
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 34},
+					},
+				}},
+			resultNumber: func() mathexp.Number {
+				v := valBasedNumber(ptr.Float64(1))
+				v.SetMeta([]EvalMatch{{Value: ptr.Float64(35)}})
+				return v
+			},
+		},
+		{
+			name: "single query and single condition - multiple series (one not true, one true == true)",
+			vars: mathexp.Vars{
+				"A": mathexp.Results{
+					Values: []mathexp.Value{
+						valBasedSeries(ptr.Float64(0), ptr.Float64(10)),
+						valBasedSeries(ptr.Float64(30), ptr.Float64(40)),
+					},
+				},
+			},
+			conditionsCmd: &ConditionsCmd{
+				Conditions: []condition{
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("avg"),
+						Operator:   "and",
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 34},
+					},
+				}},
+			resultNumber: func() mathexp.Number {
+				v := valBasedNumber(ptr.Float64(1))
+				v.SetMeta([]EvalMatch{{Value: ptr.Float64(35)}})
+				return v
+			},
+		},
+		{
+			name: "single query and single condition - multiple series (2 not true == false)",
+			vars: mathexp.Vars{
+				"A": mathexp.Results{
+					Values: []mathexp.Value{
+						valBasedSeries(ptr.Float64(0), ptr.Float64(10)),
+						valBasedSeries(ptr.Float64(20), ptr.Float64(30)),
+					},
+				},
+			},
+			conditionsCmd: &ConditionsCmd{
+				Conditions: []condition{
+					{
+						QueryRefID: "A",
+						Reducer:    classicReducer("avg"),
+						Operator:   "and",
+						Evaluator:  &thresholdEvaluator{Type: "gt", Threshold: 34},
+					},
+				}},
+			resultNumber: func() mathexp.Number {
+				v := valBasedNumber(ptr.Float64(0))
+				v.SetMeta([]EvalMatch{})
+				return v
+			},
+		},
 		{
 			name: "single query and single ranged condition",
 			vars: mathexp.Vars{