Fix SeekOpWithPatch on optrees with only internal optrees (automerge#496

)

In automerge#480 we fixed an issue where `SeekOp` calculated an incorrect
insertion index on optrees where the only visible ops were on internal
nodes. We forgot to port this fix to `SeekOpWithPatch`, which has almost
the same logic just with additional work done in order to notify an
`OpObserver` of changes. Add a test and fix to `SeekOpWithPatch`

rust/automerge/src/query/seek_op.rs

@@ -161,7 +161,7 @@ impl<'a> TreeQuery<'a> for SeekOp<'a> {
 }
 
 #[cfg(test)]
-mod tests {
+pub(crate) mod tests {
     use crate::{
         op_set::OpSet,
         op_tree::B,
@@ -170,36 +170,43 @@ mod tests {
         ActorId, ScalarValue,
     };
 
-    #[test]
-    fn seek_on_page_boundary() {
-        // Create an optree in which the only visible ops are on the boundaries of the nodes,
-        // i.e. the visible elements are in the internal nodes. Like so
-        //
-        //                      .----------------------.
-        //                      | id   |  key  |  succ |
-        //                      | B    |  "a"  |       |
-        //                      | 2B   |  "b"  |       |
-        //                      '----------------------'
-        //                           /      |      \
-        //  ;------------------------.      |       `------------------------------------.
-        //  | id     | op     | succ |      |       | id            | op     | succ      |
-        //  | 0      |set "a" |  1   |      |       | 2B + 1        |set "c" |  2B + 2   |
-        //  | 1      |set "a" |  2   |      |       | 2B + 2        |set "c" |  2B + 3   |
-        //  | 2      |set "a" |  3   |      |       ...
-        //  ...                             |       | 3B            |set "c" |           |
-        //  | B - 1  |set "a" |  B   |      |       '------------------------------------'
-        //  '--------'--------'------'      |
-        //                                  |
-        //                      .-----------------------------.
-        //                      | id         |  key  |  succ  |
-        //                      | B + 1      |  "b"  | B + 2  |
-        //                      | B + 2      |  "b"  | B + 3  |
-        //                      ....
-        //                      | B + (B - 1 |  "b"  |   2B   |
-        //                      '-----------------------------'
-        //
-        // The important point here is that the leaf nodes contain no visible ops for keys "a" and
-        // "b".
+    /// Create an optree in which the only visible ops are on the boundaries of the nodes,
+    /// i.e. the visible elements are in the internal nodes. Like so
+    ///
+    /// ```notrust
+    ///
+    ///                      .----------------------.
+    ///                      | id   |  key  |  succ |
+    ///                      | B    |  "a"  |       |
+    ///                      | 2B   |  "b"  |       |
+    ///                      '----------------------'
+    ///                           /      |      \
+    ///  ;------------------------.      |       `------------------------------------.
+    ///  | id     | op     | succ |      |       | id            | op     | succ      |
+    ///  | 0      |set "a" |  1   |      |       | 2B + 1        |set "c" |  2B + 2   |
+    ///  | 1      |set "a" |  2   |      |       | 2B + 2        |set "c" |  2B + 3   |
+    ///  | 2      |set "a" |  3   |      |       ...
+    ///  ...                             |       | 3B            |set "c" |           |
+    ///  | B - 1  |set "a" |  B   |      |       '------------------------------------'
+    ///  '--------'--------'------'      |
+    ///                                  |
+    ///                      .-----------------------------.
+    ///                      | id         |  key  |  succ  |
+    ///                      | B + 1      |  "b"  | B + 2  |
+    ///                      | B + 2      |  "b"  | B + 3  |
+    ///                      ....
+    ///                      | B + (B - 1 |  "b"  |   2B   |
+    ///                      '-----------------------------'
+    /// ```
+    ///
+    /// The important point here is that the leaf nodes contain no visible ops for keys "a" and
+    /// "b".
+    ///
+    /// # Returns
+    ///
+    /// The opset in question and an op which should be inserted at the next position after the
+    /// internally visible ops.
+    pub(crate) fn optree_with_only_internally_visible_ops() -> (OpSet, Op) {
         let mut set = OpSet::new();
         let actor = set.m.actors.cache(ActorId::random());
         let a = set.m.props.cache("a".to_string());
@@ -255,6 +262,12 @@ mod tests {
                 .sorted_opids(std::iter::once(OpId::new(B as u64 - 1, actor))),
             insert: false,
         };
+        (set, new_op)
+    }
+
+    #[test]
+    fn seek_on_page_boundary() {
+        let (set, new_op) = optree_with_only_internally_visible_ops();
 
         let q = SeekOp::new(&new_op);
         let q = set.search(&ObjId::root(), q);

rust/automerge/src/query/seek_op_with_patch.rs

@@ -136,8 +136,18 @@ impl<'a> TreeQuery<'a> for SeekOpWithPatch<'a> {
                     if self.pos + child.len() >= start {
                         // skip empty nodes
                         if child.index.visible_len(self.encoding) == 0 {
-                            self.pos += child.len();
-                            QueryResult::Next
+                            let child_contains_key =
+                                child.elements.iter().any(|e| ops[*e].key == self.op.key);
+                            if !child_contains_key {
+                                // If we are in a node which has no visible ops, but none of the
+                                // elements of the node match the key of the op, then we must have
+                                // finished processing and so we can just return.
+                                // See https://github.com/automerge/automerge-rs/pull/480
+                                QueryResult::Finish
+                            } else {
+                                self.pos += child.len();
+                                QueryResult::Next
+                            }
                         } else {
                             QueryResult::Descend
                         }
@@ -291,3 +301,23 @@ impl<'a> TreeQuery<'a> for SeekOpWithPatch<'a> {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{super::seek_op::tests::optree_with_only_internally_visible_ops, SeekOpWithPatch};
+    use crate::{
+        op_tree::B,
+        types::{ListEncoding, ObjId},
+    };
+
+    #[test]
+    fn test_insert_on_internal_only_nodes() {
+        let (set, new_op) = optree_with_only_internally_visible_ops();
+
+        let q = SeekOpWithPatch::new(&new_op, ListEncoding::List);
+        let q = set.search(&ObjId::root(), q);
+
+        // we've inserted `B - 1` elements for "a", so the index should be `B`
+        assert_eq!(q.pos, B);
+    }
+}