From e5091c51de77d89a10ef64e5fea0751c13a19524 Mon Sep 17 00:00:00 2001 From: "Dustin J. Mitchell" Date: Sun, 17 Nov 2024 22:59:06 -0500 Subject: [PATCH] Create clients on NoSuchClient error With this change, the server methods no longer take a `Client` argument and instead fetch the client directly. --- core/src/server.rs | 175 +++++++++++++--------------- server/src/api/add_snapshot.rs | 28 +---- server/src/api/add_version.rs | 118 ++++++++++++------- server/src/api/get_child_version.rs | 35 +++--- server/src/api/get_snapshot.rs | 12 +- server/src/api/mod.rs | 8 +- 6 files changed, 182 insertions(+), 194 deletions(-) diff --git a/core/src/server.rs b/core/src/server.rs index 036da64..3209848 100644 --- a/core/src/server.rs +++ b/core/src/server.rs @@ -1,5 +1,5 @@ use crate::error::ServerError; -use crate::storage::{Client, Snapshot, Storage, StorageTxn}; +use crate::storage::{Snapshot, Storage, StorageTxn}; use chrono::Utc; use uuid::Uuid; @@ -109,10 +109,12 @@ impl Server { pub fn get_child_version( &self, client_id: ClientId, - client: Client, parent_version_id: VersionId, ) -> Result { let mut txn = self.storage.txn()?; + let client = txn + .get_client(client_id)? + .ok_or(ServerError::NoSuchClient)?; // If a version with parentVersionId equal to the requested parentVersionId exists, it is // returned. @@ -145,11 +147,13 @@ impl Server { pub fn add_version( &self, client_id: ClientId, - client: Client, parent_version_id: VersionId, history_segment: HistorySegment, ) -> Result<(AddVersionResult, SnapshotUrgency), ServerError> { let mut txn = self.storage.txn()?; + let client = txn + .get_client(client_id)? + .ok_or(ServerError::NoSuchClient)?; log::debug!( "add_version(client_id: {}, parent_version_id: {})", @@ -204,11 +208,13 @@ impl Server { pub fn add_snapshot( &self, client_id: ClientId, - client: Client, version_id: VersionId, data: Vec, ) -> Result<(), ServerError> { let mut txn = self.storage.txn()?; + let client = txn + .get_client(client_id)? + .ok_or(ServerError::NoSuchClient)?; log::debug!( "add_snapshot(client_id: {}, version_id: {})", @@ -290,9 +296,11 @@ impl Server { pub fn get_snapshot( &self, client_id: ClientId, - client: Client, ) -> Result)>, ServerError> { let mut txn = self.storage.txn()?; + let client = txn + .get_client(client_id)? + .ok_or(ServerError::NoSuchClient)?; Ok(if let Some(snap) = client.snapshot { txn.get_snapshot_data(client_id, snap.version_id)? @@ -331,8 +339,8 @@ mod test { num_versions: u32, snapshot_version: Option, snapshot_days_ago: Option, - ) -> anyhow::Result<(Server, Uuid, Client, Vec)> { - let (server, (client_id, client, versions)) = setup(|txn| { + ) -> anyhow::Result<(Server, Uuid, Vec)> { + let (server, (client_id, versions)) = setup(|txn| { let client_id = Uuid::new_v4(); let mut versions = vec![]; @@ -361,10 +369,9 @@ mod test { } } - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, versions)) + Ok((client_id, versions)) })?; - Ok((server, client_id, client, versions)) + Ok((server, client_id, versions)) } /// Utility function to check the results of an add_version call @@ -447,16 +454,15 @@ mod test { #[test] fn get_child_version_not_found_initial_nil() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); txn.new_client(client_id, NIL_VERSION_ID)?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; // when no latest version exists, the first version is NotFound assert_eq!( - server.get_child_version(client_id, client, NIL_VERSION_ID)?, + server.get_child_version(client_id, NIL_VERSION_ID)?, GetVersionResult::NotFound ); Ok(()) @@ -464,18 +470,17 @@ mod test { #[test] fn get_child_version_not_found_initial_continuing() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); txn.new_client(client_id, NIL_VERSION_ID)?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; // when no latest version exists, _any_ child version is NOT_FOUND. This allows syncs to // start to a new server even if the client already has been uploading to another service. assert_eq!( - server.get_child_version(client_id, client, Uuid::new_v4(),)?, + server.get_child_version(client_id, Uuid::new_v4(),)?, GetVersionResult::NotFound ); Ok(()) @@ -483,19 +488,18 @@ mod test { #[test] fn get_child_version_not_found_up_to_date() -> anyhow::Result<()> { - let (server, (client_id, client, parent_version_id)) = setup(|txn| { + let (server, (client_id, parent_version_id)) = setup(|txn| { // add a parent version, but not the requested child version let client_id = Uuid::new_v4(); let parent_version_id = Uuid::new_v4(); txn.new_client(client_id, parent_version_id)?; txn.add_version(client_id, parent_version_id, NIL_VERSION_ID, vec![])?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, parent_version_id)) + Ok((client_id, parent_version_id)) })?; assert_eq!( - server.get_child_version(client_id, client, parent_version_id)?, + server.get_child_version(client_id, parent_version_id)?, GetVersionResult::NotFound ); Ok(()) @@ -503,7 +507,7 @@ mod test { #[test] fn get_child_version_gone_not_latest() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); // Add a parent version, but not the requested parent version @@ -511,12 +515,11 @@ mod test { txn.new_client(client_id, parent_version_id)?; txn.add_version(client_id, parent_version_id, NIL_VERSION_ID, vec![])?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; assert_eq!( - server.get_child_version(client_id, client, Uuid::new_v4(),)?, + server.get_child_version(client_id, Uuid::new_v4(),)?, GetVersionResult::Gone ); Ok(()) @@ -524,32 +527,24 @@ mod test { #[test] fn get_child_version_found() -> anyhow::Result<()> { - let (server, (client_id, client, version_id, parent_version_id, history_segment)) = - setup(|txn| { - let client_id = Uuid::new_v4(); - let version_id = Uuid::new_v4(); - let parent_version_id = Uuid::new_v4(); - let history_segment = b"abcd".to_vec(); + let (server, (client_id, version_id, parent_version_id, history_segment)) = setup(|txn| { + let client_id = Uuid::new_v4(); + let version_id = Uuid::new_v4(); + let parent_version_id = Uuid::new_v4(); + let history_segment = b"abcd".to_vec(); - txn.new_client(client_id, version_id)?; - txn.add_version( - client_id, - version_id, - parent_version_id, - history_segment.clone(), - )?; + txn.new_client(client_id, version_id)?; + txn.add_version( + client_id, + version_id, + parent_version_id, + history_segment.clone(), + )?; - let client = txn.get_client(client_id)?.unwrap(); - Ok(( - client_id, - client, - version_id, - parent_version_id, - history_segment, - )) - })?; + Ok((client_id, version_id, parent_version_id, history_segment)) + })?; assert_eq!( - server.get_child_version(client_id, client, parent_version_id)?, + server.get_child_version(client_id, parent_version_id)?, GetVersionResult::Success { version_id, parent_version_id, @@ -561,13 +556,11 @@ mod test { #[test] fn add_version_conflict() -> anyhow::Result<()> { - let (server, client_id, client, versions) = av_setup(3, None, None)?; + let (server, client_id, versions) = av_setup(3, None, None)?; // try to add a child of a version other than the latest assert_eq!( - server - .add_version(client_id, client, versions[1], vec![3, 6, 9])? - .0, + server.add_version(client_id, versions[1], vec![3, 6, 9])?.0, AddVersionResult::ExpectedParentVersion(versions[2]) ); @@ -584,9 +577,9 @@ mod test { #[test] fn add_version_with_existing_history() -> anyhow::Result<()> { - let (server, client_id, client, versions) = av_setup(1, None, None)?; + let (server, client_id, versions) = av_setup(1, None, None)?; - let result = server.add_version(client_id, client, versions[0], vec![3, 6, 9])?; + let result = server.add_version(client_id, versions[0], vec![3, 6, 9])?; av_success_check( &server, @@ -603,10 +596,10 @@ mod test { #[test] fn add_version_with_no_history() -> anyhow::Result<()> { - let (server, client_id, client, versions) = av_setup(0, None, None)?; + let (server, client_id, versions) = av_setup(0, None, None)?; let parent_version_id = Uuid::nil(); - let result = server.add_version(client_id, client, parent_version_id, vec![3, 6, 9])?; + let result = server.add_version(client_id, parent_version_id, vec![3, 6, 9])?; av_success_check( &server, @@ -623,9 +616,9 @@ mod test { #[test] fn add_version_success_recent_snapshot() -> anyhow::Result<()> { - let (server, client_id, client, versions) = av_setup(1, Some(0), None)?; + let (server, client_id, versions) = av_setup(1, Some(0), None)?; - let result = server.add_version(client_id, client, versions[0], vec![1, 2, 3])?; + let result = server.add_version(client_id, versions[0], vec![1, 2, 3])?; av_success_check( &server, @@ -643,9 +636,9 @@ mod test { #[test] fn add_version_success_aged_snapshot() -> anyhow::Result<()> { // one snapshot, but it was 50 days ago - let (server, client_id, client, versions) = av_setup(1, Some(0), Some(50))?; + let (server, client_id, versions) = av_setup(1, Some(0), Some(50))?; - let result = server.add_version(client_id, client, versions[0], vec![1, 2, 3])?; + let result = server.add_version(client_id, versions[0], vec![1, 2, 3])?; av_success_check( &server, @@ -663,10 +656,10 @@ mod test { #[test] fn add_version_success_snapshot_many_versions_ago() -> anyhow::Result<()> { // one snapshot, but it was 50 versions ago - let (mut server, client_id, client, versions) = av_setup(50, Some(0), None)?; + let (mut server, client_id, versions) = av_setup(50, Some(0), None)?; server.config.snapshot_versions = 30; - let result = server.add_version(client_id, client, versions[49], vec![1, 2, 3])?; + let result = server.add_version(client_id, versions[49], vec![1, 2, 3])?; av_success_check( &server, @@ -683,7 +676,7 @@ mod test { #[test] fn add_snapshot_success_latest() -> anyhow::Result<()> { - let (server, (client_id, client, version_id)) = setup(|txn| { + let (server, (client_id, version_id)) = setup(|txn| { let client_id = Uuid::new_v4(); let version_id = Uuid::new_v4(); @@ -692,10 +685,9 @@ mod test { txn.add_version(client_id, version_id, NIL_VERSION_ID, vec![])?; // add a snapshot for that version - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, version_id)) + Ok((client_id, version_id)) })?; - server.add_snapshot(client_id, client, version_id, vec![1, 2, 3])?; + server.add_snapshot(client_id, version_id, vec![1, 2, 3])?; // verify the snapshot let mut txn = server.txn()?; @@ -713,7 +705,7 @@ mod test { #[test] fn add_snapshot_success_older() -> anyhow::Result<()> { - let (server, (client_id, client, version_id_1)) = setup(|txn| { + let (server, (client_id, version_id_1)) = setup(|txn| { let client_id = Uuid::new_v4(); let version_id_1 = Uuid::new_v4(); let version_id_2 = Uuid::new_v4(); @@ -723,11 +715,10 @@ mod test { txn.add_version(client_id, version_id_1, NIL_VERSION_ID, vec![])?; txn.add_version(client_id, version_id_2, version_id_1, vec![])?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, version_id_1)) + Ok((client_id, version_id_1)) })?; // add a snapshot for version 1 - server.add_snapshot(client_id, client, version_id_1, vec![1, 2, 3])?; + server.add_snapshot(client_id, version_id_1, vec![1, 2, 3])?; // verify the snapshot let mut txn = server.txn()?; @@ -745,7 +736,7 @@ mod test { #[test] fn add_snapshot_fails_no_such() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); let version_id_1 = Uuid::new_v4(); let version_id_2 = Uuid::new_v4(); @@ -756,12 +747,11 @@ mod test { txn.add_version(client_id, version_id_2, version_id_1, vec![])?; // add a snapshot for unknown version - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; let version_id_unk = Uuid::new_v4(); - server.add_snapshot(client_id, client, version_id_unk, vec![1, 2, 3])?; + server.add_snapshot(client_id, version_id_unk, vec![1, 2, 3])?; // verify the snapshot does not exist let mut txn = server.txn()?; @@ -773,7 +763,7 @@ mod test { #[test] fn add_snapshot_fails_too_old() -> anyhow::Result<()> { - let (server, (client_id, client, version_ids)) = setup(|txn| { + let (server, (client_id, version_ids)) = setup(|txn| { let client_id = Uuid::new_v4(); let mut version_id = Uuid::new_v4(); let mut parent_version_id = Uuid::nil(); @@ -789,10 +779,9 @@ mod test { } // add a snapshot for the earliest of those - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, version_ids)) + Ok((client_id, version_ids)) })?; - server.add_snapshot(client_id, client, version_ids[0], vec![1, 2, 3])?; + server.add_snapshot(client_id, version_ids[0], vec![1, 2, 3])?; // verify the snapshot does not exist let mut txn = server.txn()?; @@ -804,7 +793,7 @@ mod test { #[test] fn add_snapshot_fails_newer_exists() -> anyhow::Result<()> { - let (server, (client_id, client, version_ids)) = setup(|txn| { + let (server, (client_id, version_ids)) = setup(|txn| { let client_id = Uuid::new_v4(); let mut version_id = Uuid::new_v4(); let mut parent_version_id = Uuid::nil(); @@ -830,11 +819,10 @@ mod test { )?; // add a snapshot for the earliest of those - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, version_ids)) + Ok((client_id, version_ids)) })?; - server.add_snapshot(client_id, client, version_ids[0], vec![9, 9, 9])?; + server.add_snapshot(client_id, version_ids[0], vec![9, 9, 9])?; // verify the snapshot was not replaced let mut txn = server.txn()?; @@ -852,18 +840,17 @@ mod test { #[test] fn add_snapshot_fails_nil_version() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); // just set up the client txn.new_client(client_id, NIL_VERSION_ID)?; // add a snapshot for the nil version - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; - server.add_snapshot(client_id, client, NIL_VERSION_ID, vec![9, 9, 9])?; + server.add_snapshot(client_id, NIL_VERSION_ID, vec![9, 9, 9])?; // verify the snapshot does not exist let mut txn = server.txn()?; @@ -875,7 +862,7 @@ mod test { #[test] fn get_snapshot_found() -> anyhow::Result<()> { - let (server, (client_id, client, data, snapshot_version_id)) = setup(|txn| { + let (server, (client_id, data, snapshot_version_id)) = setup(|txn| { let client_id = Uuid::new_v4(); let data = vec![1, 2, 3]; let snapshot_version_id = Uuid::new_v4(); @@ -890,11 +877,10 @@ mod test { }, data.clone(), )?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client, data, snapshot_version_id)) + Ok((client_id, data, snapshot_version_id)) })?; assert_eq!( - server.get_snapshot(client_id, client)?, + server.get_snapshot(client_id)?, Some((snapshot_version_id, data)) ); @@ -903,14 +889,13 @@ mod test { #[test] fn get_snapshot_not_found() -> anyhow::Result<()> { - let (server, (client_id, client)) = setup(|txn| { + let (server, client_id) = setup(|txn| { let client_id = Uuid::new_v4(); txn.new_client(client_id, NIL_VERSION_ID)?; - let client = txn.get_client(client_id)?.unwrap(); - Ok((client_id, client)) + Ok(client_id) })?; - assert_eq!(server.get_snapshot(client_id, client)?, None); + assert_eq!(server.get_snapshot(client_id)?, None); Ok(()) } diff --git a/server/src/api/add_snapshot.rs b/server/src/api/add_snapshot.rs index 98534f1..b5d93cf 100644 --- a/server/src/api/add_snapshot.rs +++ b/server/src/api/add_snapshot.rs @@ -1,10 +1,8 @@ -use crate::api::{ - client_id_header, failure_to_ise, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE, -}; +use crate::api::{client_id_header, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE}; use actix_web::{error, post, web, HttpMessage, HttpRequest, HttpResponse, Result}; use futures::StreamExt; use std::sync::Arc; -use taskchampion_sync_server_core::{VersionId, NIL_VERSION_ID}; +use taskchampion_sync_server_core::VersionId; /// Max snapshot size: 100MB const MAX_SIZE: usize = 100 * 1024 * 1024; @@ -48,38 +46,20 @@ pub(crate) async fn service( return Err(error::ErrorBadRequest("No snapshot supplied")); } - // note that we do not open the transaction until the body has been read - // completely, to avoid blocking other storage access while that data is - // in transit. - let client = { - let mut txn = server_state.server.txn().map_err(server_error_to_actix)?; - - // get, or create, the client - match txn.get_client(client_id).map_err(failure_to_ise)? { - Some(client) => client, - None => { - txn.new_client(client_id, NIL_VERSION_ID) - .map_err(failure_to_ise)?; - txn.get_client(client_id).map_err(failure_to_ise)?.unwrap() - } - } - }; - server_state .server - .add_snapshot(client_id, client, version_id, body.to_vec()) + .add_snapshot(client_id, version_id, body.to_vec()) .map_err(server_error_to_actix)?; Ok(HttpResponse::Ok().body("")) } #[cfg(test)] mod test { - use super::*; use crate::api::CLIENT_ID_HEADER; use crate::WebServer; use actix_web::{http::StatusCode, test, App}; use pretty_assertions::assert_eq; - use taskchampion_sync_server_core::{InMemoryStorage, Storage}; + use taskchampion_sync_server_core::{InMemoryStorage, Storage, NIL_VERSION_ID}; use uuid::Uuid; #[actix_rt::test] diff --git a/server/src/api/add_version.rs b/server/src/api/add_version.rs index 537c0e8..365d92a 100644 --- a/server/src/api/add_version.rs +++ b/server/src/api/add_version.rs @@ -6,7 +6,9 @@ use crate::api::{ use actix_web::{error, post, web, HttpMessage, HttpRequest, HttpResponse, Result}; use futures::StreamExt; use std::sync::Arc; -use taskchampion_sync_server_core::{AddVersionResult, SnapshotUrgency, VersionId, NIL_VERSION_ID}; +use taskchampion_sync_server_core::{ + AddVersionResult, ServerError, SnapshotUrgency, VersionId, NIL_VERSION_ID, +}; /// Max history segment size: 100MB const MAX_SIZE: usize = 100 * 1024 * 1024; @@ -55,48 +57,41 @@ pub(crate) async fn service( return Err(error::ErrorBadRequest("Empty body")); } - // note that we do not open the transaction until the body has been read - // completely, to avoid blocking other storage access while that data is - // in transit. - let client = { - let mut txn = server_state.server.txn().map_err(server_error_to_actix)?; - - match txn.get_client(client_id).map_err(failure_to_ise)? { - Some(client) => client, - None => { + loop { + return match server_state + .server + .add_version(client_id, parent_version_id, body.to_vec()) + { + Ok((AddVersionResult::Ok(version_id), snap_urgency)) => { + let mut rb = HttpResponse::Ok(); + rb.append_header((VERSION_ID_HEADER, version_id.to_string())); + match snap_urgency { + SnapshotUrgency::None => {} + SnapshotUrgency::Low => { + rb.append_header((SNAPSHOT_REQUEST_HEADER, "urgency=low")); + } + SnapshotUrgency::High => { + rb.append_header((SNAPSHOT_REQUEST_HEADER, "urgency=high")); + } + }; + Ok(rb.finish()) + } + Ok((AddVersionResult::ExpectedParentVersion(parent_version_id), _)) => { + let mut rb = HttpResponse::Conflict(); + rb.append_header((PARENT_VERSION_ID_HEADER, parent_version_id.to_string())); + Ok(rb.finish()) + } + Err(ServerError::NoSuchClient) => { + // Create a new client and repeate the `add_version` call. + let mut txn = server_state.server.txn().map_err(server_error_to_actix)?; txn.new_client(client_id, NIL_VERSION_ID) .map_err(failure_to_ise)?; - txn.get_client(client_id).map_err(failure_to_ise)?.unwrap() + txn.commit().map_err(failure_to_ise)?; + continue; } - } - }; - - let (result, snap_urgency) = server_state - .server - .add_version(client_id, client, parent_version_id, body.to_vec()) - .map_err(server_error_to_actix)?; - - Ok(match result { - AddVersionResult::Ok(version_id) => { - let mut rb = HttpResponse::Ok(); - rb.append_header((VERSION_ID_HEADER, version_id.to_string())); - match snap_urgency { - SnapshotUrgency::None => {} - SnapshotUrgency::Low => { - rb.append_header((SNAPSHOT_REQUEST_HEADER, "urgency=low")); - } - SnapshotUrgency::High => { - rb.append_header((SNAPSHOT_REQUEST_HEADER, "urgency=high")); - } - }; - rb.finish() - } - AddVersionResult::ExpectedParentVersion(parent_version_id) => { - let mut rb = HttpResponse::Conflict(); - rb.append_header((PARENT_VERSION_ID_HEADER, parent_version_id.to_string())); - rb.finish() - } - }) + Err(e) => Err(server_error_to_actix(e)), + }; + } } #[cfg(test)] @@ -150,6 +145,49 @@ mod test { assert_eq!(resp.headers().get("X-Parent-Version-Id"), None); } + #[actix_rt::test] + async fn test_auto_add_client() { + let client_id = Uuid::new_v4(); + let version_id = Uuid::new_v4(); + let parent_version_id = Uuid::new_v4(); + let server = WebServer::new(Default::default(), InMemoryStorage::new()); + let app = App::new().configure(|sc| server.config(sc)); + let app = test::init_service(app).await; + + let uri = format!("/v1/client/add-version/{}", parent_version_id); + let req = test::TestRequest::post() + .uri(&uri) + .append_header(( + "Content-Type", + "application/vnd.taskchampion.history-segment", + )) + .append_header((CLIENT_ID_HEADER, client_id.to_string())) + .set_payload(b"abcd".to_vec()) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), StatusCode::OK); + + // the returned version ID is random, but let's check that it's not + // the passed parent version ID, at least + let new_version_id = resp.headers().get("X-Version-Id").unwrap(); + let new_version_id = Uuid::parse_str(new_version_id.to_str().unwrap()).unwrap(); + assert!(new_version_id != version_id); + + // Shapshot should be requested, since there is no existing snapshot + let snapshot_request = resp.headers().get("X-Snapshot-Request").unwrap(); + assert_eq!(snapshot_request, "urgency=high"); + + assert_eq!(resp.headers().get("X-Parent-Version-Id"), None); + + // Check that the client really was created + { + let mut txn = server.server_state.server.txn().unwrap(); + let client = txn.get_client(client_id).unwrap().unwrap(); + assert_eq!(client.latest_version_id, new_version_id); + assert_eq!(client.snapshot, None); + } + } + #[actix_rt::test] async fn test_conflict() { let client_id = Uuid::new_v4(); diff --git a/server/src/api/get_child_version.rs b/server/src/api/get_child_version.rs index 6605532..f1b0a0a 100644 --- a/server/src/api/get_child_version.rs +++ b/server/src/api/get_child_version.rs @@ -1,10 +1,10 @@ use crate::api::{ - client_id_header, failure_to_ise, server_error_to_actix, ServerState, - HISTORY_SEGMENT_CONTENT_TYPE, PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER, + client_id_header, server_error_to_actix, ServerState, HISTORY_SEGMENT_CONTENT_TYPE, + PARENT_VERSION_ID_HEADER, VERSION_ID_HEADER, }; use actix_web::{error, get, web, HttpRequest, HttpResponse, Result}; use std::sync::Arc; -use taskchampion_sync_server_core::{GetVersionResult, VersionId}; +use taskchampion_sync_server_core::{GetVersionResult, ServerError, VersionId}; /// Get a child version. /// @@ -21,35 +21,28 @@ pub(crate) async fn service( path: web::Path, ) -> Result { let parent_version_id = path.into_inner(); - - let (client, client_id) = { - let mut txn = server_state.server.txn().map_err(server_error_to_actix)?; - - let client_id = client_id_header(&req)?; - - let client = txn - .get_client(client_id) - .map_err(failure_to_ise)? - .ok_or_else(|| error::ErrorNotFound("no such client"))?; - (client, client_id) - }; + let client_id = client_id_header(&req)?; return match server_state .server - .get_child_version(client_id, client, parent_version_id) - .map_err(server_error_to_actix)? + .get_child_version(client_id, parent_version_id) { - GetVersionResult::Success { + Ok(GetVersionResult::Success { version_id, parent_version_id, history_segment, - } => Ok(HttpResponse::Ok() + }) => Ok(HttpResponse::Ok() .content_type(HISTORY_SEGMENT_CONTENT_TYPE) .append_header((VERSION_ID_HEADER, version_id.to_string())) .append_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")), + Ok(GetVersionResult::NotFound) => Err(error::ErrorNotFound("no such version")), + Ok(GetVersionResult::Gone) => Err(error::ErrorGone("version has been deleted")), + // Note that the HTTP client cannot differentiate `NotFound` and `NoSuchClient`, as both + // are a 404 NOT FOUND response. In either case, the HTTP client will typically attempt + // to add a new version, which may create the new client at the same time. + Err(ServerError::NoSuchClient) => Err(error::ErrorNotFound("no such client")), + Err(e) => Err(server_error_to_actix(e)), }; } diff --git a/server/src/api/get_snapshot.rs b/server/src/api/get_snapshot.rs index 15fcc98..47a728d 100644 --- a/server/src/api/get_snapshot.rs +++ b/server/src/api/get_snapshot.rs @@ -1,6 +1,5 @@ use crate::api::{ - client_id_header, failure_to_ise, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE, - VERSION_ID_HEADER, + client_id_header, server_error_to_actix, ServerState, SNAPSHOT_CONTENT_TYPE, VERSION_ID_HEADER, }; use actix_web::{error, get, web, HttpRequest, HttpResponse, Result}; use std::sync::Arc; @@ -20,16 +19,9 @@ pub(crate) async fn service( ) -> Result { let client_id = client_id_header(&req)?; - let client = { - let mut txn = server_state.server.txn().map_err(server_error_to_actix)?; - txn.get_client(client_id) - .map_err(failure_to_ise)? - .ok_or_else(|| error::ErrorNotFound("no such client"))? - }; - if let Some((version_id, data)) = server_state .server - .get_snapshot(client_id, client) + .get_snapshot(client_id) .map_err(server_error_to_actix)? { Ok(HttpResponse::Ok() diff --git a/server/src/api/mod.rs b/server/src/api/mod.rs index 8965327..25812da 100644 --- a/server/src/api/mod.rs +++ b/server/src/api/mod.rs @@ -1,4 +1,4 @@ -use actix_web::{error, http::StatusCode, web, HttpRequest, Result, Scope}; +use actix_web::{error, web, HttpRequest, Result, Scope}; use taskchampion_sync_server_core::{ClientId, Server, ServerError}; mod add_snapshot; @@ -38,9 +38,9 @@ pub(crate) fn api_scope() -> Scope { .service(add_snapshot::service) } -/// Convert a failure::Error to an Actix ISE -fn failure_to_ise(err: anyhow::Error) -> impl actix_web::ResponseError { - error::InternalError::new(err, StatusCode::INTERNAL_SERVER_ERROR) +/// Convert a `anyhow::Error` to an Actix ISE +fn failure_to_ise(err: anyhow::Error) -> actix_web::Error { + error::ErrorInternalServerError(err) } /// Convert a ServerError to an Actix error