Fix wrong double primary key on generated tables

extras/integration_tests/tests/lib/suite/transactions.dart

@@ -10,7 +10,6 @@ void transactionTests(TestExecutor executor) {
 
     await db.transaction((_) async {
       final florianId = await db.writeUser(People.florian);
-      print(florianId);
 
       final dash = await db.getUserById(People.dashId);
       final florian = await db.getUserById(florianId);

moor/lib/src/dsl/table.dart

@@ -20,9 +20,14 @@ abstract class Table {
   String get tableName => null;
 
   /// Whether to append a `WITHOUT ROWID` clause in the `CREATE TABLE`
-  /// statement.
+  /// statement. This is intended to be used by generated code only.
   bool get withoutRowId => false;
 
+  /// Moor will write some table constraints automatically, for instance when
+  /// you override [primaryKey]. You can turn this behavior off if you want to.
+  /// This is intended to be used by generated code only.
+  bool get dontWriteConstraints => false;
+
   /// Override this to specify custom primary keys:
   /// ```dart
   /// class IngredientInRecipes extends Table {

moor/lib/src/runtime/migration.dart

@@ -95,8 +95,15 @@ class Migrator {
       if (i < table.$columns.length - 1) context.buffer.write(', ');
     }
 
+    final dslTable = table.asDslTable;
+
+    // we're in a bit of a hacky situation where we don't write the primary
+    // as table constraint if it has already been written on a primary key
+    // column, even though that column appears in table.$primaryKey because we
+    // need to know all primary keys for the update(table).replace(row) API
     final hasPrimaryKey = table.$primaryKey?.isNotEmpty ?? false;
-    if (hasPrimaryKey && !hasAutoIncrement) {
+    final dontWritePk = dslTable.dontWriteConstraints || hasAutoIncrement;
+    if (hasPrimaryKey && !dontWritePk) {
       context.buffer.write(', PRIMARY KEY (');
       final pkList = table.$primaryKey.toList(growable: false);
       for (var i = 0; i < pkList.length; i++) {
@@ -109,7 +116,6 @@ class Migrator {
       context.buffer.write(')');
     }
 
-    final dslTable = table.asDslTable;
     final constraints = dslTable.customConstraints ?? [];
 
     for (var i = 0; i < constraints.length; i++) {

moor/lib/src/runtime/structure/columns.dart

@@ -43,10 +43,10 @@ abstract class GeneratedColumn<T, S extends SqlType<T>> extends Column<T, S> {
   /// [here](https://www.sqlite.org/syntax/column-def.html), into the given
   /// buffer.
   void writeColumnDefinition(GenerationContext into) {
-    into.buffer.write('$escapedName $typeName ');
+    into.buffer.write('$escapedName $typeName');
 
     if ($customConstraints == null) {
-      into.buffer.write($nullable ? 'NULL' : 'NOT NULL');
+      into.buffer.write($nullable ? ' NULL' : ' NOT NULL');
 
       if (defaultValue != null) {
         into.buffer.write(' DEFAULT ');
@@ -62,8 +62,8 @@ abstract class GeneratedColumn<T, S extends SqlType<T>> extends Column<T, S> {
 
       // these custom constraints refer to builtin constraints from moor
       writeCustomConstraints(into.buffer);
-    } else {
+    } else if ($customConstraints?.isNotEmpty == true) {
-      into.buffer.write($customConstraints);
+      into.buffer..write(' ')..write($customConstraints);
     }
   }
 

moor/lib/src/runtime/structure/table_info.dart

@@ -4,7 +4,7 @@ import 'package:moor/src/runtime/expressions/variables.dart';
 /// Base class for generated classes. [TableDsl] is the type specified by the
 /// user that extends [Table], [D] is the type of the data class
 /// generated from the table.
-mixin TableInfo<TableDsl extends Table, D extends DataClass> {
+mixin TableInfo<TableDsl extends Table, D extends DataClass> on Table {
   /// Type system sugar. Implementations are likely to inherit from both
   /// [TableInfo] and [TableDsl] and can thus just return their instance.
   TableDsl get asDslTable;
@@ -13,6 +13,15 @@ mixin TableInfo<TableDsl extends Table, D extends DataClass> {
   /// key has been specified.
   Set<GeneratedColumn> get $primaryKey => null;
 
+  // The "primaryKey" is what users define on their table classes, the
+  // "$primaryKey" is what moor generates in the implementation table info
+  // classes. Having two of them is pretty pointless, we're going to remove
+  // the "$primaryKey$ getter in Moor 2.0. Until then, let's make sure they're
+  // consistent for classes from CREATE TABLE statements, where the info class
+  // and the table class is the same thing but primaryKey isn't overriden.
+  @override
+  Set<Column> get primaryKey => $primaryKey;
+
   /// The table name in the sql table. This can be an alias for the actual table
   /// name. See [actualTableName] for a table name that is not aliased.
   String get $tableName;

moor/test/data/tables/custom_tables.g.dart

@@ -122,6 +122,8 @@ class NoIds extends Table with TableInfo<NoIds, NoId> {
 
   @override
   final bool withoutRowId = true;
+  @override
+  final bool dontWriteConstraints = true;
 }
 
 class WithDefault extends DataClass implements Insertable<WithDefault> {
@@ -263,6 +265,9 @@ class WithDefaults extends Table with TableInfo<WithDefaults, WithDefault> {
   WithDefaults createAlias(String alias) {
     return WithDefaults(_db, alias);
   }
+
+  @override
+  final bool dontWriteConstraints = true;
 }
 
 class WithConstraint extends DataClass implements Insertable<WithConstraint> {
@@ -434,6 +439,161 @@ class WithConstraints extends Table
   final List<String> customConstraints = const [
     'FOREIGN KEY (a, b) REFERENCES with_defaults (a, b)'
   ];
+  @override
+  final bool dontWriteConstraints = true;
+}
+
+class ConfigData extends DataClass implements Insertable<ConfigData> {
+  final String configKey;
+  final String configValue;
+  ConfigData({@required this.configKey, this.configValue});
+  factory ConfigData.fromData(Map<String, dynamic> data, GeneratedDatabase db,
+      {String prefix}) {
+    final effectivePrefix = prefix ?? '';
+    final stringType = db.typeSystem.forDartType<String>();
+    return ConfigData(
+      configKey: stringType
+          .mapFromDatabaseResponse(data['${effectivePrefix}config_key']),
+      configValue: stringType
+          .mapFromDatabaseResponse(data['${effectivePrefix}config_value']),
+    );
+  }
+  factory ConfigData.fromJson(Map<String, dynamic> json,
+      {ValueSerializer serializer = const ValueSerializer.defaults()}) {
+    return ConfigData(
+      configKey: serializer.fromJson<String>(json['configKey']),
+      configValue: serializer.fromJson<String>(json['configValue']),
+    );
+  }
+  @override
+  Map<String, dynamic> toJson(
+      {ValueSerializer serializer = const ValueSerializer.defaults()}) {
+    return {
+      'configKey': serializer.toJson<String>(configKey),
+      'configValue': serializer.toJson<String>(configValue),
+    };
+  }
+
+  @override
+  T createCompanion<T extends UpdateCompanion<ConfigData>>(bool nullToAbsent) {
+    return ConfigCompanion(
+      configKey: configKey == null && nullToAbsent
+          ? const Value.absent()
+          : Value(configKey),
+      configValue: configValue == null && nullToAbsent
+          ? const Value.absent()
+          : Value(configValue),
+    ) as T;
+  }
+
+  ConfigData copyWith({String configKey, String configValue}) => ConfigData(
+        configKey: configKey ?? this.configKey,
+        configValue: configValue ?? this.configValue,
+      );
+  @override
+  String toString() {
+    return (StringBuffer('ConfigData(')
+          ..write('configKey: $configKey, ')
+          ..write('configValue: $configValue')
+          ..write(')'))
+        .toString();
+  }
+
+  @override
+  int get hashCode => $mrjf($mrjc(configKey.hashCode, configValue.hashCode));
+  @override
+  bool operator ==(other) =>
+      identical(this, other) ||
+      (other is ConfigData &&
+          other.configKey == configKey &&
+          other.configValue == configValue);
+}
+
+class ConfigCompanion extends UpdateCompanion<ConfigData> {
+  final Value<String> configKey;
+  final Value<String> configValue;
+  const ConfigCompanion({
+    this.configKey = const Value.absent(),
+    this.configValue = const Value.absent(),
+  });
+}
+
+class Config extends Table with TableInfo<Config, ConfigData> {
+  final GeneratedDatabase _db;
+  final String _alias;
+  Config(this._db, [this._alias]);
+  final VerificationMeta _configKeyMeta = const VerificationMeta('configKey');
+  GeneratedTextColumn _configKey;
+  GeneratedTextColumn get configKey => _configKey ??= _constructConfigKey();
+  GeneratedTextColumn _constructConfigKey() {
+    return GeneratedTextColumn('config_key', $tableName, false,
+        $customConstraints: 'not null primary key');
+  }
+
+  final VerificationMeta _configValueMeta =
+      const VerificationMeta('configValue');
+  GeneratedTextColumn _configValue;
+  GeneratedTextColumn get configValue =>
+      _configValue ??= _constructConfigValue();
+  GeneratedTextColumn _constructConfigValue() {
+    return GeneratedTextColumn('config_value', $tableName, true,
+        $customConstraints: '');
+  }
+
+  @override
+  List<GeneratedColumn> get $columns => [configKey, configValue];
+  @override
+  Config get asDslTable => this;
+  @override
+  String get $tableName => _alias ?? 'config';
+  @override
+  final String actualTableName = 'config';
+  @override
+  VerificationContext validateIntegrity(ConfigCompanion d,
+      {bool isInserting = false}) {
+    final context = VerificationContext();
+    if (d.configKey.present) {
+      context.handle(_configKeyMeta,
+          configKey.isAcceptableValue(d.configKey.value, _configKeyMeta));
+    } else if (configKey.isRequired && isInserting) {
+      context.missing(_configKeyMeta);
+    }
+    if (d.configValue.present) {
+      context.handle(_configValueMeta,
+          configValue.isAcceptableValue(d.configValue.value, _configValueMeta));
+    } else if (configValue.isRequired && isInserting) {
+      context.missing(_configValueMeta);
+    }
+    return context;
+  }
+
+  @override
+  Set<GeneratedColumn> get $primaryKey => {configKey};
+  @override
+  ConfigData map(Map<String, dynamic> data, {String tablePrefix}) {
+    final effectivePrefix = tablePrefix != null ? '$tablePrefix.' : null;
+    return ConfigData.fromData(data, _db, prefix: effectivePrefix);
+  }
+
+  @override
+  Map<String, Variable> entityToSql(ConfigCompanion d) {
+    final map = <String, Variable>{};
+    if (d.configKey.present) {
+      map['config_key'] = Variable<String, StringType>(d.configKey.value);
+    }
+    if (d.configValue.present) {
+      map['config_value'] = Variable<String, StringType>(d.configValue.value);
+    }
+    return map;
+  }
+
+  @override
+  Config createAlias(String alias) {
+    return Config(_db, alias);
+  }
+
+  @override
+  final bool dontWriteConstraints = true;
 }
 
 abstract class _$CustomTablesDb extends GeneratedDatabase {
@@ -446,6 +606,9 @@ abstract class _$CustomTablesDb extends GeneratedDatabase {
   WithConstraints _withConstraints;
   WithConstraints get withConstraints =>
       _withConstraints ??= WithConstraints(this);
+  Config _config;
+  Config get config => _config ??= Config(this);
   @override
-  List<TableInfo> get allTables => [noIds, withDefaults, withConstraints];
+  List<TableInfo> get allTables =>
+      [noIds, withDefaults, withConstraints, config];
 }

moor/test/data/tables/tables.moor

@@ -13,4 +13,9 @@ CREATE TABLE with_constraints (
     c FLOAT(10, 2),
 
     FOREIGN KEY (a, b) REFERENCES with_defaults (a, b)
-)
+)
+
+create table config (
+    config_key TEXT not null primary key,
+    config_value TEXT
+);

moor/test/parsed_sql/moor_files_integration_test.dart

@@ -0,0 +1,43 @@
+import 'package:moor/moor.dart';
+import 'package:test_api/test_api.dart';
+
+import '../data/tables/custom_tables.dart';
+import '../data/utils/mocks.dart';
+
+const _createNoIds =
+    'CREATE TABLE IF NOT EXISTS no_ids (payload BLOB NOT NULL) WITHOUT ROWID;';
+
+const _createWithDefaults = 'CREATE TABLE IF NOT EXISTS with_defaults ('
+    "a VARCHAR DEFAULT 'something', b INTEGER UNIQUE);";
+
+const _createWithConstraints = 'CREATE TABLE IF NOT EXISTS with_constraints ('
+    'a VARCHAR, b INTEGER NOT NULL, c REAL, '
+    'FOREIGN KEY (a, b) REFERENCES with_defaults (a, b)'
+    ');';
+
+const _createConfig = 'CREATE TABLE IF NOT EXISTS config ('
+    'config_key VARCHAR not null primary key, '
+    'config_value VARCHAR);';
+
+void main() {
+  // see ../data/tables/tables.moor
+  test('creates tables as specified in .moor files', () async {
+    final mockExecutor = MockExecutor();
+    final mockQueryExecutor = MockQueryExecutor();
+    final db = CustomTablesDb(mockExecutor);
+    await Migrator(db, mockQueryExecutor).createAllTables();
+
+    verify(mockQueryExecutor.call(_createNoIds, []));
+    verify(mockQueryExecutor.call(_createWithDefaults, []));
+    verify(mockQueryExecutor.call(_createWithConstraints, []));
+    verify(mockQueryExecutor.call(_createConfig, []));
+  });
+
+  test('infers primary keys correctly', () async {
+    final db = CustomTablesDb(null);
+
+    expect(db.noIds.primaryKey, isEmpty);
+    expect(db.withDefaults.primaryKey, isEmpty);
+    expect(db.config.primaryKey, [db.config.configKey]);
+  });
+}

moor_generator/lib/src/model/specified_column.dart

@@ -93,10 +93,6 @@ class SpecifiedColumn {
   /// Whether this column has auto increment.
   bool get hasAI => features.any((f) => f is AutoIncrement);
 
-  /// Whether this column has been declared as the primary key via the
-  /// column builder. The `primaryKey` field in the table class is unrelated to
-  /// this.
-  final bool declaredAsPrimaryKey;
   final List<ColumnFeature> features;
 
   /// If this columns has custom constraints that should be used instead of the
@@ -157,7 +153,6 @@ class SpecifiedColumn {
     this.name,
     this.overriddenJsonName,
     this.customConstraints,
-    this.declaredAsPrimaryKey = false,
     this.nullable = false,
     this.features = const [],
     this.defaultArgument,

moor_generator/lib/src/model/specified_table.dart

@@ -52,6 +52,10 @@ class SpecifiedTable {
   /// getter on the table class with this value.
   final bool overrideWithoutRowId;
 
+  /// When non-null, the generated table class will override the
+  /// `dontWriteConstraint` getter on the table class with this value.
+  final bool overrideDontWriteConstraints;
+
   /// When non-null, the generated table class will override the
   /// `customConstraints` getter in the table class with this value.
   final List<String> overrideTableConstraints;
@@ -64,7 +68,8 @@ class SpecifiedTable {
       this.primaryKey,
       String overriddenName,
       this.overrideWithoutRowId,
-      this.overrideTableConstraints})
+      this.overrideTableConstraints,
+      this.overrideDontWriteConstraints})
       : _overriddenName = overriddenName;
 
   /// Finds all type converters used in this tables.

moor_generator/lib/src/parser/column_parser.dart

@@ -26,7 +26,6 @@ final Set<String> starters = {
 };
 
 const String _methodNamed = 'named';
-const String _methodPrimaryKey = 'primaryKey';
 const String _methodReferences = 'references';
 const String _methodAutoIncrement = 'autoIncrement';
 const String _methodWithLength = 'withLength';
@@ -74,7 +73,6 @@ class ColumnParser extends ParserBase {
     Expression foundDefaultExpression;
     Expression createdTypeConverter;
     DartType typeConverterRuntime;
-    var wasDeclaredAsPrimaryKey = false;
     var nullable = false;
 
     final foundFeatures = <ColumnFeature>[];
@@ -114,9 +112,6 @@ class ColumnParser extends ParserBase {
             );
           });
           break;
-        case _methodPrimaryKey:
-          wasDeclaredAsPrimaryKey = true;
-          break;
         case _methodReferences:
           break;
         case _methodWithLength:
@@ -130,7 +125,6 @@ class ColumnParser extends ParserBase {
           ));
           break;
         case _methodAutoIncrement:
-          wasDeclaredAsPrimaryKey = true;
           foundFeatures.add(AutoIncrement());
           break;
         case _methodNullable:
@@ -195,7 +189,6 @@ class ColumnParser extends ParserBase {
         dartGetterName: getter.name.name,
         name: name,
         overriddenJsonName: _readJsonKey(getterElement),
-        declaredAsPrimaryKey: wasDeclaredAsPrimaryKey,
         customConstraints: foundCustomConstraint,
         nullable: nullable,
         features: foundFeatures,

moor_generator/lib/src/parser/moor/parsed_moor_file.dart

@@ -75,7 +75,6 @@ class CreateTable {
         nullable: column.type.nullable,
         dartGetterName: dartName,
         name: ColumnName.implicitly(sqlName),
-        declaredAsPrimaryKey: isPrimaryKey,
         features: features,
         customConstraints: constraintWriter.toString(),
         defaultArgument: defaultValue,
@@ -92,6 +91,14 @@ class CreateTable {
 
     final constraints = table.tableConstraints.map((c) => c.span.text).toList();
 
+    for (var keyConstraint in table.tableConstraints.whereType<KeyClause>()) {
+      if (keyConstraint.isPrimaryKey) {
+        primaryKey.addAll(keyConstraint.indexedColumns
+            .map((r) => foundColumns[r.columnName])
+            .where((c) => c != null));
+      }
+    }
+
     return SpecifiedTable(
       fromClass: null,
       columns: foundColumns.values.toList(),
@@ -101,6 +108,8 @@ class CreateTable {
       primaryKey: primaryKey,
       overrideWithoutRowId: table.withoutRowId ? true : null,
       overrideTableConstraints: constraints.isNotEmpty ? constraints : null,
+      // we take care of writing the primary key ourselves
+      overrideDontWriteConstraints: true,
     );
   }
 

moor_generator/lib/src/writer/table_writer.dart

@@ -264,7 +264,9 @@ class TableWriter {
   void _overrideFieldsIfNeeded(StringBuffer buffer) {
     if (table.overrideWithoutRowId != null) {
       final value = table.overrideWithoutRowId ? 'true' : 'false';
-      buffer..write('@override\n')..write('final bool withoutRowId = $value;');
+      buffer
+        ..write('@override\n')
+        ..write('final bool withoutRowId = $value;\n');
     }
 
     if (table.overrideTableConstraints != null) {
@@ -273,7 +275,14 @@ class TableWriter {
 
       buffer
         ..write('@override\n')
-        ..write('final List<String> customConstraints = const [$value];');
+        ..write('final List<String> customConstraints = const [$value];\n');
+    }
+
+    if (table.overrideDontWriteConstraints != null) {
+      final value = table.overrideDontWriteConstraints ? 'true' : 'false';
+      buffer
+        ..write('@override\n')
+        ..write('final bool dontWriteConstraints = $value;\n');
     }
   }
 }