From 9906dde25c134d5086097ce4c4871b942c0577b4 Mon Sep 17 00:00:00 2001
From: Lukasz Mierzwa <lukasz@cloudflare.com>
Date: Mon, 21 Mar 2022 12:29:54 +0000
Subject: [PATCH] Fix false positives from promql/fragile

---
 docs/changelog.md                      |  7 ++++
 internal/checks/promql_fragile.go      |  6 ++++
 internal/checks/promql_fragile_test.go | 46 +++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/docs/changelog.md b/docs/changelog.md
index 63230703..469d0abe 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## v0.15.4
+
+### Fixed
+
+- Fixed false positive reports from `promql/fragile` when `foo OR bar` is used inside
+  aggregation.
+
 ## v0.15.3
 
 ### Fixed
diff --git a/internal/checks/promql_fragile.go b/internal/checks/promql_fragile.go
index 1d0609bf..7d09c4ed 100644
--- a/internal/checks/promql_fragile.go
+++ b/internal/checks/promql_fragile.go
@@ -48,6 +48,12 @@ func (c FragileCheck) Check(ctx context.Context, rule parser.Rule, entries []dis
 
 func (c FragileCheck) checkNode(node *parser.PromQLNode) (problems []exprProblem) {
 	if n := utils.HasOuterBinaryExpr(node); n != nil && n.Op != promParser.LOR && n.Op != promParser.LUNLESS {
+		if _, ok := n.LHS.(*promParser.NumberLiteral); ok {
+			goto NEXT
+		}
+		if _, ok := n.RHS.(*promParser.NumberLiteral); ok {
+			goto NEXT
+		}
 		var isFragile bool
 		for _, child := range node.Children {
 			for _, agg := range utils.HasOuterAggregation(child) {
diff --git a/internal/checks/promql_fragile_test.go b/internal/checks/promql_fragile_test.go
index 6a305394..8d4a6180 100644
--- a/internal/checks/promql_fragile_test.go
+++ b/internal/checks/promql_fragile_test.go
@@ -83,7 +83,7 @@ func TestFragileCheck(t *testing.T) {
 			problems: func(uri string) []checks.Problem {
 				return []checks.Problem{
 					{
-						Fragment: `(sum(foo) without(job) + sum(bar) without(job)) > 1`,
+						Fragment: `(sum without(job) (foo) + sum without(job) (bar))`,
 						Lines:    []int{2},
 						Reporter: "promql/fragile",
 						Text:     text,
@@ -115,7 +115,7 @@ func TestFragileCheck(t *testing.T) {
 			problems: func(uri string) []checks.Problem {
 				return []checks.Problem{
 					{
-						Fragment: `(sum without(job) (foo) + sum(bar)) > 1`,
+						Fragment: `(sum without(job) (foo) + sum(bar))`,
 						Lines:    []int{2},
 						Reporter: "promql/fragile",
 						Text:     text,
@@ -125,17 +125,55 @@ func TestFragileCheck(t *testing.T) {
 			},
 		},
 		{
-			description: "ignores safe division",
+			description: "ignores safe addition",
 			content:     "- record: foo\n  expr: sum(foo) + sum(bar)\n",
 			checker:     newFragileCheck,
 			problems:    noProblems,
 		},
 		{
-			description: "ignores division if source metric is the same",
+			description: "ignores addition if source metric is the same",
 			content:     "- record: foo\n  expr: sum(foo) without(bar) + sum(foo) without(bar)\n",
 			checker:     newFragileCheck,
 			problems:    noProblems,
 		},
+		{
+			description: "handles nested aggregations correctly / LHS",
+			content: `
+- alert: foo
+  expr: |
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    ) > 3
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
+		{
+			description: "handles nested aggregations correctly / RHS",
+			content: `
+- alert: foo
+  expr: |
+    3 <
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    )
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
+		{
+			description: "handles nested aggregations correctly / both",
+			content: `
+- alert: foo
+  expr: |
+    3 <
+    count without (foo) (
+        probe_success{job="foo"} == 0 or probe_duration_seconds{job="foo"} >= 15
+    ) > 2
+`,
+			checker:  newFragileCheck,
+			problems: noProblems,
+		},
 	}
 
 	runTests(t, testCases)