Merge pull request #330 from bookingcom/parser/various-space-comma-bug-fixes

parser: return errors if there are syntax bugs in metric names wrapped by a function

Author: bom-d-van

.github/workflows/ci.yml

@@ -33,7 +33,7 @@ jobs:
         uses: golangci/golangci-lint-action@v2
         with:
           # Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version.
-          version: v1.30
+          version: v1.40.1
 
           # Optional: working directory, useful for monorepos
           # working-directory: somedir

.golangci.yml

@@ -149,6 +149,8 @@ linters:
   disable:
     - maligned
     - prealloc
+    - makezero # Disable due to panic bug in makezero ([email protected])
+    - errorlint # Disable temporarily, more could be found in: https://github.com/bookingcom/carbonapi/issues/333
   disable-all: false
   presets:
     - bugs

AUTHORS.txt

@@ -44,5 +44,6 @@ Slawomir Skowron
 Stefan Boronea
 Vladimir Smirnov
 Vsevolod Polyakov
+Xiaofan Hu
 Zachary Dykstra
 Zhang Cheng

pkg/parser/interface.go

@@ -35,6 +35,22 @@ var (
 	ErrMissingQuote = ParseError("missing quote")
 	// ErrUnexpectedCharacter is a parse error returned when an expression contains an unexpected character.
 	ErrUnexpectedCharacter = ParseError("unexpected character")
+	// ErrMissingBracket is a parse error returned when an expression is missing an opening or closing bracket.
+	ErrMissingBracket = ParseError("missing opening or closing bracket []")
+	// ErrMissingBrace is a parse error returned when an expression is missing an opening or closing brace.
+	ErrMissingBrace = ParseError("missing opening or closing brace {}")
+	// ErrCommaInBrackets is a parse error returned when an expression has comma within brackets.
+	ErrCommaInBrackets = ParseError("comma within brackets")
+	// ErrSpacesInMetricName is a parse error returned when an expression has space in metric name.
+	ErrSpacesInMetricName = ParseError("space in metric name")
+	// ErrSpacesInBraces is a parse error returned when an expression has space in braces.
+	ErrSpacesInBraces = ParseError("space in braces")
+	// ErrSpacesInBrackets is a parse error returned when an expression has space in brackets.
+	ErrSpacesInBrackets = ParseError("space in brackets")
+	// ErrBraceInBrackets is a parse error returned when an expression has brace within brackets.
+	ErrBraceInBrackets = ParseError("brace within brackets")
+	// ErrNestedBrackets is a parse error returned when an expression has nested brackets.
+	ErrNestedBrackets = ParseError("nested brackets")
 	// ErrBadType is an eval error returned when a argument has wrong type.
 	ErrBadType = ParseError("bad type")
 	// ErrMissingArgument is an eval error returned when a argument is missing.

pkg/parser/internal.go

@@ -2,7 +2,6 @@ package parser
 
 import (
 	"fmt"
-
 	"runtime/debug"
 )
 

pkg/parser/parser.go

@@ -354,8 +354,13 @@ func parseExprWithoutPipe(e string) (Expr, string, error) {
 		val, tail, err := parseString(e)
 		return &expr{valStr: val, etype: EtString}, tail, err
 	}
+
 	var name string
-	name, e = parseName(e)
+	var err error
+	name, e, err = parseName(e)
+	if err != nil {
+		return nil, e, err
+	}
 
 	if strings.ToLower(name) == "false" || strings.ToLower(name) == "true" {
 		return &expr{valStr: name, etype: EtString, target: name}, e, nil
@@ -420,7 +425,6 @@ func IsNameChar(r byte) bool {
 		r == '.' || r == '_' ||
 		r == '-' || r == '*' ||
 		r == '?' || r == ':' ||
-		r == '[' || r == ']' ||
 		r == '^' || r == '$' ||
 		r == '<' || r == '>' ||
 		r == '&' || r == '#'
@@ -557,48 +561,118 @@ func parseConst(s string) (float64, string, error) {
 // RangeTables is an array of *unicode.RangeTable
 var RangeTables []*unicode.RangeTable
 
-func parseName(s string) (string, string) {
-
+// parseName parses the next symbol from s and returns
+// 	* the parsed symbol (function or metric name),
+// 	* the rest of the string from s
+// 	* syntax error
+func parseName(s string) (string, string, error) {
 	var (
-		braces, i, w int
-		r            rune
+		braces, brackets int
+		i, w             int
+		r                rune
 	)
 
 FOR:
-	for braces, i = 0, 0; i < len(s); i += w {
-
+	for i = 0; i < len(s); i += w {
 		w = 1
 		if IsNameChar(s[i]) {
 			continue
 		}
 
+		// Graphite render spec: https://graphite.readthedocs.io/en/latest/render_api.html#graphing-metrics
 		switch s[i] {
 		case '{':
+			// No way escape { in metric names, thus using it
+			// in the range brackets should be an error.
+			if brackets > 0 {
+				return s, "", ErrBraceInBrackets
+			}
+
 			braces++
 		case '}':
-			if braces == 0 {
-				break FOR
+			// No way escape } in metric names, thus using it
+			// in the range brackets should be an error.
+			if brackets > 0 {
+				return s, "", ErrBraceInBrackets
+			} else if braces == 0 {
+				return s, "", ErrMissingBrace
 			}
+
 			braces--
+		case '[':
+			// Nested brackets support isn't really necessary as
+			// left bracket [ alone can't be in metric name. And
+			// go-carbon doesn't support it at the moment.
+			//
+			// Before this change, no errors are returned to the
+			// user and no metrics are returned. It's arguably
+			// worse than just return an error.
+			if brackets > 0 {
+				return s, "", ErrNestedBrackets
+			}
+
+			brackets++
+		case ']':
+			// No way to escape braces {} and brackets [] in
+			// graphite query, thus missing open [ means it's a query bug.
+			if brackets == 0 {
+				return s, "", ErrMissingBracket
+			}
+
+			brackets--
 		case ',':
+			// No way to escape a comma in graphite query, thus
+			// metric name is not allowed to have comma within it,
+			// thus it isn't allowed to query it within [].
+			if brackets > 0 {
+				return s, "", ErrCommaInBrackets
+			}
+
 			if braces == 0 {
 				break FOR
 			}
+		case ' ', '\t', '\n':
+			// Spaces is not allowed in metric name, so it isn't
+			// really needed for us to support it in value list
+			// {} and range list [] queries.
+			//
+			// Although it is nice to allow spaces in value list query,
+			// support in storage layer like go-carbon is also required.
+			//
+			// At the same time, if not using any graphite function,
+			// the current parser also doesn't support spaces in
+			// value list syntax {} and would return an 400 error.
+			if braces > 0 {
+				return s, "", ErrSpacesInBraces
+			}
+			if brackets > 0 {
+				return s, "", ErrSpacesInBrackets
+			}
+
+			break FOR
 		default:
 			r, w = utf8.DecodeRuneInString(s[i:])
 			if unicode.In(r, RangeTables...) {
 				continue
 			}
 			break FOR
 		}
+	}
 
+	// No way to escape braces {} and brackets [] in graphite query, thus
+	// missing closed }/] means it's a query bug.
+	if braces > 0 {
+		return s, "", ErrMissingBrace
+	}
+	if brackets > 0 {
+		return s, "", ErrMissingBracket
 	}
 
 	if i == len(s) {
-		return s, ""
+		return s, "", nil
 	}
 
-	return s[:i], s[i:]
+	return s[:i], s[i:], nil
 }
 
 func parseString(s string) (string, string, error) {