From 14675fc7613742242b68ec6eaf616e073f116964 Mon Sep 17 00:00:00 2001 From: Anindya Chatterjee Date: Fri, 4 Sep 2026 18:43:12 +0530 Subject: [PATCH] fix: port two index fixes from nitrite-java - A unique index no longer rejects a document over a key that document already holds. add_nitrite_ids 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) Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 24 +++++++++ .../src/collection/operation/index_writer.rs | 25 +++++++++ nitrite/src/index/simple_index.rs | 52 ++++++++++++++++--- 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cab4c6..acc5eda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,30 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +Ported from the corresponding fixes in `nitrite-java`, where each was found. + +### Fixed + +- **A unique index no longer rejects a document over a key that document already holds.** + `add_nitrite_ids` 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) + +### Changed + +- **An update that leaves an indexed value unchanged no longer rewrites the index.** + `update_index_entry` 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, 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) + ## [1.0.0] - 2026-09-01 **Why 1.0.0 and not 0.11.0.** The storage engine underneath the adapter changed major version, and diff --git a/nitrite/src/collection/operation/index_writer.rs b/nitrite/src/collection/operation/index_writer.rs index c1f8747..d871de9 100644 --- a/nitrite/src/collection/operation/index_writer.rs +++ b/nitrite/src/collection/operation/index_writer.rs @@ -2,6 +2,7 @@ use super::index_operations::IndexOperations; use crate::{ collection::Document, errors::NitriteResult, + Fields, get_document_values, index::{IndexDescriptor, NitriteIndexer, NitriteIndexerProvider}, is_affected_by_update, @@ -157,6 +158,16 @@ impl DocumentIndexWriterInner { let fields = index_descriptor.index_fields(); if is_affected_by_update(&fields, updated_fields) { + // "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 !self.index_operation.should_rebuild_index(&fields)? + && self.same_indexed_values(old_document, new_document, &fields)? + { + continue; + } + let index_type = index_descriptor.index_type(); let mut indexer = self.nitrite_config.find_indexer(&index_type)?; @@ -167,6 +178,20 @@ impl DocumentIndexWriterInner { Ok(()) } + /// Whether the two documents hold the same values for every field of the index. + /// `Value` compares structurally, so arrays and embedded documents count as equal + /// when their contents are. + fn same_indexed_values( + &self, + old_document: &mut Document, + new_document: &mut Document, + fields: &Fields, + ) -> NitriteResult { + let before = get_document_values(old_document, fields)?; + let after = get_document_values(new_document, fields)?; + Ok(before.values() == after.values()) + } + fn write_index_entry_internal( &self, index_descriptor: &IndexDescriptor, diff --git a/nitrite/src/index/simple_index.rs b/nitrite/src/index/simple_index.rs index b28f166..2a1aeee 100644 --- a/nitrite/src/index/simple_index.rs +++ b/nitrite/src/index/simple_index.rs @@ -164,10 +164,16 @@ impl SimpleIndexInner { nitrite_ids: &mut Vec, field_values: &FieldValues, ) -> NitriteResult> { - if self.is_unique() && nitrite_ids.len() == 1 { - // if key is already exists for unique type, throw error - log::debug!("Unique constraint violated for {:?}", field_values); - return Err(UNIQUE_CONSTRAINT_ERROR.clone()); + if self.is_unique() && !nitrite_ids.is_empty() { + // Another document already holds this key: a 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 it + // already owns. + let own_id = Value::NitriteId(*field_values.nitrite_id()); + if nitrite_ids.iter().any(|id| id != &own_id) { + log::debug!("Unique constraint violated for {:?}", field_values); + return Err(UNIQUE_CONSTRAINT_ERROR.clone()); + } } // index always are in ascending format @@ -491,14 +497,15 @@ mod tests { let simple_index = SimpleIndex::new(index_descriptor, nitrite_store); let index_map = simple_index.find_index_map().unwrap(); - let field_values = create_test_field_values(); + // two different documents under the same key: that is the violation + let first = create_test_field_values(); + let second = create_test_field_values(); let value = Value::String("test_value".to_string()); - // Add the same element twice to trigger unique constraint violation simple_index - .add_index_element(&index_map, &field_values, &value) + .add_index_element(&index_map, &first, &value) .unwrap(); - let result = simple_index.add_index_element(&index_map, &field_values, &value); + let result = simple_index.add_index_element(&index_map, &second, &value); assert!(result.is_err()); assert_eq!( @@ -507,6 +514,35 @@ mod tests { ); } + #[test] + fn test_simple_index_add_index_element_same_document_twice_is_not_a_violation() { + // A unique index over an array field visits a repeated element once per + // occurrence, and a rebuild reaches keys the document already owns. Writing the + // key a document already holds is a no-op, not a constraint violation. + let index_descriptor = create_test_index_descriptor(); + let nitrite_store = NitriteStore::default(); + let simple_index = SimpleIndex::new(index_descriptor, nitrite_store); + + let index_map = simple_index.find_index_map().unwrap(); + let field_values = create_test_field_values(); + let value = Value::String("test_value".to_string()); + + simple_index + .add_index_element(&index_map, &field_values, &value) + .unwrap(); + simple_index + .add_index_element(&index_map, &field_values, &value) + .expect("rewriting the same document under the same key must not violate"); + + // and the key still resolves to exactly that one document + let stored = index_map.get(&value).unwrap().unwrap(); + assert_eq!( + stored.as_array().map(|a| a.len()), + Some(1), + "the key must hold one id, not a duplicate" + ); + } + #[test] fn test_simple_index_remove_index_element_not_found() { let index_descriptor = create_test_index_descriptor();