Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Primary Key not required in companion insert #3039

Closed
FaFre opened this issue Jun 10, 2024 · 2 comments
Closed

Primary Key not required in companion insert #3039

FaFre opened this issue Jun 10, 2024 · 2 comments

Comments

@FaFre
Copy link
Contributor

FaFre commented Jun 10, 2024

I found a minor bug :)

In this case I have an ENUM as primary key. The companion marks the "group" parameter as optional, which is wrong, because primary key is always mandatory.

CREATE TABLE bang_sync (
  "group" ENUM(BangGroup) PRIMARY KEY NOT NULL,
  last_sync DATETIME NOT NULL
);
class BangSync extends Table with TableInfo<BangSync, BangSyncData> {
  @override
  final GeneratedDatabase attachedDatabase;
  final String? _alias;
  BangSync(this.attachedDatabase, [this._alias]);
  late final GeneratedColumnWithTypeConverter<BangGroup, int> group =
      GeneratedColumn<int>('group', aliasedName, false,
              type: DriftSqlType.int,
              requiredDuringInsert: false,
              $customConstraints: 'PRIMARY KEY NOT NULL')
          .withConverter<BangGroup>(BangSync.$convertergroup);
  late final GeneratedColumn<DateTime> lastSync = GeneratedColumn<DateTime>(
      'last_sync', aliasedName, false,
      type: DriftSqlType.dateTime,
      requiredDuringInsert: true,
      $customConstraints: 'NOT NULL');
  @override
  List<GeneratedColumn> get $columns => [group, lastSync];
  @override
  String get aliasedName => _alias ?? actualTableName;
  @override
  String get actualTableName => $name;
  static const String $name = 'bang_sync';
  @override
  Set<GeneratedColumn> get $primaryKey => {group};
  @override
  BangSyncData map(Map<String, dynamic> data, {String? tablePrefix}) {
    final effectivePrefix = tablePrefix != null ? '$tablePrefix.' : '';
    return BangSyncData(
      group: BangSync.$convertergroup.fromSql(attachedDatabase.typeMapping
          .read(DriftSqlType.int, data['${effectivePrefix}group'])!),
      lastSync: attachedDatabase.typeMapping
          .read(DriftSqlType.dateTime, data['${effectivePrefix}last_sync'])!,
    );
  }

  @override
  BangSync createAlias(String alias) {
    return BangSync(attachedDatabase, alias);
  }

  static JsonTypeConverter2<BangGroup, int, int> $convertergroup =
      const EnumIndexConverter<BangGroup>(BangGroup.values);
  @override
  bool get dontWriteConstraints => true;
}

...

class BangSyncCompanion extends UpdateCompanion<BangSyncData> {
  final Value<BangGroup> group;
  final Value<DateTime> lastSync;
  const BangSyncCompanion({
    this.group = const Value.absent(),
    this.lastSync = const Value.absent(),
  });
  BangSyncCompanion.insert({
    this.group = const Value.absent(),
    required DateTime lastSync,
  }) : lastSync = Value(lastSync);

build.yml:

targets:
  $default:
    builders:
      drift_dev:
        # These options change how drift generates code
        options:
          skip_verification_code: true
          data_class_to_companions: false
          named_parameters: true
          sql:
            dialect: sqlite
            options:
              version: "3.45"
              modules:
                - json1
                - fts5
@simolus3
Copy link
Owner

Heh, so here it's technically not required because ENUM maps values to an index, so this is an integer column in the database. And in sqlite3, a sole INTEGER PRIMARY KEY is an alias to the rowid which is auto-incrementing by default.

You could use ENUMNAME to work around the issue (while changing the schema obviously). I agree that this result is confusing, but special casing ENUM columns to be required is also deviating from the standard rules which is a bit weird.
If you prefer mapping to an index, you could consider a unique key instead of a primary key - that way you have an independent rowid column and the enum is required.

@simolus3 simolus3 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@FaFre
Copy link
Contributor Author

FaFre commented Jun 12, 2024

Aha, that's interesting, thanks for pointing out :)
I was suspecting there is something wrong with the ENUM as key in general, but now it is clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants