Forbid unqualified access to aliases in triggers

sqlparser/CHANGELOG.md

@@ -2,6 +2,8 @@
 
 - Fix explicit `NULL` column constraints being dropped when converting nodes
   to SQL.
+- Add analysis errors for illegal unqualified references to `old` and `new` in
+  triggers.
 
 ## 0.33.0
 

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

@@ -62,10 +62,23 @@ abstract class ReferenceScope {
   ///
   /// Like [addResolvedResultSet], this operation is not supported on all
   /// scopes.
-  void addAlias(AstNode origin, ResultSet resultSet, String alias) {
+  ///
+  /// [canUseUnqualifiedColumns] controls whether [resolveUnqualifiedReference]
+  /// considers the alias when resolving references. Some aliases, such as `new`
+  /// and `old` in triggers, can only be used in their qualified form and thus
+  /// have that parameter set to false.
+  void addAlias(
+    AstNode origin,
+    ResultSet resultSet,
+    String alias, {
+    bool canUseUnqualifiedColumns = true,
+  }) {
     final createdAlias = TableAlias(resultSet, alias);
     addResolvedResultSet(
-        alias, ResultSetAvailableInStatement(origin, createdAlias));
+      alias,
+      ResultSetAvailableInStatement(origin, createdAlias,
+          canUseUnqualifiedColumns: canUseUnqualifiedColumns),
+    );
   }
 
   /// Attempts to find a result set that _can_ be added to a scope.
@@ -207,10 +220,15 @@ class StatementScope extends ReferenceScope with _HasParentScope {
   }
 
   @override
-  void addAlias(AstNode origin, ResultSet resultSet, String alias) {
+  void addAlias(
+    AstNode origin,
+    ResultSet resultSet,
+    String alias, {
+    bool canUseUnqualifiedColumns = true,
+  }) {
     final createdAlias = TableAlias(resultSet, alias);
-    additionalKnownTables[alias] = createdAlias;
-    resultSets[alias] = ResultSetAvailableInStatement(origin, createdAlias);
+    resultSets[alias] = ResultSetAvailableInStatement(origin, createdAlias,
+        canUseUnqualifiedColumns: canUseUnqualifiedColumns);
   }
 
   @override
@@ -247,7 +265,10 @@ class StatementScope extends ReferenceScope with _HasParentScope {
     for (final availableSource in available) {
       final resolvedColumns =
           availableSource.resultSet.resultSet?.resolvedColumns;
-      if (resolvedColumns == null) continue;
+      if (resolvedColumns == null ||
+          !availableSource.canUseUnqualifiedColumns) {
+        continue;
+      }
 
       for (final column in resolvedColumns) {
         if (column.name.toLowerCase() == columnName.toLowerCase() &&
@@ -348,10 +369,10 @@ class MiscStatementSubScope extends ReferenceScope with _HasParentScope {
 class SingleTableReferenceScope extends ReferenceScope {
   final ReferenceScope parent;
 
-  String? addedTableName;
-  ResultSetAvailableInStatement? addedTable;
+  final String addedTableName;
+  final ResultSetAvailableInStatement? addedTable;
 
-  SingleTableReferenceScope(this.parent);
+  SingleTableReferenceScope(this.parent, this.addedTableName, this.addedTable);
 
   @override
   RootScope get rootScope => parent.rootScope;
@@ -365,13 +386,6 @@ class SingleTableReferenceScope extends ReferenceScope {
     }
   }
 
-  @override
-  void addResolvedResultSet(
-      String? name, ResultSetAvailableInStatement resultSet) {
-    addedTableName = null;
-    addedTable = null;
-  }
-
   @override
   List<Column> resolveUnqualifiedReference(String columnName,
       {bool allowReferenceToResultColumn = false}) {

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

@@ -65,10 +65,18 @@ class ResultSetAvailableInStatement with Referencable {
   /// The added result set.
   final ResolvesToResultSet resultSet;
 
+  /// Whether this result set should be considered when resolving unqualified
+  /// references.
+  ///
+  /// This is true for most result sets, but some aliases such as the `old` and
+  /// `new` result sets in triggers can only be used with a qualified reference.
+  final bool canUseUnqualifiedColumns;
+
   @override
   bool get visibleToChildren => true;
 
-  ResultSetAvailableInStatement(this.origin, this.resultSet);
+  ResultSetAvailableInStatement(this.origin, this.resultSet,
+      {this.canUseUnqualifiedColumns = true});
 }
 
 extension UnaliasResultSet on ResultSet {

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

@@ -42,14 +42,16 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
 
     final scope = e.statementScope;
 
-    // Add columns of the target table for when and update of clauses
-    scope.expansionOfStarColumn = table.resolvedColumns;
-
     if (e.target.introducesNew) {
-      scope.addAlias(e, table, 'new');
+      scope.addAlias(e, table, 'new', canUseUnqualifiedColumns: false);
     }
     if (e.target.introducesOld) {
-      scope.addAlias(e, table, 'old');
+      scope.addAlias(e, table, 'old', canUseUnqualifiedColumns: false);
+    }
+
+    if (e.target case final UpdateTarget onUpdate) {
+      onUpdate.scope = SingleTableReferenceScope(scope, e.onTable.tableName,
+          ResultSetAvailableInStatement(e.onTable, table));
     }
 
     visitChildren(e, arg);
@@ -156,7 +158,15 @@ class ColumnResolver extends RecursiveVisitor<ColumnResolverContext, void> {
 
   @override
   void visitForeignKeyClause(ForeignKeyClause e, ColumnResolverContext arg) {
-    _resolveTableReference(e.foreignTable, arg);
+    final resolved = _resolveTableReference(e.foreignTable, arg);
+    final scope = SingleTableReferenceScope(
+      e.scope,
+      e.foreignTable.tableName,
+      resolved != null
+          ? ResultSetAvailableInStatement(e.foreignTable, resolved)
+          : null,
+    );
+    e.scope = scope;
     visitExcept(e, e.foreignTable, arg);
   }
 

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

@@ -136,12 +136,6 @@ class AstPreparingVisitor extends RecursiveVisitor<void, void> {
     visitChildren(e, arg);
   }
 
-  @override
-  void visitForeignKeyClause(ForeignKeyClause e, void arg) {
-    e.scope = SingleTableReferenceScope(e.scope);
-    visitChildren(e, arg);
-  }
-
   @override
   void visitNumberedVariable(NumberedVariable e, void arg) {
     _foundVariables.add(e);

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

@@ -20,20 +20,6 @@ class ReferenceResolver
     visitChildren(e, arg);
   }
 
-  @override
-  void visitForeignKeyClause(
-      ForeignKeyClause e, ReferenceResolvingContext arg) {
-    final table = e.foreignTable.resultSet;
-    if (table == null) {
-      // If the table wasn't found, an earlier step will have reported an error
-      return super.visitForeignKeyClause(e, arg);
-    }
-
-    for (final columnName in e.columnNames) {
-      _resolveReferenceInTable(columnName, table);
-    }
-  }
-
   @override
   void visitGroupBy(GroupBy e, ReferenceResolvingContext arg) {
     return super.visitGroupBy(

sqlparser/test/analysis/column_resolver_test.dart

@@ -17,7 +17,7 @@ void main() {
       test('can use OLD references', () {
         final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE DELETE ON demo BEGIN
-  SELECT * FROM old;
+  SELECT * FROM demo WHERE id = old.id;
 END;
         ''');
 
@@ -27,16 +27,15 @@ END;
       test("can't use NEW references", () {
         final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE DELETE ON demo BEGIN
-  SELECT * FROM new;
+   SELECT * FROM demo WHERE id = new.id;
 END;
         ''');
 
         expect(
           context.errors,
-          contains(const TypeMatcher<AnalysisError>()
-              .having((e) => e.type, 'type',
-                  AnalysisErrorType.referencedUnknownTable)
-              .having((e) => e.span!.text, 'span.text', 'new')),
+          contains(analysisErrorWith(
+              type: AnalysisErrorType.referencedUnknownTable,
+              lexeme: 'new.id')),
         );
       });
     });
@@ -45,7 +44,7 @@ END;
       test('can use NEW references', () {
         final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE INSERT ON demo BEGIN
-  SELECT * FROM new;
+  SELECT * FROM demo WHERE id = new.id;
 END;
         ''');
 
@@ -55,16 +54,15 @@ END;
       test("can't use OLD references", () {
         final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE INSERT ON demo BEGIN
-  SELECT * FROM old;
+  SELECT * FROM demo WHERE id = old.id;
 END;
         ''');
 
         expect(
           context.errors,
-          contains(const TypeMatcher<AnalysisError>()
-              .having((e) => e.type, 'type',
-                  AnalysisErrorType.referencedUnknownTable)
-              .having((e) => e.span!.text, 'span.text', 'old')),
+          contains(analysisErrorWith(
+              type: AnalysisErrorType.referencedUnknownTable,
+              lexeme: 'old.id')),
         );
       });
     });
@@ -72,8 +70,8 @@ END;
     test('update can use NEW and OLD references', () {
       final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE UPDATE ON demo BEGIN
-  SELECT * FROM new;
-  INSERT INTO old VALUES (1, 'foo');
+  SELECT * FROM demo WHERE id = new.id;
+  INSERT INTO demo VALUES (1, old.content);
 END;
       ''');
       expect(context.errors, isEmpty);
@@ -89,14 +87,30 @@ END;
       expect(context.errors, isEmpty);
     });
 
-    test('can refer to column in UPDATE OF', () {
+    test("can't use unqualified reference", () {
       final context = engine.analyze('''
 CREATE TRIGGER my_trigger BEFORE DELETE ON DEMO WHEN id < 10 BEGIN
   SELECT * FROM demo;
 END;
       ''');
 
-      expect(context.errors, isEmpty);
+      expect(context.errors, [
+        analysisErrorWith(
+            lexeme: 'id', type: AnalysisErrorType.referencedUnknownColumn)
+      ]);
+    });
+
+    test("can't select from alias", () {
+      final context = engine.analyze('''
+CREATE TRIGGER my_trigger BEFORE DELETE ON demo BEGIN
+  SELECT * FROM old;
+END;
+      ''');
+
+      expect(context.errors, [
+        analysisErrorWith(
+            lexeme: 'old', type: AnalysisErrorType.referencedUnknownTable)
+      ]);
     });
   });
 

sqlparser/test/analysis/reference_resolver_test.dart

@@ -417,7 +417,7 @@ CREATE TABLE points (
 
     final parseResult = engine.parse('''
 CREATE TABLE routes (
-  id INTEGER NOT NULL PRIMARY KEY,
+  route_id INTEGER NOT NULL PRIMARY KEY,
   "from" INTEGER NOT NULL REFERENCES points (id),
   "to" INTEGER NOT NULL REFERENCES points (id)
 );
@@ -437,7 +437,7 @@ CREATE TABLE routes (
         fromReference.clause.columnNames.single.resolvedColumn;
 
     expect(fromReferenced, isNotNull);
-    expect(
-        fromReferenced!.containingSet, result.rootScope.knownTables['points']);
+    expect(fromReferenced!.source.containingSet,
+        result.rootScope.knownTables['points']);
   });
 }

sqlparser/test/utils/find_referenced_tables_test.dart

@@ -115,21 +115,5 @@ void main() {
     test('does not include references resolved in the query', () {
       testWith('WITH foo AS (VALUES(1,2,3)) SELECT * FROM foo', {});
     });
-
-    test('works for complex statements', () {
-      testWith('''
-CREATE TRIGGER my_trigger BEFORE DELETE on target BEGIN
-  SELECT * FROM old;
-  SELECT * FROM new; -- note: This is a delete trigger
-END
-''', {'target', 'new'});
-
-      testWith('''
-CREATE TRIGGER my_trigger BEFORE UPDATE on target BEGIN
-  SELECT * FROM old;
-  SELECT * FROM new;
-END
-''', {'target'});
-    });
   });
 }