Skip to content

Commit

Permalink
fix: correct index calculations for large patch sets
Browse files Browse the repository at this point in the history
Problem: when a change produced large numbers of patches (more than 100)
some patches could end up with incorrect indexes. The reason for this
was that the optimization which kicks in at > 100 patches which
precalculates the paths for every visible object in the document was
using the wrong encoding for calculating the indexes in lists.

Solution: use the correct encoding.
  • Loading branch information
alexjg committed Apr 22, 2024
1 parent ea212a4 commit ac01930
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 1 deletion.
6 changes: 5 additions & 1 deletion rust/automerge/src/automerge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1689,9 +1689,13 @@ impl Automerge {
let prop = match op.key() {
Key::Map(prop) => Prop::Map(self.ops.osd.props.get(*prop).clone()),
Key::Seq(_) => {
let encoding = match obj.typ {
ObjType::Text => ListEncoding::Text,
_ => ListEncoding::List,
};
let found = self
.ops
.seek_list_opid(&obj.id, *op.id(), ListEncoding::Text, None)
.seek_list_opid(&obj.id, *op.id(), encoding, at.as_ref())
.unwrap();
Prop::Seq(found.index)
}
Expand Down
43 changes: 43 additions & 0 deletions rust/automerge/tests/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2013,3 +2013,46 @@ fn save_with_empty_commits() {
// This will panic if we failed to encode the referenced actor ID
let _ = Automerge::load(&saved).unwrap();
}

#[test]
fn large_patches_in_lists_are_correct() {
// Reproduces a bug caused by an incorrect use of ListEncoding in Automerge::live_obj_paths.
// This is a function which precalculates the path of every visible object in the document.
// The problem was that when calculating the index into a sequence it was using
// ListEncoding::List to determine the index, which meant that when a string was inserted into
// a list then the index of elements following the list was based on the number of elements in
// the string, when it should just increase the index by one for the whole string.
//
// This bug was a little tricky to track down because it was only triggered by an optimization
// which kicks in when there are > 100 patches to render.

let mut doc = Automerge::new();
let heads_before = doc.get_heads();
let list = doc
.transact::<_, _, AutomergeError>(|tx| {
let list = tx.put_object(ROOT, "list", ObjType::List)?;
// This should just count as one
tx.insert(&list, 0, "123456")?;
for i in 1..501 {
let inner = tx.insert_object(&list, i, ObjType::Map)?;
tx.put(&inner, "a", i as i64)?;
}
Ok(list)
})
.unwrap()
.result;
let heads_after = doc.get_heads();
let patches = doc.diff(&heads_before, &heads_after, TextRepresentation::String);
let final_patch = patches.last().unwrap();
assert_eq!(
final_patch.path,
vec![
(ROOT, Prop::Map("list".into())),
(list, Prop::Seq(500)) // In the buggy code this was incorrectly coming out as 505 due to
// the counting of "123456" as 6 elements rather than 1
]
);
let PatchAction::PutMap { .. } = &final_patch.action else {
panic!("Expected PutMap, got {:?}", final_patch.action);
};
}

0 comments on commit ac01930

Please sign in to comment.