From 1a2d3bdee75d6dc4defeef480283c38db5fba1fb Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Sun, 22 Mar 2020 11:51:39 +0100 Subject: [PATCH] Parse comma separated tables as proper join (#453) --- sqlparser/CHANGELOG.md | 2 + .../src/analysis/steps/column_resolver.dart | 4 +- .../analysis/steps/reference_resolver.dart | 4 +- sqlparser/lib/src/ast/common/queryables.dart | 4 +- sqlparser/lib/src/ast/statements/select.dart | 4 +- sqlparser/lib/src/reader/parser/crud.dart | 52 +++++----- sqlparser/lib/src/reader/parser/parser.dart | 8 -- .../analysis/reference_resolver_test.dart | 2 +- .../test/parser/create_trigger_test.dart | 2 +- sqlparser/test/parser/expression_test.dart | 8 +- sqlparser/test/parser/inline_dart_test.dart | 8 +- sqlparser/test/parser/insert_test.dart | 2 +- sqlparser/test/parser/moor_file_test.dart | 8 +- .../test/parser/multiple_statements.dart | 6 +- .../select/common_table_expression_test.dart | 4 +- .../test/parser/select/compound_test.dart | 2 +- sqlparser/test/parser/select/from_test.dart | 98 +++++++++++++------ .../test/parser/select/generic_test.dart | 10 +- 18 files changed, 123 insertions(+), 105 deletions(-) diff --git a/sqlparser/CHANGELOG.md b/sqlparser/CHANGELOG.md index a41ad65e..b53759cd 100644 --- a/sqlparser/CHANGELOG.md +++ b/sqlparser/CHANGELOG.md @@ -1,6 +1,8 @@ ## unreleased - Remove `SqlEngine.withOptions` constructor - just use the regular one +- Changed `SelectStatement.from` from `List` to `Queryable?`. Selecting from multiple + tables with a comma will now be parsed as a `JoinClause`. ## 0.7.0 diff --git a/sqlparser/lib/src/analysis/steps/column_resolver.dart b/sqlparser/lib/src/analysis/steps/column_resolver.dart index 08035adf..4b5971a7 100644 --- a/sqlparser/lib/src/analysis/steps/column_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/column_resolver.dart @@ -170,8 +170,8 @@ class ColumnResolver extends RecursiveVisitor { void _resolveSelect(SelectStatement s) { final availableColumns = []; - for (final queryable in s.from) { - _handle(queryable, availableColumns); + if (s.from != null) { + _handle(s.from, availableColumns); } final usedColumns = []; diff --git a/sqlparser/lib/src/analysis/steps/reference_resolver.dart b/sqlparser/lib/src/analysis/steps/reference_resolver.dart index 33dbc9bb..a03366af 100644 --- a/sqlparser/lib/src/analysis/steps/reference_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/reference_resolver.dart @@ -89,11 +89,11 @@ class ReferenceResolver extends RecursiveVisitor { orElse: () => null) as SelectStatement; if (select == null) return null; - if (select.from.length != 1 || select.from.single is! TableReference) { + if (select.from is! TableReference) { return null; } - final table = (select.from.single as TableReference).resolved as Table; + final table = (select.from as TableReference).resolved as Table; if (table == null) return null; // table.findColumn contains logic to resolve row id aliases diff --git a/sqlparser/lib/src/ast/common/queryables.dart b/sqlparser/lib/src/ast/common/queryables.dart index 425ab406..2e4e8b28 100644 --- a/sqlparser/lib/src/ast/common/queryables.dart +++ b/sqlparser/lib/src/ast/common/queryables.dart @@ -116,13 +116,13 @@ class Join extends AstNode { final bool natural; final JoinOperator operator; final TableOrSubquery query; - final JoinConstraint constraint; + final JoinConstraint /*?*/ constraint; Join( {this.natural = false, @required this.operator, @required this.query, - @required this.constraint}); + this.constraint}); @override Iterable get childNodes { diff --git a/sqlparser/lib/src/ast/statements/select.dart b/sqlparser/lib/src/ast/statements/select.dart index 149dc7af..c1a36165 100644 --- a/sqlparser/lib/src/ast/statements/select.dart +++ b/sqlparser/lib/src/ast/statements/select.dart @@ -13,7 +13,7 @@ class SelectStatement extends BaseSelectStatement implements StatementWithWhere { final bool distinct; final List columns; - final List from; + final Queryable /*?*/ from; @override final Expression where; @@ -45,7 +45,7 @@ class SelectStatement extends BaseSelectStatement return [ if (withClause != null) withClause, ...columns, - if (from != null) ...from, + if (from != null) from, if (where != null) where, if (groupBy != null) groupBy, for (var windowDecl in windowDeclarations) windowDecl.definition, diff --git a/sqlparser/lib/src/reader/parser/crud.dart b/sqlparser/lib/src/reader/parser/crud.dart index 503a6686..b25e9db2 100644 --- a/sqlparser/lib/src/reader/parser/crud.dart +++ b/sqlparser/lib/src/reader/parser/crud.dart @@ -1,5 +1,14 @@ part of 'parser.dart'; +const _startJoinOperators = [ + TokenType.natural, + TokenType.left, + TokenType.inner, + TokenType.cross, + TokenType.join, + TokenType.comma, +]; + mixin CrudParser on ParserBase { CrudStatement _crud() { final withClause = _withClause(); @@ -235,25 +244,14 @@ mixin CrudParser on ParserBase { } } - List _from() { - if (!_matchOne(TokenType.from)) return []; + Queryable /*?*/ _from() { + if (!_matchOne(TokenType.from)) return null; // 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 it is one - final join = _joinClause(start); - if (join != null) { - return [join]; - } - - // not a join. Keep the TableOrSubqueries coming! - final queries = [start]; - while (_matchOne(TokenType.comma)) { - queries.add(_tableOrSubquery()); - } - - return queries; + // parse join, if there is one + return _joinClause(start) ?? start; } TableOrSubquery _tableOrSubquery() { @@ -306,7 +304,7 @@ mixin CrudParser on ParserBase { } JoinClause _joinClause(TableOrSubquery start) { - var operator = _parseJoinOperatorNoComma(); + var operator = _parseJoinOperator(); if (operator == null) { return null; } @@ -341,24 +339,21 @@ mixin CrudParser on ParserBase { )..setSpan(first, _previous)); // parse the next operator, if there is more than one join - if (_matchOne(TokenType.comma)) { - operator = [TokenType.comma]; - } else { - operator = _parseJoinOperatorNoComma(); - } + operator = _parseJoinOperator(); } return JoinClause(primary: start, joins: joins) ..setSpan(start.first, _previous); } - /// Parses https://www.sqlite.org/syntax/join-operator.html, minus the comma. - List _parseJoinOperatorNoComma() { - if (_match(_startOperators)) { + /// Parses https://www.sqlite.org/syntax/join-operator.html + List _parseJoinOperator() { + if (_match(_startJoinOperators)) { final operators = [_previous.type]; - if (_previous.type == TokenType.join) { - // just join, without any specific operators + if (_previous.type == TokenType.join || + _previous.type == TokenType.comma) { + // just join or comma, without any specific operators return operators; } else { // natural is a prefix, another operator can follow. @@ -379,7 +374,7 @@ mixin CrudParser on ParserBase { } /// Parses https://www.sqlite.org/syntax/join-constraint.html - JoinConstraint _joinConstraint() { + JoinConstraint /*?*/ _joinConstraint() { if (_matchOne(TokenType.on)) { return OnConstraint(expression: expression()); } else if (_matchOne(TokenType.using)) { @@ -395,8 +390,9 @@ mixin CrudParser on ParserBase { _consume(TokenType.rightParen, 'Expected an closing paranthesis'); return UsingConstraint(columnNames: columnNames); + } else { + return null; } - _error('Expected a constraint with ON or USING'); } /// Parses a where clause if there is one at the current position diff --git a/sqlparser/lib/src/reader/parser/parser.dart b/sqlparser/lib/src/reader/parser/parser.dart index 57238d75..ffce1ea7 100644 --- a/sqlparser/lib/src/reader/parser/parser.dart +++ b/sqlparser/lib/src/reader/parser/parser.dart @@ -22,14 +22,6 @@ const _binaryOperators = [ TokenType.pipe, ]; -const _startOperators = [ - TokenType.natural, - TokenType.left, - TokenType.inner, - TokenType.cross, - TokenType.join, -]; - class ParsingError implements Exception { final Token token; final String message; diff --git a/sqlparser/test/analysis/reference_resolver_test.dart b/sqlparser/test/analysis/reference_resolver_test.dart index ccbdbf45..811f02f8 100644 --- a/sqlparser/test/analysis/reference_resolver_test.dart +++ b/sqlparser/test/analysis/reference_resolver_test.dart @@ -31,7 +31,7 @@ void main() { final firstColumn = select.columns[0] as ExpressionResultColumn; final secondColumn = select.columns[1] as ExpressionResultColumn; - final from = select.from[0] as TableReference; + final from = select.from as TableReference; expect((firstColumn.expression as Reference).resolved, id); expect((secondColumn.expression as Reference).resolved, content); diff --git a/sqlparser/test/parser/create_trigger_test.dart b/sqlparser/test/parser/create_trigger_test.dart index 6b9f62da..e503e5de 100644 --- a/sqlparser/test/parser/create_trigger_test.dart +++ b/sqlparser/test/parser/create_trigger_test.dart @@ -12,7 +12,7 @@ final _block = Block([ ]), SelectStatement( columns: [StarResultColumn()], - from: [TableReference('tbl')], + from: TableReference('tbl'), ), ]); diff --git a/sqlparser/test/parser/expression_test.dart b/sqlparser/test/parser/expression_test.dart index b21315f8..d557c352 100644 --- a/sqlparser/test/parser/expression_test.dart +++ b/sqlparser/test/parser/expression_test.dart @@ -77,9 +77,7 @@ final Map _testCases = { ExistsExpression( select: SelectStatement( columns: [StarResultColumn(null)], - from: [ - TableReference('demo', null), - ], + from: TableReference('demo', null), ), ), ), @@ -114,9 +112,7 @@ final Map _testCases = { expression: Reference(columnName: 'col'), ) ], - from: [ - TableReference('tbl', null), - ], + from: TableReference('tbl', null), ), ), ), diff --git a/sqlparser/test/parser/inline_dart_test.dart b/sqlparser/test/parser/inline_dart_test.dart index 16cb9b1f..793fcde3 100644 --- a/sqlparser/test/parser/inline_dart_test.dart +++ b/sqlparser/test/parser/inline_dart_test.dart @@ -9,7 +9,7 @@ void main() { r'SELECT * FROM tbl LIMIT $limit', SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), limit: DartLimitPlaceholder(name: 'limit'), ), moorMode: true, @@ -21,7 +21,7 @@ void main() { r'SELECT * FROM tbl LIMIT $amount OFFSET 3', SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), limit: Limit( count: DartExpressionPlaceholder(name: 'amount'), offsetSeparator: token(TokenType.offset), @@ -37,7 +37,7 @@ void main() { r'SELECT * FROM tbl ORDER BY $term, $expr DESC', SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), orderBy: OrderBy( terms: [ DartOrderingTermPlaceholder(name: 'term'), @@ -57,7 +57,7 @@ void main() { r'SELECT * FROM tbl ORDER BY $order', SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), orderBy: DartOrderByPlaceholder(name: 'order'), ), moorMode: true, diff --git a/sqlparser/test/parser/insert_test.dart b/sqlparser/test/parser/insert_test.dart index 49a77c7a..8b937e97 100644 --- a/sqlparser/test/parser/insert_test.dart +++ b/sqlparser/test/parser/insert_test.dart @@ -48,7 +48,7 @@ void main() { source: SelectInsertSource( SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), ), ), diff --git a/sqlparser/test/parser/moor_file_test.dart b/sqlparser/test/parser/moor_file_test.dart index 25495a75..22a240e7 100644 --- a/sqlparser/test/parser/moor_file_test.dart +++ b/sqlparser/test/parser/moor_file_test.dart @@ -58,7 +58,7 @@ void main() { SimpleName('all'), SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), where: DartExpressionPlaceholder(name: 'predicate'), ), ), @@ -66,7 +66,7 @@ void main() { SpecialStatementIdentifier('special'), SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), ), DeclaredStatement( @@ -116,7 +116,7 @@ void main() { final engine = SqlEngine(EngineOptions(useMoorExtensions: true)); final result = engine.parseMoorFile(''' worksByComposer: -SELECT DISTINCT A.* FROM works A, works B ON A.id = B.part_of +SELECT DISTINCT A.* FROM works A, works B ON A.id = WHERE A.composer = :id OR B.composer = :id; '''); @@ -124,6 +124,6 @@ SELECT DISTINCT A.* FROM works A, works B ON A.id = B.part_of expect( result.errors.single, isA() - .having((e) => e.token.lexeme, 'token.lexeme', 'ON')); + .having((e) => e.token.lexeme, 'token.lexeme', 'WHERE')); }); } diff --git a/sqlparser/test/parser/multiple_statements.dart b/sqlparser/test/parser/multiple_statements.dart index 8669201e..9e54e6b1 100644 --- a/sqlparser/test/parser/multiple_statements.dart +++ b/sqlparser/test/parser/multiple_statements.dart @@ -29,7 +29,7 @@ void main() { SimpleName('b'), SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), ), ]), @@ -48,7 +48,7 @@ void main() { SimpleName('b'), SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), ), ); @@ -78,7 +78,7 @@ void main() { SimpleName('query'), SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), ), ); diff --git a/sqlparser/test/parser/select/common_table_expression_test.dart b/sqlparser/test/parser/select/common_table_expression_test.dart index 0d4cb660..41c177cf 100644 --- a/sqlparser/test/parser/select/common_table_expression_test.dart +++ b/sqlparser/test/parser/select/common_table_expression_test.dart @@ -47,7 +47,7 @@ void main() { ), ), ], - from: [TableReference('cnt')], + from: TableReference('cnt'), limit: Limit( count: NumericLiteral( 1000000, @@ -64,7 +64,7 @@ void main() { columns: [ ExpressionResultColumn(expression: Reference(columnName: 'x')), ], - from: [TableReference('cnt')], + from: TableReference('cnt'), ), ); }); diff --git a/sqlparser/test/parser/select/compound_test.dart b/sqlparser/test/parser/select/compound_test.dart index dcafb548..800a7c98 100644 --- a/sqlparser/test/parser/select/compound_test.dart +++ b/sqlparser/test/parser/select/compound_test.dart @@ -10,7 +10,7 @@ void main() { CompoundSelectStatement( base: SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), ), additional: [ CompoundSelectPart( diff --git a/sqlparser/test/parser/select/from_test.dart b/sqlparser/test/parser/select/from_test.dart index bf03dc4b..78f48495 100644 --- a/sqlparser/test/parser/select/from_test.dart +++ b/sqlparser/test/parser/select/from_test.dart @@ -5,13 +5,9 @@ import 'package:sqlparser/src/utils/ast_equality.dart'; import '../utils.dart'; -void _enforceFrom(SelectStatement stmt, List expected) { +void _enforceFrom(SelectStatement stmt, Queryable expected) { enforceHasSpan(stmt); - expect(stmt.from.length, expected.length); - - for (var i = 0; i < stmt.from.length; i++) { - enforceEqual(stmt.from[i], expected[i]); - } + enforceEqual(stmt.from, expected); } void main() { @@ -20,7 +16,7 @@ void main() { final stmt = SqlEngine().parse('SELECT * FROM tbl').rootNode as SelectStatement; - enforceEqual(stmt.from.single, TableReference('tbl', null)); + enforceEqual(stmt.from, TableReference('tbl', null)); }); test('from more than one table', () { @@ -30,10 +26,37 @@ void main() { _enforceFrom( stmt, - [ - TableReference('tbl', 'test'), - TableReference('table2', null), - ], + JoinClause( + primary: TableReference('tbl', 'test'), + joins: [ + Join( + operator: JoinOperator.comma, + query: TableReference('table2', null), + ), + ], + ), + ); + }); + + test('more than one table with ON constraint', () { + final stmt = SqlEngine() + .parse('SELECT * FROM tbl AS test, table2 ON TRUE') + .rootNode as SelectStatement; + + _enforceFrom( + stmt, + JoinClause( + primary: TableReference('tbl', 'test'), + joins: [ + Join( + operator: JoinOperator.comma, + query: TableReference('table2', null), + constraint: OnConstraint( + expression: BooleanLiteral.withTrue(token(TokenType.$true)), + ), + ), + ], + ), ); }); @@ -45,17 +68,22 @@ void main() { _enforceFrom( stmt, - [ - TableReference('table1', null), - SelectStatementAsSource( - statement: SelectStatement( - columns: [StarResultColumn(null)], - from: [TableReference('table2', null)], - where: Reference(columnName: 'a'), + JoinClause( + primary: TableReference('table1', null), + joins: [ + Join( + operator: JoinOperator.comma, + query: SelectStatementAsSource( + statement: SelectStatement( + columns: [StarResultColumn(null)], + from: TableReference('table2', null), + where: Reference(columnName: 'a'), + ), + as: 'inner', + ), ), - as: 'inner', - ), - ], + ], + ), ); }); @@ -66,7 +94,8 @@ void main() { 'LEFT OUTER JOIN table3 ON TRUE') .rootNode as SelectStatement; - _enforceFrom(stmt, [ + _enforceFrom( + stmt, JoinClause( primary: TableReference('table1', null), joins: [ @@ -84,7 +113,7 @@ void main() { ), ], ), - ]); + ); }); test('table valued function', () { @@ -101,15 +130,20 @@ WHERE json_each.value LIKE '704-%'; expression: Reference(tableName: 'user', columnName: 'name'), ), ], - from: [ - TableReference('user'), - TableValuedFunction( - 'json_each', - ExprFunctionParameters(parameters: [ - Reference(tableName: 'user', columnName: 'phone') - ]), - ), - ], + from: JoinClause( + primary: TableReference('user'), + joins: [ + Join( + operator: JoinOperator.comma, + query: TableValuedFunction( + 'json_each', + ExprFunctionParameters(parameters: [ + Reference(tableName: 'user', columnName: 'phone') + ]), + ), + ), + ], + ), where: StringComparisonExpression( left: Reference(tableName: 'json_each', columnName: 'value'), operator: token(TokenType.like), diff --git a/sqlparser/test/parser/select/generic_test.dart b/sqlparser/test/parser/select/generic_test.dart index 7df8178b..a6b58cdf 100644 --- a/sqlparser/test/parser/select/generic_test.dart +++ b/sqlparser/test/parser/select/generic_test.dart @@ -32,18 +32,16 @@ final Map testCases = { expression: SubQuery( select: SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('table2', null)], + from: TableReference('table2', null), ), ), ), ], - from: [ - TableReference('tbl', null), - ], + from: TableReference('tbl', null), ), 'SELECT * FROM tbl WHERE id IN ()': SelectStatement( columns: [StarResultColumn(null)], - from: [TableReference('tbl', null)], + from: TableReference('tbl', null), where: InExpression( left: Reference(columnName: 'id'), inside: Tuple(expressions: []), @@ -53,7 +51,7 @@ final Map testCases = { columns: [ ExpressionResultColumn(expression: Reference(columnName: 'rowid')), ], - from: [TableReference('tbl')], + from: TableReference('tbl'), ), };