fix: return errors for pre-shallow frontiers in diff and checkout (#931)

* fix: handle pre-shallow frontier errors in diff and checkout

* chore: add changeset for shallow frontier error handling

Author: zxch3n

.changeset/popular-ads-vanish.md

@@ -0,0 +1,6 @@
+---
+"loro-crdt": patch
+"loro-crdt-map": patch
+---
+
+Return errors instead of panicking when diff or checkout targets frontiers before a shallow snapshot root.

crates/loro-internal/src/loro.rs

@@ -1006,18 +1006,25 @@ impl LoroDoc {
     // FIXME: This method needs testing (no event should be emitted during processing this)
     pub fn diff(&self, a: &Frontiers, b: &Frontiers) -> LoroResult<DiffBatch> {
         {
-            // check whether a and b are valid
+            // Check whether a and b are valid before checkout so this returns a normal error
+            // instead of panicking on shallow docs.
             let oplog = self.oplog.lock().unwrap();
-            for id in a.iter() {
-                if !oplog.dag.contains(id) {
-                    return Err(LoroError::FrontiersNotFound(id));
+            let validate_frontiers = |frontiers: &Frontiers| -> LoroResult<()> {
+                for id in frontiers.iter() {
+                    if !oplog.dag.contains(id) {
+                        return Err(LoroError::FrontiersNotFound(id));
+                    }
                 }
-            }
-            for id in b.iter() {
-                if !oplog.dag.contains(id) {
-                    return Err(LoroError::FrontiersNotFound(id));
+
+                if oplog.dag.is_before_shallow_root(frontiers) {
+                    return Err(LoroError::SwitchToVersionBeforeShallowRoot);
                 }
-            }
+
+                Ok(())
+            };
+
+            validate_frontiers(a)?;
+            validate_frontiers(b)?;
         }
 
         let (options, txn) = self.implicit_commit_then_stop();
@@ -1354,18 +1361,27 @@ impl LoroDoc {
     /// This will make the current [DocState] detached from the latest version of [OpLog].
     /// Any further import will not be reflected on the [DocState], until user call [LoroDoc::attach()]
     pub fn checkout(&self, frontiers: &Frontiers) -> LoroResult<()> {
+        let was_detached = self.is_detached();
         let (options, guard) = self.implicit_commit_then_stop();
-        self._checkout_without_emitting(frontiers, true, true)?;
-        self.emit_events();
+        let result = self._checkout_without_emitting(frontiers, true, true);
+        if result.is_ok() {
+            self.emit_events();
+        }
         drop(guard);
         if self.config.detached_editing() {
-            self.renew_peer_id();
+            if result.is_ok() {
+                self.renew_peer_id();
+            }
             self.renew_txn_if_auto_commit(options);
+        } else if result.is_err() {
+            if !was_detached {
+                self.renew_txn_if_auto_commit(options);
+            }
         } else if !self.is_detached() {
             self.renew_txn_if_auto_commit(options);
         }
 
-        Ok(())
+        result
     }
 
     /// NOTE: The caller of this method should ensure the txn is locked and set to None

crates/loro/tests/issue.rs

@@ -1,8 +1,8 @@
 #![allow(deprecated)]
 #![allow(unexpected_cfgs)]
 use loro::{
-    cursor::Cursor, ContainerID, ContainerTrait, EncodedBlobMode, ExportMode, LoroDoc, LoroList,
-    LoroText, UndoManager,
+    cursor::Cursor, ContainerID, ContainerTrait, EncodedBlobMode, ExportMode, LoroDoc, LoroError,
+    LoroList, LoroText, UndoManager,
 };
 use std::sync::{Arc, Mutex};
 use tracing::{trace, trace_span};
@@ -344,6 +344,68 @@ fn issue_822_tree_shallow_snapshot_roundtrip() {
     assert_eq!(doc_before, doc_after, "doc shallow value should roundtrip");
 }
 
+#[test]
+fn issue_928_diff_before_shallow_root_should_error_without_poisoning_doc() {
+    let doc = LoroDoc::new();
+    doc.set_peer_id(1).unwrap();
+
+    let text = doc.get_text("t");
+    text.insert(0, "hello").unwrap();
+    doc.commit();
+    let pre_shallow_frontiers = doc.oplog_frontiers();
+
+    text.insert(5, " world").unwrap();
+    doc.commit();
+    let current_frontiers = doc.oplog_frontiers();
+
+    let shallow_snapshot = doc
+        .export(ExportMode::shallow_snapshot(&current_frontiers))
+        .unwrap();
+    let shallow_doc = LoroDoc::new();
+    shallow_doc.import(&shallow_snapshot).unwrap();
+
+    let err = shallow_doc
+        .diff(&pre_shallow_frontiers, &current_frontiers)
+        .unwrap_err();
+    assert_eq!(err, LoroError::SwitchToVersionBeforeShallowRoot);
+    assert!(!shallow_doc.is_detached());
+    assert_eq!(shallow_doc.get_text("t").to_string(), "hello world");
+
+    shallow_doc.get_text("t").insert(11, "!").unwrap();
+    shallow_doc.commit();
+    assert_eq!(shallow_doc.get_text("t").to_string(), "hello world!");
+}
+
+#[test]
+fn issue_928_checkout_before_shallow_root_should_error_without_stopping_auto_commit() {
+    let doc = LoroDoc::new();
+    doc.set_peer_id(1).unwrap();
+
+    let text = doc.get_text("t");
+    text.insert(0, "hello").unwrap();
+    doc.commit();
+    let pre_shallow_frontiers = doc.oplog_frontiers();
+
+    text.insert(5, " world").unwrap();
+    doc.commit();
+    let current_frontiers = doc.oplog_frontiers();
+
+    let shallow_snapshot = doc
+        .export(ExportMode::shallow_snapshot(&current_frontiers))
+        .unwrap();
+    let shallow_doc = LoroDoc::new();
+    shallow_doc.import(&shallow_snapshot).unwrap();
+
+    let err = shallow_doc.checkout(&pre_shallow_frontiers).unwrap_err();
+    assert_eq!(err, LoroError::SwitchToVersionBeforeShallowRoot);
+    assert!(!shallow_doc.is_detached());
+    assert_eq!(shallow_doc.get_text("t").to_string(), "hello world");
+
+    shallow_doc.get_text("t").insert(11, "!").unwrap();
+    shallow_doc.commit();
+    assert_eq!(shallow_doc.get_text("t").to_string(), "hello world!");
+}
+
 #[test]
 fn fix_get_unknown_cursor_position() {
     let doc = LoroDoc::new();