diff --git a/lex/dialect_filterql_test.go b/lex/dialect_filterql_test.go index 771148c..ead3ab1 100644 --- a/lex/dialect_filterql_test.go +++ b/lex/dialect_filterql_test.go @@ -2,6 +2,7 @@ package lex import ( "testing" + "time" u "github.com/araddon/gou" "github.com/stretchr/testify/assert" @@ -239,3 +240,194 @@ func TestFilterQLIntersects(t *testing.T) { tv(TokenRightParenthesis, ")"), }) } + +// An unquoted negative numeric literal in a value position must lex as a +// single signed TokenInteger/TokenFloat, not a TokenMinus followed by a +// positive number. +func TestFilterQLNegativeLiteral(t *testing.T) { + verifyFilterQLTokens(t, `FILTER visitct = -1`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenInteger, "-1"), + }) + + verifyFilterQLTokens(t, `FILTER visitct = -1.5`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenFloat, "-1.5"), + }) + + verifyFilterQLTokens(t, `FILTER visitct IN (-1)`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenIN, "IN"), + tv(TokenLeftParenthesis, "("), + tv(TokenInteger, "-1"), + tv(TokenRightParenthesis, ")"), + }) + + verifyFilterQLTokens(t, `FILTER visitct IN (-1, 3)`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenIN, "IN"), + tv(TokenLeftParenthesis, "("), + tv(TokenInteger, "-1"), + tv(TokenComma, ","), + tv(TokenInteger, "3"), + tv(TokenRightParenthesis, ")"), + }) + + verifyFilterQLTokens(t, `FILTER city IN (-1)`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "city"), + tv(TokenIN, "IN"), + tv(TokenLeftParenthesis, "("), + tv(TokenInteger, "-1"), + tv(TokenRightParenthesis, ")"), + }) +} + +// A negative literal must not consume the clause continuation: everything +// after it (infix AND/OR, the rest of a list) still has to lex. +func TestFilterQLNegativeLiteralInfix(t *testing.T) { + verifyFilterQLTokens(t, `FILTER visitct = -1 AND city = "sf"`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenInteger, "-1"), + tv(TokenLogicAnd, "AND"), + tv(TokenIdentity, "city"), + tv(TokenEqual, "="), + tv(TokenValue, "sf"), + }) + + verifyFilterQLTokens(t, `FILTER visitct = -1 OR city = "sf"`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenInteger, "-1"), + tv(TokenLogicOr, "OR"), + tv(TokenIdentity, "city"), + tv(TokenEqual, "="), + tv(TokenValue, "sf"), + }) + + verifyFilterQLTokens(t, `FILTER visitct > -1 AND visitct < 5`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenGT, ">"), + tv(TokenInteger, "-1"), + tv(TokenLogicAnd, "AND"), + tv(TokenIdentity, "visitct"), + tv(TokenLT, "<"), + tv(TokenInteger, "5"), + }) + + verifyFilterQLTokens(t, `FILTER visitct BETWEEN 5 AND -1`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenBetween, "BETWEEN"), + tv(TokenInteger, "5"), + tv(TokenLogicAnd, "AND"), + tv(TokenInteger, "-1"), + }) + + // Sign directly after BETWEEN: the only shape where the previous token is + // TokenBetween itself. + verifyFilterQLTokens(t, `FILTER visitct BETWEEN -5 AND -1`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenBetween, "BETWEEN"), + tv(TokenInteger, "-5"), + tv(TokenLogicAnd, "AND"), + tv(TokenInteger, "-1"), + }) +} + +// A negative anywhere but first in a list: the `,` continuation must survive. +func TestFilterQLNegativeLiteralNotFirstInList(t *testing.T) { + verifyFilterQLTokens(t, `FILTER visitct IN (1, -3)`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenIN, "IN"), + tv(TokenLeftParenthesis, "("), + tv(TokenInteger, "1"), + tv(TokenComma, ","), + tv(TokenInteger, "-3"), + tv(TokenRightParenthesis, ")"), + }) + + verifyFilterQLTokens(t, `FILTER visitct IN ("a", -1)`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenIN, "IN"), + tv(TokenLeftParenthesis, "("), + tv(TokenValue, "a"), + tv(TokenComma, ","), + tv(TokenInteger, "-1"), + tv(TokenRightParenthesis, ")"), + }) +} + +// A sign the number scanner would reject must fall back to TokenMinus rather +// than commit to a literal LexNumber then hard-errors on. +func TestFilterQLSignedLiteralScannerDisagreement(t *testing.T) { + verifyFilterQLTokens(t, `FILTER visitct = -.5`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenMinus, "-"), + tv(TokenIdentity, ".5"), + }) + + verifyFilterQLTokens(t, `FILTER visitct = -0x1A`, + []Token{ + tv(TokenFilter, "FILTER"), + tv(TokenIdentity, "visitct"), + tv(TokenEqual, "="), + tv(TokenMinus, "-"), + tv(TokenInteger, "0x1A"), + }) +} + +// A trailing operator after a negative literal must terminate the scan. An +// unbalanced state stack live-locks here instead, so the whole lex runs on a +// goroutine and the test fails on timeout rather than hanging the suite. +func TestFilterQLNegativeLiteralTrailingOperatorTerminates(t *testing.T) { + for _, ql := range []string{`FILTER visitct = -1-`, `FILTER visitct = -1/`} { + done := make(chan bool, 1) + go func() { + l := NewFilterQLLexer(ql) + for i := 0; i < 100; i++ { + tok := l.NextToken() + if tok.T == TokenEOF || tok.T == TokenError { + done <- true + return + } + } + done <- false + }() + + select { + case ok := <-done: + assert.True(t, ok, "%s must reach EOF or Error within 100 tokens", ql) + case <-time.After(10 * time.Second): + t.Fatalf("%s did not terminate: lexer state stack is unbalanced", ql) + } + } +} diff --git a/lex/lexer.go b/lex/lexer.go index ee007f2..5cf0315 100644 --- a/lex/lexer.go +++ b/lex/lexer.go @@ -1291,6 +1291,14 @@ func LexListOfArgs(l *Lexer) StateFn { l.backup() return LexExpression case '!', '=', '>', '<', '-', '+', '%', '&', '/', '|': + if r == '-' && valueExpectedTokens[l.lastToken.T] && l.numericAfterSign() { + // A negative literal is a single list value, not a binary operator + // between two args: push this list back on so the following `,`/`)` + // is lexed here rather than by the enclosing LexParenRight. + l.backup() + l.Push("LexListOfArgs", LexListOfArgs) + return LexNumber + } l.backup() return LexExpression case ';': @@ -2244,6 +2252,47 @@ func LexLogical(l *Lexer) StateFn { return LexExpression(l) } +// valueExpectedTokens are the previously-emitted tokens after which an +// unquoted `-` begins a signed numeric literal rather than the binary-minus +// operator: comparators, arithmetic operators, open-paren, comma, logic, +// IN/BETWEEN, and the start of input (TokenNil). +var valueExpectedTokens = map[TokenType]bool{ + TokenNil: true, + TokenEqual: true, + TokenEqualEqual: true, + TokenNE: true, + TokenGE: true, + TokenLE: true, + TokenGT: true, + TokenLT: true, + TokenMinus: true, + TokenPlus: true, + TokenMultiply: true, + TokenDivide: true, + TokenModulus: true, + TokenLeftParenthesis: true, + TokenComma: true, + TokenLogicAnd: true, + TokenLogicOr: true, + TokenAnd: true, + TokenOr: true, + TokenIN: true, + TokenBetween: true, +} + +// numericAfterSign reports whether the runes after an already-consumed sign +// begin a literal scanNumericOrDuration will accept. LexNumber runs with +// SUPPORT_DURATION, so signed durations (`-1d`, `-30d`) are included. +func (l *Lexer) numericAfterSign() bool { + next := l.PeekX(2) + if len(next) == 0 || !isDigit(rune(next[0])) { + return false + } + // The scanner refuses a sign before hex, and a committed gate has no + // fallback: LexNumber would hard-error instead of emitting TokenMinus. + return !(len(next) == 2 && next[0] == '0' && (next[1] == 'x' || next[1] == 'X')) +} + // Handle single logical expression which may be nested and has // // user defined function names that are NOT validated by lexer @@ -2313,13 +2362,21 @@ func LexExpression(l *Lexer) StateFn { foundLogical := false foundOperator := false switch r { - case '-': // comment? or minus? + case '-': // negative numeric literal, comment, or minus? p := l.Peek() - if p == '-' { + switch { + case p == '-': l.backup() l.Push("LexExpression", LexExpression) return LexInlineComment - } else { + case valueExpectedTokens[l.lastToken.T] && l.numericAfterSign(): + // LexNumber ends with `return nil`, which pops a frame; push the + // clause continuation so it unwinds into this clause rather than + // consuming the enclosing statement's. + l.backup() + l.Push("LexExpression", l.clauseState()) + return LexNumber + default: l.Emit(TokenMinus) return l.clauseState() } diff --git a/lex/lexer_test.go b/lex/lexer_test.go index 7b15151..21ac483 100644 --- a/lex/lexer_test.go +++ b/lex/lexer_test.go @@ -270,6 +270,83 @@ func TestLexDuration(t *testing.T) { } } +// Binary minus (subtraction) must be unaffected by the signed-numeric-literal +// fix: LexExpression is shared with the SQL dialect, and `a - b` / `5 - 3` +// have an identity/number as the previous token, not a value-expected one. +func TestLexBinaryMinusUnchanged(t *testing.T) { + verifyTokens(t, `SELECT a - b FROM x`, + []Token{ + tv(TokenSelect, "SELECT"), + tv(TokenIdentity, "a"), + tv(TokenMinus, "-"), + tv(TokenIdentity, "b"), + tv(TokenFrom, "FROM"), + tv(TokenIdentity, "x"), + }) + + verifyTokens(t, `SELECT 5 - 3 FROM x`, + []Token{ + tv(TokenSelect, "SELECT"), + tv(TokenInteger, "5"), + tv(TokenMinus, "-"), + tv(TokenInteger, "3"), + tv(TokenFrom, "FROM"), + tv(TokenIdentity, "x"), + }) + + // The column-list cases above never reach the changed branch; a WHERE + // clause does, so these are what actually guard it. + verifyTokens(t, `SELECT a FROM t WHERE (x - 1) > 5`, + []Token{ + tv(TokenSelect, "SELECT"), + tv(TokenIdentity, "a"), + tv(TokenFrom, "FROM"), + tv(TokenIdentity, "t"), + tv(TokenWhere, "WHERE"), + tv(TokenLeftParenthesis, "("), + tv(TokenIdentity, "x"), + tv(TokenMinus, "-"), + tv(TokenInteger, "1"), + tv(TokenRightParenthesis, ")"), + tv(TokenGT, ">"), + tv(TokenInteger, "5"), + }) + + verifyTokens(t, `SELECT a FROM t WHERE x > 5 - 3`, + []Token{ + tv(TokenSelect, "SELECT"), + tv(TokenIdentity, "a"), + tv(TokenFrom, "FROM"), + tv(TokenIdentity, "t"), + tv(TokenWhere, "WHERE"), + tv(TokenIdentity, "x"), + tv(TokenGT, ">"), + tv(TokenInteger, "5"), + tv(TokenMinus, "-"), + tv(TokenInteger, "3"), + }) +} + +// A signed literal in a SQL WHERE clause must lex as one token and leave the +// infix continuation intact, same as FilterQL. +func TestLexSignedLiteralInWhere(t *testing.T) { + verifyTokens(t, `SELECT a FROM t WHERE age > -1 AND name = "bob"`, + []Token{ + tv(TokenSelect, "SELECT"), + tv(TokenIdentity, "a"), + tv(TokenFrom, "FROM"), + tv(TokenIdentity, "t"), + tv(TokenWhere, "WHERE"), + tv(TokenIdentity, "age"), + tv(TokenGT, ">"), + tv(TokenInteger, "-1"), + tv(TokenLogicAnd, "AND"), + tv(TokenIdentity, "name"), + tv(TokenEqual, "="), + tv(TokenValue, "bob"), + }) +} + func verifyTokens(t *testing.T, sql string, tokens []Token) { l := NewSqlLexer(sql) u.Debugf("sql: %v", sql) diff --git a/rel/parse_filterql_test.go b/rel/parse_filterql_test.go index 998e4f6..dec52b1 100644 --- a/rel/parse_filterql_test.go +++ b/rel/parse_filterql_test.go @@ -100,6 +100,23 @@ var FilterTests = []string{ LIMIT 100 -- and some more `, + // Unquoted negative numeric literals must round-trip like any other value + // literal, on both int- and string-named fields. + `FILTER visitct = -1`, + `FILTER visitct = -1.5`, + `FILTER city = -1`, + `FILTER visitct IN (-1)`, + `FILTER visitct IN (-1, 3)`, + `FILTER visitct IN (1, -3)`, + `FILTER visitct IN ("a", -1)`, + `FILTER city IN (-1)`, + // A negative literal must not swallow the clause continuation. + `FILTER visitct = -1 AND city = "sf"`, + `FILTER visitct = -1 OR city = "sf"`, + `FILTER visitct > -1 AND visitct < 5`, + `FILTER visitct != -1 AND city = "sf"`, + `FILTER visitct = -1.5 AND city = "sf"`, + `FILTER visitct BETWEEN 5 AND -1`, } func init() { @@ -227,6 +244,146 @@ func TestFilterQlRoundTrip(t *testing.T) { } } +// numberNodesOf extracts the *expr.NumberNode(s) from a comparison's RHS, +// which is either a bare NumberNode (`=`) or an ArrayNode of them (`IN`). +func numberNodesOf(t *testing.T, rhs expr.Node) []*expr.NumberNode { + t.Helper() + switch rhs := rhs.(type) { + case *expr.NumberNode: + return []*expr.NumberNode{rhs} + case *expr.ArrayNode: + nums := make([]*expr.NumberNode, len(rhs.Args)) + for i, arg := range rhs.Args { + n, ok := arg.(*expr.NumberNode) + require.True(t, ok, "expected *expr.NumberNode array element, got %T", arg) + nums[i] = n + } + return nums + default: + t.Fatalf("expected *expr.NumberNode or *expr.ArrayNode, got %T", rhs) + return nil + } +} + +// A negative literal followed by an infix AND/OR must still parse: the sign +// handling must leave the clause continuation on the lexer's state stack. +func TestFilterQLNegativeLiteralsInfix(t *testing.T) { + t.Parallel() + + for _, ql := range []string{ + `FILTER visitct = -1 AND city = "sf" FROM user`, + `FILTER visitct = -1 OR city = "sf" FROM user`, + `FILTER visitct > -1 AND visitct < 5 FROM user`, + `FILTER visitct != -1 AND city = "sf" FROM user`, + `FILTER visitct = -1.5 AND city = "sf" FROM user`, + `FILTER visitct BETWEEN 5 AND -1 FROM user`, + // An IN list only combines via the prefix form; `IN (..) AND ..` is a + // pre-existing FilterQL limitation, unrelated to the sign. + `FILTER AND ( visitct IN (1, -3), city = "sf" ) FROM user`, + } { + req, err := rel.ParseFilterQL(ql) + require.NoError(t, err, "must parse %s", ql) + assert.Equal(t, ql, req.String(), "canonical form for %s", ql) + + req2, err := rel.ParseFilterQL(req.String()) + require.NoError(t, err, "must reparse %q", req.String()) + assert.Equal(t, req.String(), req2.String(), "round-trip must be idempotent for %s", ql) + } +} + +// A sign the number scanner refuses must stay lexable rather than becoming a +// hard parse error. +func TestFilterQLSignedLiteralScannerDisagreement(t *testing.T) { + t.Parallel() + + for _, ql := range []string{ + `FILTER visitct = -.5 FROM user`, + `FILTER visitct = -0x1A FROM user`, + } { + _, err := rel.ParseFilterQL(ql) + require.NoError(t, err, "must parse %s", ql) + } +} + +func TestFilterQLNegativeLiterals(t *testing.T) { + t.Parallel() + + tests := []struct { + ql string + wantText []string + }{ + {`FILTER visitct = -1 FROM user`, []string{"-1"}}, + {`FILTER city = -1 FROM user`, []string{"-1"}}, + {`FILTER visitct = -1.5 FROM user`, []string{"-1.5"}}, + {`FILTER visitct IN (-1) FROM user`, []string{"-1"}}, + {`FILTER visitct IN (-1, 3) FROM user`, []string{"-1", "3"}}, + {`FILTER city IN (-1) FROM user`, []string{"-1"}}, + } + + for _, tc := range tests { + req, err := rel.ParseFilterQL(tc.ql) + require.NoError(t, err, "must parse %s", tc.ql) + + bn, ok := req.Filter.(*expr.BinaryNode) + require.True(t, ok, "expected *expr.BinaryNode for %s, got %T", tc.ql, req.Filter) + + nums := numberNodesOf(t, bn.Args[1]) + require.Len(t, nums, len(tc.wantText), "element count for %s", tc.ql) + for i, n := range nums { + assert.Equal(t, tc.wantText[i], n.Text, "signed literal text for %s", tc.ql) + } + + // The canonical form must be bare (e.g. `-1`), never `- (1)`, and + // re-parsing it must reproduce the same canonical string. + out := req.String() + assert.Equal(t, tc.ql, out, "canonical form for %s", tc.ql) + req2, err := rel.ParseFilterQL(out) + require.NoError(t, err, "must reparse canonical form %q", out) + assert.Equal(t, out, req2.String(), "round-trip must be idempotent for %s", tc.ql) + } + + // Positive and quoted forms must be unaffected. + req, err := rel.ParseFilterQL(`FILTER visitct = 1 FROM user`) + require.NoError(t, err) + bn := req.Filter.(*expr.BinaryNode) + n, ok := bn.Args[1].(*expr.NumberNode) + require.True(t, ok, "expected *expr.NumberNode, got %T", bn.Args[1]) + assert.Equal(t, "1", n.Text) + + req, err = rel.ParseFilterQL(`FILTER visitct = "-1" FROM user`) + require.NoError(t, err) + bn = req.Filter.(*expr.BinaryNode) + sn, ok := bn.Args[1].(*expr.StringNode) + require.True(t, ok, "expected *expr.StringNode, got %T", bn.Args[1]) + assert.Equal(t, "-1", sn.Text) +} + +// TestFilterQLNegativeLiteralDirectASTRoundTrip covers the stored-QL +// round-trip bug from the ticket: a NumberNode{Text:"-1"} built directly +// (as expr.NodeFromExpr would from a stored JSON AST, bypassing the lexer) +// must print bare `-1` and that printed form must re-parse to an equivalent +// NumberNode. +func TestFilterQLNegativeLiteralDirectASTRoundTrip(t *testing.T) { + t.Parallel() + + num, err := expr.NewNumberStr("-1") + require.NoError(t, err) + + fs := rel.NewFilterStatement() + fs.Filter = expr.NewBinaryNode(lex.Token{T: lex.TokenEqual, V: "="}, expr.NewIdentityNodeVal("visitct"), num) + + out := fs.String() + assert.Equal(t, `FILTER visitct = -1`, out) + + req2, err := rel.ParseFilterQL(out) + require.NoError(t, err, "must reparse %q", out) + bn2, ok := req2.Filter.(*expr.BinaryNode) + require.True(t, ok, "expected *expr.BinaryNode, got %T", req2.Filter) + n2, ok := bn2.Args[1].(*expr.NumberNode) + require.True(t, ok, "expected *expr.NumberNode, got %T", bn2.Args[1]) + assert.Equal(t, "-1", n2.Text) +} + func TestFilterQlFingerPrint(t *testing.T) { t.Parallel()