Add upsertsWriteNullValues option so upserts don't treat nulls as absent (#2998) - #3851
Open
Adam-Langley wants to merge 1 commit into
Open
Adam-Langley wants to merge 1 commit into
Adam-Langley wants to merge 1 commit into
Conversation
The `DO UPDATE SET` clause of an upsert is built with `toColumns(true)`, which treats `null` values as absent and leaves those columns out of the statement entirely. A conflicting row therefore keeps whatever value it had before, so `INSERT ... ON CONFLICT DO UPDATE` produces a different row depending on whether it inserted or updated. This adds `DriftDatabaseOptions.upsertsWriteNullValues`, defaulting to `false` so existing behavior is unchanged. When enabled, `null` values are written out explicitly and an upsert always yields the row that was passed to it. The option can be enabled declaratively with the `upserts_write_null_values` build option, which makes `drift_dev` emit the corresponding `options` override on the generated database - the same mechanism already used by `store_date_time_values_as_text`. Note that this only affects insertables which distinguish `null` from absent by value, such as generated row classes. Companions always write exactly the values that are `present`, with or without the option.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2998.
The problem
The
DO UPDATE SETclause of an upsert is built withtoColumns(true)—nullToAbsent— so columns holdingnullare dropped from the statement entirely and a conflicting row keeps whatever value it had before:INSERT ... ON CONFLICT DO UPDATEtherefore yields a different row depending on whether it inserted or updated. Passing a complete row and getting a half-updated one back is surprising, and it's the behavior reported in #2998.This only affects insertables that distinguish
nullfrom absent by value — generated row classes andRawValuesInsertable. Companions are unaffected either way: their generatedtoColumnsignoresnullToAbsentand writes exactly the fields that arepresent.The change
A new
DriftDatabaseOptions.upsertsWriteNullValues, defaulting tofalse, so behavior is unchanged unless opted in. When enabled:The runtime change is one expression in
insert.dart:It's enabled declaratively through a new
upserts_write_null_valuesbuild option, which makesdrift_devemit theoptionsoverride on the generated database — reusing the mechanismstore_date_time_values_as_textalready uses, and composing with it:I went with an opt-in flag rather than just changing the default, since the current behavior is observable and someone may well be relying on it. Happy to flip it to a default (or a breaking change slated for drift 3) if you'd prefer — the flag plumbing is the same either way.
Results
Before — the same assertions against
developunmodifiedThe new test file with the three
upsertsWriteNullValues: truelines removed (the API doesn't exist yet); everything else byte-identical.7 passed, 3 failed. The integration failure (
Expected: <null> Actual: 'initial title') is #2998 reproduced against a real sqlite3 database.After
10 passed. The two "with the default options" tests pass in both runs — that's the backwards-compatibility guarantee: with the flag off the emitted SQL is unchanged.
Generator tests
Full suites
driftdrift_devdart analyze --fatal-infos(both)dart format(changed files)Scope / notes
INSERThalf of the statement is untouched; onlyDO UPDATE SETchanges.DoNothingandDoUpdate.withExcludedare unaffected — both covered by tests.drift3_preview's compiler has the sametoColumns(true)inaddDoUpdate(drift/lib/src/drift3_preview/src/query_builder/compiler.dart:1096), but there's noDriftDatabaseOptionsequivalent in that runtime to read the flag from. I left it alone rather than introducing a database-options concept to the preview package unilaterally — glad to follow up if you'd like it covered, and if so, guidance on where you'd want those options to live would help.