Merge pull request #1622 from grafana/expr-more-lenient

expr: be more lenient: allow quoted ints and floats

expr/expr.go

@@ -101,15 +101,34 @@ func (e expr) consumeBasicArg(pos int, exp Arg) (int, error) {
 		}
 		return 0, ErrBadArgumentStr{strings.Join(expStr, ","), got.etype.String()}
 	case ArgInt:
-		if got.etype != etInt {
-			return 0, ErrBadArgumentStr{"int", got.etype.String()}
-		}
-		for _, va := range v.validator {
-			if err := va(got); err != nil {
-				return 0, generateValidatorError(v.key, err)
+		switch got.etype {
+		case etInt:
+			for _, va := range v.validator {
+				if err := va(got); err != nil {
+					return 0, generateValidatorError(v.key, err)
+				}
 			}
+			*v.val = got.int
+		case etString:
+			// allow "2", '5', etc. some people put quotes around their ints.
+			i, ok := strToInt(got.str)
+			if !ok {
+				return 0, ErrBadArgumentStr{"int", got.etype.String()}
+			}
+			gotFake := expr{
+				str:   got.str,
+				int:   int64(i),
+				etype: etInt,
+			}
+			for _, va := range v.validator {
+				if err := va(&gotFake); err != nil {
+					return 0, generateValidatorError(v.key, err)
+				}
+			}
+			*v.val = int64(i)
+		default:
+			return 0, ErrBadArgumentStr{"int", got.etype.String()}
 		}
-		*v.val = got.int
 	case ArgInts:
 		// consume all args (if any) in args that will yield an integer
 		for ; pos < len(e.args) && e.args[pos].etype == etInt; pos++ {
@@ -122,20 +141,40 @@ func (e expr) consumeBasicArg(pos int, exp Arg) (int, error) {
 		}
 		return pos, nil
 	case ArgFloat:
+		switch got.etype {
 		// integer is also a valid float, just happened to have no decimals
-		if got.etype != etFloat && got.etype != etInt {
-			return 0, ErrBadArgumentStr{"float", got.etype.String()}
-		}
-		for _, va := range v.validator {
-			if err := va(got); err != nil {
-				return 0, generateValidatorError(v.key, err)
+		case etInt, etFloat:
+			for _, va := range v.validator {
+				if err := va(got); err != nil {
+					return 0, generateValidatorError(v.key, err)
+				}
 			}
+			if got.etype == etInt {
+				*v.val = float64(got.int)
+			} else {
+				*v.val = got.float
+			}
+		case etString:
+			// allow "2.5", '5.4', etc. some people put quotes around their floats.
+			f, ok := strToFloat(got.str)
+			if !ok {
+				return 0, ErrBadArgumentStr{"int", got.etype.String()}
+			}
+			gotFake := expr{
+				str:   got.str,
+				float: f,
+				etype: etFloat,
+			}
+			for _, va := range v.validator {
+				if err := va(&gotFake); err != nil {
+					return 0, generateValidatorError(v.key, err)
+				}
+			}
+			*v.val = f
+		default:
+			return 0, ErrBadArgumentStr{"float", got.etype.String()}
 		}
-		if got.etype == etInt {
-			*v.val = float64(got.int)
-		} else {
-			*v.val = got.float
-		}
+
 	case ArgRegex:
 		if got.etype != etString {
 			return 0, ErrBadArgumentStr{"string (regex)", got.etype.String()}

expr/parse.go

@@ -190,6 +190,38 @@ func strToBool(val string) (bool, bool) {
 	return false, false
 }
 
+func strToInt(val string) (int, bool) {
+	if len(val) <= 2 {
+		return 0, false
+	}
+	if val[0] == '"' && val[len(val)-1] == '"' {
+		val = val[1 : len(val)-1]
+	} else if val[0] == '\'' && val[len(val)-1] == '\'' {
+		val = val[1 : len(val)-1]
+	}
+	i, err := strconv.Atoi(val)
+	if err != nil {
+		return 0, false
+	}
+	return i, true
+}
+
+func strToFloat(val string) (float64, bool) {
+	if len(val) <= 2 {
+		return 0, false
+	}
+	if val[0] == '"' && val[len(val)-1] == '"' {
+		val = val[1 : len(val)-1]
+	} else if val[0] == '\'' && val[len(val)-1] == '\'' {
+		val = val[1 : len(val)-1]
+	}
+	f, err := strconv.ParseFloat(val, 64)
+	if err != nil {
+		return 0, false
+	}
+	return f, true
+}
+
 // caller must assure s starts with opening paren
 func parseArgList(e string) (string, []*expr, map[string]*expr, string, error) {
 

expr/plan_test.go

@@ -9,6 +9,7 @@ import (
 	"github.com/grafana/metrictank/consolidation"
 )
 
+// TestArgs tests that after planning the given args against smartSummarize, the right error or requests come out
 // here we use smartSummarize because it has multiple optional arguments which allows us to test some interesting things
 func TestArgs(t *testing.T) {
 
@@ -206,6 +207,64 @@ func TestArgs(t *testing.T) {
 	}
 }
 
+// TestArgQuotedInt tests that a function (perSecond in this case) can be given a quoted integer argument
+func TestArgQuotedInt(t *testing.T) {
+	cases := []expr{
+		{etype: etInt, int: 5},
+		{etype: etString, str: "'5'"},
+		{etype: etString, str: "\"5\""},
+	}
+	for _, ourArg := range cases {
+		fn := NewPerSecond()
+		e := &expr{
+			etype: etFunc,
+			str:   "perSecond",
+			args: []*expr{
+				{etype: etName, str: "in.*"},
+				&ourArg,
+			},
+			namedArgs: nil,
+		}
+		_, err := newplanFunc(e, fn, Context{from: 0, to: 1000}, true, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		ap := fn.(*FuncPerSecond)
+		if ap.maxValue != 5 {
+			t.Fatalf("maxValue should be 5. got %d", ap.maxValue)
+		}
+	}
+}
+
+// TestArgQuotedFloat tests that a function (asPercent in this case) can be given a quoted floating point argument
+func TestArgQuotedFloat(t *testing.T) {
+	cases := []expr{
+		{etype: etFloat, float: 5.0},
+		{etype: etString, str: "'5.0'"},
+		{etype: etString, str: "\"5.0\""},
+	}
+	for _, ourArg := range cases {
+		fn := NewAsPercent()
+		e := &expr{
+			etype: etFunc,
+			str:   "asPercent",
+			args: []*expr{
+				{etype: etName, str: "in.*"},
+				&ourArg,
+			},
+			namedArgs: nil,
+		}
+		_, err := newplanFunc(e, fn, Context{from: 0, to: 1000}, true, nil)
+		if err != nil {
+			t.Fatal(err)
+		}
+		ap := fn.(*FuncAsPercent)
+		if ap.totalFloat != 5 {
+			t.Fatalf("totalFloat should be 5. got %f", ap.totalFloat)
+		}
+	}
+}
+
 // for the ArgIn tests, we use perSecond because it has a nice example of an ArgIn that can be specified via position or keyword.
 func TestArgInMissing(t *testing.T) {
 	fn := NewAsPercent()