Add upsertsWriteNullValues option so upserts don't treat nulls as absent (#2998)

docs/content/generation_options/index.md

@@ -77,6 +77,15 @@ At the moment, drift supports these options:
   The function has a parameter for each table that is available in the query, making it easier to get aliases right when using
   Dart placeholders.
 - `store_date_time_values_as_text`: Whether date-time columns should be stored as ISO 8601 string instead of a unix timestamp.
+- `upserts_write_null_values` (defaults to `false`): Controls how the `DO UPDATE SET` clause of an upsert
+  (e.g. `insertOnConflictUpdate`, or `insert` with a `DoUpdate` clause) handles columns that are `null`.
+  By default, drift treats `null` values as absent and leaves those columns out of the update clause, so a
+  conflicting row keeps whatever value it had before. That means the same upsert produces a different row
+  depending on whether it inserted or updated. With this option enabled, `null` values are written out
+  explicitly (`SET "col" = NULL`), so the resulting row always matches the row that was passed to the insert.
+  This only affects insertables that distinguish `null` from absent by value (like generated row classes) -
+  companions always write exactly the values that are `present`, with or without this option.
+  See [issue 2998](https://github.com/simolus3/drift/issues/2998) for context.
   For more information on these modes, see [datetime options](../dart_api/tables.md#datetime-options).
 - `case_from_dart_to_sql` (defaults to `snake_case`): Controls how the table and column names are re-cased from the Dart identifiers.
   The possible values are `preserve`, `camelCase`, `CONSTANT_CASE`, `snake_case`, `PascalCase`, `lowercase` and `UPPERCASE` (default: `snake_case`).

drift/CHANGELOG.md

@@ -1,3 +1,10 @@
+## 2.34.5-wip
+
+- Add `DriftDatabaseOptions.upsertsWriteNullValues`, an opt-in option making the
+  `DO UPDATE SET` clause of upserts write `null` values instead of treating them
+  as absent. This makes an upsert yield the row that was passed to it regardless
+  of whether it inserted or updated (#2998).
+
 ## 2.34.4
 
 - Allow using `rowid` extension on virtual tables, as they usually provide rowids as well.

drift/lib/src/runtime/api/options.dart

@@ -12,6 +12,40 @@ class DriftDatabaseOptions {
 
   final bool _storeDateTimeAsText;
 
+  /// Whether the `DO UPDATE SET` clause of an upsert should assign columns
+  /// holding a `null` value instead of leaving them out of the statement.
+  ///
+  /// By default (and for backwards-compatibility), drift builds the update
+  /// clause of an upsert by treating `null` values as absent. So for an
+  /// [Insertable] describing a row with a `null` column, that column is
+  /// omitted from `DO UPDATE SET` entirely, meaning that a conflicting row
+  /// keeps whatever value it had before:
+  ///
+  /// ```sql
+  /// -- with upsertsWriteNullValues: false (the default)
+  /// INSERT INTO "users" ("id", "name", "nickname") VALUES (?, ?, NULL)
+  ///   ON CONFLICT("id") DO UPDATE SET "id" = ?, "name" = ?
+  /// ```
+  ///
+  /// This is a sensible default for partial updates, but it is surprising for
+  /// upserts: the row that was inserted and the row that was updated no longer
+  /// describe the same data. When this option is enabled, `null` values are
+  /// written out explicitly, so that a conflicting row ends up matching the row
+  /// that was passed to the insert:
+  ///
+  /// ```sql
+  /// -- with upsertsWriteNullValues: true
+  /// INSERT INTO "users" ("id", "name", "nickname") VALUES (?, ?, NULL)
+  ///   ON CONFLICT("id") DO UPDATE SET "id" = ?, "name" = ?, "nickname" = NULL
+  /// ```
+  ///
+  /// Note that this only affects columns that are explicitly set to `null`.
+  /// Columns that are absent from the [Insertable] (e.g. a `Value.absent()` in
+  /// a companion) are never written, regardless of this option.
+  ///
+  /// See also: https://github.com/simolus3/drift/issues/2998
+  final bool upsertsWriteNullValues;
+
   /// Creates database-specific database options.
   ///
   /// When [storeDateTimeAsText] is enabled (it defaults to `false` for
@@ -20,13 +54,19 @@ class DriftDatabaseOptions {
   ///
   /// For details on how datetimes can be stored, see [the documentation].
   ///
+  /// When [upsertsWriteNullValues] is enabled (it defaults to `false` for
+  /// backwards-compatibility), `null` values are written into the
+  /// `DO UPDATE SET` clause of upserts instead of being treated as absent.
+  ///
   /// [the documentation]: https://drift.simonbinder.eu/docs/getting-started/advanced_dart_tables/#supported-column-types
-  const DriftDatabaseOptions({bool storeDateTimeAsText = false})
-    : _storeDateTimeAsText = storeDateTimeAsText,
-      // ignore: deprecated_member_use_from_same_package
-      types = storeDateTimeAsText
-          ? const SqlTypes(true)
-          : const SqlTypes(false);
+  const DriftDatabaseOptions({
+    bool storeDateTimeAsText = false,
+    this.upsertsWriteNullValues = false,
+  }) : _storeDateTimeAsText = storeDateTimeAsText,
+       // ignore: deprecated_member_use_from_same_package
+       types = storeDateTimeAsText
+           ? const SqlTypes(true)
+           : const SqlTypes(false);
 
   /// Creates a type mapping suitable for these options and the given [dialect].
   SqlTypes createTypeMapping(SqlDialect dialect) {

drift/lib/src/runtime/query_builder/statements/insert.dart

@@ -389,7 +389,14 @@ class InsertStatement<T extends Table, D> {
             .throwIfInvalid(upsertInsertable);
       }
 
-      final updateSet = upsertInsertable.toColumns(true);
+      // Unless `upsertsWriteNullValues` is enabled, `null` values are treated
+      // as absent here, which means that they are left out of the update
+      // clause entirely. That keeps the previous value of a conflicting row
+      // instead of overwriting it with `null`.
+      // https://github.com/simolus3/drift/issues/2998
+      final updateSet = upsertInsertable.toColumns(
+        !ctx.options.upsertsWriteNullValues,
+      );
 
       writeOnConflictConstraint(
         onConflict.target,

drift/test/database/statements/upsert_null_values_test.dart

@@ -0,0 +1,200 @@
+@TestOn('vm')
+library;
+
+import 'package:drift/drift.dart' hide isNull;
+import 'package:mockito/mockito.dart';
+import 'package:test/test.dart';
+
+import '../../generated/todos.dart';
+import '../../test_utils/test_utils.dart';
+
+/// Tests for [DriftDatabaseOptions.upsertsWriteNullValues].
+///
+/// Without the option, the `DO UPDATE SET` clause of an upsert treats `null`
+/// values as absent, so a conflicting row keeps whatever value it had before.
+/// That makes `INSERT ... ON CONFLICT DO UPDATE` behave differently depending
+/// on whether the row already existed, see
+/// https://github.com/simolus3/drift/issues/2998.
+void main() {
+  group('SQL generation', () {
+    late TodoDb db;
+    late MockExecutor executor;
+
+    setUp(() {
+      executor = MockExecutor();
+      db = TodoDb(createConnection(executor, MockStreamQueries()));
+    });
+
+    /// A full row with `null` in every nullable column.
+    const entry = TodoEntry(id: RowId(3), content: 'content');
+
+    group('with the default options', () {
+      test('drops null columns from the update clause', () async {
+        await db.into(db.todosTable).insertOnConflictUpdate(entry);
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "content") VALUES (?, ?) '
+            'ON CONFLICT("id") DO UPDATE SET "id" = ?, "content" = ?',
+            [3, 'content', 3, 'content'],
+          ),
+        );
+      });
+
+      test('drops null columns for an explicit DoUpdate clause', () async {
+        await db
+            .into(db.todosTable)
+            .insert(entry, onConflict: DoUpdate((_) => entry));
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "content") VALUES (?, ?) '
+            'ON CONFLICT("id") DO UPDATE SET "id" = ?, "content" = ?',
+            [3, 'content', 3, 'content'],
+          ),
+        );
+      });
+    });
+
+    group('with upsertsWriteNullValues', () {
+      setUp(() {
+        db.options = const DriftDatabaseOptions(upsertsWriteNullValues: true);
+      });
+
+      test('writes null columns into the update clause', () async {
+        await db.into(db.todosTable).insertOnConflictUpdate(entry);
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "content") VALUES (?, ?) '
+            'ON CONFLICT("id") DO UPDATE SET '
+            '"id" = ?, "title" = ?, "content" = ?, '
+            '"target_date" = ?, "category" = ?, "status" = ?',
+            [3, 'content', 3, null, 'content', null, null, null],
+          ),
+        );
+      });
+
+      test('writes null columns for an explicit DoUpdate clause', () async {
+        await db
+            .into(db.todosTable)
+            .insert(entry, onConflict: DoUpdate((_) => entry));
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "content") VALUES (?, ?) '
+            'ON CONFLICT("id") DO UPDATE SET '
+            '"id" = ?, "title" = ?, "content" = ?, '
+            '"target_date" = ?, "category" = ?, "status" = ?',
+            [3, 'content', 3, null, 'content', null, null, null],
+          ),
+        );
+      });
+
+      test('does not change the INSERT part of the statement', () async {
+        await db.into(db.todosTable).insertOnConflictUpdate(entry);
+
+        final sql =
+            verify(executor.runInsert(captureAny, any)).captured.single
+                as String;
+        expect(sql, startsWith('INSERT INTO "todos" ("id", "content")'));
+      });
+
+      test('leaves absent companion values out of the update clause', () async {
+        // Companions distinguish between `Value.absent()` and `Value(null)`,
+        // so they must not be affected by this option at all.
+        await db
+            .into(db.todosTable)
+            .insertOnConflictUpdate(
+              const TodosTableCompanion(
+                id: Value(RowId(3)),
+                content: Value('content'),
+                title: Value(null),
+              ),
+            );
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "title", "content") '
+            'VALUES (?, ?, ?) '
+            'ON CONFLICT("id") DO UPDATE SET '
+            '"id" = ?, "title" = ?, "content" = ?',
+            [3, null, 'content', 3, null, 'content'],
+          ),
+        );
+      });
+
+      test('does not affect DoNothing clauses', () async {
+        await db.into(db.todosTable).insert(entry, onConflict: DoNothing());
+
+        verify(
+          executor.runInsert(
+            'INSERT INTO "todos" ("id", "content") VALUES (?, ?) '
+            'ON CONFLICT("id") DO NOTHING',
+            [3, 'content'],
+          ),
+        );
+      });
+    });
+  });
+
+  group('against a real database', () {
+    late TodoDb db;
+
+    setUp(() => db = TodoDb(testInMemoryDatabase()));
+    tearDown(() => db.close());
+
+    Future<TodoEntry> insertInitialRow() async {
+      await db
+          .into(db.todosTable)
+          .insert(
+            TodosTableCompanion.insert(
+              content: 'initial content',
+              title: const Value('initial title'),
+              status: const Value(TodoStatus.open),
+            ),
+          );
+      return db.select(db.todosTable).getSingle();
+    }
+
+    test('keeps stale values by default', () async {
+      final row = await insertInitialRow();
+
+      // Upsert the same row, but with the nullable columns cleared.
+      final cleared = TodoEntry(id: row.id, content: 'new content');
+      await db.into(db.todosTable).insertOnConflictUpdate(cleared);
+
+      final updated = await db.select(db.todosTable).getSingle();
+      expect(updated.content, 'new content');
+      // The bug: the row that comes back does not match the row we upserted.
+      expect(updated.title, 'initial title');
+      expect(updated.status, TodoStatus.open);
+      expect(updated, isNot(cleared));
+    });
+
+    test('writes nulls with upsertsWriteNullValues', () async {
+      final row = await insertInitialRow();
+      db.options = const DriftDatabaseOptions(upsertsWriteNullValues: true);
+
+      final cleared = TodoEntry(id: row.id, content: 'new content');
+      await db.into(db.todosTable).insertOnConflictUpdate(cleared);
+
+      final updated = await db.select(db.todosTable).getSingle();
+      expect(updated.content, 'new content');
+      expect(updated.title, null);
+      expect(updated.status, null);
+      // The whole point: an upsert now yields the row that was passed in,
+      // regardless of whether it inserted or updated.
+      expect(updated, cleared);
+    });
+
+    test('upserting a new row is unaffected by the option', () async {
+      db.options = const DriftDatabaseOptions(upsertsWriteNullValues: true);
+
+      const fresh = TodoEntry(id: RowId(1), content: 'content');
+      await db.into(db.todosTable).insertOnConflictUpdate(fresh);
+
+      expect(await db.select(db.todosTable).getSingle(), fresh);
+    });
+  });
+}

