From 5332d90c5786bcf39d8bfc8a5f6ab4e321efe159 Mon Sep 17 00:00:00 2001
From: "Dustin J. Mitchell" <dustin@v.igoro.us>
Date: Mon, 13 Jan 2025 08:32:27 -0500
Subject: [PATCH] Improve error handling in the inmemory storage
 implementation. (#79)

Improve error handling in the inmemory storage

This addresses a TODO, in a type that is really only used for testing.

This also adds a test for a similar circumstance -- adding the same
version twice -- in the SQLite storage, but it is already handled
correctly.
---
 core/src/inmemory.rs | 48 ++++++++++++++++++++++++++++++++++++++------
 sqlite/src/lib.rs    | 17 ++++++++++++++++
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/core/src/inmemory.rs b/core/src/inmemory.rs
index 9c7ac48..a6ace2f 100644
--- a/core/src/inmemory.rs
+++ b/core/src/inmemory.rs
@@ -130,7 +130,6 @@ impl StorageTxn for InnerTxn<'_> {
         parent_version_id: Uuid,
         history_segment: Vec<u8>,
     ) -> anyhow::Result<()> {
-        // TODO: verify it doesn't exist (`.entry`?)
         let version = Version {
             version_id,
             parent_version_id,
@@ -143,15 +142,33 @@ impl StorageTxn for InnerTxn<'_> {
                 snap.versions_since += 1;
             }
         } else {
-            return Err(anyhow::anyhow!("Client {} does not exist", self.client_id));
+            anyhow::bail!("Client {} does not exist", self.client_id);
         }
 
-        self.guard
+        if self
+            .guard
             .children
-            .insert((self.client_id, parent_version_id), version_id);
-        self.guard
+            .insert((self.client_id, parent_version_id), version_id)
+            .is_some()
+        {
+            anyhow::bail!(
+                "Client {} already has a child for {}",
+                self.client_id,
+                parent_version_id
+            );
+        }
+        if self
+            .guard
             .versions
-            .insert((self.client_id, version_id), version);
+            .insert((self.client_id, version_id), version)
+            .is_some()
+        {
+            anyhow::bail!(
+                "Client {} already has a version {}",
+                self.client_id,
+                version_id
+            );
+        }
 
         self.written = true;
         Ok(())
@@ -259,6 +276,25 @@ mod test {
         Ok(())
     }
 
+    #[test]
+    fn test_add_version_exists() -> anyhow::Result<()> {
+        let storage = InMemoryStorage::new();
+        let client_id = Uuid::new_v4();
+        let mut txn = storage.txn(client_id)?;
+
+        let version_id = Uuid::new_v4();
+        let parent_version_id = Uuid::new_v4();
+        let history_segment = b"abc".to_vec();
+
+        txn.new_client(parent_version_id)?;
+        txn.add_version(version_id, parent_version_id, history_segment.clone())?;
+        assert!(txn
+            .add_version(version_id, parent_version_id, history_segment.clone())
+            .is_err());
+        txn.commit()?;
+        Ok(())
+    }
+
     #[test]
     fn test_snapshots() -> anyhow::Result<()> {
         let storage = InMemoryStorage::new();
diff --git a/sqlite/src/lib.rs b/sqlite/src/lib.rs
index 7441b17..bd9f93f 100644
--- a/sqlite/src/lib.rs
+++ b/sqlite/src/lib.rs
@@ -385,6 +385,23 @@ mod test {
         Ok(())
     }
 
+    #[test]
+    fn test_add_version_exists() -> anyhow::Result<()> {
+        let tmp_dir = TempDir::new()?;
+        let storage = SqliteStorage::new(tmp_dir.path())?;
+        let client_id = Uuid::new_v4();
+        let mut txn = storage.txn(client_id)?;
+
+        let version_id = Uuid::new_v4();
+        let parent_version_id = Uuid::new_v4();
+        let history_segment = b"abc".to_vec();
+        txn.add_version(version_id, parent_version_id, history_segment.clone())?;
+        assert!(txn
+            .add_version(version_id, parent_version_id, history_segment.clone())
+            .is_err());
+        Ok(())
+    }
+
     #[test]
     fn test_snapshots() -> anyhow::Result<()> {
         let tmp_dir = TempDir::new()?;