diff --git a/corpus/outline-edit/README.md b/corpus/outline-edit/README.md index 289eb81d93d2539ecd768a063bc11f0dfea1b3f6..8ea0c1938367d1eeebccf312f1cf91d5238d1283 100644 --- a/corpus/outline-edit/README.md +++ b/corpus/outline-edit/README.md @@ -1,5 +1,12 @@ # Native outline editing controls +`empty-children` retains a mixed-client deletion regression: `before.one` exposes +transaction 22 of the retained local image; `remote.one` was observed after native +OneNote removed an empty child-list property and coalesced equivalent immutable +styles. The retained intent addresses the same subtree in both files. The offline +test requires deletion and its dependent edit to publish after reopening the +cache, while an actual remote text change retains a content conflict. + OneNote 2010 authored fourteen cases plus an unchanged source page. `before`, `after`, and `cold` retain notebook bytes and independent native XML. The final phase uses a fresh application cache. Cases preserve Unicode, hyperlinks, tags, diff --git a/corpus/outline-edit/empty-children/before.one b/corpus/outline-edit/empty-children/before.one new file mode 100644 index 0000000000000000000000000000000000000000..c6b4fbe2e6be0cc3d274cdb2617ecb66be26d194 Binary files /dev/null and b/corpus/outline-edit/empty-children/before.one differ diff --git a/corpus/outline-edit/empty-children/intent.json b/corpus/outline-edit/empty-children/intent.json new file mode 100644 index 0000000000000000000000000000000000000000..6d9536fe9868d237c7eee67db433dfee0be86f22 --- /dev/null +++ b/corpus/outline-edit/empty-children/intent.json @@ -0,0 +1,27 @@ +[ + "{C9F8B62B-D016-44A6-BD6D-AD427C226A53},1", + { + "guid": [ + 247, + 43, + 64, + 79, + 30, + 47, + 235, + 73, + 132, + 188, + 117, + 194, + 60, + 28, + 252, + 6 + ], + "object": "{91881B6E-F0D0-424E-917E-9D3BEF7D35C7},1", + "placement": "Delete", + "author": "Offline document writer", + "created": 1473370703 + } +] diff --git a/corpus/outline-edit/empty-children/remote.one b/corpus/outline-edit/empty-children/remote.one new file mode 100644 index 0000000000000000000000000000000000000000..0ca404488be0d32820867e538c8fa3d66d34e38d Binary files /dev/null and b/corpus/outline-edit/empty-children/remote.one differ diff --git a/crates/onestore-offline/README.md b/crates/onestore-offline/README.md index a283e6034e093233f7b14a5c2680b71fee2dc49f..3b7de1d28cae619876fbb2d18d24fcddcdcb599b 100644 --- a/crates/onestore-offline/README.md +++ b/crates/onestore-offline/README.md @@ -107,16 +107,19 @@ already satisfied move still requires guarded confirmation before acknowledgemen Deletion compares the selected raw property graph, including referenced styles, tags, unknown fields and internal attachments. Changed content produces `ContentChanged`; property order, CompactID numbering and modification timestamps -do not affect that comparison. Missing targets retain `TargetUnavailable`. +do not affect that comparison. Immutable records compare by content, and empty +child lists compare equally to absent child lists. Mutable object identities +remain significant. Missing targets retain `TargetUnavailable`. `rebase_tree_conflict(id, local, remote)` reviews the original move/deletion against both current images. An emptied cell's replacement paragraph/text identities must remain the same before replay or review; later queued edits keep their targets. Uncertain tree attempts require their original revision for confirmation, even when an independent move or deletion has the same visible effect. -Recognized earlier caches migrate transactionally to version eight, which adds -tree intents and content conflicts. The migration retains -images, local IDs, publication attempts, conflicts, +Recognized earlier caches migrate transactionally to version nine. Version-eight +deletion observations retain their original identity-sensitive preconditions; +explicit conflict review upgrades them to the immutable-content comparison. +The migration retains images, local IDs, publication attempts, conflicts, receipts and the autoincrement sequence; it does not reuse acknowledged IDs when the pending queue is empty. diff --git a/crates/onestore-offline/examples/smb_offline_client.rs b/crates/onestore-offline/examples/smb_offline_client.rs index 3afd08aa5ccbfa29883df2225dcb87a6b9614c37..4440ebb379af1b2f9c7c252b0f098919ff6edb41 100644 --- a/crates/onestore-offline/examples/smb_offline_client.rs +++ b/crates/onestore-offline/examples/smb_offline_client.rs @@ -3,7 +3,7 @@ mod concurrent; use onestore::{ CommitError, CommitState, ExGuid, Insertion, ParagraphJoin, ParagraphSplit, PreparedEdit, - RevisionIndex, Store, TextAttribute, document::Document, + RevisionIndex, Store, TextAttribute, TreeEdit, document::Document, }; use onestore_offline::{EditStatus, Error, Remote, Replica, SmbRemote}; use onestore_smb::{Client, Credentials}; @@ -41,7 +41,18 @@ impl DocumentView { } } -const DOCUMENT_OPERATIONS: [&str; 6] = ["insert", "format", "text", "split", "right_text", "join"]; +const DOCUMENT_OPERATIONS: [&str; 10] = [ + "insert", + "format", + "text", + "split", + "right_text", + "nest", + "unnest", + "join", + "tail_split", + "delete", +]; fn now() -> u128 { SystemTime::now() @@ -331,6 +342,8 @@ fn queue_document( }; let end = u32::try_from(text.encode_utf16().count())?; let split = ParagraphSplit::new(insertion.text_object(), end - 2, "Offline document writer")?; + let tail_split = + ParagraphSplit::new(insertion.text_object(), end + 1, "Offline document writer")?; let join = ParagraphJoin::new( insertion.text_object(), split.text_object(), @@ -344,14 +357,50 @@ fn queue_document( let mut ids = [0; DOCUMENT_OPERATIONS.len()]; for (step, id) in ids.iter_mut().enumerate() { let kind = DOCUMENT_OPERATIONS[step]; + let tree = match kind { + "nest" => { + let source = cache.snapshot()?; + let store = Store::parse(&source)?; + let index = RevisionIndex::parse(&store)?; + let document = Document::parse(&index)?; + let section = &document.spaces[&space]; + let view = §ion.revisions[§ion.contexts[&ExGuid::default()]]; + let paragraph = view + .nodes + .iter() + .find(|(_, node)| node.content == [insertion.text_object()]) + .map(|(id, _)| *id) + .ok_or("Missing inserted paragraph")?; + Some(TreeEdit::move_to( + split.object(), + paragraph, + None, + "Offline document writer", + )?) + } + "unnest" => Some(TreeEdit::move_to( + split.object(), + parent, + None, + "Offline document writer", + )?), + "delete" => Some(TreeEdit::delete( + tail_split.object(), + "Offline document writer", + )?), + _ => None, + }; let range = match kind { "text" => end - 3..end, "split" => end - 2..end - 2, + "tail_split" => end + 1..end + 1, "right_text" => 0..2, _ => 1..end - 2, }; let target = if kind == "right_text" { split.text_object() + } else if kind == "delete" { + tail_split.text_object() } else { insertion.text_object() }; @@ -373,7 +422,9 @@ fn queue_document( cache.edit_text(&source, space, target, range.clone(), replacement.unwrap()) } "split" => cache.split(&source, space, &split), + "tail_split" => cache.split(&source, space, &tail_split), "join" => cache.join(&source, space, &join), + "nest" | "unnest" | "delete" => cache.tree(&source, space, tree.as_ref().unwrap()), _ => unreachable!(), }; match result { @@ -384,7 +435,8 @@ fn queue_document( json!({"event":"local_document_commit","id":acknowledged,"operation":operation,"kind":kind, "space":space.to_string(),"object":target.to_string(),"document":insertion.text_object().to_string(), "text":text,"insertion":if kind=="insert" {Some(&insertion)} else {None}, - "split":if kind=="split" {Some(&split)} else {None}, + "split":match kind { "split" => Some(&split), "tail_split" => Some(&tail_split), _ => None }, + "tree":tree, "joined":if kind=="join" {Some(join.texts().map(|id| id.to_string()))} else {None}, "range":[range.start,range.end],"attributes":attributes,"replacement":replacement, "started_us":started,"finished_us":now()}) @@ -555,7 +607,12 @@ fn main() -> Result<(), Box> { let remote = cache.remote_snapshot()?; let current = view(&remote)?; let onestore_offline::Operation::Text(edit) = &intent.operation else { - return Err("Expected text probe intents".into()); + return Err(format!( + "Document intent {} requires review: {:?}", + intent.id, + cache.status(intent.id)? + ) + .into()); }; let at = append_position(&edit.before, ¤t.text, &edit.replacement) .ok_or("Append model disagrees with retained history")?; @@ -713,9 +770,15 @@ mod tests { cache = Replica::open(&path).unwrap(); } let local = cache.snapshot().unwrap(); - assert_eq!(cache.pending().unwrap().len(), 12); + assert_eq!( + cache.pending().unwrap().len(), + 2 * DOCUMENT_OPERATIONS.len() + ); for (step, id) in ids.iter().enumerate() { - remote.remote.lost_reply = matches!(step % 6, 3 | 5); + remote.remote.lost_reply = matches!( + DOCUMENT_OPERATIONS[step % DOCUMENT_OPERATIONS.len()], + "split" | "nest" | "unnest" | "join" | "tail_split" | "delete" + ); let result = cache.sync_once(&mut remote); let state = if result.is_err() { assert!(matches!( diff --git a/crates/onestore-offline/src/lib.rs b/crates/onestore-offline/src/lib.rs index 28746754a78fe2c29ed534a70270b6ddf6b31434..e95c56c6e1c9c577930580b3fcc09c0715d1c6be 100644 --- a/crates/onestore-offline/src/lib.rs +++ b/crates/onestore-offline/src/lib.rs @@ -51,7 +51,7 @@ pub enum Error { type Result = std::result::Result; const APPLICATION_ID: u32 = 0x4f4e454f; -const SCHEMA_VERSION: u32 = 8; +const SCHEMA_VERSION: u32 = 9; /// Text and its observed precondition, retained across cache reopen and rebasing. #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] diff --git a/crates/onestore-offline/src/tree.rs b/crates/onestore-offline/src/tree.rs index 614f6f3b7597263331c76c8b45392a6a8d247e83..ab8077974486d99c57f1da6f46ec1fc7557779a0 100644 --- a/crates/onestore-offline/src/tree.rs +++ b/crates/onestore-offline/src/tree.rs @@ -4,13 +4,20 @@ use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; use std::collections::{BTreeMap, BTreeSet}; +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +#[serde(untagged, deny_unknown_fields)] +enum Content { + Legacy([u8; 32]), + Semantic { sha256: [u8; 32] }, +} + #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] struct Observed { path: Vec<(ExGuid, u8)>, siblings: BTreeMap, destination: Vec<(ExGuid, u8)>, - content: Option<[u8; 32]>, + content: Option, } /// A subtree intent with observed placement, deletion content and replacement identities. @@ -22,23 +29,41 @@ pub struct TreeEdit { created: BTreeSet, } -fn fingerprint(store: &Store<'_>, raw: &ResolvedRevision<'_>, root: ExGuid) -> Result<[u8; 32]> { +fn fingerprint( + store: &Store<'_>, + raw: &ResolvedRevision<'_>, + root: ExGuid, + legacy: bool, +) -> Result<[u8; 32]> { let mut objects = BTreeMap::new(); - let mut pending = vec![root]; - while let Some(id) = pending.pop() { + let mut visiting = BTreeSet::new(); + let mut pending = vec![(root, false)]; + while let Some((id, complete)) = pending.pop() { if objects.contains_key(&id) { continue; } let object = &raw.objects[&id]; let references = object.references()?; - pending.extend(&references.objects); + if !complete { + if !visiting.insert(id) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Subtree property references form a cycle", + ) + .into()); + } + pending.push((id, true)); + pending.extend(references.objects.iter().map(|id| (*id, false))); + continue; + } + visiting.remove(&id); let mut hash = Sha256::new(); hash.update(object.jcid.to_le_bytes()); match object.data { ObjectData::Properties(bytes) => { hash.update([0]); let properties = PropertySets::parse(bytes)?; - let mut objects = references.objects.into_iter(); + let mut object_ids = references.objects.into_iter(); let mut spaces = references.object_spaces.into_iter(); let mut contexts = references.contexts.into_iter(); let mut fields = Vec::new(); @@ -54,20 +79,36 @@ fn fingerprint(store: &Store<'_>, raw: &ResolvedRevision<'_>, root: ExGuid) -> R compact_ids, } => { let references = match stream { - IdStream::Objects => &mut objects, + IdStream::Objects => &mut object_ids, IdStream::ObjectSpaces => &mut spaces, IdStream::Contexts => &mut contexts, }; for reference in references.take(compact_ids.len() / 4) { - value.update(reference.guid); - value.update(reference.n.to_le_bytes()); + if !legacy && *stream == IdStream::Objects { + let immutable = + raw.objects[&reference].jcid & 0x100000 != 0; + value.update([u8::from(immutable)]); + if !immutable { + value.update(reference.guid); + value.update(reference.n.to_le_bytes()); + } + value.update(objects[&reference]); + } else { + value.update(reference.guid); + value.update(reference.n.to_le_bytes()); + } } } Value::Sets(_) => {} } - if property.id != 0x14001d7a { - values.insert(property.id, value); + if property.id == 0x14001d7a + || (!legacy + && property.id == 0x24001c20 + && matches!(&property.value, Value::References { compact_ids, .. } if compact_ids.is_empty())) + { + continue; } + values.insert(property.id, value); } fields.push(values); } @@ -106,6 +147,9 @@ fn fingerprint(store: &Store<'_>, raw: &ResolvedRevision<'_>, root: ExGuid) -> R } objects.insert(id, <[u8; 32]>::from(hash.finalize())); } + if !legacy { + return Ok(objects[&root]); + } let mut hash = Sha256::new(); for (id, value) in objects { hash.update(id.guid); @@ -115,7 +159,12 @@ fn fingerprint(store: &Store<'_>, raw: &ResolvedRevision<'_>, root: ExGuid) -> R Ok(hash.finalize().into()) } -fn observe(source: &[u8], space: ExGuid, intent: &onestore::TreeEdit) -> Result> { +fn observe( + source: &[u8], + space: ExGuid, + intent: &onestore::TreeEdit, + legacy: bool, +) -> Result> { let store = Store::parse(source)?; let index = RevisionIndex::parse(&store)?; let document = Document::parse(&index)?; @@ -168,7 +217,12 @@ fn observe(source: &[u8], space: ExGuid, intent: &onestore::TreeEdit) -> Result< .collect(), destination, content: if intent.destination().is_none() { - Some(fingerprint(&store, &index.resolve(space, *rid)?, object)?) + let sha256 = fingerprint(&store, &index.resolve(space, *rid)?, object, legacy)?; + Some(if legacy { + Content::Legacy(sha256) + } else { + Content::Semantic { sha256 } + }) } else { None }, @@ -206,7 +260,7 @@ impl TreeEdit { intent: &onestore::TreeEdit, ) -> Result<(Self, PreparedEdit<'a>)> { let prepared = PreparedEdit::tree(source, space, intent)?; - let observed = observe(source, space, intent)?.ok_or_else(|| { + let observed = observe(source, space, intent, false)?.ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, "Prepared subtree edit has no active target", @@ -229,7 +283,13 @@ impl TreeEdit { source: &'a [u8], space: ExGuid, ) -> Result, ConflictKind>> { - let Some(current) = observe(source, space, &self.intent)? else { + let Some(current) = observe( + source, + space, + &self.intent, + matches!(self.observed.content, Some(Content::Legacy(_))), + )? + else { return Ok(Err(ConflictKind::TargetUnavailable)); }; let prepared = match PreparedEdit::tree(source, space, &self.intent) { @@ -304,7 +364,7 @@ mod tests { guid: [2; 16], n: 7, }; - let hash = |reverse: bool, global: u32, timestamp: u32, change: usize| { + let hash = |reverse: bool, global: u32, timestamp: u32, change: usize, immutable: bool| { let mut fields = vec![ (0x14001d7a, timestamp.to_le_bytes().to_vec()), (0x20000001, vec![]), @@ -317,6 +377,12 @@ mod tests { set(&[(if change == 2 { 0x08000005 } else { 0x88000005 }, vec![])]), ), ]; + if change != 5 { + fields.push((0x24001c20, 0_u32.to_le_bytes().to_vec())); + } + if change == 6 { + fields.push((0x24000007, 0_u32.to_le_bytes().to_vec())); + } if reverse { fields.reverse(); } @@ -351,7 +417,7 @@ mod tests { ( target, Object { - jcid: 0x6000e, + jcid: if immutable { 0x12004d } else { 0x6000e }, reference_count: 1, data: ObjectData::Properties(&child_bytes), global_ids: ids, @@ -359,12 +425,124 @@ mod tests { ), ]), }; - fingerprint(&store, &raw, root).unwrap() + fingerprint(&store, &raw, root, false).unwrap() }; - let original = hash(false, 0, 1, 0); - assert_eq!(original, hash(true, 137, 2, 0)); + let original = hash(false, 0, 1, 0, false); + assert_eq!(original, hash(true, 137, 2, 0, false)); + assert_eq!(original, hash(true, 137, 2, 5, false)); + assert_ne!(original, hash(true, 137, 2, 6, false)); for change in 1..=4 { - assert_ne!(original, hash(true, 137, 2, change)); + assert_ne!(original, hash(true, 137, 2, change, false)); } + let immutable = hash(false, 0, 1, 0, true); + assert_eq!(immutable, hash(true, 137, 2, 4, true)); + for change in 1..=3 { + assert_ne!(immutable, hash(true, 137, 2, change, true)); + } + } + + #[test] + fn version_eight_deletion_observations_survive_migration_and_upgrade_only_after_review() { + struct Server(Vec); + impl Remote for Server { + fn read(&mut self) -> io::Result> { + Ok(self.0.clone()) + } + fn publish( + &mut self, + edit: &PreparedEdit<'_>, + ) -> std::result::Result<(), onestore::CommitError> { + self.0 = edit.as_bytes().to_vec(); + Ok(()) + } + fn confirm( + &mut self, + snapshot: &[u8], + ) -> std::result::Result<(), onestore::CommitError> { + assert!(self.0 == snapshot); + Ok(()) + } + } + let source = onestore::create_section("legacy.one", "AlphaOmega", "Author").unwrap(); + let store = Store::parse(&source).unwrap(); + let index = RevisionIndex::parse(&store).unwrap(); + let document = Document::parse(&index).unwrap(); + let (sid, _) = document.pages().unwrap()[0]; + let space = &document.spaces[&sid]; + let view = &space.revisions[&space.contexts[&ExGuid::default()]]; + let text = *view.nodes.iter().find(|(_, node)| matches!(&node.kind, Kind::RichText { text, .. } if text == "AlphaOmega")).unwrap().0; + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("legacy.sqlite"); + let cache = Replica::create(&path, &source).unwrap(); + cache + .format( + &source, + sid, + text, + 0..10, + &[onestore::TextAttribute::Bold(true)], + ) + .unwrap(); + let split = onestore::ParagraphSplit::new(text, 5, "Author").unwrap(); + cache + .split(&cache.snapshot().unwrap(), sid, &split) + .unwrap(); + let before = cache.snapshot().unwrap(); + let intent = onestore::TreeEdit::delete(split.object(), "Author").unwrap(); + let deletion = cache.tree(&before, sid, &intent).unwrap().unwrap(); + let dependent = cache + .edit_text(&cache.snapshot().unwrap(), sid, text, 0..0, "Local ") + .unwrap() + .unwrap(); + let mut queue = cache.pending().unwrap(); + let Operation::Tree(edit) = &mut queue[2].operation else { + panic!() + }; + edit.observed = observe(&before, sid, &intent, true).unwrap().unwrap(); + let serialized = serde_json::to_string(&queue[2].operation).unwrap(); + let local = cache.snapshot().unwrap(); + drop(cache); + let db = Connection::open(&path).unwrap(); + db.execute( + "UPDATE edits SET operation=?1 WHERE id=?2", + params![serialized, i64::try_from(deletion).unwrap()], + ) + .unwrap(); + db.pragma_update(None, "user_version", 8).unwrap(); + drop(db); + let cache = Replica::open(&path).unwrap(); + assert_eq!(cache.pending().unwrap(), queue); + assert!(cache.snapshot().unwrap() == local); + let mut server = Server(source); + for _ in 0..2 { + assert!(matches!( + cache.sync_once(&mut server).unwrap(), + Some((_, EditStatus::Published { .. })) + )); + } + assert_eq!( + cache.sync_once(&mut server).unwrap(), + Some((deletion, EditStatus::Conflict(ConflictKind::ContentChanged))) + ); + cache + .rebase_tree_conflict(deletion, &local, &server.0) + .unwrap(); + let queue = cache.pending().unwrap(); + let Operation::Tree(edit) = &queue[0].operation else { + panic!() + }; + assert!(matches!( + edit.observed.content, + Some(Content::Semantic { .. }) + )); + for id in [deletion, dependent] { + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((actual, EditStatus::Published { .. })) if actual == id) + ); + } + assert_eq!( + paragraph(&server.0, sid, text).unwrap().as_deref(), + Some("Local Alpha") + ); } } diff --git a/crates/onestore-offline/tests/cache.rs b/crates/onestore-offline/tests/cache.rs index ab72febb3523fb4460b9eeb790d3709f98a96d46..dc9efa6f7116b46952eecb7cc41e4343110346f9 100644 --- a/crates/onestore-offline/tests/cache.rs +++ b/crates/onestore-offline/tests/cache.rs @@ -797,7 +797,7 @@ fn unrecognized_persisted_operations_are_rejected_without_dropping_fields() { #[test] fn prior_schema_migrations_retain_queue_evidence_assets_and_enable_content_conflicts() { - for (version, ceiling) in [(5, 3), (6, 4), (7, 5)] { + for (version, ceiling) in [(5, 3), (6, 4), (7, 5), (8, 6)] { use onestore_offline::{ConflictKind, EditStatus, Recovery}; use sha2::{Digest, Sha256}; let directory = tempfile::tempdir().unwrap(); diff --git a/crates/onestore-offline/tests/support/tree_schedule.rs b/crates/onestore-offline/tests/support/tree_schedule.rs index 015711a85d75145ba36d6ec292eadbe16996fd38..8032dba58d40dfd020373396b0c058f667615bbb 100644 --- a/crates/onestore-offline/tests/support/tree_schedule.rs +++ b/crates/onestore-offline/tests/support/tree_schedule.rs @@ -105,6 +105,20 @@ impl Remote for Session<'_> { .unwrap(); text.replace_range(0..1, &edit.replacement); } + Operation::Format(format) => { + let [onestore::TextAttribute::FontSize(size)] = format.attributes.as_slice() else { + panic!() + }; + let store = Store::parse(edit.as_bytes()).unwrap(); + let index = RevisionIndex::parse(&store).unwrap(); + let document = Document::parse(&index).unwrap(); + let space = &document.spaces[&SOURCE.1]; + let view = &space.revisions[&space.contexts[&ExGuid::default()]]; + assert_eq!( + view.text_runs(format.object).unwrap()[0].format.font_size, + Some(*size) + ); + } _ => panic!(), } assert_eq!(rows(edit.as_bytes()), expected); @@ -163,7 +177,7 @@ pub fn run(input: &[u8]) { &TreeEdit::delete(local[target].0, "Offline").unwrap(), ) .unwrap(), - _ => cache + _ if step[3] & 1 == 0 => cache .edit_text( &snapshot, *sid, @@ -172,6 +186,17 @@ pub fn run(input: &[u8]) { &char::from(b'A' + step[3] % 26).to_string(), ) .unwrap(), + _ => cache + .format( + &snapshot, + *sid, + local[target].1, + 0..1, + &[onestore::TextAttribute::FontSize( + 12.0 + f32::from(step[3] % 20), + )], + ) + .unwrap(), }; let next = cache.pending().unwrap(); assert_eq!(next[..pending.len()], pending); diff --git a/crates/onestore-offline/tests/sync/tree.rs b/crates/onestore-offline/tests/sync/tree.rs index 51047122949425ea702d954bdc09ce80da481c70..b55b304ca3921551c45b1d17173faaed27c1fd60 100644 --- a/crates/onestore-offline/tests/sync/tree.rs +++ b/crates/onestore-offline/tests/sync/tree.rs @@ -100,6 +100,111 @@ fn move_preserves_remote_content_and_dependent_edits_through_reopen() { assert_eq!(server.publications, 2); } +#[test] +fn queued_format_split_and_delete_reconcile_equivalent_immutable_style_identities() { + let (source, sid, _, _, texts) = fixture(); + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("cache.sqlite"); + let cache = Replica::create(&path, &source).unwrap(); + cache + .format( + &source, + sid, + texts[0], + 0..8, + &[onestore::TextAttribute::Bold(true)], + ) + .unwrap(); + let split = onestore::ParagraphSplit::new(texts[0], 4, "Author").unwrap(); + cache + .split(&cache.snapshot().unwrap(), sid, &split) + .unwrap(); + cache + .tree( + &cache.snapshot().unwrap(), + sid, + &TreeEdit::delete(split.object(), "Author").unwrap(), + ) + .unwrap(); + cache + .edit_text(&cache.snapshot().unwrap(), sid, texts[0], 0..0, "Local ") + .unwrap(); + let queue = cache.pending().unwrap(); + drop(cache); + let mut server = Server::new(&source); + for intent in queue { + let cache = Replica::open(&path).unwrap(); + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((id, EditStatus::Published { .. })) if id == intent.id) + ); + } + assert_eq!( + super::outline::node(&server.durable, sid, texts[0])["kind"]["text"], + "Local Orig" + ); +} + +#[test] +fn native_empty_child_list_normalization_preserves_deletion_and_its_dependents() { + let source = include_bytes!("../../../../corpus/outline-edit/empty-children/before.one"); + let remote = include_bytes!("../../../../corpus/outline-edit/empty-children/remote.one"); + let (sid, intent): (ExGuid, TreeEdit) = serde_json::from_str(include_str!( + "../../../../corpus/outline-edit/empty-children/intent.json" + )) + .unwrap(); + let store = Store::parse(source).unwrap(); + let index = RevisionIndex::parse(&store).unwrap(); + let document = Document::parse(&index).unwrap(); + let space = &document.spaces[&sid]; + let view = &space.revisions[&space.contexts[&ExGuid::default()]]; + let tail = view.nodes[&intent.object()].content[0]; + let (outline_id, outline) = view + .nodes + .iter() + .find(|(_, node)| node.children.contains(&intent.object())) + .unwrap(); + let left = view.nodes[&outline.children[0]].content[0]; + for changed in [false, true] { + let directory = tempfile::tempdir().unwrap(); + let path = directory.path().join("cache.sqlite"); + let cache = Replica::create(&path, source).unwrap(); + let deletion = cache.tree(source, sid, &intent).unwrap().unwrap(); + let dependent = cache + .edit_text(&cache.snapshot().unwrap(), sid, left, 0..0, "Local ") + .unwrap() + .unwrap(); + let local = cache.snapshot().unwrap(); + drop(cache); + let cache = Replica::open(&path).unwrap(); + let mut server = if changed { + let edited = PreparedEdit::text(remote, sid, tail, 0..2, "Changed").unwrap(); + Server::new(edited.as_bytes()) + } else { + Server::new(remote) + }; + if changed { + assert_eq!( + cache.sync_once(&mut server).unwrap(), + Some((deletion, EditStatus::Conflict(ConflictKind::ContentChanged))) + ); + assert_eq!(cache.snapshot().unwrap(), local); + assert_eq!(cache.pending().unwrap().len(), 2); + assert_eq!(server.publications, 0); + } else { + for expected in [deletion, dependent] { + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((id, EditStatus::Published { .. })) if id == expected) + ); + } + assert!(cache.pending().unwrap().is_empty()); + assert_eq!(server.publications, 2); + assert!(!children(&server.durable, sid, *outline_id).contains(&intent.object())); + let node = super::outline::node(&server.durable, sid, left); + assert!(node["kind"]["text"].as_str().unwrap().starts_with("Local ")); + } + } +} + #[test] fn deletion_requires_review_of_remote_content_and_preserves_later_work() { let (source, sid, outline, paragraphs, texts) = fixture(); diff --git a/tools/offline_document_history.py b/tools/offline_document_history.py index b6f0d007c4f66e2d6ad7bee21e2d9cfe430fad45..b46972ee8c83e6e77b52c0b7b1fac5ea031af9f6 100644 --- a/tools/offline_document_history.py +++ b/tools/offline_document_history.py @@ -18,7 +18,8 @@ def characters(observed): def operation_kinds(events): kinds = tuple(events[0].get('document_kinds', ('insert', 'format'))) assert kinds in (('insert', 'format'), ('insert', 'format', 'text'), - ('insert', 'format', 'text', 'split', 'right_text', 'join')), 'Unknown document workload' + ('insert', 'format', 'text', 'split', 'right_text', 'join'), + ('insert', 'format', 'text', 'split', 'right_text', 'nest', 'unnest', 'join', 'tail_split', 'delete')), 'Unknown document workload' return kinds @@ -41,8 +42,9 @@ def document_history(logs, operations): linked = {} for intent, receipt in zip(edits, receipts, strict=True): changed_targets = {intent['object']} - if intent['kind'] == 'split': changed_targets.add(identity(intent['split'], 2)) + if intent['kind'] in ('split', 'tail_split'): changed_targets.add(identity(intent['split'], 2)) if intent['kind'] == 'join': changed_targets = set(intent['joined']) + if intent['kind'] in ('nest', 'unnest'): changed_targets = set() attempts = [row for row in events if row['event'] == 'remote_attempt' and row['revision'] == receipt['revision'] and set(row.get('document_changes') or {}) == changed_targets] if not attempts and intent['kind'] == 'format': @@ -51,10 +53,12 @@ def document_history(logs, operations): assert len(attempts) == 1, 'Document receipt lacks one publication attempt' attempt, = attempts assert attempt['state'] in ('Committed', 'Unknown'), 'Receipt identifies an unpublished document operation' - assert intent['object'] in attempt['documents'] and attempt['document_changes'] == {target: attempt['documents'].get(target) for target in changed_targets}, 'Document publication changed another target' + assert (intent['object'] in attempt['documents']) == (intent['kind'] != 'delete'), 'Document publication retained or lost its target' + assert attempt['document_changes'] == {target: attempt['documents'].get(target) for target in changed_targets}, 'Document publication changed another target' successful = [row for row in events if row['event'] == 'remote_attempt' and row['state'] in ('Committed', 'Unknown') - and row.get('document_changes') == attempt['document_changes']] + and row.get('document_changes') == attempt['document_changes'] + and row.get('document_graph_changes') == attempt.get('document_graph_changes')] assert successful == [attempt], 'Document intent was published or attempted uncertainly more than once' assert intent['started_us'] <= attempt['started_us'] <= attempt['finished_us'] <= receipt['at_us'] and intent['started_us'] <= intent['finished_us'] <= receipt['at_us'], 'Document acknowledgement order is invalid' if attempt['state'] == 'Unknown': @@ -77,7 +81,7 @@ def document_history(logs, operations): linked[intent['id']] = {**attempt, 'acknowledged_us': receipt['at_us'], 'receipt_revision': receipt['revision']} assert {(row['revision'], row['started_us']) for row in events if row['event'] == 'remote_attempt' and row['state'] in ('Committed', 'Unknown') - and row.get('document_changes')} == { + and (row.get('document_changes') or row.get('document_graph_changes'))} == { (attempt['revision'], attempt['started_us']) for attempt in linked.values() }, 'A document publication lacks its recorded intent and receipt' for at in range(0, len(edits), len(kinds)): @@ -125,7 +129,8 @@ def document_history(logs, operations): for state in states.values(): state['parts'] = [(paragraph, target, 0, len(state['characters']))] if replaced and boundaries: - split, right_edit, join = boundaries + split, right_edit, *following = boundaries + join = next(row for row in following if row['kind'] == 'join') assert events[0].get('document_graph') is True, 'Boundary workload requires structural observations' assert all(row.get('document') == target and row['space'] == inserted['space'] for row in edits[at:at + len(kinds)]), 'Boundary operation lost its owning document' @@ -137,17 +142,37 @@ def document_history(logs, operations): assert right_edit['object'] == right and right_edit['range'] == [0, 2] and right_edit['replacement'] == 'B🦋', 'Dependent right edit differs from workload' assert join['object'] == target and join['joined'] == [target, right], 'Join does not retain its original targets' updated = final[:boundary] + [(char, *final[boundary][1:]) for char in 'B🦋'] + final[boundary+2:] - for row, value, parts in [ - (split, final, [(paragraph, target, 0, boundary), (right_paragraph, right, boundary, len(final))]), - (right_edit, updated, [(paragraph, target, 0, boundary), (right_paragraph, right, boundary, len(updated))]), - (join, updated, [(paragraph, target, 0, len(updated))]), - ]: + split_parts = [(paragraph, target, 0, boundary), (right_paragraph, right, boundary, len(final))] + edited_parts = [(paragraph, target, 0, boundary), (right_paragraph, right, boundary, len(updated))] + stages = [(split, final, split_parts, {}), (right_edit, updated, edited_parts, {})] + if len(following) > 1: + nest, unnest, _, tail_split, deleted = following + outline = identity(insertion, 1) if 'Outline' in insertion['placement'] else insertion['parent'] + for row, parent in [(nest, paragraph), (unnest, outline)]: + tree = row['tree'] + assert row['object'] == target and tree['object'] == right_paragraph and tree['author'] == insertion['author'], 'Tree move addresses another subtree or author' + assert tree['placement'] == {'Move': {'parent': parent, 'before': None}}, 'Tree destination differs from workload' + stages += [(nest, updated, edited_parts, {right_paragraph: paragraph}), + (unnest, updated, edited_parts, {})] + stages.append((join, updated, [(paragraph, target, 0, len(updated))], {})) + if len(following) > 1: + tail = tail_split['split'] + tail_text, tail_paragraph = identity(tail, 2), identity(tail, 1) + assert tail_split['object'] == tail['text'] == target and tail['author'] == insertion['author'], 'Tail split addresses another text or author' + assert tail['offset'] == end+1 and tail_split['range'] == [end+1, end+1], 'Tail split boundary differs from workload' + tree = deleted['tree'] + assert deleted['object'] == tail_text and tree['object'] == tail_paragraph and tree['author'] == insertion['author'], 'Deletion addresses another subtree or author' + assert tree['placement'] == 'Delete', 'Deletion became a move' + stages += [(tail_split, updated, [(paragraph, target, 0, len(updated)-1), (tail_paragraph, tail_text, len(updated)-1, len(updated))], {}), + (deleted, updated[:-1], [(paragraph, target, 0, len(updated)-1)], {})] + known_texts = {oid for _, _, pieces, _ in stages for _, oid, _, _ in pieces} + for row, value, parts, parents in stages: attempt = linked[row['id']] assert list(states.values())[-1]['attempt']['finished_us'] <= attempt['started_us'], 'Boundary publication preceded its dependency' - assert set(attempt['documents']) & {target, right} == {oid for _, oid, _, _ in parts}, 'Boundary publication omitted or resurrected a text object' + assert set(attempt['documents']) & known_texts == {oid for _, oid, _, _ in parts}, 'Boundary publication omitted or resurrected a text object' for _, oid, start, stop in parts: assert characters(attempt['documents'][oid]) == value[start:stop], 'Boundary publication differs from its local intent' - states[row['kind']] = {'characters': value, 'parts': parts, 'attempt': attempt} + states[row['kind']] = {'characters': value, 'parts': parts, 'parents': parents, 'attempt': attempt} allocated = {oid for state in states.values() for paragraph, text, _, _ in state['parts'] for oid in (paragraph, text)} existing = {oid for document in documents.values() for state in document['states'].values() for paragraph, text, _, _ in state['parts'] for oid in (paragraph, text)} @@ -177,6 +202,8 @@ def document_history(logs, operations): states = list(document['states'].values()) known_texts = {oid for state in states for _, oid, _, _ in state['parts']} known_paragraphs = {oid for state in states for oid, _, _, _ in state['parts']} + insertion = document['insertion'] + outline = identity(insertion, 1) if 'Outline' in insertion['placement'] else insertion['parent'] if is_read and row['started_us'] > states[0]['attempt']['acknowledged_us']: assert target in observed, 'Reader missed an acknowledged insertion' if target not in observed: @@ -185,7 +212,9 @@ def document_history(logs, operations): actual = {oid: characters(observed[oid]) for oid in known_texts & observed.keys()} matches = [i for i, state in enumerate(states) if actual == {oid: state['characters'][start:stop] for _, oid, start, stop in state['parts']} - and (not structural or known_paragraphs & graph.keys() == {oid for oid, _, _, _ in state['parts']})] + and (not structural or (known_paragraphs & graph.keys() == {oid for oid, _, _, _ in state['parts']} + and all(graph[p]['parent'] == state.get('parents', {}).get(p, outline) + for p, _, _, _ in state['parts'])))] assert matches, 'Reader observed partial or invented document content' if is_read: matches = [i for i in matches if i >= previous.get(target, 0)] @@ -197,16 +226,14 @@ def document_history(logs, operations): assert matches, 'Reader missed acknowledged document content' previous[target] = min(matches) if structural: - insertion = document['insertion'] - outline = insertion['parent'] if 'Outline' in insertion['placement']: - outline = identity(insertion, 1) expected_graph[outline] = {'parent': insertion['parent'], 'children': [], 'content': [], 'child_level': 1, 'position': insertion['placement']['Outline']} assert outline in expected_graph, 'Snapshot omitted the inserted outline' for paragraph, oid, _, _ in states[min(matches)]['parts']: - expected_graph[outline]['children'].append(paragraph) - expected_graph[paragraph] = {'parent': outline, 'children': [], 'content': [oid], + parent = states[min(matches)].get('parents', {}).get(paragraph, outline) + expected_graph[parent]['children'].append(paragraph) + expected_graph[paragraph] = {'parent': parent, 'children': [], 'content': [oid], 'child_level': 1, 'position': None} if structural: assert graph == expected_graph, 'Snapshot contains partial, reordered or invented paragraph structure' diff --git a/tools/test_offline_document_history.py b/tools/test_offline_document_history.py index 76aace55ff7c9990d1d64d4baf94bbbf7cd32fd0..59e33f449ab2da2dc9c33542a36acb61fa74e56c 100644 --- a/tools/test_offline_document_history.py +++ b/tools/test_offline_document_history.py @@ -149,9 +149,48 @@ class DocumentHistoryTests(unittest.TestCase): self.assertEqual(result.returncode, 0, result.stdout + result.stderr) events = [json.loads(line) for line in result.stdout.splitlines() if line.startswith('{')] documents = document_history({'w0': events}, 2) - self.assertEqual(sum(len(document['states']) for document in documents.values()), 12) - self.assertEqual(sum(row['event'] == 'remote_attempt' and row['state'] == 'Unknown' for row in events), 4) - self.assertEqual(sum(row['event'] == 'remote_confirm' and row['state'] == 'Committed' for row in events), 4) + self.assertEqual(sum(len(document['states']) for document in documents.values()), 20) + self.assertEqual(sum(row['event'] == 'remote_attempt' and row['state'] == 'Unknown' for row in events), 12) + self.assertEqual(sum(row['event'] == 'remote_confirm' and row['state'] == 'Committed' for row in events), 12) + document = next(iter(documents.values())) + states = document['states'] + for phase, next_phase, stale in [('nest', 'unnest', 'right_text'), ('delete', None, 'tail_split')]: + read = copy.deepcopy(states[stale]['attempt']) + started = states[phase]['attempt']['acknowledged_us'] + 1 + if next_phase: + self.assertLess(started + 1, states[next_phase]['attempt']['started_us']) + read.update(event='read', started_us=started, finished_us=started + 1) + logs = {'w0': events, 'r0': [{'event': 'ready', 'document_graph': True}, read, {'event': 'done'}]} + with self.subTest(phase=phase), self.assertRaisesRegex(AssertionError, 'missed acknowledged'): + document_history(logs, 2) + for mutation in ('nest-intent', 'unnest-intent', 'delete-intent', 'tail-boundary', + 'nest-children', 'delete-resurrection', 'graph-delta', 'extra-attempt'): + changed = copy.deepcopy(events) + intents = {row['kind']: row for row in changed if row['event'] == 'local_document_commit' and row['operation'] == 0} + attempts = {kind: next(row for row in changed if row['event'] == 'remote_attempt' and row['revision'] == state['attempt']['revision']) + for kind, state in states.items()} + if mutation == 'nest-intent': + intents['nest']['tree']['placement']['Move']['parent'] = intents['unnest']['tree']['placement']['Move']['parent'] + elif mutation == 'unnest-intent': + intents['unnest']['tree']['placement']['Move']['before'] = intents['unnest']['tree']['object'] + elif mutation == 'delete-intent': + intents['delete']['tree']['object'] = intents['nest']['tree']['object'] + elif mutation == 'tail-boundary': + intents['tail_split']['split']['offset'] += 1 + elif mutation == 'nest-children': + parent = intents['nest']['tree']['placement']['Move']['parent'] + attempts['nest']['document_graph'][parent]['children'] = [] + elif mutation == 'delete-resurrection': + tail = intents['delete']['object'] + attempts['delete']['documents'][tail] = attempts['tail_split']['documents'][tail] + elif mutation == 'graph-delta': + attempts['nest']['document_graph_changes'] = {} + else: + extra = copy.deepcopy(attempts['nest']) + extra.update(revision='unrecorded-tree-revision', state='Committed') + changed.append(extra) + with self.subTest(mutation=mutation), self.assertRaises(AssertionError): + document_history({'w0': changed}, 2) def test_boundary_histories_retain_every_intermediate_graph_and_retired_identity(self): baseline = history(boundaries=True)