drift_dev/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 2.34.7-wip
+
+- Add the `upserts_write_null_values` build option, enabling
+  `DriftDatabaseOptions.upsertsWriteNullValues` on generated databases (#2998).
+
 ## 2.34.6
 
 - Remove legacy command used to migrate from `moor` from `drift`.

drift_dev/lib/src/analysis/options.dart

@@ -135,6 +135,13 @@ class DriftOptions {
   @JsonKey(defaultValue: false)
   final bool storeDateTimeValuesAsText;
 
+  /// Whether the `DO UPDATE SET` clause of an upsert should assign columns
+  /// holding a `null` value instead of treating them as absent.
+  ///
+  /// See `DriftDatabaseOptions.upsertsWriteNullValues` for details.
+  @JsonKey(name: 'upserts_write_null_values', defaultValue: false)
+  final bool upsertsWriteNullValues;
+
   @JsonKey(name: 'case_from_dart_to_sql', defaultValue: CaseFromDartToSql.snake)
   final CaseFromDartToSql caseFromDartToSql;
 
@@ -185,6 +192,7 @@ class DriftOptions {
     this.modules = const [],
     this.sqliteAnalysisOptions,
     this.storeDateTimeValuesAsText = false,
+    this.upsertsWriteNullValues = false,
     this.dialect = const DialectOptions(null, [SqlDialect.sqlite], null),
     this.caseFromDartToSql = CaseFromDartToSql.snake,
     this.preamble,
@@ -221,6 +229,7 @@ class DriftOptions {
     required this.modules,
     required this.sqliteAnalysisOptions,
     required this.storeDateTimeValuesAsText,
+    required this.upsertsWriteNullValues,
     required this.caseFromDartToSql,
     required this.writeToColumnsMixins,
     required this.fatalWarnings,