drift icon indicating copy to clipboard operation
drift copied to clipboard

How to ensure that the generated entity has the ‘required’ keyword regardless of whether it can be null or not?

Open ddxl123 opened this issue 1 year ago • 1 comments
trafficstars

How to ensure that the generated entity has the ‘required’ keyword regardless of whether it can be null or not? raw :

class Test2 extends DataClass implements Insertable<Test2> {
  String? client_content;
  DateTime created_at;
  int id;
  DateTime updated_at;
  Test2(
      {this.client_content,
      required this.created_at,
      required this.id,
      required this.updated_at});

to:

class Test2 extends DataClass implements Insertable<Test2> {
  String? client_content; // ----------  is nullable
  DateTime created_at;
  int id;
  DateTime updated_at;
  Test2(
      {required this.client_content, // ------- but mark required 
      required this.created_at,
      required this.id,
      required this.updated_at});

The purpose of this requirement is to sometimes add members to an entity. If the added member is nullable, I cannot know whether there is code elsewhere that needs to write a value to this member. If we mark the nullable member as required, it would be easy for me to spot the error in the editor and make the necessary modifications.

ddxl123 avatar Oct 21 '24 22:10 ddxl123

At the moment, this is not possible. You'd have to temporarily make the column non-nullable, use that to spot missing use-sites not setting the value, and then make the column nullable. But then as you write new code using the constructor you might miss values then.

I think we could have a build option to always make all parameters on these generated classes required. Thinking about how these classes are supposed to always represent a complete row in the database, it would even make sense to me to eventually make that option the default.

simolus3 avatar Oct 23 '24 14:10 simolus3

@simolus3 I've set row_class_constructor_all_required to true but I still have the null properties not required at the generated typedef XTableCreateCompanionBuilder. Is there a way to make them required?

AhmedLSayed9 avatar Jan 15 '25 15:01 AhmedLSayed9

I'm thinking about making this possible, but it's not obvious how. The create companion builder callback mirrors the function signature of the .insert constructor of companions. That should definitely stay that way, but one could argue that row_class_constructor_all_required should also affect the .insert constructor.

However:

  1. That means row_class_constructor_all_required is now a pretty poor name for what it's doing.
  2. Making everything required for row classes is reasonable because row classes are supposed to represent a full row in the database. Companions, by design, aren't. So requiring everything on a companion feels wrong to me. It's also weird to require a parameter when an explicit withDefault has been set on the table.
  3. Some parameters, like an auto-incrementing integer, really shouldn't be required under any circumstances. It breaks their whole point.

@AhmedLSayed9 Could you share your motivation for that option. I'm thinking about perhaps adding an option that marks columns as required if they're nullable and don't have an explicit default. That way, you'd have to explicitly mention null values but don't have to apply them for columns with defaults.

simolus3 avatar Jan 15 '25 19:01 simolus3

The option to make companion properties as required (for inserting) is necessary when using custom dataclass to spot places where I need to decide what to do with the new added nullable properties.

i.e, If I've the following table:

class User {
  User({required this.id, required this.username, required this.address});
  final int id;
  final String username;
  // New added property:
  final String? address;
}

@UseRowClass(User)
class Users extends Table {
  late final id = integer().autoIncrement()();
  late final username = text()();
  late final address = text().nullable()();
}

The address property is newly added, but the following code won't complain about it:

Future<void> addUser(User user) async {
  await _appDatabase.managers.users.create(
    (o) => o(
      username: user.username,
    ),
  );
}

That could lead to issues that are hard to spot (I experienced this myself recently).

I'm thinking about perhaps adding an option that marks columns as required if they're nullable and don't have an explicit default.

I agree that auto-incrementing integer shouldn't be required, but for the default value, I think making it required is possible and reasonable (unlike dart default constructors, we can pass Value.absent() to use the default value or null "when creating not updating").

That means row_class_constructor_all_required is now a pretty poor name for what it's doing

We can rename it into classes_constructors_all_required, or add a new option companion_class_constructor_all_required.

AhmedLSayed9 avatar Jan 18 '25 21:01 AhmedLSayed9