From 53d1f8dbc2df92761b47b7dd221b91577962ff41 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Wed, 29 Sep 2021 22:41:40 +0000 Subject: [PATCH] update get_child_version to distinguish gone and not-found --- sync-server/src/api/get_child_version.rs | 48 +++++--- sync-server/src/server.rs | 137 ++++++++++++++++++++--- 2 files changed, 152 insertions(+), 33 deletions(-) diff --git a/sync-server/src/api/get_child_version.rs b/sync-server/src/api/get_child_version.rs index 917c65126..fd12e1243 100644 --- a/sync-server/src/api/get_child_version.rs +++ b/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)?; - if let Some(result) = result { - Ok(HttpResponse::Ok() + return match get_child_version(txn, client_key, client, parent_version_id) + .map_err(failure_to_ise)? + { + 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( - PARENT_VERSION_ID_HEADER, - result.parent_version_id.to_string(), - ) - .body(result.history_segment)) - } else { - Err(error::ErrorNotFound("no such version")) - } + .header(VERSION_ID_HEADER, version_id.to_string()) + .header(PARENT_VERSION_ID_HEADER, parent_version_id.to_string()) + .body(history_segment)), + GetVersionResult::NotFound => Err(error::ErrorNotFound("no such version")), + GetVersionResult::Gone => Err(error::ErrorGone("version has been deleted")), + }; } #[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 = 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); diff --git a/sync-server/src/server.rs b/sync-server/src/server.rs index c20d0b955..2b8f92445 100644 --- a/sync-server/src/server.rs +++ b/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; 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) version_id: Uuid, - pub(crate) parent_version_id: Uuid, - pub(crate) history_segment: HistorySegment, +pub(crate) enum GetVersionResult { + NotFound, + Gone, + 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, client_key: ClientKey, + client: Client, parent_version_id: VersionId, -) -> anyhow::Result> { - Ok(txn - .get_version_by_parent(client_key, parent_version_id)? - .map(|version| GetVersionResult { +) -> anyhow::Result { + // If a version with parentVersionId equal to the requested parentVersionId exists, it is returned. + if let Some(version) = txn.get_version_by_parent(client_key, parent_version_id)? { + 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, 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)?, - Some(GetVersionResult { + get_child_version(txn, client_key, client, parent_version_id)?, + GetVersionResult::Success { version_id, parent_version_id, history_segment, - }) + } ); Ok(()) }