From 86e90c4169cca2065cb5d0d47758e1abe1ba824f Mon Sep 17 00:00:00 2001
From: Cyril Tovena <cyril.tovena@gmail.com>
Date: Fri, 10 Sep 2021 10:11:05 +0200
Subject: [PATCH] Fixes the pattern parser validation. (#4308)

We should never have 2 consecutive capture, since when we do we can't figure which data goes where.
The previous validation was validating two nodes at the time and moving to the next two nodes so in the end it wasn't validating properly.

:facepalm:

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
---
 pkg/logql/log/pattern/ast.go          | 25 ++++++++++++++-----------
 pkg/logql/log/pattern/pattern_test.go |  6 ++++--
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/pkg/logql/log/pattern/ast.go b/pkg/logql/log/pattern/ast.go
index f8309d1c480b..b4cf8e813f96 100644
--- a/pkg/logql/log/pattern/ast.go
+++ b/pkg/logql/log/pattern/ast.go
@@ -19,19 +19,18 @@ func (e expr) validate() error {
 	if !e.hasCapture() {
 		return ErrNoCapture
 	}
-	// if there is at least 2 node, verify that none are consecutive.
-	if len(e) >= 2 {
-		for i := 0; i < len(e); i = i + 2 {
-			if i+1 >= len(e) {
-				break
-			}
-			if _, ok := e[i].(capture); ok {
-				if _, ok := e[i+1].(capture); ok {
-					return fmt.Errorf("found consecutive capture: %w", ErrInvalidExpr)
-				}
+	// Consecutive captures are not allowed.
+	for i, n := range e {
+		if i+1 >= len(e) {
+			break
+		}
+		if _, ok := n.(capture); ok {
+			if _, ok := e[i+1].(capture); ok {
+				return fmt.Errorf("found consecutive capture '%s': %w", n.String()+e[i+1].String(), ErrInvalidExpr)
 			}
 		}
 	}
+
 	caps := e.captures()
 	uniq := map[string]struct{}{}
 	for _, c := range caps {
@@ -46,7 +45,7 @@ func (e expr) validate() error {
 func (e expr) captures() (captures []string) {
 	for _, n := range e {
 		if c, ok := n.(capture); ok && !c.isUnamed() {
-			captures = append(captures, c.String())
+			captures = append(captures, c.Name())
 		}
 	}
 	return
@@ -59,6 +58,10 @@ func (e expr) captureCount() (count int) {
 type capture string
 
 func (c capture) String() string {
+	return "<" + string(c) + ">"
+}
+
+func (c capture) Name() string {
 	return string(c)
 }
 
diff --git a/pkg/logql/log/pattern/pattern_test.go b/pkg/logql/log/pattern/pattern_test.go
index 6a0a760dd29f..da0c6a180527 100644
--- a/pkg/logql/log/pattern/pattern_test.go
+++ b/pkg/logql/log/pattern/pattern_test.go
@@ -150,9 +150,11 @@ func Test_Error(t *testing.T) {
 		{"<_>", ErrNoCapture},
 		{"foo <_> bar <_>", ErrNoCapture},
 		{"foo bar buzz", ErrNoCapture},
-		{"<f><f>", fmt.Errorf("found consecutive capture: %w", ErrInvalidExpr)},
-		{"<f> f<d><b>", fmt.Errorf("found consecutive capture: %w", ErrInvalidExpr)},
+		{"<f><f>", fmt.Errorf("found consecutive capture '<f><f>': %w", ErrInvalidExpr)},
+		{"<f> f<d><b>", fmt.Errorf("found consecutive capture '<d><b>': %w", ErrInvalidExpr)},
 		{"<f> f<f>", fmt.Errorf("duplicate capture name (f): %w", ErrInvalidExpr)},
+		{`f<f><_>`, fmt.Errorf("found consecutive capture '<f><_>': %w", ErrInvalidExpr)},
+		{`<f>f<f><_>`, fmt.Errorf("found consecutive capture '<f><_>': %w", ErrInvalidExpr)},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			_, err := New(tt.name)