Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correct index calculations for large patch sets #907

Merged
merged 1 commit into from
Apr 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
};
}
Loading