From 18eae2395863e32e4b7413010adafe1ffc95076e Mon Sep 17 00:00:00 2001 From: Bryan Newbold Date: Fri, 7 Sep 2018 10:49:15 -0700 Subject: major CRUD refactor This is the start of a large refactor to move all entity CRUD (create, read, update, delete) model/database code into it's own file. HACKING has been updated with an overview of what happens in each file. Next steps: - split rev (and sub-table) insertion in to db_rev_insert and make create/update generic - inserts should be batch (vector) by default - move all other entities into this new trait framework - bypass api_server wrappers and call into CRUD from api_wrappers for entity ops (should be a big cleanup) --- rust/HACKING.md | 34 ++++++ rust/src/api_helpers.rs | 27 +++++ rust/src/api_server.rs | 109 ++++------------- rust/src/database_entity_crud.rs | 250 +++++++++++++++++++++++++++++++++++++++ rust/src/lib.rs | 1 + 5 files changed, 336 insertions(+), 85 deletions(-) create mode 100644 rust/src/database_entity_crud.rs (limited to 'rust') diff --git a/rust/HACKING.md b/rust/HACKING.md index a399164c..622a4b5a 100644 --- a/rust/HACKING.md +++ b/rust/HACKING.md @@ -1,4 +1,38 @@ +## Code Structure + +Almost all of the rust code is either auto-generated or "glue" between the +swagger API spec on one side and the SQL schema on the other. + +- `./migrations/*/up.sql`: SQL schema +- `./src/database_schema.rs`: autogenerated per-table Diesel schemas +- `./src/database_models.rs`: hand- and macro-generated Rust structs matching + Diesel schemas, and a small number of row-level helpers +- `./src/database_entity_crud.rs`: "struct-relational-mapping"; trait + implementations of CRUD (create, read, update, delete) actions for each + entity model, hitting the database (and building on `database_model` structs) +- `./src/api_server.rs`: one function for each API endpoint, with rust-style + arguments and return types. mostly calls in to `database_entity_crud`. +- `./src/api_wrappers.rs`: hand- and macro-generated wrapper functions, one per + API endpoint, that map between API request and return types, and + rust-idiomatic request and return types (plus API models). +- `./fatcat-api`: autogenerated API models and endpoint types/signatures +- `../fatcat-openapi2.yaml`: + +When deciding to use this structure, it wasn't expected that either +`api_wrappers.rs` or `database_models.rs` would need to hand-maintained; both +are verbose and implemented in a very mechanical fashion. The return type +mapping in `api_wrappers` might be necessary, but `database_models.rs` in +particular feels unnecessary; other projects have attempted to completely +automate generation of this file, but it doesn't sound reliable. In particular, +both regular "Row" (queriable) and "NewRow" (insertable) structs need to be +defined. + +## Test Structure + +- `./tests/test_api_server.rs`: Iron (web framework) level raw HTTP JSON + request/response tests of API endpoints. + ## Updating Schemas Regenerate API schemas after editing the fatcat-openapi2 schema. This will, as diff --git a/rust/src/api_helpers.rs b/rust/src/api_helpers.rs index a68ed8a9..6c9a4e5f 100644 --- a/rust/src/api_helpers.rs +++ b/rust/src/api_helpers.rs @@ -6,6 +6,9 @@ use diesel::prelude::*; use errors::*; use regex::Regex; use uuid::Uuid; +use std::str::FromStr; + +pub type DbConn = diesel::r2d2::PooledConnection>; /// This function should always be run within a transaction pub fn get_or_create_editgroup(editor_id: Uuid, conn: &PgConnection) -> Result { @@ -87,6 +90,30 @@ pub fn accept_editgroup(editgroup_id: Uuid, conn: &PgConnection) -> Result String { + uuid2fcid(&self.to_uuid()) + } +} + +impl FromStr for FatCatId { + type Err = Error; + fn from_str(s: &str) -> Result { + fcid2uuid(s).map(|u| FatCatId(u)) + } +} + +impl FatCatId { + pub fn to_uuid(&self) -> Uuid { + self.0 + } + pub fn from_uuid(u: &Uuid) -> FatCatId { + FatCatId(u.clone()) + } +} + /// Convert fatcat IDs (base32 strings) to UUID pub fn fcid2uuid(fcid: &str) -> Result { if fcid.len() != 26 { diff --git a/rust/src/api_server.rs b/rust/src/api_server.rs index 66c31f61..20f71c7a 100644 --- a/rust/src/api_server.rs +++ b/rust/src/api_server.rs @@ -17,8 +17,8 @@ use fatcat_api::models::*; use sha1::Sha1; use uuid::Uuid; use ConnectionPool; - -type DbConn = diesel::r2d2::PooledConnection>; +use database_entity_crud::{EntityCrud, EditContext}; +use std::str::FromStr; macro_rules! entity_batch_handler { ($post_handler:ident, $post_batch_handler:ident, $model:ident) => { @@ -77,6 +77,19 @@ macro_rules! count_entity { }}; } +fn make_edit_context(conn: &DbConn, editgroup_id: Option) -> Result { + let editor_id = Uuid::parse_str("00000000-0000-0000-AAAA-000000000001")?; // TODO: auth + let editgroup_id = match editgroup_id { + None => FatCatId::from_uuid(&get_or_create_editgroup(editor_id, conn).expect("current editgroup")), + Some(param) => param, + }; + Ok(EditContext { + editor_id: FatCatId::from_uuid(&editor_id), + editgroup_id: editgroup_id, + extra_json: None, + }) +} + #[derive(Clone)] pub struct Server { pub db_pool: ConnectionPool, @@ -281,6 +294,7 @@ fn release_row2entity( }) } +/* XXX: fn work_row2entity(ident: Option, rev: WorkRevRow) -> Result { let (state, ident_id, redirect_id) = match ident { Some(i) => ( @@ -299,6 +313,7 @@ fn work_row2entity(ident: Option, rev: WorkRevRow) -> Result, conn: &DbConn, ) -> Result { - let (ident, rev): (WorkIdentRow, WorkRevRow) = work_ident::table - .find(id) - .inner_join(work_rev::table) - .first(conn)?; - - work_row2entity(Some(ident), rev) + WorkEntity::db_get(conn, FatCatId::from_uuid(id)) } pub fn get_work_releases_handler(&self, id: &str, conn: &DbConn) -> Result> { @@ -886,27 +896,8 @@ impl Server { entity: models::WorkEntity, conn: &DbConn, ) -> Result { - let editor_id = Uuid::parse_str("00000000-0000-0000-AAAA-000000000001")?; // TODO: auth - let editgroup_id = match entity.editgroup_id { - None => get_or_create_editgroup(editor_id, conn).expect("current editgroup"), - Some(param) => fcid2uuid(¶m)?, - }; - - let edit: WorkEditRow = - diesel::sql_query( - "WITH rev AS ( INSERT INTO work_rev (extra_json) - VALUES ($1) - RETURNING id ), - ident AS ( INSERT INTO work_ident (rev_id) - VALUES ((SELECT rev.id FROM rev)) - RETURNING id ) - INSERT INTO work_edit (editgroup_id, ident_id, rev_id) VALUES - ($2, (SELECT ident.id FROM ident), (SELECT rev.id FROM rev)) - RETURNING *", - ).bind::, _>(entity.extra) - .bind::(editgroup_id) - .get_result(conn)?; - + let edit_context = make_edit_context(conn, entity.parse_editgroup_id()?)?; + let edit = entity.db_create(conn, &edit_context)?; edit.into_model() } @@ -916,66 +907,14 @@ impl Server { entity: models::WorkEntity, conn: &DbConn, ) -> Result { - let editor_id = Uuid::parse_str("00000000-0000-0000-AAAA-000000000001")?; // TODO: auth - let editgroup_id = match entity.editgroup_id { - None => get_or_create_editgroup(editor_id, conn).expect("current editgroup"), - Some(param) => fcid2uuid(¶m)?, - }; - - // TODO: refactor this into a check on WorkIdentRow - let current: WorkIdentRow = work_ident::table.find(id).first(conn)?; - if current.is_live != true { - // TODO: what if isn't live? 4xx not 5xx - bail!("can't delete an entity that doesn't exist yet"); - } - if current.rev_id.is_none() { - // TODO: what if it's already deleted? 4xx not 5xx - bail!("entity was already deleted"); - } - - let edit: WorkEditRow = - diesel::sql_query( - "WITH rev AS ( INSERT INTO work_rev (extra_json) - VALUES ($1) - RETURNING id ), - INSERT INTO work_edit (editgroup_id, ident_id, rev_id, prev_rev) VALUES - ($2, $3, (SELECT rev.id FROM rev), $4) - RETURNING *", - ).bind::, _>(entity.extra) - .bind::(editgroup_id) - .bind::(id) - .bind::(current.rev_id.unwrap()) - .get_result(conn)?; - + let edit_context = make_edit_context(conn, entity.parse_editgroup_id()?)?; + let edit = entity.db_update(conn, &edit_context, FatCatId::from_uuid(id))?; edit.into_model() } pub fn delete_work_handler(&self, id: &Uuid, editgroup_id: Option, conn: &DbConn) -> Result { - let editor_id = Uuid::parse_str("00000000-0000-0000-AAAA-000000000001")?; // TODO: auth - let editgroup_id = match editgroup_id { - Some(egid) => egid, - None => get_or_create_editgroup(editor_id, conn)? - }; - - let current: WorkIdentRow = work_ident::table.find(id).first(conn)?; - if current.is_live != true { - // TODO: what if isn't live? 4xx not 5xx - bail!("can't delete an entity that doesn't exist yet"); - } - if current.rev_id.is_none() { - // TODO: what if it's already deleted? 4xx not 5xx - bail!("entity was already deleted"); - } - let edit: WorkEditRow = insert_into(work_edit::table) - .values(( - work_edit::editgroup_id.eq(editgroup_id), - work_edit::ident_id.eq(id), - work_edit::rev_id.eq(None::), - work_edit::redirect_id.eq(None::), - work_edit::prev_rev.eq(current.rev_id), - //work_edit::extra_json.eq(extra), - )) - .get_result(conn)?; + let edit_context = make_edit_context(conn, editgroup_id.map(|u| FatCatId::from_uuid(&u)))?; + let edit = WorkEntity::db_delete(conn, &edit_context, FatCatId::from_uuid(id))?; edit.into_model() } diff --git a/rust/src/database_entity_crud.rs b/rust/src/database_entity_crud.rs new file mode 100644 index 00000000..d532d166 --- /dev/null +++ b/rust/src/database_entity_crud.rs @@ -0,0 +1,250 @@ + +use diesel::prelude::*; +use diesel::{self, insert_into}; +use database_schema::*; +use database_models::*; +use errors::*; +use fatcat_api::models::*; +use api_helpers::{FatCatId, DbConn}; +use uuid::Uuid; +use std::marker::Sized; +use std::str::FromStr; +use serde_json; + +pub struct EditContext { + pub editor_id: FatCatId, + pub editgroup_id: FatCatId, + pub extra_json: Option, +} + +/* One goal here is to abstract the non-entity-specific bits into generic traits or functions, + * instead of macros. + * + * Notably: + * + * db_get + * db_get_rev + * db_create + * db_create_batch + * db_update + * db_delete + * db_get_history + * + * For now, these will probably be macros, until we can level up our trait/generics foo. + */ + +// Associated Type, not parametric +pub trait EntityCrud where Self: Sized { + type EditRow; // EntityEditRow + type IdentRow; // EntityIdentRow + type RevRow; + + fn parse_editgroup_id(&self) -> Result>; + + // Generic Methods + fn db_get(conn: &DbConn, ident: FatCatId) -> Result; + fn db_get_rev(conn: &DbConn, rev_id: Uuid) -> Result; + fn db_create(&self, conn: &DbConn, edit_context: &EditContext) -> Result; + fn db_create_batch(conn: &DbConn, edit_context: &EditContext, models: &[Self]) -> Result>; + fn db_update(&self, conn: &DbConn, edit_context: &EditContext, ident: FatCatId) -> Result; + fn db_delete(conn: &DbConn, edit_context: &EditContext, ident: FatCatId) -> Result; + fn db_get_history(conn: &DbConn, ident: FatCatId, limit: Option) -> Result>; + + // Entity-specific Methods + fn db_from_row(conn: &DbConn, rev_row: Self::RevRow, ident_row: Option) -> Result; + fn db_insert_rev(&self, conn: &DbConn, edit_context: &EditContext) -> Result; +} + +// TODO: this could be a separate trait on all entities? +macro_rules! generic_parse_editgroup_id{ + () => { + fn parse_editgroup_id(&self) -> Result> { + match &self.editgroup_id { + Some(s) => Ok(Some(FatCatId::from_str(&s)?)), + None => Ok(None), + } + } + } +} + +macro_rules! generic_db_get { + ($ident_table: ident, $rev_table: ident) => { + fn db_get(conn: &DbConn, ident: FatCatId) -> Result { + let (ident, rev): (Self::IdentRow, Self::RevRow) = $ident_table::table + .find(ident.to_uuid()) + .inner_join($rev_table::table) + .first(conn)?; + + Self::db_from_row(conn, rev, Some(ident)) + } + } +} + +macro_rules! generic_db_get_rev { + ($rev_table: ident) => { + fn db_get_rev(conn: &DbConn, rev_id: Uuid) -> Result { + let rev = $rev_table::table + .find(rev_id) + .first(conn)?; + + Self::db_from_row(conn, rev, None) + } + } +} + +macro_rules! generic_db_create_batch { + () => { + fn db_create_batch(conn: &DbConn, edit_context: &EditContext, models: &[Self]) -> Result> { + let mut ret: Vec = vec![]; + for entity in models { + ret.push(entity.db_create(conn, edit_context)?); + } + Ok(ret) + } + } +} + +macro_rules! generic_db_delete { + ($ident_table: ident, $edit_table:ident) => { + fn db_delete(conn: &DbConn, edit_context: &EditContext, ident: FatCatId) -> Result { + + let current: Self::IdentRow = $ident_table::table.find(ident.to_uuid()).first(conn)?; + if current.is_live != true { + // TODO: what if isn't live? 4xx not 5xx + bail!("can't delete an entity that doesn't exist yet"); + } + if current.rev_id.is_none() { + // TODO: what if it's already deleted? 4xx not 5xx + bail!("entity was already deleted"); + } + let edit: Self::EditRow = insert_into($edit_table::table) + .values(( + $edit_table::editgroup_id.eq(edit_context.editgroup_id.to_uuid()), + $edit_table::ident_id.eq(ident.to_uuid()), + $edit_table::rev_id.eq(None::), + $edit_table::redirect_id.eq(None::), + $edit_table::prev_rev.eq(current.rev_id), + $edit_table::extra_json.eq(&edit_context.extra_json), + )) + .get_result(conn)?; + + Ok(edit) + } + } +} + +macro_rules! generic_db_get_history { + // TODO: only actually need edit table? and maybe not that? + ($edit_table:ident) => { + fn db_get_history(conn: &DbConn, ident: FatCatId, limit: Option) -> Result> { + let limit = limit.unwrap_or(50); // XXX: make a static + + let rows: Vec<(EditgroupRow, ChangelogRow, Self::EditRow)> = editgroup::table + .inner_join(changelog::table) + .inner_join($edit_table::table) + .filter($edit_table::ident_id.eq(ident.to_uuid())) + .order(changelog::id.desc()) + .limit(limit) + .get_results(conn)?; + + let history: Vec = rows.into_iter() + .map(|(eg_row, cl_row, e_row)| EntityHistoryEntry { + edit: e_row.into_model().expect("edit row to model"), + editgroup: eg_row.into_model_partial(), + changelog_entry: cl_row.into_model(), + }) + .collect(); + Ok(history) + } + } +} + +impl EntityCrud for WorkEntity { + type EditRow = WorkEditRow; + type IdentRow = WorkIdentRow; + type RevRow = WorkRevRow; + + generic_parse_editgroup_id!(); + generic_db_get!(work_ident, work_rev); + generic_db_get_rev!(work_rev); + generic_db_create_batch!(); + generic_db_delete!(work_ident, work_edit); + generic_db_get_history!(work_edit); + + fn db_create(&self, conn: &DbConn, edit_context: &EditContext) -> Result { + + // TODO: refactor to use insert_rev + let edit: WorkEditRow = + diesel::sql_query( + "WITH rev AS ( INSERT INTO work_rev (extra_json) + VALUES ($1) + RETURNING id ), + ident AS ( INSERT INTO work_ident (rev_id) + VALUES ((SELECT rev.id FROM rev)) + RETURNING id ) + INSERT INTO work_edit (editgroup_id, ident_id, rev_id) VALUES + ($2, (SELECT ident.id FROM ident), (SELECT rev.id FROM rev)) + RETURNING *", + ).bind::, _>(&self.extra) + .bind::(edit_context.editgroup_id.to_uuid()) + .get_result(conn)?; + + Ok(edit) + } + + fn db_update(&self, conn: &DbConn, edit_context: &EditContext, ident: FatCatId) -> Result { + // TODO: refactor this into a check on WorkIdentRow + let current: WorkIdentRow = work_ident::table.find(ident.to_uuid()).first(conn)?; + if current.is_live != true { + // TODO: what if isn't live? 4xx not 5xx + bail!("can't delete an entity that doesn't exist yet"); + } + if current.rev_id.is_none() { + // TODO: what if it's already deleted? 4xx not 5xx + bail!("entity was already deleted"); + } + + let edit: WorkEditRow = + diesel::sql_query( + "WITH rev AS ( INSERT INTO work_rev (extra_json) + VALUES ($1) + RETURNING id ), + INSERT INTO work_edit (editgroup_id, ident_id, rev_id, prev_rev) VALUES + ($2, $3, (SELECT rev.id FROM rev), $4) + RETURNING *", + ).bind::, _>(&self.extra) + .bind::(edit_context.editgroup_id.to_uuid()) + .bind::(ident.to_uuid()) + .bind::(current.rev_id.unwrap()) + .get_result(conn)?; + + Ok(edit) + } + + + fn db_from_row(conn: &DbConn, rev_row: Self::RevRow, ident_row: Option) -> Result { + + let (state, ident_id, redirect_id) = match ident_row { + Some(i) => ( + Some(i.state().unwrap().shortname()), + Some(FatCatId::from_uuid(&i.id).to_string()), + i.redirect_id.map(|u| FatCatId::from_uuid(&u).to_string()), + ), + None => (None, None, None), + }; + + Ok(WorkEntity { + state: state, + ident: ident_id, + revision: Some(rev_row.id.to_string()), + redirect: redirect_id, + editgroup_id: None, + extra: rev_row.extra_json, + }) + } + + fn db_insert_rev(&self, conn: &DbConn, edit_context: &EditContext) -> Result { + unimplemented!() + } +} + diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 12ce0973..a938486b 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -27,6 +27,7 @@ pub mod api_server; pub mod api_wrappers; pub mod database_models; pub mod database_schema; +pub mod database_entity_crud; mod errors { // Create the Error, ErrorKind, ResultExt, and Result types -- cgit v1.2.3