(#1952) Fix fact filter parsing

With the addition of GJson to describe fact values the old fact parser is no
longer able to correctly identify facts vs operators in examples like
`'storage.#(name=="vda").size'`.

Here we rewrite the ParseFactFilterString function to no longer used complicated
regular expressions. We will now correctly identify GJson facts and parse the
filters correctly.

In addition we're adding a wider range of tests for ParseFactFilterString so
that we're covered if we were to add more fact filter functionality in the
future.

Author: ploubser

filter/facts/facts.go

@@ -18,6 +18,8 @@ import (
 	"github.com/choria-io/go-choria/internal/util"
 )
 
+var validOperators = regexp.MustCompile(`<=|>=|=>|=<|<|>|!=|=~|={1,2}`)
+
 // Logger provides logging facilities
 type Logger interface {
 	Warnf(format string, args ...any)
@@ -173,18 +175,49 @@ func HasFact(fact string, operator string, value string, file string, log Logger
 }
 
 // ParseFactFilterString parses a fact filter string as typically typed on the CLI
-func ParseFactFilterString(f string) (pf [3]string, err error) {
-	if matched := regexp.MustCompile("^([^ ]+?)[ ]*=>[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], ">=", matched[2]}, nil
-	} else if matched := regexp.MustCompile("^([^ ]+?)[ ]*=<[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "<=", matched[2]}, nil
-	} else if matched := regexp.MustCompile("^([^ ]+?)[ ]*(<=|>=|<|>|!=|==|=~)[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], matched[2], matched[3]}, nil
-	} else if matched := regexp.MustCompile("^(.+?)[ ]*=[ ]*/(.+)/$").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "=~", "/" + matched[2] + "/"}, nil
-	} else if matched := regexp.MustCompile("^([^= ]+?)[ ]*=[ ]*(.+)").FindStringSubmatch(f); len(matched) > 0 {
-		return [3]string{matched[1], "==", matched[2]}, nil
+func ParseFactFilterString(f string) ([3]string, error) {
+	operatorIndexes := validOperators.FindAllStringIndex(f, -1)
+	var mainOpIndex []int
+
+	if opCount := len(operatorIndexes); opCount > 1 {
+		// This is a special case where the left operand contains a valid operator.
+		// We skip over everything and use the right most operator.
+		mainOpIndex = operatorIndexes[len(operatorIndexes)-1]
+	} else if opCount == 1 {
+		mainOpIndex = operatorIndexes[0]
 	} else {
 		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
 	}
+
+	op := f[mainOpIndex[0]:mainOpIndex[1]]
+	leftOp := strings.TrimSpace(f[:mainOpIndex[0]])
+	rightOp := strings.TrimSpace(f[mainOpIndex[1]:])
+
+	// validate that the left and right operands are both valid
+	if len(leftOp) == 0 || len(rightOp) == 0 {
+		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
+	}
+
+	lStartString := string(leftOp[0])
+	rEndString := string(rightOp[len(rightOp)-1])
+	if validOperators.MatchString(lStartString) || validOperators.Match([]byte(rEndString)) {
+		return [3]string{}, fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", f)
+	}
+
+	// transform op and value for processing
+	switch op {
+	case "=":
+		op = "=="
+	case "=<":
+		op = "<="
+	case "=>":
+		op = ">="
+	}
+
+	// finally check for old style regex fact matches
+	if rightOp[0] == '/' && rightOp[len(rightOp)-1] == '/' {
+		op = "=~"
+	}
+
+	return [3]string{leftOp, op, rightOp}, nil
 }

filter/filter_test.go

@@ -5,6 +5,7 @@
 package filter
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/choria-io/go-choria/protocol"
@@ -160,10 +161,12 @@ var _ = Describe("Filter", func() {
 			t("foo == bar", "foo", "==", "bar")
 		})
 
-		It("Should fail for facts in the wrong format", func() {
-			pf, err := ParseFactFilterString("foo")
-			Expect(err).To(MatchError("could not parse fact foo it does not appear to be in a valid format"))
-			Expect(pf).To(BeNil())
+		It("Should fail on invalid fact filters", func() {
+			badFilters := []string{"foobarbaz", "=foo(bar=baz)", "foo=", "foo(bar=baz)>", "=><=="}
+			for _, filterString := range badFilters {
+				_, err := ParseFactFilterString(filterString)
+				Expect(err).To(MatchError(fmt.Errorf("could not parse fact %s it does not appear to be in a valid format", filterString)))
+			}
 		})
 	})
 })