From 8517c76b92d5fa7041c330438e33f2b08924ac7b Mon Sep 17 00:00:00 2001 From: Bryan Newbold Date: Tue, 15 May 2018 23:05:07 -0700 Subject: refactor error handling --- rust/TODO | 4 +- rust/src/api_server.rs | 199 ++++++++++++++++++++++++++++++++----------------- rust/src/lib.rs | 12 ++- 3 files changed, 145 insertions(+), 70 deletions(-) diff --git a/rust/TODO b/rust/TODO index 99d6a1bd..e374dae7 100644 --- a/rust/TODO +++ b/rust/TODO @@ -9,10 +9,10 @@ x cleanup pooled database: https://github.com/diesel-rs/diesel/pull/1466 x clean up blasse error handling a bit x add 404s to gets x wow. fix a bunch of api schema names ("FindASingleContainerByExternalIdentifer") -- creators, releases, works, files (?) +- refactor handlers to have a proper Result<_,_> error-chain type, so I can use '?' - one-to-many relationship (eg, works) - many-to-many relationship (eg, creators) -- refactor handlers to have a proper Result<_,_> error-chain type, so I can use '?' +- creators, releases, works, files (?) - copypasta a bunch of CRUD - helper to calculate state of idents diff --git a/rust/src/api_server.rs b/rust/src/api_server.rs index d70c3d56..200ecdb2 100644 --- a/rust/src/api_server.rs +++ b/rust/src/api_server.rs @@ -8,6 +8,7 @@ use database_schema::{changelog, container_edit, container_ident, container_rev, work_rev}; use diesel::prelude::*; use diesel::{self, insert_into}; +use errors::*; use fatcat_api::models; use fatcat_api::models::*; use fatcat_api::{Api, ApiError, ContainerIdGetResponse, ContainerLookupGetResponse, @@ -25,40 +26,21 @@ pub struct Server { pub db_pool: ConnectionPool, } -impl Api for Server { - fn container_id_get( - &self, - id: String, - _context: &Context, - ) -> Box + Send> { +impl Server { + + fn container_id_get_handler(&self, id: String) -> Result> { let conn = self.db_pool.get().expect("db_pool error"); - let id = match uuid::Uuid::parse_str(&id) { - Ok(some_uuid) => some_uuid, - Err(_) => { - return Box::new(futures::done(Ok(ContainerIdGetResponse::BadRequest( - Error { - message: "Failed to parse UUID".to_string(), - }, - )))); - } - }; + let id = uuid::Uuid::parse_str(&id)?; - let res: Result<(ContainerIdentRow, ContainerRevRow), _> = container_ident::table + let res: ::std::result::Result<(ContainerIdentRow, ContainerRevRow), _> = container_ident::table .find(id) .inner_join(container_rev::table) .first(&conn); let (ident, rev) = match res { Ok(r) => r, - Err(_) => { - return Box::new(futures::done(Ok( - // TODO: UGH, need to add 404 responses everywhere, not 400 - //ContainerIdGetResponse::NotFound( - ContainerIdGetResponse::BadRequest(Error { - message: "No such container".to_string(), - }), - ))); - } + Err(diesel::result::Error::NotFound) => return Ok(None), + Err(e) => return Err(e.into()), }; let entity = ContainerEntity { @@ -72,9 +54,72 @@ impl Api for Server { redirect: ident.redirect_id.map(|u| u.to_string()), editgroup: None, }; - Box::new(futures::done(Ok(ContainerIdGetResponse::FoundEntity( - entity, - )))) + Ok(Some(entity)) + } + + fn creator_id_get_handler(&self, id: String) -> Result> { + let conn = self.db_pool.get().expect("db_pool error"); + let id = uuid::Uuid::parse_str(&id)?; + + let res: ::std::result::Result<(CreatorIdentRow, CreatorRevRow), _> = creator_ident::table + .find(id) + .inner_join(creator_rev::table) + .first(&conn); + + let (ident, rev) = match res { + Ok(r) => r, + Err(diesel::result::Error::NotFound) => return Ok(None), + Err(e) => return Err(e.into()), + }; + + let entity = CreatorEntity { + name: rev.name, + orcid: rev.orcid, + state: None, // TODO: + ident: Some(ident.id.to_string()), + revision: ident.rev_id.map(|v| v as isize), + redirect: ident.redirect_id.map(|u| u.to_string()), + editgroup: None, + }; + Ok(Some(entity)) + } + + // TODO: + fn file_id_get_handler(&self, id: String) -> Result> { + Ok(None) + } + + // TODO: + fn work_id_get_handler(&self, id: String) -> Result> { + Ok(None) + } + + // TODO: + fn release_id_get_handler(&self, id: String) -> Result> { + Ok(None) + } +} + +impl Api for Server { + + fn container_id_get( + &self, + id: String, + _context: &Context, + ) -> Box + Send> { + match self.container_id_get_handler(id) { + Ok(Some(entity)) => + Box::new(futures::done(Ok(ContainerIdGetResponse::FoundEntity(entity)))), + Ok(None) => + Box::new(futures::done(Ok(ContainerIdGetResponse::NotFound( + ErrorResponse { message: "No such entity".to_string() }), + ))), + Err(e) => + // TODO: dig in to error type here + Box::new(futures::done(Ok(ContainerIdGetResponse::BadRequest( + ErrorResponse { message: e.to_string() }, + )))), + } } fn container_lookup_get( @@ -84,7 +129,7 @@ impl Api for Server { ) -> Box + Send> { let conn = self.db_pool.get().expect("db_pool error"); - let res: Result<(ContainerIdentRow, ContainerRevRow), _> = container_ident::table + let res: ::std::result::Result<(ContainerIdentRow, ContainerRevRow), _> = container_ident::table .inner_join(container_rev::table) .first(&conn); // XXX: actually do a filter/lookup @@ -95,7 +140,7 @@ impl Api for Server { return Box::new(futures::done(Ok( // TODO: UGH, need to add 404 responses everywhere, not 400 //ContainerIdGetResponse::NotFound( - ContainerLookupGetResponse::BadRequest(Error { + ContainerLookupGetResponse::BadRequest(ErrorResponse { message: "No such container".to_string(), }), ))); @@ -121,7 +166,7 @@ impl Api for Server { fn container_post( &self, body: models::ContainerEntity, - context: &Context, + _context: &Context, ) -> Box + Send> { println!("{:?}", body); //let editgroup_id: i64 = body.editgroup.expect("need editgroup_id") as i64; @@ -166,17 +211,19 @@ impl Api for Server { id: String, _context: &Context, ) -> Box + Send> { - let conn = self.db_pool.get().expect("db_pool error"); - let ce = CreatorEntity { - orcid: None, - name: "Dummy Name".into(), - state: None, - ident: None, - revision: None, - redirect: None, - editgroup: None, - }; - Box::new(futures::done(Ok(CreatorIdGetResponse::FoundEntity(ce)))) + match self.creator_id_get_handler(id) { + Ok(Some(entity)) => + Box::new(futures::done(Ok(CreatorIdGetResponse::FoundEntity(entity)))), + Ok(None) => + Box::new(futures::done(Ok(CreatorIdGetResponse::NotFound( + ErrorResponse { message: "No such entity".to_string() }), + ))), + Err(e) => + // TODO: dig in to error type here + Box::new(futures::done(Ok(CreatorIdGetResponse::BadRequest( + ErrorResponse { message: e.to_string() }, + )))), + } } fn creator_lookup_get( @@ -210,15 +257,21 @@ impl Api for Server { fn file_id_get( &self, id: String, - context: &Context, + _context: &Context, ) -> Box + Send> { - let context = context.clone(); - println!( - "file_id_get(\"{}\") - X-Span-ID: {:?}", - id, - context.x_span_id.unwrap_or(String::from("")).clone() - ); - Box::new(futures::failed("Generic failure".into())) + match self.file_id_get_handler(id) { + Ok(Some(entity)) => + Box::new(futures::done(Ok(FileIdGetResponse::FoundEntity(entity)))), + Ok(None) => + Box::new(futures::done(Ok(FileIdGetResponse::NotFound( + ErrorResponse { message: "No such entity".to_string() }), + ))), + Err(e) => + // TODO: dig in to error type here + Box::new(futures::done(Ok(FileIdGetResponse::BadRequest( + ErrorResponse { message: e.to_string() }, + )))), + } } fn file_lookup_get( @@ -252,15 +305,21 @@ impl Api for Server { fn work_id_get( &self, id: String, - context: &Context, + _context: &Context, ) -> Box + Send> { - let context = context.clone(); - println!( - "work_id_get(\"{}\") - X-Span-ID: {:?}", - id, - context.x_span_id.unwrap_or(String::from("")).clone() - ); - Box::new(futures::failed("Generic failure".into())) + match self.work_id_get_handler(id) { + Ok(Some(entity)) => + Box::new(futures::done(Ok(WorkIdGetResponse::FoundEntity(entity)))), + Ok(None) => + Box::new(futures::done(Ok(WorkIdGetResponse::NotFound( + ErrorResponse { message: "No such entity".to_string() }), + ))), + Err(e) => + // TODO: dig in to error type here + Box::new(futures::done(Ok(WorkIdGetResponse::BadRequest( + ErrorResponse { message: e.to_string() }, + )))), + } } fn work_post( @@ -280,15 +339,21 @@ impl Api for Server { fn release_id_get( &self, id: String, - context: &Context, + _context: &Context, ) -> Box + Send> { - let context = context.clone(); - println!( - "release_id_get(\"{}\") - X-Span-ID: {:?}", - id, - context.x_span_id.unwrap_or(String::from("")).clone() - ); - Box::new(futures::failed("Generic failure".into())) + match self.release_id_get_handler(id) { + Ok(Some(entity)) => + Box::new(futures::done(Ok(ReleaseIdGetResponse::FoundEntity(entity)))), + Ok(None) => + Box::new(futures::done(Ok(ReleaseIdGetResponse::NotFound( + ErrorResponse { message: "No such entity".to_string() }), + ))), + Err(e) => + // TODO: dig in to error type here + Box::new(futures::done(Ok(ReleaseIdGetResponse::BadRequest( + ErrorResponse { message: e.to_string() }, + )))), + } } fn release_lookup_get( diff --git a/rust/src/lib.rs b/rust/src/lib.rs index b59d6024..8069ad0b 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -20,9 +20,19 @@ pub mod database_models; pub mod database_schema; mod errors { - error_chain!{} + // Create the Error, ErrorKind, ResultExt, and Result types + error_chain! { + foreign_links { Fmt(::std::fmt::Error); + Diesel(::diesel::result::Error); + Uuid(::uuid::ParseError); + Io(::std::io::Error) #[cfg(unix)]; + } + } } +#[doc(hidden)] +pub use errors::*; + pub use self::errors::*; use diesel::pg::PgConnection; use diesel::prelude::*; -- cgit v1.2.3