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();