Refactor column resolver in sqlparser

Fixes #1208
This commit is contained in:
Simon Binder 2021-05-21 17:46:27 +02:00
parent 473477e109
commit a46cc07ed4
No known key found for this signature in database
GPG Key ID: 7891917E4147B8C0
19 changed files with 355 additions and 76 deletions

View File

@ -64,7 +64,8 @@ class _PatchReferences extends Transformer<void> {
@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) {

View File

@ -139,7 +139,7 @@ class _LintingVisitor extends RecursiveVisitor<void, void> {
}
// 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 '

View File

@ -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));
}

View File

@ -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"

View File

@ -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]);
});
}

View File

@ -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

View File

@ -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;
}
}

View File

@ -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.

View File

@ -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;

View File

@ -57,7 +57,7 @@ class ColumnResolver extends RecursiveVisitor<void, void> {
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<void, void> {
final availableColumns = <Column>[];
// 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<void, void> {
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<void, void> {
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<Column> availableColumns) {
void addColumns(Iterable<Column> 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<void, void> {
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<void, void> {
));
} else {
function.resultSet = resolved;
availableColumns.addAll(resolved.resolvedColumns!);
addColumns(resolved.resolvedColumns!);
}
},
);
@ -244,7 +254,7 @@ class ColumnResolver extends RecursiveVisitor<void, void> {
Iterable<Column>? visibleColumnsForStar;
if (resultColumn.tableName != null) {
final tableResolver = scope.resolve<ResolvesToResultSet>(
final tableResolver = scope.resolve<ResultSetAvailableInStatement>(
resultColumn.tableName!, orElse: () {
context.reportError(AnalysisError(
type: AnalysisErrorType.referencedUnknownTable,
@ -254,7 +264,8 @@ class ColumnResolver extends RecursiveVisitor<void, void> {
});
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<void, void> {
}
}
} else if (resultColumn is NestedStarResultColumn) {
final target = scope
.resolve<ResolvesToResultSet>(resultColumn.tableName, orElse: () {
final target = scope.resolve<ResultSetAvailableInStatement>(
resultColumn.tableName, orElse: () {
context.reportError(AnalysisError(
type: AnalysisErrorType.referencedUnknownTable,
message: 'Unknown table: ${resultColumn.tableName}',
@ -297,7 +308,7 @@ class ColumnResolver extends RecursiveVisitor<void, void> {
));
});
if (target != null) resultColumn.resultSet = target.resultSet;
if (target != null) resultColumn.resultSet = target.resultSet.resultSet;
}
}
@ -383,10 +394,36 @@ class ColumnResolver extends RecursiveVisitor<void, void> {
ResultSet? _resolveTableReference(TableReference r) {
final scope = r.scope;
final resolvedTable = scope.resolve<ResultSet>(r.tableName, orElse: () {
final available = scope.allOf<ResultSet>().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<ResultSet>(r.tableName);
final resolvedInQuery =
scope.resolve<ResultSetAvailableInStatement>(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<ResolvesToResultSet>()
.followedBy(scope
.allOf<ResultSetAvailableInStatement>()
.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<void, void> {
reference: r.tableName,
available: available,
));
});
return r.resolved = resolvedTable;
}
}
}

View File

@ -122,8 +122,13 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
@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<void, void> {
// 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<void, void> {
// 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);
},
);

View File

@ -6,6 +6,29 @@ class ReferenceResolver extends RecursiveVisitor<void, void> {
ReferenceResolver(this.context);
@override
void visitAggregateExpression(AggregateExpression e, void arg) {
if (e.windowName != null && e.resolved == null) {
final resolved = e.scope.resolve<NamedWindowDeclaration>(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, void> {
}
}
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<void, void> {
if (e.entityName != null) {
// first find the referenced table or view,
// then use the column on that table or view.
final entityResolver = scope.resolve<ResolvesToResultSet>(e.entityName!);
final resultSet = entityResolver?.resultSet;
final entityResolver =
scope.resolve<ResultSetAvailableInStatement>(e.entityName!);
final resultSet = entityResolver?.resultSet.resultSet;
if (resultSet == null) {
context.reportError(AnalysisError(
@ -87,6 +102,19 @@ class ReferenceResolver extends RecursiveVisitor<void, void> {
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<Column>? columns}) {
columns ??= e.scope.availableColumns;
final columnNames = e.scope.availableColumns
@ -100,6 +128,15 @@ class ReferenceResolver extends RecursiveVisitor<void, void> {
));
}
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<void, void> {
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<NamedWindowDeclaration>(e.windowName!);
e.resolved = resolved;
}
visitChildren(e, arg);
return resolved.findColumn(e.columnName);
}
}

View File

@ -31,7 +31,7 @@ import '../analysis.dart';
///
/// In the future, we'll also consider foreign key constraints.
class JoinModel {
final List<ResolvesToResultSet> nonNullable;
final List<ResultSetAvailableInStatement> 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<bool, void> {
final List<ResolvesToResultSet> nonNullable = [];
final List<ResultSetAvailableInStatement> 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<bool, void> {
@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);
}
}

View File

@ -428,8 +428,7 @@ class TypeResolver extends RecursiveVisitor<TypeExpectation, void> {
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);
}

View File

@ -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;
}

View File

@ -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});

View File

@ -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

View File

@ -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<AnalysisError>()
.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<AnalysisError>()
.having(
(e) => e.type, 'type', AnalysisErrorType.referencedUnknownTable)
.having((e) => e.span?.text, 'span.text', 'demo.id'));
});
}

View File

@ -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<ResultSetAvailableInStatement>('a1')!
.resultSet
.resultSet!),
isFalse,
);
expect(
model.isNullableTable(stmt.scope
.resolve<ResultSetAvailableInStatement>('a2')!
.resultSet
.resultSet!),
isTrue,
);
expect(
model.isNullableTable(stmt.scope
.resolve<ResultSetAvailableInStatement>('a3')!
.resultSet
.resultSet!),
isFalse,
);
});
}