fix: port three index and catalog fixes from nitrite-java

packages/nitrite/CHANGELOG.md

@@ -1,3 +1,12 @@
+## Unreleased
+
+Ported from the corresponding fixes in `nitrite-java`, where each was found.
+
+- A unique index no longer rejects a document over a key that document already holds. `addNitriteIds` treated *any* existing id under the key as a violation, so it counted the writer's own id against it. That bites a unique index over an array field with a repeated element - `['a', 'b', 'a']` visits `a` twice, and the second visit collided with the entry the first had just written - and any path that reaches a key the document already owns, such as an index rebuild or a replayed write. Another document under the key is still a violation. (nitrite/nitrite-java#1295)
+- An update that leaves an indexed value unchanged no longer rewrites the index. `DocumentIndexWriter.updateIndexEntry` treated an index as affected whenever the update document carried the indexed field, and then removed and rewrote the entry. An update that writes the whole document back - the common upsert shape - carries every indexed field with its old value, so every index was rebuilt on every update for nothing. The old and new values are now compared with `deepEquals`, and the index is left alone when they match. A dirty index is still rebuilt, since that has to happen on the first write regardless. (nitrite/nitrite-java#1297)
+- `getById` hands out a copy instead of the stored instance, and returns `null` for an unknown id without putting `null` through the processor chain. The in-memory store returns the very `Document` it holds, so a caller's `doc.put(...)` on the result edited the store directly and bypassed every index. `find` already copied through `ProcessedDocumentStream`. (nitrite/nitrite-java#1294)
+- `MapMetaData` copies the stored name set instead of adopting it. `document[tagMapMetaData]?.cast<String>()` returns a *view* onto the set held in the catalog document, so `mapNames.add(name)` edited the stored set in place - before the write that was supposed to record it, and past any rollback. It now takes a copy, so a write always stores a new set. (nitrite/nitrite-java#1296)
+
 ## 3.3.0
 
 - Paging a collection no longer pays for the documents it skips past. `skip` was applied by pulling and discarding, and on a persistent store each discarded row had already been read off disk and decoded, so a page-by-page lap over 20k rows of ~1KB cost 27.6x a single full scan of the same collection - 19.8 seconds against 0.7. The offset is now taken at the source: `NitriteMap.valuesSkipping` steps over keys without reading the values behind them, and an index plan skips its ids before they turn into document reads. The same lap now costs 1.0x - what one full scan costs.

packages/nitrite/lib/src/collection/operations/document_index_writer.dart

@@ -3,6 +3,7 @@ import 'dart:async';
 import 'package:nitrite/nitrite.dart';
 import 'package:nitrite/src/collection/operations/index_operations.dart';
 import 'package:nitrite/src/common/util/document_utils.dart';
+import 'package:nitrite/src/common/util/object_utils.dart';
 
 /// @nodoc
 class DocumentIndexWriter {
@@ -34,6 +35,16 @@ class DocumentIndexWriter {
 
       // if the index is affected by the update
       if (fields.fieldNames.any((field) => updatedFields.containsKey(field))) {
+        // "affected" only means the update carries the field. An update that
+        // writes the whole document back, the common upsert shape, carries every
+        // indexed field with its old value, and rewriting those entries is pure
+        // cost. A dirty index still has to be rebuilt, so that case is not
+        // skipped.
+        if (!await _indexOperations.shouldRebuildIndex(fields) &&
+            _sameIndexedValues(oldDoc, newDoc, fields)) {
+          continue;
+        }
+
         var indexType = indexDescriptor.indexType;
         var nitriteIndexer = await _nitriteConfig.findIndexer(indexType);
 
@@ -61,6 +72,20 @@ class DocumentIndexWriter {
     }
   }
 
+  /// Whether the two documents hold the same values for every field of the
+  /// index, compared deeply so that lists and embedded values count as equal
+  /// when their contents are.
+  bool _sameIndexedValues(Document oldDoc, Document newDoc, Fields fields) {
+    var before = getDocumentValues(oldDoc, fields).values;
+    var after = getDocumentValues(newDoc, fields).values;
+    if (before.length != after.length) return false;
+    for (var i = 0; i < before.length; i++) {
+      if (before[i].$1 != after[i].$1) return false;
+      if (!deepEquals(before[i].$2, after[i].$2)) return false;
+    }
+    return true;
+  }
+
   Future<void> _writeIndexEntryInternal(
     IndexDescriptor indexDescriptor,
     Document document,

packages/nitrite/lib/src/collection/operations/read_operations.dart

@@ -52,10 +52,12 @@ class ReadOperations {
 
   Future<Document?> getById(NitriteId nitriteId) async {
     var doc = await _nitriteMap[nitriteId];
-    if (doc != null) {
-      doc = await _processorChain.processAfterRead(doc);
-    }
-    return doc;
+    if (doc == null) return null;
+    // Hand out a copy, as the cursor does. The in-memory store returns the very
+    // instance it holds, so without this a caller's `doc.put(...)` edits the
+    // store directly and bypasses every index.
+    var copy = doc.clone();
+    return _processorChain.processAfterRead(copy);
   }
 
   void _prepareFilter(Filter filter) {

packages/nitrite/lib/src/index/nitrite_index.dart

@@ -52,11 +52,17 @@ abstract class NitriteIndex {
   ) {
     nitriteIds = nitriteIds ?? <NitriteId>[];
 
-    if (isUnique && nitriteIds.length == 1) {
-      // if key is already exists for unique type, throw error
-      throw UniqueConstraintException(
-        'Unique key constraint violation for ${fieldValues.fields}',
-      );
+    if (isUnique && nitriteIds.isNotEmpty) {
+      // Another document already holding this key is the violation. The same
+      // document again is not: a unique index over an array field visits a
+      // repeated element once per occurrence, and a rebuild or a replayed write
+      // reaches the key the document already owns.
+      if (nitriteIds.any((id) => id != fieldValues.nitriteId)) {
+        throw UniqueConstraintException(
+          'Unique key constraint violation for ${fieldValues.fields}',
+        );
+      }
+      return nitriteIds;
     }
 
     // index always are in ascending format

packages/nitrite/lib/src/store/meta_data.dart

@@ -14,9 +14,13 @@ class MapMetaData implements MetaData {
   Set<String> get mapNames => _mapNames;
 
   MapMetaData(Document document) {
-    var mapNames = document[tagMapMetaData]?.cast<String>();
-    mapNames ??= HashSet<String>();
-    _mapNames = mapNames;
+    // Copy, never adopt. `cast<String>()` hands back a view onto the set held in
+    // the catalog document, so adding a name here would edit the stored set in
+    // place - a write that has not been made yet, and one no rollback undoes.
+    var stored = document[tagMapMetaData]?.cast<String>();
+    _mapNames = stored == null
+        ? HashSet<String>()
+        : HashSet<String>.from(stored);
   }
 
   @override

packages/nitrite/test/integration/collection/get_by_id_copy_test.dart

@@ -0,0 +1,37 @@
+import 'package:nitrite/nitrite.dart';
+import 'package:test/expect.dart';
+import 'package:test/scaffolding.dart';
+
+import '../../test_utils.dart';
+import 'base_collection_test_loader.dart';
+
+void main() {
+  group(retry: 3, 'GetById Copy Test Suite', () {
+    setUp(() async {
+      setUpLog();
+      await setUpNitriteTest();
+    });
+
+    tearDown(() async {
+      await cleanUp();
+    });
+
+    // The in-memory store hands back the instance it holds, so a getById that
+    // did not copy let a caller edit the store directly, bypassing the indexes.
+    test('Test GetById Does Not Hand Out The Stored Instance', () async {
+      var doc = createDocument('name', 'original');
+      var result = await collection.insert(doc);
+      var id = await result.first;
+
+      var first = await collection.getById(id);
+      first!.put('name', 'mutated');
+
+      var second = await collection.getById(id);
+      expect(second!.get('name'), 'original');
+    });
+
+    test('Test GetById Returns Null For An Unknown Id', () async {
+      expect(await collection.getById(NitriteId.newId()), isNull);
+    });
+  });
+}

packages/nitrite/test/integration/collection/unique_index_self_rewrite_test.dart

@@ -0,0 +1,43 @@
+import 'package:nitrite/nitrite.dart';
+import 'package:test/expect.dart';
+import 'package:test/scaffolding.dart';
+
+import '../../test_utils.dart';
+import 'base_collection_test_loader.dart';
+
+void main() {
+  group(retry: 3, 'Unique Index Self Rewrite Test Suite', () {
+    setUp(() async {
+      setUpLog();
+      await setUpNitriteTest();
+    });
+
+    tearDown(() async {
+      await cleanUp();
+    });
+
+    // A unique index over an array field visits every element of the array,
+    // including a repeated one. The second visit reaches the key the very same
+    // document already owns, which is not another document holding it.
+    test('Test Repeated Element In One Document Is Not A Violation', () async {
+      await collection.createIndex(['tags'], indexOptions(IndexType.unique));
+
+      var doc = createDocument('tags', ['a', 'b', 'a']);
+      await collection.insert(doc);
+
+      expect(await collection.size, 1);
+      var found = await collection.find(filter: where('tags').eq('a')).toList();
+      expect(found.length, 1);
+    });
+
+    test('Test Two Documents Sharing A Key Is A Violation', () async {
+      await collection.createIndex(['tags'], indexOptions(IndexType.unique));
+
+      await collection.insert(createDocument('tags', ['a', 'b']));
+      expect(
+        () async => await collection.insert(createDocument('tags', ['c', 'a'])),
+        throwsUniqueConstraintException,
+      );
+    });
+  });
+}