From 83ba93c867d9a29a98fa7d167b53740aa4dbf1e7 Mon Sep 17 00:00:00 2001
From: Travis Patterson <travis.patterson@grafana.com>
Date: Tue, 14 Mar 2023 19:09:22 +0100
Subject: [PATCH] fix to-string for noop label filters (#8799)

Label Filter optimizations omit `.*` label filters from the execution
pipeline but intruduced a bug where those stages were also ommitted from
calls to `String` on the expression. This results in unparsable queries
and panics when queries are cloned via `query.String()`

This PR makes `noopLabelFilter` embed matchers and inherit their
`String` method just like the `StringLabelFilters` do
---
 pkg/logql/log/label_filter.go      | 23 ++++++++++++++---------
 pkg/logql/log/label_filter_test.go |  2 +-
 pkg/logql/syntax/ast.go            |  4 +++-
 pkg/logql/syntax/parser_test.go    | 11 +++++++++++
 4 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/pkg/logql/log/label_filter.go b/pkg/logql/log/label_filter.go
index dcb00a2266..1fda5430b7 100644
--- a/pkg/logql/log/label_filter.go
+++ b/pkg/logql/log/label_filter.go
@@ -19,9 +19,6 @@ var (
 	_ LabelFilterer = &DurationLabelFilter{}
 	_ LabelFilterer = &NumericLabelFilter{}
 	_ LabelFilterer = &StringLabelFilter{}
-
-	// NoopLabelFilter is a label filter that doesn't filter out any values.
-	NoopLabelFilter = noopLabelFilter{}
 )
 
 // LabelFilterType is an enum for label filtering types.
@@ -118,18 +115,26 @@ func (b *BinaryLabelFilter) String() string {
 	return sb.String()
 }
 
-type noopLabelFilter struct{}
+type NoopLabelFilter struct {
+	*labels.Matcher
+}
 
-func (noopLabelFilter) String() string { return "" }
-func (noopLabelFilter) Process(_ int64, line []byte, _ *LabelsBuilder) ([]byte, bool) {
+func (NoopLabelFilter) Process(_ int64, line []byte, _ *LabelsBuilder) ([]byte, bool) {
 	return line, true
 }
-func (noopLabelFilter) RequiredLabelNames() []string { return []string{} }
+func (NoopLabelFilter) RequiredLabelNames() []string { return []string{} }
+
+func (f NoopLabelFilter) String() string {
+	if f.Matcher != nil {
+		return f.Matcher.String()
+	}
+	return ""
+}
 
 // ReduceAndLabelFilter Reduces multiple label filterer into one using binary and operation.
 func ReduceAndLabelFilter(filters []LabelFilterer) LabelFilterer {
 	if len(filters) == 0 {
-		return NoopLabelFilter
+		return &NoopLabelFilter{}
 	}
 	if len(filters) == 1 {
 		return filters[0]
@@ -340,7 +345,7 @@ func NewStringLabelFilter(m *labels.Matcher) LabelFilterer {
 	}
 
 	if f == TrueFilter {
-		return NoopLabelFilter
+		return &NoopLabelFilter{m}
 	}
 
 	return &lineFilterLabelFilter{
diff --git a/pkg/logql/log/label_filter_test.go b/pkg/logql/log/label_filter_test.go
index 113c62ff82..52a1cffcb1 100644
--- a/pkg/logql/log/label_filter_test.go
+++ b/pkg/logql/log/label_filter_test.go
@@ -318,7 +318,7 @@ func TestReduceAndLabelFilter(t *testing.T) {
 		filters []LabelFilterer
 		want    LabelFilterer
 	}{
-		{"empty", nil, NoopLabelFilter},
+		{"empty", nil, &NoopLabelFilter{}},
 		{"1", []LabelFilterer{NewBytesLabelFilter(LabelFilterEqual, "foo", 5)}, NewBytesLabelFilter(LabelFilterEqual, "foo", 5)},
 		{
 			"2",
diff --git a/pkg/logql/syntax/ast.go b/pkg/logql/syntax/ast.go
index c8c37dbaf2..c01042e49d 100644
--- a/pkg/logql/syntax/ast.go
+++ b/pkg/logql/syntax/ast.go
@@ -82,7 +82,7 @@ func (m MultiStageExpr) stages() ([]log.Stage, error) {
 		if err != nil {
 			return nil, logqlmodel.NewStageError(e.String(), err)
 		}
-		if p == log.NoopStage || p == log.NoopLabelFilter {
+		if p == log.NoopStage {
 			continue
 		}
 		c = append(c, p)
@@ -410,6 +410,8 @@ func (e *LabelFilterExpr) Stage() (log.Stage, error) {
 	switch ip := e.LabelFilterer.(type) {
 	case *log.IPLabelFilter:
 		return ip, ip.PatternError()
+	case *log.NoopLabelFilter:
+		return log.NoopStage, nil
 	}
 	return e.LabelFilterer, nil
 }
diff --git a/pkg/logql/syntax/parser_test.go b/pkg/logql/syntax/parser_test.go
index 37fe5ea436..c77259c22c 100644
--- a/pkg/logql/syntax/parser_test.go
+++ b/pkg/logql/syntax/parser_test.go
@@ -3409,3 +3409,14 @@ func TestParseLabels(t *testing.T) {
 		})
 	}
 }
+
+func TestNoOpLabelToString(t *testing.T) {
+	logExpr := `{container_name="app"} | foo=~".*"`
+	l, err := ParseLogSelector(logExpr, false)
+	require.NoError(t, err)
+	require.Equal(t, logExpr, l.String())
+
+	stages, err := l.(*PipelineExpr).MultiStages.stages()
+	require.NoError(t, err)
+	require.Len(t, stages, 0)
+}