Merge pull request #2617 from jmatth/fix-migration-conflicts

Preventing getter name conflicts in step-by-step migrations

drift_dev/lib/src/writer/schema_version_writer.dart

@@ -158,9 +158,19 @@ class SchemaVersionWriter {
     });
   }
 
+  String _suffixForElement(DriftSchemaElement element) => switch (element) {
+        DriftTable() => 'Table',
+        DriftView() => 'View',
+        DriftIndex() => 'Index',
+        DriftTrigger() => 'Trigger',
+        _ => throw ArgumentError('Unhandled element type $element'),
+      };
+
   String _writeWithResultSet(
-      DriftElementWithResultSet entity, TextEmitter writer) {
-    final getterName = entity.dbGetterName;
+    String getterName,
+    DriftElementWithResultSet entity,
+    TextEmitter writer,
+  ) {
     final shape = _shapeClass(entity);
     writer
       ..write('late final $shape $getterName = ')
@@ -259,26 +269,27 @@ class SchemaVersionWriter {
     writer.write('attachedDatabase: database,');
     writer.write('), alias: null)');
 
-    return getterName!;
+    return getterName;
   }
 
   String _writeEntity({
     required DriftSchemaElement element,
     required TextEmitter definition,
   }) {
-    String name;
+    final name = definition.parent!.getNonConflictingName(
+      element.dbGetterName!,
+      (name) => name + _suffixForElement(element),
+    );
 
     if (element is DriftElementWithResultSet) {
-      name = _writeWithResultSet(element, definition);
+      _writeWithResultSet(name, element, definition);
     } else if (element is DriftIndex) {
-      name = element.dbGetterName;
       final index = definition.drift('Index');
 
       definition
         ..write('final $index $name = ')
         ..writeln(DatabaseWriter.createIndex(definition.parent!, element));
     } else if (element is DriftTrigger) {
-      name = element.dbGetterName;
       final trigger = definition.drift('Trigger');
 
       definition
@@ -314,6 +325,17 @@ class SchemaVersionWriter {
       final versionClass = '_S$versionNo';
       final versionScope = libraryScope.child();
 
+      // Reserve all the names already in use in [VersionedSchema] and its
+      // superclasses. Without this certain table names would cause us to
+      // generate invalid code.
+      versionScope.reserveNames([
+        'database',
+        'entities',
+        'version',
+        'stepByStepHelper',
+        'runMigrationSteps',
+      ]);
+
       // Write an _S<x> class for each schema version x.
       versionScope.leaf()
         ..write('final class $versionClass extends ')

drift_dev/lib/src/writer/writer.dart

@@ -1,10 +1,10 @@
 import 'package:drift/drift.dart';
+import 'package:path/path.dart' show url;
 import 'package:recase/recase.dart';
 import 'package:sqlparser/sqlparser.dart' as sql;
-import 'package:path/path.dart' show url;
 
-import '../analysis/results/results.dart';
 import '../analysis/options.dart';
+import '../analysis/results/results.dart';
 import 'import_manager.dart';
 import 'queries/sql_writer.dart';
 
@@ -298,6 +298,10 @@ class Scope extends _Node {
   /// This can be used to generated methods which must have a unique name-
   int counter = 0;
 
+  /// The set of names already used in this scope. Used by methods like
+  /// [getNonConflictingName] to prevent name collisions.
+  final Set<String> _usedNames = {};
+
   Scope({required Scope? parent, Writer? writer})
       : writer = writer ?? parent!.writer,
         super(parent);
@@ -325,6 +329,28 @@ class Scope extends _Node {
     _children.add(child);
     return child;
   }
+
+  /// Reserve a collection of names in this scope. See [getNonConflictingName]
+  /// for more information.
+  void reserveNames(Iterable<String> names) {
+    _usedNames.addAll(names);
+  }
+
+  /// Returns a variation of [name] that does not conflict with any names
+  /// already in use in this [Scope].
+  ///
+  /// If [name] does not conflict with any existing names then it is returned
+  /// unmodified. If a conflict is detected then [name] is repeatedly passed to
+  /// [modify] until the result no longer conflicts. Each result returned from
+  /// this method is recorded in an internal set, so subsequent calls with the
+  /// same name will produce a different, non-conflicting result.
+  String getNonConflictingName(String name, String Function(String) modify) {
+    while (_usedNames.contains(name)) {
+      name = modify(name);
+    }
+    _usedNames.add(name);
+    return name;
+  }
 }
 
 class TextEmitter extends _Node {

drift_dev/test/writer/schema_version_writer_test.dart

@@ -0,0 +1,102 @@
+import 'package:drift/drift.dart';
+import 'package:drift_dev/src/analysis/options.dart';
+import 'package:drift_dev/src/analysis/results/results.dart';
+import 'package:drift_dev/src/writer/import_manager.dart';
+import 'package:drift_dev/src/writer/schema_version_writer.dart';
+import 'package:drift_dev/src/writer/writer.dart';
+import 'package:test/test.dart';
+
+void main() {
+  final fakeUri = Uri.parse('drift:hidden');
+
+  DriftTable buildTable(String name) {
+    return DriftTable(
+      DriftElementId(fakeUri, name),
+      DriftDeclaration(fakeUri, -1, ''),
+      columns: [
+        DriftColumn(
+          sqlType: DriftSqlType.int,
+          nullable: false,
+          nameInSql: 'foo',
+          nameInDart: 'foo',
+          declaration: DriftDeclaration(
+            fakeUri,
+            -1,
+            '',
+          ),
+        ),
+      ],
+      baseDartName: name,
+      nameOfRowClass: name.substring(0, 1).toUpperCase() + name.substring(1),
+    );
+  }
+
+  String containsTableRegex(String name, {bool withSuffix = false}) =>
+      'late final Shape\\d+ $name${withSuffix ? r'\w+' : ''} =';
+
+  test('avoids conflict with getters in schema class', () async {
+    final imports = LibraryImportManager();
+    final writer = Writer(
+      const DriftOptions.defaults(),
+      generationOptions: GenerationOptions(imports: imports),
+    );
+    imports.linkToWriter(writer);
+
+    final normalTable = buildTable('myFirstTable');
+
+    final problemTables = [
+      'database',
+      'entities',
+      'version',
+      'stepByStepHelper',
+      'runMigrationSteps',
+    ].map(buildTable).toList();
+    final secondaryProblemTables = problemTables
+        .map((t) => '${t.baseDartName}Table')
+        .map(buildTable)
+        .toList();
+    SchemaVersionWriter(
+      [
+        SchemaVersion(
+          1,
+          [normalTable],
+          const {},
+        ),
+        SchemaVersion(
+          2,
+          [
+            normalTable,
+            ...problemTables,
+            ...secondaryProblemTables,
+          ],
+          const {},
+        ),
+      ],
+      writer.child(),
+    ).write();
+
+    final output = writer.writeGenerated();
+
+    // Tables without conflicting names shouldn't be modified.
+    expect(output, matches(containsTableRegex(normalTable.baseDartName)));
+
+    // Tables that directly conflict with member names from VersionedSchema and
+    // its superclasses should have their names modified and not appear with
+    // their original name at all.
+    for (final tableName in problemTables.map((t) => t.baseDartName)) {
+      expect(
+        output,
+        isNot(matches(containsTableRegex(tableName))),
+      );
+      expect(output, matches(containsTableRegex(tableName, withSuffix: true)));
+    }
+
+    // Tables that conflict with modified table names should themselves be
+    // modified to prevent the conflict. We can't check for nonexistence here
+    // because the the entire point is the name conficts with an in-use table
+    // name, so we only check for the existence of the doubly modified name.
+    for (final tableName in secondaryProblemTables.map((t) => t.baseDartName)) {
+      expect(output, matches(containsTableRegex(tableName, withSuffix: true)));
+    }
+  });
+}