Fix resolving table aliases (#2453)

sqlparser/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 0.30.2
+
+- Fix false-positive "unknown table" errors when the same table is used in a
+  join with and then without an alias.
+
 ## 0.30.1
 
 - Report syntax error for `WITH` clauses in triggers.

sqlparser/lib/src/analysis/schema/references.dart

@@ -45,7 +45,8 @@ abstract class ReferenceScope {
   /// This is useful to resolve qualified references (e.g. to resolve `foo.bar`
   /// the resolver would call [resolveResultSet]("foo") and then look up the
   /// `bar` column in that result set).
-  ResultSetAvailableInStatement? resolveResultSet(String name) => null;
+  ResultSetAvailableInStatement? resolveResultSetForReference(String name) =>
+      null;
 
   /// Adds an added result set to this scope.
   ///
@@ -126,8 +127,8 @@ mixin _HasParentScope on ReferenceScope {
       _parentScopeForLookups.resultSetAvailableToChildScopes;
 
   @override
-  ResultSetAvailableInStatement? resolveResultSet(String name) =>
-      _parentScopeForLookups.resolveResultSet(name);
+  ResultSetAvailableInStatement? resolveResultSetForReference(String name) =>
+      _parentScopeForLookups.resolveResultSetForReference(name);
 
   @override
   ResultSet? resolveResultSetToAdd(String name) =>
@@ -219,8 +220,8 @@ class StatementScope extends ReferenceScope with _HasParentScope {
   }
 
   @override
-  ResultSetAvailableInStatement? resolveResultSet(String name) {
-    return resultSets[name] ?? parent.resolveResultSet(name);
+  ResultSetAvailableInStatement? resolveResultSetForReference(String name) {
+    return resultSets[name] ?? parent.resolveResultSetForReference(name);
   }
 
   @override
@@ -279,12 +280,19 @@ class StatementScope extends ReferenceScope with _HasParentScope {
   }
 }
 
-/// A special intermediate scope used for subqueries appearing in a `FROM`
-/// clause so that the subquery can't see outer columns and tables being added.
-class SubqueryInFromScope extends ReferenceScope with _HasParentScope {
+/// A special intermediate scope used for nodes that don't see columns and
+/// tables added to the statement they're in.
+///
+/// An example for this are subqueries appearing in a `FROM` clause, which can't
+/// see outer columns and tables of the select statement.
+///
+/// Another example is the [InsertStatement.source] of an [InsertStatement],
+/// which cannot refer to columns of the table being inserted to of course.
+/// Things like `INSERT INTO tbl (col) VALUES (tbl.col)` are not allowed.
+class SourceScope extends ReferenceScope with _HasParentScope {
   final StatementScope enclosingStatement;
 
-  SubqueryInFromScope(this.enclosingStatement);
+  SourceScope(this.enclosingStatement);
 
   @override
   RootScope get rootScope => enclosingStatement.rootScope;
@@ -321,8 +329,9 @@ class MiscStatementSubScope extends ReferenceScope with _HasParentScope {
   RootScope get rootScope => parent.rootScope;
 
   @override
-  ResultSetAvailableInStatement? resolveResultSet(String name) {
-    return additionalResultSets[name] ?? parent.resolveResultSet(name);
+  ResultSetAvailableInStatement? resolveResultSetForReference(String name) {
+    return additionalResultSets[name] ??
+        parent.resolveResultSetForReference(name);
   }
 
   @override
@@ -348,7 +357,7 @@ class SingleTableReferenceScope extends ReferenceScope {
   RootScope get rootScope => parent.rootScope;
 
   @override
-  ResultSetAvailableInStatement? resolveResultSet(String name) {
+  ResultSetAvailableInStatement? resolveResultSetForReference(String name) {
     if (name == addedTableName) {
       return addedTable;
     } else {

sqlparser/lib/src/analysis/steps/column_resolver.dart

@@ -26,7 +26,7 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
   @override
   void visitCreateIndexStatement(
       CreateIndexStatement e, ColumnResolverContext arg) {
-    _resolveTableReference(e.on, arg);
+    _handle(e.on, [], arg);
     visitExcept(e, e.on, arg);
   }
 
@@ -185,12 +185,13 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
 
   ResultSet? _addIfResolved(
       AstNode node, TableReference ref, ColumnResolverContext arg) {
-    final table = _resolveTableReference(ref, arg);
-    if (table != null) {
-      node.statementScope.expansionOfStarColumn = table.resolvedColumns;
-    }
+    final availableColumns = <Column>[];
+    _handle(ref, availableColumns, arg);
 
-    return table;
+    final scope = node.statementScope;
+    scope.expansionOfStarColumn = availableColumns;
+
+    return ref.resultSet;
   }
 
   @override
@@ -198,11 +199,11 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
     // Resolve CTEs first
     e.withClause?.accept(this, arg);
 
-    final into = _addIfResolved(e, e.table, arg);
+    _handle(e.table, [], arg);
     for (final child in e.childNodes) {
       if (child != e.withClause) visit(child, arg);
     }
-    _resolveReturningClause(e, into, arg);
+    _resolveReturningClause(e, e.table.resultSet, arg);
   }
 
   @override
@@ -272,14 +273,32 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
       }
     }
 
+    final scope = queryable.scope;
+
+    void markAvailableResultSet(
+        Queryable source, ResolvesToResultSet resultSet, String? name) {
+      final added = ResultSetAvailableInStatement(source, resultSet);
+
+      if (source is TableOrSubquery) {
+        source.availableResultSet = added;
+      }
+
+      scope.addResolvedResultSet(name, added);
+    }
+
     queryable.when(
       isTable: (table) {
         final resolved = _resolveTableReference(table, state);
+        markAvailableResultSet(
+            table, resolved ?? table, table.as ?? table.tableName);
+
         if (resolved != null) {
           addColumns(table.resultSet!.resolvedColumns!);
         }
       },
       isSelect: (select) {
+        markAvailableResultSet(select, select.statement, select.as);
+
         // Inside subqueries, references don't take the name of the referenced
         // column.
         final childState = ColumnResolverContext(
@@ -307,6 +326,9 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
             .engineOptions.addedTableFunctions[function.name.toLowerCase()];
         final resolved = handler?.resolveTableValued(context, function);
 
+        markAvailableResultSet(
+            function, resolved ?? function, function.as ?? function.name);
+
         if (resolved == null) {
           context.reportError(AnalysisError(
             type: AnalysisErrorType.unknownFunction,
@@ -346,7 +368,8 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
         Iterable<Column>? visibleColumnsForStar;
 
         if (resultColumn.tableName != null) {
-          final tableResolver = scope.resolveResultSet(resultColumn.tableName!);
+          final tableResolver =
+              scope.resolveResultSetForReference(resultColumn.tableName!);
           if (tableResolver == null) {
             context.reportError(AnalysisError(
               type: AnalysisErrorType.referencedUnknownTable,
@@ -417,7 +440,8 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
           }
         }
       } else if (resultColumn is NestedStarResultColumn) {
-        final target = scope.resolveResultSet(resultColumn.tableName);
+        final target =
+            scope.resolveResultSetForReference(resultColumn.tableName);
 
         if (target == null) {
           context.reportError(AnalysisError(
@@ -531,18 +555,9 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
     // 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.resolveResultSetToAdd(r.tableName);
-    final resolvedInQuery = scope.resolveResultSet(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) {
+    if (resolvedInSchema != null) {
       return r.resolved = createdName != null
           ? TableAlias(resolvedInSchema, createdName)
           : resolvedInSchema;

sqlparser/lib/src/analysis/steps/prepare_ast.dart

@@ -19,6 +19,12 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
     resolveIndexOfVariables(_foundVariables);
   }
 
+  @override
+  void defaultInsertSource(InsertSource e, void arg) {
+    e.scope = SourceScope(e.parent!.statementScope);
+    visitChildren(e, arg);
+  }
+
   @override
   void visitCreateTableStatement(CreateTableStatement e, void arg) {
     final scope = e.scope = StatementScope.forStatement(context.rootScope, e);
@@ -52,6 +58,7 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
     // query: "SELECT * FROM demo d1,
     //   (SELECT * FROM demo i WHERE i.id = d1.id) d2;"
     // it won't work.
+
     final isInFROM = e.parent is Queryable;
     StatementScope scope;
 
@@ -59,7 +66,7 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
       final surroundingSelect = e.parents
           .firstWhere((node) => node is HasFrom)
           .scope as StatementScope;
-      scope = StatementScope(SubqueryInFromScope(surroundingSelect));
+      scope = StatementScope(SourceScope(surroundingSelect));
     } else {
       scope = StatementScope.forStatement(context.rootScope, e);
     }
@@ -107,37 +114,6 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
     visitChildren(e, arg);
   }
 
-  @override
-  void defaultQueryable(Queryable e, void arg) {
-    final scope = e.scope;
-
-    e.when(
-      isTable: (table) {
-        final added = ResultSetAvailableInStatement(table, table);
-        table.availableResultSet = added;
-
-        scope.addResolvedResultSet(table.as ?? table.tableName, added);
-      },
-      isSelect: (select) {
-        final added = ResultSetAvailableInStatement(select, select.statement);
-        select.availableResultSet = added;
-        scope.addResolvedResultSet(select.as, added);
-      },
-      isJoin: (join) {
-        // the join can contain multiple tables. Luckily for us, all of them are
-        // Queryables, so we can deal with them by visiting the children and
-        // dont't need to do anything here.
-      },
-      isTableFunction: (function) {
-        final added = ResultSetAvailableInStatement(function, function);
-        function.availableResultSet = added;
-        scope.addResolvedResultSet(function.as ?? function.name, added);
-      },
-    );
-
-    visitChildren(e, arg);
-  }
-
   @override
   void visitCommonTableExpression(CommonTableExpression e, void arg) {
     StatementScope.cast(e.scope).additionalKnownTables[e.cteTableName] = e;

sqlparser/lib/src/analysis/steps/reference_resolver.dart

@@ -57,7 +57,7 @@ class ReferenceResolver
     if (e.entityName != null) {
       // first find the referenced table or view,
       // then use the column on that table or view.
-      final entityResolver = scope.resolveResultSet(e.entityName!);
+      final entityResolver = scope.resolveResultSetForReference(e.entityName!);
       final resultSet = entityResolver?.resultSet.resultSet;
 
       if (resultSet == null) {

sqlparser/lib/src/analysis/types/join_analysis.dart

@@ -89,7 +89,7 @@ class JoinModel {
 }
 
 // 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)
+// result to have any rows (which, in particular, means its non-nullable)
 class _FindNonNullableJoins extends RecursiveVisitor<bool, void> {
   final List<ResultSetAvailableInStatement> nonNullable = [];
 

sqlparser/test/analysis/column_resolver_test.dart

@@ -125,6 +125,12 @@ INSERT INTO demo VALUES (?, ?)
     expect(context.errors, isEmpty);
   });
 
+  test('columns in an insert cannot refer to table', () {
+    engine
+        .analyze('INSERT INTO demo (content) VALUES (demo.content)')
+        .expectError('demo.content');
+  });
+
   test('columns from values statement', () {
     final context = engine.analyze("VALUES ('foo', 3), ('bar', 5)");
 
@@ -142,6 +148,33 @@ INSERT INTO demo VALUES (?, ?)
     expect(context.errors, isEmpty);
   });
 
+  test('joining table with and without alias', () {
+    final context = engine.analyze('''
+      SELECT * FROM demo a
+        JOIN demo ON demo.id = a.id
+    ''');
+
+    context.expectNoError();
+  });
+
+  test("from clause can't use its own table aliases", () {
+    final context = engine.analyze('''
+      SELECT * FROM demo a
+        JOIN a b ON b.id = a.id
+    ''');
+
+    expect(context.errors, [
+      analysisErrorWith(
+          lexeme: 'a b', type: AnalysisErrorType.referencedUnknownTable),
+      analysisErrorWith(
+          lexeme: 'b.id', type: AnalysisErrorType.referencedUnknownTable),
+    ]);
+  });
+
+  test('can use columns from deleted table', () {
+    engine.analyze('DELETE FROM demo WHERE demo.id = 2').expectNoError();
+  });
+
   test('gracefully handles tuples of different lengths in VALUES', () {
     final context = engine.analyze("VALUES ('foo', 3), ('bar')");
 
@@ -292,4 +325,18 @@ INSERT INTO demo VALUES (?, ?)
       analysisErrorWith(lexeme: 'x', type: AnalysisErrorType.circularReference),
     ]);
   });
+
+  test('regression test for #2453', () {
+    // https://github.com/simolus3/drift/issues/2453
+    engine
+      ..registerTableFromSql('CREATE TABLE persons (id INTEGER);')
+      ..registerTableFromSql('CREATE TABLE cars (driver INTEGER);');
+
+    final query = engine.analyze('''
+SELECT * FROM cars
+  JOIN persons second_person ON second_person.id = cars.driver
+  JOIN persons ON persons.id = cars.driver;
+''');
+    query.expectNoError();
+  });
 }

sqlparser/test/analysis/types2/join_analysis_test.dart

@@ -19,17 +19,17 @@ void main() {
     final model = JoinModel.of(stmt)!;
     expect(
       model.isNullableTable(
-          stmt.scope.resolveResultSet('a1')!.resultSet.resultSet!),
+          stmt.scope.resolveResultSetForReference('a1')!.resultSet.resultSet!),
       isFalse,
     );
     expect(
       model.isNullableTable(
-          stmt.scope.resolveResultSet('a2')!.resultSet.resultSet!),
+          stmt.scope.resolveResultSetForReference('a2')!.resultSet.resultSet!),
       isTrue,
     );
     expect(
       model.isNullableTable(
-          stmt.scope.resolveResultSet('a3')!.resultSet.resultSet!),
+          stmt.scope.resolveResultSetForReference('a3')!.resultSet.resultSet!),
       isFalse,
     );
   });