fix: Reject updates referencing soft-deleted rows (#36)

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Marcelo Soares <marcelo.me.soares@gmail.com>

packages/serverpod_offline_sync_client/lib/src/database/recorder.dart

@@ -205,6 +205,8 @@ class CrdtMutationRecorder {
     List<Column>? columns,
     Transaction transaction,
   ) async {
+    await _assertVisibleForeignKeyTargets(updatedRows, columns, transaction);
+
     await _forTrackedRows(updatedRows, transaction, (
       tableName,
       rowIds,
@@ -220,6 +222,101 @@ class CrdtMutationRecorder {
     });
   }
 
+  Future<void> _assertVisibleForeignKeyTargets<T extends TableRow>(
+    List<T> rows,
+    List<Column>? columns,
+    Transaction transaction,
+  ) async {
+    if (rows.isEmpty) return;
+    if (!isCrdtTracked<T>(rows.first.table)) return;
+
+    final tableName = rows.first.table.tableName;
+    final tableDefinition = _tableDefinitionsByName[tableName];
+    if (tableDefinition == null) return;
+
+    final updatedColumnNames = columns?.map((column) => column.columnName).toSet();
+    final foreignKeysToCheck = [
+      for (final foreignKey in tableDefinition.foreignKeys)
+        if (foreignKey.columns.length == 1 &&
+            foreignKey.referenceColumns.length == 1 &&
+            (updatedColumnNames == null ||
+                updatedColumnNames.contains(foreignKey.columns.single)) &&
+            _isCrdtTrackedTableName(foreignKey.referenceTable))
+          foreignKey,
+    ];
+    if (foreignKeysToCheck.isEmpty) return;
+
+    final invalid = await _findInvalidForeignKeyReference(
+      childTableName: tableName,
+      childRowIds: rows.uuidRowIds,
+      foreignKeys: foreignKeysToCheck,
+      transaction: transaction,
+    );
+    if (invalid == null) return;
+
+    final foreignKey = foreignKeysToCheck[invalid.foreignKeyIndex];
+    throw Exception(
+      'Cannot reference deleted row ${foreignKey.referenceTable}.'
+      '${foreignKey.referenceColumns.single} = ${invalid.value}.',
+    );
+  }
+
+  /// Finds the first child row whose foreign key points at a parent row that
+  /// is missing or tombstoned, returning the index of the offending foreign
+  /// key in [foreignKeys] together with the offending value.
+  ///
+  /// All [foreignKeys] are checked in a single round-trip via `UNION ALL`.
+  /// This avoids serializing the just-updated rows to JSON only to extract a
+  /// single column value, and keeps the assertion at one query regardless of
+  /// how many foreign key columns are being updated.
+  Future<({int foreignKeyIndex, Object value})?> _findInvalidForeignKeyReference({
+    required String childTableName,
+    required Set<UuidValue> childRowIds,
+    required List<ForeignKeyDefinition> foreignKeys,
+    required Transaction transaction,
+  }) async {
+    if (foreignKeys.isEmpty) return null;
+
+    final userId = _getHlcManager(transaction).normalizedUserId;
+    final escapedChildTable = _escapeIdentifier(childTableName);
+    final whereRowIds = _sqlLiteralList(childRowIds);
+
+    final foreignKeyBranches = foreignKeys.indexed.map((e) {
+      final (index, foreignKey) = e;
+      final childColumn = _escapeIdentifier(foreignKey.columns.single);
+      final parentTable = _escapeIdentifier(foreignKey.referenceTable);
+      final parentColumn = _escapeIdentifier(
+        foreignKey.referenceColumns.single,
+      );
+      final (parentTableId, _) = _schema[foreignKey.referenceTable]!;
+
+      return '''
+SELECT $index AS fk_idx, c."$childColumn" AS invalid_value
+FROM "$escapedChildTable" c
+LEFT JOIN "$parentTable" p
+  ON p."$parentColumn" = c."$childColumn"
+LEFT JOIN "crdt_data_rows" r
+  ON r."userId" = $userId AND r."tblId" = $parentTableId AND r."uuidRowId" = p."id"
+LEFT JOIN "crdt_data_tombstone" tomb
+  ON tomb."rowId" = r."id"
+WHERE c."id" IN ($whereRowIds)
+  AND c."$childColumn" IS NOT NULL
+  AND (p."id" IS NULL OR tomb."isDeleted" = TRUE)
+''';
+    });
+
+    final result = await _db.unsafeQuery(
+      '${foreignKeyBranches.join('UNION ALL\n')}LIMIT 1',
+      transaction: transaction,
+    );
+    if (result.isEmpty) return null;
+
+    return (
+      foreignKeyIndex: result.first[0] as int,
+      value: result.first[1] as Object,
+    );
+  }
+
   Future<void> _forTrackedRows<T extends TableRow>(
     List<T> rows,
     Transaction transaction,

test/serverpod_offline_sync_test_server/test/integration/crud/delete_test.dart

@@ -480,7 +480,7 @@ void main() {
 
       test('then the town row is updated.', () async {
         final updatedTown = await Town.db.findFirstRow(
-          testSession,
+          session,
           where: (t) => t.id.equals(town.id),
         );
         expect(updatedTown, isNotNull);

test/serverpod_offline_sync_test_server/test/integration/crud/find_test.dart

@@ -1,3 +1,4 @@
+import 'package:serverpod_offline_sync_server/serverpod_offline_sync_server.dart';
 import 'package:serverpod_offline_sync_test_client/serverpod_offline_sync_test_client.dart';
 import 'package:test/test.dart';
 
@@ -48,4 +49,93 @@ void main() {
       );
     });
   });
+
+  // This test is not expected to happen in production since the tombstone
+  // should never be manually accessed. It is here mostly to validate the
+  // correct filtering of include filters.
+  group(
+    'Given a town row that references a manually tombstoned person row,',
+    () {
+      late Town town;
+      late Person person;
+
+      setUp(() async {
+        town = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Town.db.insertRow(session, Town(name: 'Rio'), transaction: tx),
+        );
+
+        person = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Person.db.insertRow(session, Person(name: 'Lisa'), transaction: tx),
+        );
+
+        await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Town.db.attachRow.mayor(session, town, person, transaction: tx),
+        );
+
+        final personCrdtRow = await CrdtDataRow.db.findFirstRow(
+          session,
+          where: (t) => t.uuidRowId.equals(person.id),
+        );
+
+        expect(personCrdtRow, isNotNull);
+
+        // Manually tombstone the person row, since a delete will apply the
+        // ON DELETE RESTRICT constraint on the town row.
+        await CrdtDataDeleted.db.insert(
+          session,
+          [
+            CrdtDataDeleted(
+              rowId: personCrdtRow!.id!,
+              nodeId: 1,
+              hlcDatetime: DateTime.now().toUtc(),
+              hlcCounter: 1,
+              isDeleted: true,
+            ),
+          ],
+        );
+      });
+
+      test(
+        'when calling findById with includes, then it does not return the town.',
+        () async {
+          final found = await Town.db.findById(
+            session,
+            town.id!,
+            include: Town.include(mayor: Person.include()),
+          );
+
+          expect(found, isNull);
+        },
+      );
+
+      test(
+        'when calling findFirstRow with includes, then it does not return the town.',
+        () async {
+          final found = await Town.db.findFirstRow(
+            session,
+            where: (t) => t.id.equals(town.id),
+            include: Town.include(mayor: Person.include()),
+          );
+
+          expect(found, isNull);
+        },
+      );
+
+      test(
+        'when calling find with includes, then it does not return the town.',
+        () async {
+          final found = await Town.db.find(
+            session,
+            where: (t) => t.id.equals(town.id),
+            include: Town.include(mayor: Person.include()),
+          );
+
+          expect(found, isEmpty);
+        },
+      );
+    },
+  );
 }

