Repository navigation
Upsert against db object behaves differently to upsert against batch #2998
Description
I recently changed some code in my project, from using a batch (to not).
entity.stringProperty = null;
batch.insert(table, entity, onConflict: DoUpdate((_) => entity));
to
entity.stringProperty = null;
await db.into(table).insert(entity, onConflict: DoUpdate((_) => return entity ))
The use case for my code, is to basically upsert - meaning all columns are present in the entity, and need to either insert or overwrite the destination record.
The version which uses a batch will successfully nullify the previously non-null column in the database.
However, the latter version, will not.
It appears that the latter version follows a code-path which chooses to omit SQL for columns which are considered 'default values' - i.e - a null string.
However, this logic is incorrect, as this is an update.
My interpretation of 'insert or update' is that all columns are present for an insert, so if an update is chosen transparently to be performed by the library, all columns should still be honored. Am I misinterpreting? If so, what should I be calling instead?
insert.dart:323
final updateSet = upsertInsertable.toColumns(true); // the 'null to absent' appears to be incorrect logic for an upsert
Note: The reason I use the version of method with the onConflict callback, instead of insertOnConflictUpdate, is because I want to know if drift decided to do an update instead of an insert, so I can log the fact.
Activity
simolus3 commentedon May 8, 2024
The line you've referenced would actually be executed both for batches and for regular inserts, so I don't see how that should make a difference. I have slightly different line numbers so it's possible this changed recently, but both
batch.insertandInsertStatement.insertcallInsertStatement.createContextwith the same arguments in the end, right?I don't think that's possible - we'll execute the callback either way to construct the update statement. The actual decision happens in the database, so the only way to be sure is to add a returning clause in SQL.
I agree with you here, but changing this now is also a subtle potentially breaking change. I think we should fix it in the next breaking release.
Adam-Langley commentedon May 8, 2024
Thanks @simolus3 - I'll build off a fork until then.
Regarding your statement about onConflict always being called - I didn't realise this - thanks.
I see that in SQLite, it's possible to do an
INSERT... ON CONFLICT UPDATE... RETURNING *to determine whether the INSERT or UPDATE was executed (RETURNING * will return * on an INSERT, but nothing on an UPDATE)Could I request we use this to avoid triggering the 'onConflict'? Or something else suitable for determining what an UPSERT resulted in?
Thanks
Adam-Langley commentedon Sep 1, 2026
Hi there @simolus3
I wonder if I can bring this item to your attention again? I've been maintaining a fork for this little one-liner fix for a while.
Maybe we integrate it with a feature flag to mitigate the breakiness?
Thanks
feat: add `upsertsWriteNullValues` option for upserts (simolus3#2998)
drift3: Fix toColumns for batch insert