diff --git a/packages/nitrite/CHANGELOG.md b/packages/nitrite/CHANGELOG.md index 91290fb..fbf4fff 100644 --- a/packages/nitrite/CHANGELOG.md +++ b/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()` 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/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/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/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); + }); + }); +} 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, + ); + }); + }); +}