update get_child_version to distinguish gone and not-found

sync-server/src/api/get_child_version.rs

@@ -2,7 +2,7 @@ use crate::api::{
     client_key_header, failure_to_ise, ServerState, HISTORY_SEGMENT_CONTENT_TYPE,
     PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER,
 };
-use crate::server::{get_child_version, VersionId};
+use crate::server::{get_child_version, GetVersionResult, VersionId};
 use actix_web::{error, get, web, HttpRequest, HttpResponse, Result};
 
 /// Get a child version.
@@ -23,27 +23,31 @@ pub(crate) async fn service(
 
     let client_key = client_key_header(&req)?;
 
-    txn.get_client(client_key)
+    let client = txn
+        .get_client(client_key)
         .map_err(failure_to_ise)?
         .ok_or_else(|| error::ErrorNotFound("no such client"))?;
 
-    let result = get_child_version(txn, client_key, parent_version_id).map_err(failure_to_ise)?;
+    return match get_child_version(txn, client_key, client, parent_version_id)
-    if let Some(result) = result {
+        .map_err(failure_to_ise)?
-        Ok(HttpResponse::Ok()
+    {
+        GetVersionResult::Success {
+            version_id,
+            parent_version_id,
+            history_segment,
+        } => Ok(HttpResponse::Ok()
             .content_type(HISTORY_SEGMENT_CONTENT_TYPE)
-            .header(VERSION_ID_HEADER, result.version_id.to_string())
+            .header(VERSION_ID_HEADER, version_id.to_string())
-            .header(
+            .header(PARENT_VERSION_ID_HEADER, parent_version_id.to_string())
-                PARENT_VERSION_ID_HEADER,
+            .body(history_segment)),
-                result.parent_version_id.to_string(),
+        GetVersionResult::NotFound => Err(error::ErrorNotFound("no such version")),
-            )
+        GetVersionResult::Gone => Err(error::ErrorGone("version has been deleted")),
-            .body(result.history_segment))
+    };
-    } else {
-        Err(error::ErrorNotFound("no such version"))
-    }
 }
 
 #[cfg(test)]
 mod test {
+    use crate::server::NO_VERSION_ID;
     use crate::storage::{InMemoryStorage, Storage};
     use crate::Server;
     use actix_web::{http::StatusCode, test, App};
@@ -113,7 +117,7 @@ mod test {
     }
 
     #[actix_rt::test]
-    async fn test_version_not_found() {
+    async fn test_version_not_found_and_gone() {
         let client_key = Uuid::new_v4();
         let parent_version_id = Uuid::new_v4();
         let storage: Box<dyn Storage> = Box::new(InMemoryStorage::new());
@@ -126,12 +130,26 @@ mod test {
         let server = Server::new(storage);
         let mut app = test::init_service(App::new().service(server.service())).await;
 
+        // the child of an unknown parent_version_id is GONE
         let uri = format!("/v1/client/get-child-version/{}", parent_version_id);
         let req = test::TestRequest::get()
             .uri(&uri)
             .header("X-Client-Key", client_key.to_string())
             .to_request();
         let resp = test::call_service(&mut app, req).await;
+        assert_eq!(resp.status(), StatusCode::GONE);
+        assert_eq!(resp.headers().get("X-Version-Id"), None);
+        assert_eq!(resp.headers().get("X-Parent-Version-Id"), None);
+
+        // but the child of the nil parent_version_id is NOT FOUND, since
+        // there is no snapshot.  The tests in crate::server test more
+        // corner cases.
+        let uri = format!("/v1/client/get-child-version/{}", NO_VERSION_ID);
+        let req = test::TestRequest::get()
+            .uri(&uri)
+            .header("X-Client-Key", client_key.to_string())
+            .to_request();
+        let resp = test::call_service(&mut app, req).await;
         assert_eq!(resp.status(), StatusCode::NOT_FOUND);
         assert_eq!(resp.headers().get("X-Version-Id"), None);
         assert_eq!(resp.headers().get("X-Parent-Version-Id"), None);

sync-server/src/server.rs

@@ -1,5 +1,6 @@
 //! This module implements the core logic of the server: handling transactions, upholding
-//! invariants, and so on.
+//! invariants, and so on.  This does not implement the HTTP-specific portions; those
+//! are in [`crate::api`].  See the protocol documentation for details.
 use crate::storage::{Client, StorageTxn};
 use uuid::Uuid;
 
@@ -10,26 +11,53 @@ pub(crate) type HistorySegment = Vec<u8>;
 pub(crate) type ClientKey = Uuid;
 pub(crate) type VersionId = Uuid;
 
-/// Response to get_child_version
+/// Response to get_child_version.  See the protocol documentation.
 #[derive(Clone, PartialEq, Debug)]
-pub(crate) struct GetVersionResult {
+pub(crate) enum GetVersionResult {
-    pub(crate) version_id: Uuid,
+    NotFound,
-    pub(crate) parent_version_id: Uuid,
+    Gone,
-    pub(crate) history_segment: HistorySegment,
+    Success {
+        version_id: Uuid,
+        parent_version_id: Uuid,
+        history_segment: HistorySegment,
+    },
 }
 
+/// Implementation of the GetChildVersion protocol transaction
 pub(crate) fn get_child_version<'a>(
     mut txn: Box<dyn StorageTxn + 'a>,
     client_key: ClientKey,
+    client: Client,
     parent_version_id: VersionId,
-) -> anyhow::Result<Option<GetVersionResult>> {
+) -> anyhow::Result<GetVersionResult> {
-    Ok(txn
+    // If a version with parentVersionId equal to the requested parentVersionId exists, it is returned.
-        .get_version_by_parent(client_key, parent_version_id)?
+    if let Some(version) = txn.get_version_by_parent(client_key, parent_version_id)? {
-        .map(|version| GetVersionResult {
+        return Ok(GetVersionResult::Success {
             version_id: version.version_id,
             parent_version_id: version.parent_version_id,
             history_segment: version.history_segment,
-        }))
+        });
+    }
+
+    // If the requested parentVersionId is the nil UUID ..
+    if parent_version_id == NO_VERSION_ID {
+        return Ok(match client.snapshot {
+            // ..and snapshotVersionId is nil, the response is _not-found_ (the client has no
+            // versions).
+            None => GetVersionResult::NotFound,
+            // ..and snapshotVersionId is not nil, the response is _gone_ (the first version has
+            // been deleted).
+            Some(_) => GetVersionResult::Gone,
+        });
+    }
+
+    // If a version with versionId equal to the requested parentVersionId exists, the response is _not-found_ (the client is up-to-date)
+    if txn.get_version(client_key, parent_version_id)?.is_some() {
+        return Ok(GetVersionResult::NotFound);
+    }
+
+    // Otherwise, the response is _gone_ (the requested version has been deleted).
+    Ok(GetVersionResult::Gone)
 }
 
 /// Response to add_version
@@ -41,6 +69,7 @@ pub(crate) enum AddVersionResult {
     ExpectedParentVersion(VersionId),
 }
 
+/// Implementation of the AddVersion protocol transaction
 pub(crate) fn add_version<'a>(
     mut txn: Box<dyn StorageTxn + 'a>,
     client_key: ClientKey,
@@ -79,16 +108,87 @@ pub(crate) fn add_version<'a>(
 #[cfg(test)]
 mod test {
     use super::*;
-    use crate::storage::{InMemoryStorage, Storage};
+    use crate::storage::{InMemoryStorage, Snapshot, Storage};
+    use chrono::{TimeZone, Utc};
     use pretty_assertions::assert_eq;
 
     #[test]
-    fn gcv_not_found() -> anyhow::Result<()> {
+    fn gcv_not_found_initial() -> anyhow::Result<()> {
         let storage = InMemoryStorage::new();
-        let txn = storage.txn()?;
+        let mut txn = storage.txn()?;
         let client_key = Uuid::new_v4();
+        txn.new_client(client_key, NO_VERSION_ID)?;
+
+        // when no snapshot exists, the first version is NotFound
+        let client = txn.get_client(client_key)?.unwrap();
+        assert_eq!(
+            get_child_version(txn, client_key, client, NO_VERSION_ID)?,
+            GetVersionResult::NotFound
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn gcv_gone_initial() -> anyhow::Result<()> {
+        let storage = InMemoryStorage::new();
+        let mut txn = storage.txn()?;
+        let client_key = Uuid::new_v4();
+
+        txn.new_client(client_key, Uuid::new_v4())?;
+        txn.set_snapshot(
+            client_key,
+            Snapshot {
+                version_id: Uuid::new_v4(),
+                versions_since: 0,
+                timestamp: Utc.ymd(2001, 9, 9).and_hms(1, 46, 40),
+            },
+            vec![1, 2, 3],
+        )?;
+
+        // when a snapshot exists, the first version is GONE
+        let client = txn.get_client(client_key)?.unwrap();
+        assert_eq!(
+            get_child_version(txn, client_key, client, NO_VERSION_ID)?,
+            GetVersionResult::Gone
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn gcv_not_found_up_to_date() -> anyhow::Result<()> {
+        let storage = InMemoryStorage::new();
+        let mut txn = storage.txn()?;
+        let client_key = Uuid::new_v4();
+
+        // add a parent version, but not the requested child version
         let parent_version_id = Uuid::new_v4();
-        assert_eq!(get_child_version(txn, client_key, parent_version_id)?, None);
+        txn.new_client(client_key, parent_version_id)?;
+        txn.add_version(client_key, parent_version_id, NO_VERSION_ID, vec![])?;
+
+        let client = txn.get_client(client_key)?.unwrap();
+        assert_eq!(
+            get_child_version(txn, client_key, client, parent_version_id)?,
+            GetVersionResult::NotFound
+        );
+        Ok(())
+    }
+
+    #[test]
+    fn gcv_gone() -> anyhow::Result<()> {
+        let storage = InMemoryStorage::new();
+        let mut txn = storage.txn()?;
+        let client_key = Uuid::new_v4();
+
+        // make up a parent version id, but neither that version
+        // nor its child exists (presumed to have been deleted)
+        let parent_version_id = Uuid::new_v4();
+        txn.new_client(client_key, Uuid::new_v4())?;
+
+        let client = txn.get_client(client_key)?.unwrap();
+        assert_eq!(
+            get_child_version(txn, client_key, client, parent_version_id)?,
+            GetVersionResult::Gone
+        );
         Ok(())
     }
 
@@ -109,13 +209,14 @@ mod test {
             history_segment.clone(),
         )?;
 
+        let client = txn.get_client(client_key)?.unwrap();
         assert_eq!(
-            get_child_version(txn, client_key, parent_version_id)?,
+            get_child_version(txn, client_key, client, parent_version_id)?,
-            Some(GetVersionResult {
+            GetVersionResult::Success {
                 version_id,
                 parent_version_id,
                 history_segment,
-            })
+            }
         );
         Ok(())
     }