Using copyWith on a nullable FK results in no changes if value is null

[kevinhikaruevans]

Describe the bug

I have a table with an FK deviceGroupId which references another table called deviceGroup:

  @ReferenceName('deviceGroup')
  IntColumn get deviceGroupId => integer().nullable().references(
        DeviceGroups,
        #id,
        onDelete: KeyAction.setNull,
      )();

In my DAO, I have a function updatePopulatedDeviceGroup that can update the FK to either another ID or to null. Although updating it to an actual value works, setting it to null does not affect the row, as writeReturning returns the same row with the same value. Here, I am calling updatePopulatedDeviceGroup(..., null):

  /// Sets a single [populatedDevice]'s [group]
  /// Returns the new populated device
  Future<PopulatedDevice?> updatePopulatedDeviceGroup(PopulatedDevice populatedDevice, DeviceGroup? group) async {
    if (populatedDevice.deviceGroup?.id == group?.id) {
      _logger.info("nothing to update");
      
      return populatedDevice;
    }

    final query = update(devices)..whereSamePrimaryKey(populatedDevice.device);
    final copy = populatedDevice.device.copyWith(
      deviceGroupId: Value(group?.id), // <-- This doesn't seem to work if `group` is null!
    );
    // I am thinking the error is in the write itself, not the `copyWith`, as `copyWith` correctly sets the `copy.deviceGroupId` as null when inspecting in the debugger.
    final x = await query.writeReturning(copy);
    // at this point, x.deviceGroupId will still be non-null!
    // [...]

Note that, if I set a new companion with all other values absent, it works just fine, e.g.

    var x = await query.writeReturning(DevicesCompanion(
      deviceGroupId: Value(group?.id), // <-- But this works!
    ));

[simolus3]

This is a known problem with data classes (and part of the reason companions exist). I'm not sure if we should change the behavior or forbid using data classes for updates and deletes outright.

This is even documented: writeReturning mentions that it works like write, which in turn says that it will write all non-null fields. So, since deviceGroupId is null, it's not being written. Being documented doesn't make this a good decision though, and if I remember correctly this is an artifact from an old time where we didn't have companions yet.

You can fix this in your code by using query.writeReturning(copy.toCompanion(false)) (since false does not replace null values with Value.absent()). I think forcing users to be explicit about how drift should deal with null values in data classes would be a more sensible default. The current behavior of ignoring null values is horrible, but changing it is a subtle breaking change, which is also really bad. Maybe we just should forbid updates with data classes and force users to put a toCompanion in there to spell the behavior out.

[kevinhikaruevans]

Ah, got it now. Thanks for the explanation. I don't mind either way---explicitly handling nulls somewhere vs forbidding updates with data classes.