Upsert against db object behaves differently to upsert against batch

[Adam-Langley]

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.

[simolus3]

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.insert and InsertStatement.insert call InsertStatement.createContext with the same arguments in the end, right?

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.

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.

However, this logic is incorrect, as this is an update.

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]

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