From a46cc07ed46ae09e415bb28e73550a884d36ff2a Mon Sep 17 00:00:00 2001 From: Simon Binder Date: Fri, 21 May 2021 17:46:27 +0200 Subject: [PATCH] Refactor column resolver in sqlparser Fixes #1208 --- .../explicit_alias_transformer.dart | 3 +- .../analyzer/sql_queries/lints/linter.dart | 2 +- .../analyzer/sql_queries/query_handler.dart | 6 +- moor_generator/pubspec.yaml | 2 +- .../sql_queries/query_handler_test.dart | 41 +++++++++- sqlparser/CHANGELOG.md | 8 ++ sqlparser/lib/src/analysis/schema/column.dart | 34 ++++++++ .../lib/src/analysis/schema/references.dart | 8 ++ .../lib/src/analysis/schema/result_set.dart | 23 ++++++ .../src/analysis/steps/column_resolver.dart | 82 +++++++++++++------ .../lib/src/analysis/steps/prepare_ast.dart | 17 +++- .../analysis/steps/reference_resolver.dart | 75 +++++++++++------ .../lib/src/analysis/types/join_analysis.dart | 38 ++++++--- .../src/analysis/types/resolving_visitor.dart | 3 +- sqlparser/lib/src/ast/common/queryables.dart | 8 +- .../lib/src/ast/expressions/reference.dart | 3 + sqlparser/pubspec.yaml | 2 +- .../analysis/reference_resolver_test.dart | 34 +++++++- .../analysis/types2/join_analysis_test.dart | 42 ++++++++++ 19 files changed, 355 insertions(+), 76 deletions(-) create mode 100644 sqlparser/test/analysis/types2/join_analysis_test.dart diff --git a/moor_generator/lib/src/analyzer/sql_queries/explicit_alias_transformer.dart b/moor_generator/lib/src/analyzer/sql_queries/explicit_alias_transformer.dart index 7b69e60a..3b8888ed 100644 --- a/moor_generator/lib/src/analyzer/sql_queries/explicit_alias_transformer.dart +++ b/moor_generator/lib/src/analyzer/sql_queries/explicit_alias_transformer.dart @@ -64,7 +64,8 @@ class _PatchReferences extends Transformer { @override AstNode? visitReference(Reference e, void arg) { - final resolved = e.resolvedColumn; + final resolved = e.resolvedColumn?.source; + if (resolved != null) { final name = _transformer.newNameFor(resolved); if (name != null) { diff --git a/moor_generator/lib/src/analyzer/sql_queries/lints/linter.dart b/moor_generator/lib/src/analyzer/sql_queries/lints/linter.dart index f8cce80e..1525ee38 100644 --- a/moor_generator/lib/src/analyzer/sql_queries/lints/linter.dart +++ b/moor_generator/lib/src/analyzer/sql_queries/lints/linter.dart @@ -139,7 +139,7 @@ class _LintingVisitor extends RecursiveVisitor { } // check that it actually refers to a table - if (e.resultSet is! Table) { + if (e.resultSet?.unalias() is! Table) { linter.lints.add(AnalysisError( type: AnalysisErrorType.other, message: 'Nested star columns must refer to a table directly. They ' diff --git a/moor_generator/lib/src/analyzer/sql_queries/query_handler.dart b/moor_generator/lib/src/analyzer/sql_queries/query_handler.dart index f7de6eca..0a4ff609 100644 --- a/moor_generator/lib/src/analyzer/sql_queries/query_handler.dart +++ b/moor_generator/lib/src/analyzer/sql_queries/query_handler.dart @@ -218,11 +218,13 @@ class QueryHandler { for (final column in (query as SelectStatement).columns) { if (column is NestedStarResultColumn) { - final result = column.resultSet; + final originalResult = column.resultSet; + final result = originalResult.unalias(); if (result is! Table) continue; final moorTable = mapper.tableToMoor(result as Table); - final isNullable = analysis == null || analysis.isNullableTable(result); + final isNullable = + analysis == null || analysis.isNullableTable(originalResult); nestedTables.add(NestedResultTable(column, column.tableName, moorTable, isNullable: isNullable)); } diff --git a/moor_generator/pubspec.yaml b/moor_generator/pubspec.yaml index 3adab39f..e65ca68b 100644 --- a/moor_generator/pubspec.yaml +++ b/moor_generator/pubspec.yaml @@ -25,7 +25,7 @@ dependencies: # Moor-specific analysis and apis moor: ^4.1.0 sqlite3: '>=0.1.6 <2.0.0' - sqlparser: ^0.16.0 + sqlparser: ^0.17.0 # Dart analysis analyzer: "^1.5.0" diff --git a/moor_generator/test/analyzer/sql_queries/query_handler_test.dart b/moor_generator/test/analyzer/sql_queries/query_handler_test.dart index c56ab70e..47565ac1 100644 --- a/moor_generator/test/analyzer/sql_queries/query_handler_test.dart +++ b/moor_generator/test/analyzer/sql_queries/query_handler_test.dart @@ -92,7 +92,7 @@ FROM routes r INNER JOIN points "from" ON "from".id = routes.from INNER JOIN points "to" ON "to".id = routes."to"; ''', - }); + }, enableAnalyzer: false); final file = await state.analyze('package:foo/main.moor'); final result = file.currentResult as ParsedMoorFile; @@ -109,4 +109,43 @@ FROM routes r expect(resultSet.nestedResults.map((e) => e.table.sqlName), ['points', 'points']); }); + + test('resolves nullability of aliases in nested result sets', () async { + final state = TestState.withContent({ + 'foo|lib/main.moor': r''' +CREATE TABLE tableA1 (id INTEGER); +CREATE TABLE tableB1 (id INTEGER); + +query: SELECT + tableA1.**, + tableA2.**, + tableB1.**, + tableB2.** +FROM tableA1 -- not nullable + +LEFT JOIN tableA1 AS tableA2 -- nullable + ON FALSE + +INNER JOIN tableB1 -- not nullable + ON TRUE + +LEFT JOIN tableB1 AS tableB2 -- nullable + ON FALSE; + ''', + }, enableAnalyzer: false); + + final file = await state.analyze('package:foo/main.moor'); + final result = file.currentResult as ParsedMoorFile; + state.close(); + + expect(file.errors.errors, isEmpty); + + final query = result.resolvedQueries.single; + final resultSet = (query as SqlSelectQuery).resultSet; + + final nested = resultSet.nestedResults; + expect(nested.map((e) => e.name), + ['tableA1', 'tableA2', 'tableB1', 'tableB2']); + expect(nested.map((e) => e.isNullable), [false, true, false, true]); + }); } diff --git a/sqlparser/CHANGELOG.md b/sqlparser/CHANGELOG.md index b3248fcf..fa1d0dc3 100644 --- a/sqlparser/CHANGELOG.md +++ b/sqlparser/CHANGELOG.md @@ -1,3 +1,11 @@ +## 0.17.0 + +- Refactor how tables and columns are resolved in statements + - The new `ResultSetAvailableInStatement` class describes a result set that + has been added to a statement, for instance through a from clause + - A `TableOrSubquery` with an alias now introduces a `TableAlias` instead of + the original table + ## 0.16.0 - New analysis checks for `RETURNING`: Disallow `table.*` syntax and aggregate expressions diff --git a/sqlparser/lib/src/analysis/schema/column.dart b/sqlparser/lib/src/analysis/schema/column.dart index 3f599a9a..0735023d 100644 --- a/sqlparser/lib/src/analysis/schema/column.dart +++ b/sqlparser/lib/src/analysis/schema/column.dart @@ -210,6 +210,9 @@ mixin DelegatedColumn on Column { @override String get name => innerColumn!.name; + + @override + bool get includedInResults => innerColumn?.includedInResults ?? true; } /// The result column of a [CompoundSelectStatement]. @@ -251,3 +254,34 @@ class ValuesSelectColumn extends Column { ValuesSelectColumn(this.name, this.expressions) : assert(expressions.isNotEmpty); } + +/// A column that is available in the scope of a statement. +/// +/// In addition to the [innerColumn], this provides the [source] which brought +/// this column into scope. This can be used to determine nullability. +class AvailableColumn extends Column with DelegatedColumn { + @override + final Column innerColumn; + final ResultSetAvailableInStatement source; + + AvailableColumn(this.innerColumn, this.source); +} + +extension UnaliasColumn on Column { + /// Attempts to resolve the source of this column, if this is an + /// [AvailableColumn] or a [CommonTableExpressionColumn] that refers to + /// another column. + Column get source { + var current = this; + while (current is DelegatedColumn) { + final inner = current.innerColumn; + if (inner != null) { + current = inner; + } else { + return current; + } + } + + return current; + } +} diff --git a/sqlparser/lib/src/analysis/schema/references.dart b/sqlparser/lib/src/analysis/schema/references.dart index ad09bedc..b9bcab81 100644 --- a/sqlparser/lib/src/analysis/schema/references.dart +++ b/sqlparser/lib/src/analysis/schema/references.dart @@ -86,6 +86,14 @@ class ReferenceScope { _references.putIfAbsent(identifier.toUpperCase(), () => []).add(ref); } + /// Registers both a [TableAlias] and a [ResultSetAvailableInStatement] so + /// that the alias can be used in expressions without being selected. + void registerUsableAlias(AstNode origin, ResultSet resultSet, String alias) { + final createdAlias = TableAlias(resultSet, alias); + register(alias, createdAlias); + register(alias, ResultSetAvailableInStatement(origin, createdAlias)); + } + /// Resolves to a [Referencable] with the given [name] and of the type [T]. /// If the reference couldn't be found, null is returned and [orElse] will be /// called. diff --git a/sqlparser/lib/src/analysis/schema/result_set.dart b/sqlparser/lib/src/analysis/schema/result_set.dart index 0bd0114a..c3783264 100644 --- a/sqlparser/lib/src/analysis/schema/result_set.dart +++ b/sqlparser/lib/src/analysis/schema/result_set.dart @@ -46,6 +46,29 @@ abstract class NamedResultSet extends ResultSet { } } +/// Information about a result set that is available in a statement. +/// +/// Regular result sets include tables or views that are available globally. +/// However, columns from those result sets can't be used in statements unless +/// the result set appears in a `FROM` clause or a similar construct. +/// +/// This class stores information about added result sets and the syntactic +/// construct that added them. +class ResultSetAvailableInStatement with Referencable { + /// The node responsible for adding the [resultSet]. + /// + /// This may typically be a [TableOrSubquery] appearing a `FROM` clause. + final AstNode origin; + + /// The added result set. + final ResolvesToResultSet resultSet; + + @override + bool get visibleToChildren => true; + + ResultSetAvailableInStatement(this.origin, this.resultSet); +} + extension UnaliasResultSet on ResultSet { ResultSet unalias() { var $this = this; diff --git a/sqlparser/lib/src/analysis/steps/column_resolver.dart b/sqlparser/lib/src/analysis/steps/column_resolver.dart index 4a102542..ba78697f 100644 --- a/sqlparser/lib/src/analysis/steps/column_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/column_resolver.dart @@ -57,7 +57,7 @@ class ColumnResolver extends RecursiveVisitor { if (table != null) { // add "excluded" table qualifier that referring to the row that would // have been inserted had the uniqueness constraint not been violated. - e.scope.register('excluded', TableAlias(table, 'excluded')); + e.scope.registerUsableAlias(e, table, 'excluded'); } visitChildren(e, arg); @@ -75,11 +75,7 @@ class ColumnResolver extends RecursiveVisitor { final availableColumns = []; // Add columns from the main table, if it was resolved - final baseTable = _resolveTableReference(e.table); - if (baseTable != null) { - availableColumns.addAll(baseTable.resolvedColumns ?? const []); - } - + _handle(e.table, availableColumns); // Also add columns from a FROM clause, if one is present final from = e.from; if (from != null) _handle(from, availableColumns); @@ -90,7 +86,7 @@ class ColumnResolver extends RecursiveVisitor { if (child != e.table && child != e.from) visit(child, arg); } - _resolveReturningClause(e, baseTable); + _resolveReturningClause(e, e.table.resultSet); } ResultSet? _addIfResolved(AstNode node, TableReference ref) { @@ -159,23 +155,37 @@ class ColumnResolver extends RecursiveVisitor { scope.availableColumns = table.resolvedColumns; if (e.target.introducesNew) { - scope.register('new', TableAlias(table, 'new')); + scope.registerUsableAlias(e, table, 'new'); } if (e.target.introducesOld) { - scope.register('old', TableAlias(table, 'old')); + scope.registerUsableAlias(e, table, 'old'); } visitChildren(e, arg); } void _handle(Queryable queryable, List availableColumns) { + void addColumns(Iterable columns) { + ResultSetAvailableInStatement? available; + if (queryable is TableOrSubquery) { + available = queryable.availableResultSet; + } + + if (available != null) { + availableColumns.addAll( + [for (final column in columns) AvailableColumn(column, available)]); + } else { + availableColumns.addAll(columns); + } + } + queryable.when( isTable: (table) { final resolved = _resolveTableReference(table); if (resolved != null) { // an error will be logged when resolved is null, so the != null check // is fine and avoids crashes - availableColumns.addAll(table.resultSet!.resolvedColumns!); + addColumns(table.resultSet!.resolvedColumns!); } }, isSelect: (select) { @@ -193,7 +203,7 @@ class ColumnResolver extends RecursiveVisitor { throw AssertionError('Unknown type of select statement: $stmt'); } - availableColumns.addAll(stmt.resolvedColumns!); + addColumns(stmt.resolvedColumns!); }, isJoin: (join) { _handle(join.primary, availableColumns); @@ -214,7 +224,7 @@ class ColumnResolver extends RecursiveVisitor { )); } else { function.resultSet = resolved; - availableColumns.addAll(resolved.resolvedColumns!); + addColumns(resolved.resolvedColumns!); } }, ); @@ -244,7 +254,7 @@ class ColumnResolver extends RecursiveVisitor { Iterable? visibleColumnsForStar; if (resultColumn.tableName != null) { - final tableResolver = scope.resolve( + final tableResolver = scope.resolve( resultColumn.tableName!, orElse: () { context.reportError(AnalysisError( type: AnalysisErrorType.referencedUnknownTable, @@ -254,7 +264,8 @@ class ColumnResolver extends RecursiveVisitor { }); if (tableResolver == null) continue; - visibleColumnsForStar = tableResolver.resultSet!.resolvedColumns; + visibleColumnsForStar = + tableResolver.resultSet.resultSet?.resolvedColumns; } else { // we have a * column without a table, that resolves to every column // available @@ -288,8 +299,8 @@ class ColumnResolver extends RecursiveVisitor { } } } else if (resultColumn is NestedStarResultColumn) { - final target = scope - .resolve(resultColumn.tableName, orElse: () { + final target = scope.resolve( + resultColumn.tableName, orElse: () { context.reportError(AnalysisError( type: AnalysisErrorType.referencedUnknownTable, message: 'Unknown table: ${resultColumn.tableName}', @@ -297,7 +308,7 @@ class ColumnResolver extends RecursiveVisitor { )); }); - if (target != null) resultColumn.resultSet = target.resultSet; + if (target != null) resultColumn.resultSet = target.resultSet.resultSet; } } @@ -383,10 +394,36 @@ class ColumnResolver extends RecursiveVisitor { ResultSet? _resolveTableReference(TableReference r) { final scope = r.scope; - final resolvedTable = scope.resolve(r.tableName, orElse: () { - final available = scope.allOf().map((t) { - if (t is HumanReadable) { - return (t as HumanReadable).humanReadableDescription(); + + // Try resolving to a top-level table in the schema and to a result set that + // may have been added to the table + final resolvedInSchema = scope.resolve(r.tableName); + final resolvedInQuery = + scope.resolve(r.tableName); + final createdName = r.as; + + // Prefer using a table that has already been added if this isn't the + // definition of the added table reference + if (resolvedInQuery != null && resolvedInQuery.origin != r) { + final resolved = resolvedInQuery.resultSet.resultSet; + if (resolved != null) { + return r.resolved = + createdName != null ? TableAlias(resolved, createdName) : resolved; + } + } else if (resolvedInSchema != null) { + return r.resolved = createdName != null + ? TableAlias(resolvedInSchema, createdName) + : resolvedInSchema; + } else { + final available = scope + .allOf() + .followedBy(scope + .allOf() + .map((added) => added.resultSet)) + .map((t) { + final resultSet = t.resultSet; + if (resultSet is HumanReadable) { + return (resultSet as HumanReadable).humanReadableDescription(); } return t.toString(); @@ -398,7 +435,6 @@ class ColumnResolver extends RecursiveVisitor { reference: r.tableName, available: available, )); - }); - return r.resolved = resolvedTable; + } } } diff --git a/sqlparser/lib/src/analysis/steps/prepare_ast.dart b/sqlparser/lib/src/analysis/steps/prepare_ast.dart index 05715377..c1a0a8f5 100644 --- a/sqlparser/lib/src/analysis/steps/prepare_ast.dart +++ b/sqlparser/lib/src/analysis/steps/prepare_ast.dart @@ -122,8 +122,13 @@ class AstPreparingVisitor extends RecursiveVisitor { @override void defaultQueryable(Queryable e, void arg) { final scope = e.scope; + e.when( isTable: (table) { + final added = ResultSetAvailableInStatement(table, table); + scope.register(table.tableName, added); + table.availableResultSet = added; + // we're looking at something like FROM table (AS alias). The alias // acts like a table for expressions in the same scope, so let's // register it. @@ -132,12 +137,14 @@ class AstPreparingVisitor extends RecursiveVisitor { // package and moor_generator might depend on this being a table // directly (e.g. nested result sets in moor). // Same for nested selects, joins and table-valued functions below. - scope.register(table.as!, table); + scope.register(table.as!, added); } }, isSelect: (select) { if (select.as != null) { - scope.register(select.as!, select.statement); + final added = ResultSetAvailableInStatement(select, select.statement); + select.availableResultSet = added; + scope.register(select.as!, added); } }, isJoin: (join) { @@ -146,10 +153,12 @@ class AstPreparingVisitor extends RecursiveVisitor { // dont't need to do anything here. }, isTableFunction: (function) { + final added = ResultSetAvailableInStatement(function, function); + function.availableResultSet = added; if (function.as != null) { - scope.register(function.as!, function); + scope.register(function.as!, added); } - scope.register(function.name, function); + scope.register(function.name, added); }, ); diff --git a/sqlparser/lib/src/analysis/steps/reference_resolver.dart b/sqlparser/lib/src/analysis/steps/reference_resolver.dart index 37bbab52..d9f4ae4d 100644 --- a/sqlparser/lib/src/analysis/steps/reference_resolver.dart +++ b/sqlparser/lib/src/analysis/steps/reference_resolver.dart @@ -6,6 +6,29 @@ class ReferenceResolver extends RecursiveVisitor { ReferenceResolver(this.context); + @override + void visitAggregateExpression(AggregateExpression e, void arg) { + if (e.windowName != null && e.resolved == null) { + final resolved = e.scope.resolve(e.windowName!); + e.resolved = resolved; + } + + visitChildren(e, arg); + } + + @override + void visitInsertStatement(InsertStatement e, void arg) { + final table = e.table.resultSet; + if (table != null) { + // Resolve columns in the main table + for (final column in e.targetColumns) { + _resolveReferenceInTable(column, table); + } + } + + visitChildren(e, arg); + } + @override void visitForeignKeyClause(ForeignKeyClause e, void arg) { final table = e.foreignTable.resultSet; @@ -19,15 +42,6 @@ class ReferenceResolver extends RecursiveVisitor { } } - void _resolveReferenceInTable(Reference ref, ResultSet resultSet) { - final column = resultSet.findColumn(ref.columnName); - if (column == null) { - _reportUnknownColumnError(ref, columns: resultSet.resolvedColumns); - } else { - ref.resolved = column; - } - } - @override void visitReference(Reference e, void arg) { if (e.resolved != null) { @@ -39,8 +53,9 @@ class ReferenceResolver extends RecursiveVisitor { if (e.entityName != null) { // first find the referenced table or view, // then use the column on that table or view. - final entityResolver = scope.resolve(e.entityName!); - final resultSet = entityResolver?.resultSet; + final entityResolver = + scope.resolve(e.entityName!); + final resultSet = entityResolver?.resultSet.resultSet; if (resultSet == null) { context.reportError(AnalysisError( @@ -87,6 +102,19 @@ class ReferenceResolver extends RecursiveVisitor { visitChildren(e, arg); } + @override + void visitUpdateStatement(UpdateStatement e, void arg) { + final table = e.table.resultSet; + if (table != null) { + // Resolve the set components against the primary table + for (final set in e.set) { + _resolveReferenceInTable(set.column, table); + } + } + + visitChildren(e, arg); + } + void _reportUnknownColumnError(Reference e, {Iterable? columns}) { columns ??= e.scope.availableColumns; final columnNames = e.scope.availableColumns @@ -100,6 +128,15 @@ class ReferenceResolver extends RecursiveVisitor { )); } + void _resolveReferenceInTable(Reference ref, ResultSet resultSet) { + final column = resultSet.findColumn(ref.columnName); + if (column == null) { + _reportUnknownColumnError(ref, columns: resultSet.resolvedColumns); + } else { + ref.resolved = column; + } + } + Column? _resolveRowIdAlias(Reference e) { // to resolve those aliases when they're not bound to a table, the // surrounding statement may only read from one table @@ -112,20 +149,10 @@ class ReferenceResolver extends RecursiveVisitor { return null; } - final table = from.resolved as Table?; - if (table == null) return null; + final resolved = from.resultSet?.unalias(); + if (resolved is! Table) return null; // table.findColumn contains logic to resolve row id aliases - return table.findColumn(e.columnName); - } - - @override - void visitAggregateExpression(AggregateExpression e, void arg) { - if (e.windowName != null && e.resolved == null) { - final resolved = e.scope.resolve(e.windowName!); - e.resolved = resolved; - } - - visitChildren(e, arg); + return resolved.findColumn(e.columnName); } } diff --git a/sqlparser/lib/src/analysis/types/join_analysis.dart b/sqlparser/lib/src/analysis/types/join_analysis.dart index 5ebd93b7..e93d2054 100644 --- a/sqlparser/lib/src/analysis/types/join_analysis.dart +++ b/sqlparser/lib/src/analysis/types/join_analysis.dart @@ -31,7 +31,7 @@ import '../analysis.dart'; /// /// In the future, we'll also consider foreign key constraints. class JoinModel { - final List nonNullable; + final List nonNullable; JoinModel._(this.nonNullable); @@ -54,28 +54,37 @@ class JoinModel { return created; } - /// Checks whether the column comes from a nullable table. - bool isFromNullableTable(Column column) { - final resultSet = column.containingSet; - if (resultSet == null) return false; + bool? referenceIsNullable(Reference reference) { + final resolved = reference.resolvedColumn; + if (resolved is AvailableColumn) { + return !nonNullable.contains(resolved.source); + } - return isNullableTable(resultSet); + final resultSet = reference.resultEntity; + // ignore: avoid_returning_null + if (resultSet == null) return null; + + return !nonNullable.contains(resultSet); } /// Checks whether the result set is nullable in the surrounding select /// statement. bool isNullableTable(ResultSet resultSet) { return nonNullable.every((nonNullableRef) { - return nonNullableRef.resultSet != resultSet; + return nonNullableRef.resultSet.resultSet != resultSet; }); } } +// The boolean arg indicates whether a visited queryable is needed for the +// result to have any rows (which, in particular, mean's its non-nullable) class _FindNonNullableJoins extends RecursiveVisitor { - final List nonNullable = []; + final List nonNullable = []; - // The boolean arg indicates whether a visited queryable is needed for the - // result to have any rows (which, in particular, mean's its non-nullable) + void _addIfMakesResultStatementAvailable(TableOrSubquery node) { + final resultSet = node.availableResultSet; + if (resultSet != null) nonNullable.add(resultSet); + } @override void visitSelectStatement(SelectStatement e, bool arg) { @@ -97,11 +106,16 @@ class _FindNonNullableJoins extends RecursiveVisitor { @override void visitTableReference(TableReference e, bool arg) { - if (arg) nonNullable.add(e); + if (arg) _addIfMakesResultStatementAvailable(e); } @override void visitSelectStatementAsSource(SelectStatementAsSource e, bool arg) { - if (arg) nonNullable.add(e.statement); + if (arg) _addIfMakesResultStatementAvailable(e); + } + + @override + void visitTableValuedFunction(TableValuedFunction e, bool arg) { + if (arg) _addIfMakesResultStatementAvailable(e); } } diff --git a/sqlparser/lib/src/analysis/types/resolving_visitor.dart b/sqlparser/lib/src/analysis/types/resolving_visitor.dart index bb6ae34f..b1926470 100644 --- a/sqlparser/lib/src/analysis/types/resolving_visitor.dart +++ b/sqlparser/lib/src/analysis/types/resolving_visitor.dart @@ -428,8 +428,7 @@ class TypeResolver extends RecursiveVisitor { if (resolved == null) return; _handleColumn(resolved); - - final isNullable = JoinModel.of(e)?.isFromNullableTable(resolved) ?? false; + final isNullable = JoinModel.of(e)?.referenceIsNullable(e) ?? false; _lazyCopy(e, resolved, makeNullable: isNullable); } diff --git a/sqlparser/lib/src/ast/common/queryables.dart b/sqlparser/lib/src/ast/common/queryables.dart index 830069b7..ec52508d 100644 --- a/sqlparser/lib/src/ast/common/queryables.dart +++ b/sqlparser/lib/src/ast/common/queryables.dart @@ -26,7 +26,10 @@ abstract class Queryable extends AstNode { /// https://www.sqlite.org/syntax/table-or-subquery.html /// Marker interface -abstract class TableOrSubquery extends Queryable {} +abstract class TableOrSubquery extends Queryable { + /// The result set that this node made available, if any + ResultSetAvailableInStatement? availableResultSet; +} /// A table. The first path in https://www.sqlite.org/syntax/table-or-subquery.html /// @@ -202,4 +205,7 @@ class TableValuedFunction extends Queryable @override bool get visibleToChildren => false; + + @override + ResultSetAvailableInStatement? availableResultSet; } diff --git a/sqlparser/lib/src/ast/expressions/reference.dart b/sqlparser/lib/src/ast/expressions/reference.dart index 72b412c8..f3e8cb52 100644 --- a/sqlparser/lib/src/ast/expressions/reference.dart +++ b/sqlparser/lib/src/ast/expressions/reference.dart @@ -12,6 +12,9 @@ class Reference extends Expression with ReferenceOwner { final String? entityName; final String columnName; + /// The resolved result set from the [entityName]. + ResultSetAvailableInStatement? resultEntity; + Column? get resolvedColumn => resolved as Column?; Reference({this.entityName, required this.columnName}); diff --git a/sqlparser/pubspec.yaml b/sqlparser/pubspec.yaml index 3e56f3d1..56e9c6aa 100644 --- a/sqlparser/pubspec.yaml +++ b/sqlparser/pubspec.yaml @@ -1,6 +1,6 @@ name: sqlparser description: Parses sqlite statements and performs static analysis on them -version: 0.16.0 +version: 0.17.0-dev homepage: https://github.com/simolus3/moor/tree/develop/sqlparser #homepage: https://moor.simonbinder.eu/ issue_tracker: https://github.com/simolus3/moor/issues diff --git a/sqlparser/test/analysis/reference_resolver_test.dart b/sqlparser/test/analysis/reference_resolver_test.dart index 2c48812d..4d9ae3f0 100644 --- a/sqlparser/test/analysis/reference_resolver_test.dart +++ b/sqlparser/test/analysis/reference_resolver_test.dart @@ -33,9 +33,10 @@ void main() { final secondColumn = select.columns[1] as ExpressionResultColumn; final from = select.from as TableReference; - expect((firstColumn.expression as Reference).resolved, id); - expect((secondColumn.expression as Reference).resolved, content); - expect(from.resolved, demoTable); + expect((firstColumn.expression as Reference).resolvedColumn?.source, id); + expect( + (secondColumn.expression as Reference).resolvedColumn?.source, content); + expect(from.resultSet?.unalias(), demoTable); final where = select.where as BinaryExpression; expect((where.left as Reference).resolved, id); @@ -178,4 +179,31 @@ SELECT row_number() OVER wnd FROM demo ), ); }); + + test('warns about ambigious references', () { + final engine = SqlEngine()..registerTable(demoTable); + + final context = + engine.analyze('SELECT id FROM demo, (SELECT id FROM demo) AS a'); + expect(context.errors, hasLength(1)); + expect( + context.errors.single, + isA() + .having((e) => e.type, 'type', AnalysisErrorType.ambiguousReference) + .having((e) => e.span?.text, 'span.text', 'id'), + ); + }); + + test("does not allow columns from tables that haven't been aded", () { + final engine = SqlEngine()..registerTable(demoTable); + + final context = engine.analyze('SELECT demo.id;'); + expect(context.errors, hasLength(1)); + expect( + context.errors.single, + isA() + .having( + (e) => e.type, 'type', AnalysisErrorType.referencedUnknownTable) + .having((e) => e.span?.text, 'span.text', 'demo.id')); + }); } diff --git a/sqlparser/test/analysis/types2/join_analysis_test.dart b/sqlparser/test/analysis/types2/join_analysis_test.dart new file mode 100644 index 00000000..eb064f7b --- /dev/null +++ b/sqlparser/test/analysis/types2/join_analysis_test.dart @@ -0,0 +1,42 @@ +import 'package:sqlparser/sqlparser.dart'; +import 'package:test/test.dart'; + +import '../data.dart'; + +void main() { + test('correctly reports results for aliases', () { + final engine = SqlEngine()..registerTable(demoTable); + + final stmt = engine.analyze(''' + SELECT + a1.*, + a2.* + FROM demo AS a1 + LEFT JOIN demo AS a2 ON FALSE + INNER JOIN demo AS a3 ON TRUE; + ''').root; + + final model = JoinModel.of(stmt)!; + expect( + model.isNullableTable(stmt.scope + .resolve('a1')! + .resultSet + .resultSet!), + isFalse, + ); + expect( + model.isNullableTable(stmt.scope + .resolve('a2')! + .resultSet + .resultSet!), + isTrue, + ); + expect( + model.isNullableTable(stmt.scope + .resolve('a3')! + .resultSet + .resultSet!), + isFalse, + ); + }); +}