From 07500dab344daab7276022b75418e2a4be104528 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Wed, 15 Jan 2025 14:12:30 +0800 Subject: [PATCH] fix: map.keys() may return keys from deleted entries (#618) * fix: map.keys() may return keys from deleted entries * chore: changeset * chore: fix latest clippy warning --- .changeset/moody-icons-mix.md | 5 ++ crates/delta/benches/rope.rs | 2 +- crates/delta/tests/array_delta.rs | 1 + crates/delta/tests/test_text_delta.rs | 1 + crates/examples/tests/failed_tests.rs | 1 + crates/fuzz/tests/compatibility.rs | 1 + crates/fuzz/tests/counter.rs | 1 + crates/fuzz/tests/json.rs | 1 + crates/fuzz/tests/kv.rs | 31 ++++++------ crates/fuzz/tests/test.rs | 1 + crates/fuzz/tests/test_tree.rs | 1 + crates/fuzz/tests/undo.rs | 1 + crates/kv-store/tests/test.rs | 1 + .../examples/encoding_refactored.rs | 2 +- .../src/container/tree/bool_rle_vec.rs | 2 +- crates/loro-internal/src/handler.rs | 16 +++---- crates/loro-internal/src/state/tree_state.rs | 4 +- crates/loro-internal/src/version/frontiers.rs | 2 +- crates/loro/tests/issue.rs | 1 + crates/loro/tests/loro_rust_test.rs | 48 +++++++++++++++++++ crates/loro/tests/mov.rs | 1 + 21 files changed, 92 insertions(+), 32 deletions(-) create mode 100644 .changeset/moody-icons-mix.md diff --git a/.changeset/moody-icons-mix.md b/.changeset/moody-icons-mix.md new file mode 100644 index 00000000..28b201f5 --- /dev/null +++ b/.changeset/moody-icons-mix.md @@ -0,0 +1,5 @@ +--- +"loro-crdt": patch +--- + +fix: map.keys() may return keys from deleted entries #618 diff --git a/crates/delta/benches/rope.rs b/crates/delta/benches/rope.rs index b252710a..60406a1a 100644 --- a/crates/delta/benches/rope.rs +++ b/crates/delta/benches/rope.rs @@ -149,7 +149,7 @@ impl RandomCharIter { pub fn new(rng: T) -> Self { Self { rng, - simple_text: std::env::var("SIMPLE_TEXT").map_or(false, |v| !v.is_empty()), + simple_text: std::env::var("SIMPLE_TEXT").is_ok_and(|v| !v.is_empty()), } } diff --git a/crates/delta/tests/array_delta.rs b/crates/delta/tests/array_delta.rs index cdae4abf..bd664a1b 100644 --- a/crates/delta/tests/array_delta.rs +++ b/crates/delta/tests/array_delta.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use loro_delta::{array_vec::ArrayVec, DeltaRope, DeltaRopeBuilder}; use tracing_subscriber::fmt::format::FmtSpan; diff --git a/crates/delta/tests/test_text_delta.rs b/crates/delta/tests/test_text_delta.rs index 072d7705..c81fcbb8 100644 --- a/crates/delta/tests/test_text_delta.rs +++ b/crates/delta/tests/test_text_delta.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use std::collections::HashMap; use loro_delta::{ diff --git a/crates/examples/tests/failed_tests.rs b/crates/examples/tests/failed_tests.rs index 37284ef1..ec0bb6a1 100644 --- a/crates/examples/tests/failed_tests.rs +++ b/crates/examples/tests/failed_tests.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use examples::json::fuzz; use loro::loro_value; diff --git a/crates/fuzz/tests/compatibility.rs b/crates/fuzz/tests/compatibility.rs index 95ebf2b1..1ef6e485 100644 --- a/crates/fuzz/tests/compatibility.rs +++ b/crates/fuzz/tests/compatibility.rs @@ -1,4 +1,5 @@ #![allow(deprecated)] +#![allow(unexpected_cfgs)] use std::sync::Arc; use loro::{ToJson as _, ID}; diff --git a/crates/fuzz/tests/counter.rs b/crates/fuzz/tests/counter.rs index 95eca7f0..af9710ef 100644 --- a/crates/fuzz/tests/counter.rs +++ b/crates/fuzz/tests/counter.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use fuzz::{ actions::{ActionWrapper::*, GenericAction}, crdt_fuzzer::{test_multi_sites, Action::*, FuzzTarget, FuzzValue::*}, diff --git a/crates/fuzz/tests/json.rs b/crates/fuzz/tests/json.rs index 68dbdbf8..0b62cc8d 100644 --- a/crates/fuzz/tests/json.rs +++ b/crates/fuzz/tests/json.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] #![allow(deprecated)] use fuzz::{ actions::{ActionWrapper::*, GenericAction}, diff --git a/crates/fuzz/tests/kv.rs b/crates/fuzz/tests/kv.rs index fd5c12e7..ec8676b1 100644 --- a/crates/fuzz/tests/kv.rs +++ b/crates/fuzz/tests/kv.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use fuzz::{kv_minify_simple, test_mem_kv_fuzzer, KVAction::*}; #[ctor::ctor] @@ -146,21 +147,21 @@ fn merge_import() { #[test] fn scan_empty() { test_mem_kv_fuzzer(&mut [ - Add{ - key: vec![0, 255], - value: vec![] - }, - Add{ - key: vec![], - value: vec![] - }, - Scan{ - start: 129, - end: 0, - start_include: false, - end_include: false - }, - ]) + Add { + key: vec![0, 255], + value: vec![], + }, + Add { + key: vec![], + value: vec![], + }, + Scan { + start: 129, + end: 0, + start_include: false, + end_include: false, + }, + ]) } #[test] fn minify() { diff --git a/crates/fuzz/tests/test.rs b/crates/fuzz/tests/test.rs index 96026b64..87f3e2bf 100644 --- a/crates/fuzz/tests/test.rs +++ b/crates/fuzz/tests/test.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] #![allow(deprecated)] use arbitrary::Unstructured; diff --git a/crates/fuzz/tests/test_tree.rs b/crates/fuzz/tests/test_tree.rs index d0364381..f948af6d 100644 --- a/crates/fuzz/tests/test_tree.rs +++ b/crates/fuzz/tests/test_tree.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] #[allow(unused_imports)] use fuzz::{ actions::{ActionInner, ActionWrapper::*, GenericAction}, diff --git a/crates/fuzz/tests/undo.rs b/crates/fuzz/tests/undo.rs index 83896c74..95b2d338 100644 --- a/crates/fuzz/tests/undo.rs +++ b/crates/fuzz/tests/undo.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use fuzz::{ actions::{ActionWrapper::*, GenericAction}, crdt_fuzzer::{minify_simple, test_multi_sites, Action::*, FuzzTarget, FuzzValue::*}, diff --git a/crates/kv-store/tests/test.rs b/crates/kv-store/tests/test.rs index 306e031b..4684e1cb 100644 --- a/crates/kv-store/tests/test.rs +++ b/crates/kv-store/tests/test.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use bytes::Bytes; use loro_kv_store::{mem_store::MemKvConfig, MemKvStore}; diff --git a/crates/loro-internal/examples/encoding_refactored.rs b/crates/loro-internal/examples/encoding_refactored.rs index 613acdeb..da792100 100644 --- a/crates/loro-internal/examples/encoding_refactored.rs +++ b/crates/loro-internal/examples/encoding_refactored.rs @@ -32,7 +32,7 @@ fn log_size() { println!("\n"); println!("Snapshot size={}", snapshot.len()); println!("Updates size={}", updates.len()); - println!("Json Updates size={}", json_updates.as_bytes().len()); + println!("Json Updates size={}", json_updates.len()); println!("\n"); loro.diagnose_size(); } diff --git a/crates/loro-internal/src/container/tree/bool_rle_vec.rs b/crates/loro-internal/src/container/tree/bool_rle_vec.rs index 16762cc5..4f6a31cc 100644 --- a/crates/loro-internal/src/container/tree/bool_rle_vec.rs +++ b/crates/loro-internal/src/container/tree/bool_rle_vec.rs @@ -77,7 +77,7 @@ impl BoolRleVec { self.len -= n; // Remove any trailing zero-length runs - while self.rle_vec.last().map_or(false, |&x| x == 0) { + while self.rle_vec.last().is_some_and(|&x| x == 0) { self.rle_vec.pop(); } } diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index 92f59ccd..8b072657 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -1615,10 +1615,7 @@ impl TextHandler { match &self.inner { MaybeDetached::Detached(t) => { let mut t = t.try_lock().unwrap(); - let ranges = match t.value.get_text_entity_ranges(pos, len, PosType::Bytes) { - Err(x) => return Err(x), - Ok(x) => x, - }; + let ranges = t.value.get_text_entity_ranges(pos, len, PosType::Bytes)?; for range in ranges.iter().rev() { t.value .drain_by_entity_index(range.entity_start, range.entity_len(), None); @@ -1635,10 +1632,7 @@ impl TextHandler { match &self.inner { MaybeDetached::Detached(t) => { let mut t = t.try_lock().unwrap(); - let ranges = match t.value.get_text_entity_ranges(pos, len, PosType::Unicode) { - Err(x) => return Err(x), - Ok(x) => x, - }; + let ranges = t.value.get_text_entity_ranges(pos, len, PosType::Unicode)?; for range in ranges.iter().rev() { t.value .drain_by_entity_index(range.entity_start, range.entity_len(), None); @@ -3827,8 +3821,10 @@ impl MapHandler { } MaybeDetached::Attached(a) => { a.with_state(|state| { - for (k, _) in state.as_map_state().unwrap().iter() { - keys.push(k.clone()); + for (k, v) in state.as_map_state().unwrap().iter() { + if v.value.is_some() { + keys.push(k.clone()); + } } }); } diff --git a/crates/loro-internal/src/state/tree_state.rs b/crates/loro-internal/src/state/tree_state.rs index a3a4976b..9cfee543 100644 --- a/crates/loro-internal/src/state/tree_state.rs +++ b/crates/loro-internal/src/state/tree_state.rs @@ -903,9 +903,7 @@ impl TreeState { /// /// O(1) pub fn is_parent(&self, target: &TreeID, parent: &TreeParentId) -> bool { - self.trees - .get(target) - .map_or(false, |x| x.parent == *parent) + self.trees.get(target).is_some_and(|x| x.parent == *parent) } /// Delete the position cache of the node diff --git a/crates/loro-internal/src/version/frontiers.rs b/crates/loro-internal/src/version/frontiers.rs index 87da4362..46e89b40 100644 --- a/crates/loro-internal/src/version/frontiers.rs +++ b/crates/loro-internal/src/version/frontiers.rs @@ -66,7 +66,7 @@ impl InternalMap { fn contains(&self, id: &ID) -> bool { self.0 .get(&id.peer) - .map_or(false, |&counter| counter == id.counter) + .is_some_and(|&counter| counter == id.counter) } fn insert(&mut self, id: ID) { diff --git a/crates/loro/tests/issue.rs b/crates/loro/tests/issue.rs index f5ccdbec..115d0b44 100644 --- a/crates/loro/tests/issue.rs +++ b/crates/loro/tests/issue.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] use loro::LoroDoc; #[ctor::ctor] diff --git a/crates/loro/tests/loro_rust_test.rs b/crates/loro/tests/loro_rust_test.rs index b25379d9..ea950098 100644 --- a/crates/loro/tests/loro_rust_test.rs +++ b/crates/loro/tests/loro_rust_test.rs @@ -1,7 +1,9 @@ #![allow(deprecated)] +#![allow(unexpected_cfgs)] use pretty_assertions::assert_eq; use std::{ cmp::Ordering, + collections::HashSet, ops::ControlFlow, sync::{ atomic::{AtomicBool, AtomicU64}, @@ -2960,6 +2962,7 @@ fn test_diff_apply_with_unknown_container() -> LoroResult<()> { Ok(()) } +#[test] fn test_set_merge_interval() { let doc = LoroDoc::new(); doc.set_record_timestamp(true); @@ -2982,3 +2985,48 @@ fn test_set_merge_interval() { assert_eq!(new_doc.len_changes(), 2); } } + +#[test] +fn test_child_container_attach_behavior() { + let map = LoroMap::new(); + let child = map.insert_container("child", LoroMap::new()).unwrap(); + let doc = LoroDoc::new(); + doc.get_map("meta").insert_container("map", map).unwrap(); + assert_eq!( + doc.get_deep_value().to_json_value(), + json!({ + "meta": { "map": { "child": {} } } + }) + ); + let attached = child.get_attached().unwrap(); + attached.insert("key", "value").unwrap(); + assert_eq!( + doc.get_deep_value().to_json_value(), + json!({ + "meta": { "map": { "child": { "key": "value" } } } + }) + ); +} + +#[test] +fn test_map_keys_values_for_each() { + let doc = LoroDoc::new(); + let map = doc.get_map("map"); + map.insert("a", "b").unwrap(); + map.insert("c", "d").unwrap(); + map.insert("e", "f").unwrap(); + map.delete("c").unwrap(); + let mut keys = HashSet::new(); + let mut values = HashSet::new(); + map.for_each(|k, v| { + keys.insert(k.to_string()); + values.insert(v.into_value().unwrap().into_string().unwrap().to_string()); + }); + let keys2 = map.keys().map(|k| k.to_string()).collect::>(); + let values2 = map + .values() + .map(|v| v.into_value().unwrap().into_string().unwrap().to_string()) + .collect::>(); + assert_eq!(keys, keys2); + assert_eq!(values, values2); +} diff --git a/crates/loro/tests/mov.rs b/crates/loro/tests/mov.rs index e4bc26e3..2ec55279 100644 --- a/crates/loro/tests/mov.rs +++ b/crates/loro/tests/mov.rs @@ -1,3 +1,4 @@ +#![allow(unexpected_cfgs)] #![allow(deprecated)] use loro::{LoroDoc, LoroError, ToJson};