From 898d34102cca281d87dee62cd32925db6a4e5b15 Mon Sep 17 00:00:00 2001 From: Anindya Chatterjee Date: Fri, 4 Sep 2026 18:42:18 +0530 Subject: [PATCH 1/2] fix: port three index and catalog fixes from nitrite-java - 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: a unique index over an array field with a repeated element (['a', 'b', 'a']) collided with the entry it had just written, and so did 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. "Affected" only meant the update carried the field, and an upsert that writes the whole document back carries every indexed field with its old value, so every index was rebuilt on every update for nothing. A dirty index is still rebuilt. (nitrite/nitrite-java#1297) - MapMetaData copies the stored name set instead of adopting it. cast() returns a view onto the set held in the catalog document, so mapNames.add() edited the stored set in place, before the write meant to record it. (nitrite/nitrite-java#1296) Co-Authored-By: Claude Opus 5 --- packages/nitrite/CHANGELOG.md | 8 ++++ .../operations/document_index_writer.dart | 25 +++++++++++ .../nitrite/lib/src/index/nitrite_index.dart | 16 ++++--- packages/nitrite/lib/src/store/meta_data.dart | 10 +++-- .../unique_index_self_rewrite_test.dart | 43 +++++++++++++++++++ 5 files changed, 94 insertions(+), 8 deletions(-) create mode 100644 packages/nitrite/test/integration/collection/unique_index_self_rewrite_test.dart diff --git a/packages/nitrite/CHANGELOG.md b/packages/nitrite/CHANGELOG.md index 91290fb..805f924 100644 --- a/packages/nitrite/CHANGELOG.md +++ b/packages/nitrite/CHANGELOG.md @@ -1,3 +1,11 @@ +## 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) +- `MapMetaData` copies the stored name set instead of adopting it. `document[tagMapMetaData]?.cast()` 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. diff --git a/packages/nitrite/lib/src/collection/operations/document_index_writer.dart b/packages/nitrite/lib/src/collection/operations/document_index_writer.dart index 922565c..e3c719f 100644 --- a/packages/nitrite/lib/src/collection/operations/document_index_writer.dart +++ b/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 _writeIndexEntryInternal( IndexDescriptor indexDescriptor, Document document, diff --git a/packages/nitrite/lib/src/index/nitrite_index.dart b/packages/nitrite/lib/src/index/nitrite_index.dart index 85dc6a8..83da9f6 100644 --- a/packages/nitrite/lib/src/index/nitrite_index.dart +++ b/packages/nitrite/lib/src/index/nitrite_index.dart @@ -52,11 +52,17 @@ abstract class NitriteIndex { ) { nitriteIds = nitriteIds ?? []; - 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 diff --git a/packages/nitrite/lib/src/store/meta_data.dart b/packages/nitrite/lib/src/store/meta_data.dart index 287c9bc..ace54fd 100644 --- a/packages/nitrite/lib/src/store/meta_data.dart +++ b/packages/nitrite/lib/src/store/meta_data.dart @@ -14,9 +14,13 @@ class MapMetaData implements MetaData { Set get mapNames => _mapNames; MapMetaData(Document document) { - var mapNames = document[tagMapMetaData]?.cast(); - mapNames ??= HashSet(); - _mapNames = mapNames; + // Copy, never adopt. `cast()` 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(); + _mapNames = stored == null + ? HashSet() + : HashSet.from(stored); } @override diff --git a/packages/nitrite/test/integration/collection/unique_index_self_rewrite_test.dart b/packages/nitrite/test/integration/collection/unique_index_self_rewrite_test.dart new file mode 100644 index 0000000..3d92302 --- /dev/null +++ b/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, + ); + }); + }); +} From b24db4cb90d5f3e7ea575392d166a65969f312f2 Mon Sep 17 00:00:00 2001 From: Anindya Chatterjee Date: Fri, 4 Sep 2026 18:50:52 +0530 Subject: [PATCH 2/2] fix: getById hands out a copy, not the stored instance The in-memory store returns the very Document it holds, so a caller's doc.put(...) on a getById result edited the store directly and bypassed every index. find() already copied, through ProcessedDocumentStream. getById now clones as the cursor does, and returns null for an unknown id instead of putting null through the processor chain. (nitrite/nitrite-java#1294) Co-Authored-By: Claude Opus 5 --- packages/nitrite/CHANGELOG.md | 1 + .../operations/read_operations.dart | 10 +++-- .../collection/get_by_id_copy_test.dart | 37 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) create mode 100644 packages/nitrite/test/integration/collection/get_by_id_copy_test.dart diff --git a/packages/nitrite/CHANGELOG.md b/packages/nitrite/CHANGELOG.md index 805f924..fbf4fff 100644 --- a/packages/nitrite/CHANGELOG.md +++ b/packages/nitrite/CHANGELOG.md @@ -4,6 +4,7 @@ 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()` 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 diff --git a/packages/nitrite/lib/src/collection/operations/read_operations.dart b/packages/nitrite/lib/src/collection/operations/read_operations.dart index f3d549d..0ea9205 100644 --- a/packages/nitrite/lib/src/collection/operations/read_operations.dart +++ b/packages/nitrite/lib/src/collection/operations/read_operations.dart @@ -52,10 +52,12 @@ class ReadOperations { Future 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) { diff --git a/packages/nitrite/test/integration/collection/get_by_id_copy_test.dart b/packages/nitrite/test/integration/collection/get_by_id_copy_test.dart new file mode 100644 index 0000000..83a9dff --- /dev/null +++ b/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); + }); + }); +}