simolus3 / drift

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

insertOnConflictUpdate behaves as an insert #3084

Closed im-trisha closed 1 week ago

im-trisha commented 2 weeks ago

The db.into(table).insertOnConflictUpdate function seems to behave too much like an insert query, whereas it should behave as an upsert/patch query.

If i have this table (removed a few elements to make this clearer):

class Users extends Table {
  // generated from another service
  IntColumn get id => integer()();

  TextColumn get firstName => text()();
  TextColumn get lastName => text().nullable()();

  TextColumn get nickname => text().nullable()();

  @override
  Set<Column<Object>>? get primaryKey => {id};
}

And i try to upsert like such:

db.into(db.users).insertOnConflictUpdate(
  UsersCompanion(
    id: Value(id),
    firstName: Value.absentIfNull(firstName),
    lastName: Value.absentIfNull(lastName),
    nickname: Value.absentIfNull(friendlyNickname),
  ),
);

In case "firstName" is not provided, and the user actually exists (100% sure) then i get the following error, like if i was trying to just insert the object:

Error: InvalidDataException: Sorry, UsersCompanion(id: Value(1234567890), firstName: Value.absent(), lastName: Value.absent(), friendlyNickname: Value.absent(), createdAt: Value.absent()) cannot be used for that because:
│ • firstName: This value was required, but isn't present
im-trisha commented 2 weeks ago

p.s. I just hope to not have said anything dumb, but as far as i'm aware of, this shouldn't happen

kuhnroyal commented 2 weeks ago

Well, since drift doesn't know that the row already exists, it always needs an object that would be a valid insert. If you as developer already know that the row exists, then you can just use any other update call.

im-trisha commented 2 weeks ago

@kuhnroyal That's what I mean. Drift pre-emptively says it is not allowed to leave firstName empty, but it shouldn't As a developer, I'd like to handle the error in case I, for some reason, try to create an invalid query (In this case), and not to be prevented of doing it altogether

There are situations where i want to patch without using firstName, and situations where i want to patch while having firstName

simolus3 commented 1 week ago

Drift pre-emptively says it is not allowed to leave firstName empty, but it shouldn't

It looks like we disagree here. My position is that drift essentially offers type-safe SQL. And in particular for inserts, that should also include safety around not potentially inserting null values into columns that shouldn't be null. You don't know whether an upsert is an insert or an update, so the data you provide must be compatible with both, which drift checks for you.

If you know that a row already exists and you want to update it (and that is a requirement for calling upserts with a null firstName), why aren't you using an update?

You can disable these checks by enabling the skip_verification_code generation option, but I think drift is working as intended here.

simolus3 commented 1 week ago

There are situations where i want to patch without using firstName, and situations where i want to patch while having firstName

In this case I would actually suggest an explicit update when you don't have a firstName and an upsert if you do. It might seem pedantic, but I still think that unconditionally using upserts here is not correct. Upserts are for when you don't know whether a row exists or not, but when you don't have a firstName you are asserting that one exists. It's better to make that explicit with an update.

im-trisha commented 1 week ago

I mean, you are not wrong thinking about it... I think I'll update in those cases, but having to add additional methods to my DAO's on a simple projects makes it look kinda messy, as the more tables there are the more repetitive code there is... and the more error prone it gets (Especially if you want to add some columns)

Thanks for your nice answers and sorry for wasting your time!

simolus3 commented 1 week ago

No problem, and no time was wasted here at all :) The added complexity is a valid point, I'm mostly concerned about giving up on safety. So maybe an "upsert but only an update if columns are missing with runtime checks" is a valid addition to make this easier.

im-trisha commented 1 week ago

It could be useful, there are some instances where SQL errors are thrown anyway... But I'm not sure

As an added point (Forgot to mention it), the reason that makes creating DAOs so "painful" is the fact that if a table has more than 3 or 4 columns it becames a mess of functions with 10 parameters and just one or two lines of code for the insert/update/delete