test/serverpod_offline_sync_test_server/test/integration/crud/update_test.dart

@@ -166,8 +166,98 @@ void main() {
         reason: 'This should fail due to the person being tombstoned.',
       );
     });
+
+    group('when updating the deleted person with updateWhere,', () {
+      late List<Person> updatedRows;
+
+      setUp(() async {
+        updatedRows = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Person.db.updateWhere(
+            session,
+            columnValues: (t) => [t.name('test2')],
+            where: (t) => t.id.equals(person.id),
+            transaction: tx,
+          ),
+        );
+      });
+
+      test('then an empty list is returned.', () async {
+        expect(updatedRows, isEmpty);
+      });
+
+      test('then the deleted row keeps its original value.', () async {
+        final row = await Person.db.findById(testSession, person.id!);
+
+        expect(row, isNotNull);
+        expect(row!.name, person.name);
+      });
+    });
   });
 
+  group(
+    'Given a person table with a row and an organization table with a deleted row,',
+    () {
+      late Person person;
+      late Organization deletedOrganization;
+
+      setUp(() async {
+        person = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Person.db.insertRow(session, Person(name: 'new'), transaction: tx),
+        );
+
+        final organization = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Organization.db.insertRow(
+            session,
+            Organization(name: 'deleted'),
+            transaction: tx,
+          ),
+        );
+
+        deletedOrganization = await session.db.transactionForUser(
+          testCrdtUserId,
+          (tx) => Organization.db.deleteRow(
+            session,
+            organization,
+            transaction: tx,
+          ),
+        );
+      });
+
+      group(
+        'when updating the person with updateWhere to set the organization foreign key to the deleted row id,',
+        () {
+          late Future<List<Person>> updateFuture;
+
+          setUp(() async {
+            updateFuture = session.db.transactionForUser(
+              testCrdtUserId,
+              (tx) => Person.db.updateWhere(
+                session,
+                columnValues: (t) => [t.organizationId(deletedOrganization.id)],
+                where: (t) => t.id.equals(person.id),
+                transaction: tx,
+              ),
+            );
+          });
+
+          test(
+            'then an exception is thrown and the row keeps its original foreign key value.',
+            () async {
+              await expectLater(updateFuture, throwsA(isA<Exception>()));
+
+              final row = await Person.db.findById(session, person.id!);
+              expect(row, isNotNull);
+              expect(row!.organizationId, person.organizationId);
+            },
+          );
+        },
+      );
+    },
+  );
+
   group(
     'Given a person table with an existing row that was inserted and updated from a different node,',
     () {