simolus3 / drift

Drift is an easy to use, reactive, typesafe persistence library for Dart & Flutter.
https://drift.simonbinder.eu/
MIT License
2.66k stars 372 forks source link

Define References without Column #3267

Closed dickermoshe closed 1 month ago

dickermoshe commented 1 month ago

@simolus3 What do you think about making the second argument optional and use the pk of the other table:

class User extends Table {
  late final id = integer().autoIncrement()();
  late final name = text().unique()();
  late final groupId = integer().references(Group)(); // Thats it!
}

class Group extends Table {
  late final id = integer().autoIncrement()();
  late final name = text()();
}

If there are multiple pks, require it be provided:

class User extends Table {
  late final id = integer().autoIncrement()();
  late final name = text().unique()();
  late final groupId = integer().references(Group,)(); // Throw exception!!
}

class Group extends Table {
  late final id1 = integer();
  late final id2 = integer();
  late final name = text()();
  @override
  Set<Column<Object>>? get primaryKey => {id1,id2};
}

It would require parsing the tables in 2 passes, if this will require too much work, it's we can just skip it.

simolus3 commented 1 month ago

I agree that this is a useful change :+1:

It would require parsing the tables in 2 passes, if this will require too much work, it's we can just skip it.

I don't think it's that much more work, or we probably have the information already. Since we disallow circular references, we can obtain the fully analyzed referenced table (currently line 257 in resolver/dart/column.dart). We can obtain the primary key from there to infer the reference if necessary.

The bigger problem is that we can't have both named and optional indexed parameters on references(), and there are already named parameters for onUpdate and onDelete. So we can't make the column symbol optional anymore. We could perhaps add a referencesTable method that doesn't have column as a parameter at all?

dickermoshe commented 1 month ago

Nah, I'd hate to clutter the namespace anymore.