From 0ed132f64261c973f6e02d39631b8361fbdc60f3 Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Wed, 9 Aug 2023 15:24:10 +0200 Subject: [PATCH] Fix crash at trailing comma in FROM clause --- sqlparser/lib/src/reader/parser.dart | 25 ++++++++++++--------- sqlparser/test/parser/errors_test.dart | 30 +++++++++++--------------- sqlparser/test/parser/utils.dart | 15 ++++++++----- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/sqlparser/lib/src/reader/parser.dart b/sqlparser/lib/src/reader/parser.dart index 01b60f86..01811f4e 100644 --- a/sqlparser/lib/src/reader/parser.dart +++ b/sqlparser/lib/src/reader/parser.dart @@ -62,7 +62,12 @@ class Parser { bool get _isAtEnd => _peek.type == TokenType.eof; Token get _peek => tokens[_current]; - Token get _peekNext => tokens[_current + 1]; + Token? get _peekNext { + if (_isAtEnd) return null; + + return tokens[_current + 1]; + } + Token get _previous => tokens[_current - 1]; bool _match(Iterable types) { @@ -96,14 +101,14 @@ class Parser { /// "NOT" followed by [type]. Does not consume any tokens. bool _checkWithNot(TokenType type) { if (_check(type)) return true; - if (_check(TokenType.not) && _peekNext.type == type) return true; + if (_check(TokenType.not) && _peekNext?.type == type) return true; return false; } /// Like [_checkWithNot], but with more than one token type. bool _checkAnyWithNot(List types) { if (types.any(_check)) return true; - if (_check(TokenType.not) && types.contains(_peekNext.type)) return true; + if (_check(TokenType.not) && types.contains(_peekNext?.type)) return true; return false; } @@ -866,7 +871,7 @@ class Parser { if (_peek is KeywordToken) { // Improve error messages for possible function calls, https://github.com/simolus3/drift/discussions/2277 if (tokens.length > _current + 1 && - _peekNext.type == TokenType.leftParen) { + _peekNext?.type == TokenType.leftParen) { _error( 'Expected an expression here, but got a reserved keyword. Did you ' 'mean to call a function? Try wrapping the keyword in double quotes.', @@ -1346,6 +1351,7 @@ class Parser { // Can either be a list of or a join. Joins also start // with a TableOrSubquery, so let's first parse that. final start = _tableOrSubquery(); + // parse join, if there is one return _joinClause(start) ?? start; } @@ -1399,7 +1405,7 @@ class Parser { final joins = []; while (operator != null) { - final first = _peekNext; + final first = _peek; final subquery = _tableOrSubquery(); final constraint = _joinConstraint(); @@ -2786,10 +2792,8 @@ class Parser { DeferrableClause? deferrable; if (_checkWithNot(TokenType.deferrable)) { - final first = _peekNext; - - final not = _matchOne(TokenType.not); - _consume(TokenType.deferrable); + final not = _matchOneAndGet(TokenType.not); + final deferrableToken = _consume(TokenType.deferrable); InitialDeferrableMode? mode; if (_matchOne(TokenType.initially)) { @@ -2802,7 +2806,8 @@ class Parser { } } - deferrable = DeferrableClause(not, mode)..setSpan(first, _previous); + deferrable = DeferrableClause(not != null, mode) + ..setSpan(not ?? deferrableToken, _previous); } return ForeignKeyClause( diff --git a/sqlparser/test/parser/errors_test.dart b/sqlparser/test/parser/errors_test.dart index 112c965b..9785e826 100644 --- a/sqlparser/test/parser/errors_test.dart +++ b/sqlparser/test/parser/errors_test.dart @@ -1,6 +1,8 @@ import 'package:sqlparser/sqlparser.dart'; import 'package:test/test.dart'; +import 'utils.dart'; + void main() { test('WITH without following statement', () { expectError('WITH foo AS (SELECT * FROM bar)', @@ -12,7 +14,7 @@ void main() { expectError('SELECT replace(a, b, c);', [ isParsingError( message: contains('Did you mean to call a function?'), - lexeme: 'replace', + span: 'replace', ), ]); }); @@ -21,14 +23,22 @@ void main() { expectError('SELECT group FROM foo;', [ isParsingError( message: contains('Did you mean to use it as a column?'), - lexeme: 'group', + span: 'group', ), ]); expectError('CREATE TABLE x (table TEXT NOT NULL, foo INTEGER);', [ isParsingError( message: 'Expected a column name (got keyword TABLE)', - lexeme: 'table', + span: 'table', + ), + ]); + }); + + test('recovers from invalid comma after table reference', () { + expectError('SELECT * FROM table_name,', [ + isParsingError( + message: 'Expected a table name or a nested select statement', ), ]); }); @@ -40,17 +50,3 @@ void expectError(String sql, errorsMatcher) { expect(parsed.errors, errorsMatcher); } - -TypeMatcher isParsingError({message, lexeme}) { - var matcher = isA(); - - if (lexeme != null) { - matcher = matcher.having((e) => e.token.lexeme, 'token.lexeme', lexeme); - } - - if (message != null) { - matcher = matcher.having((e) => e.message, 'message', message); - } - - return matcher; -} diff --git a/sqlparser/test/parser/utils.dart b/sqlparser/test/parser/utils.dart index 69c96ca7..ff174bdc 100644 --- a/sqlparser/test/parser/utils.dart +++ b/sqlparser/test/parser/utils.dart @@ -54,11 +54,16 @@ void expectParseError( }) { final result = SqlEngine().parse(sql); - expect(result.errors, [ - isA() - .having((e) => e.message, 'message', wrapMatcher(message)) - .having((e) => e.token.span.text, 'span', wrapMatcher(span)) - ]); + expect(result.errors, [isParsingError(message: message, span: span)]); +} + +TypeMatcher isParsingError({ + dynamic message = anything, + dynamic span = anything, +}) { + return isA() + .having((e) => e.message, 'message', wrapMatcher(message)) + .having((e) => e.token.span.text, 'span', wrapMatcher(span)); } FileSpan fakeSpan(String content) {