many redirect implementations

Probably should have split this commit up, it's huge:

- accept the state of "redirect to a deletion", where redirect_id is
  Some but rev_id is None. call this a "redirect"; previously this was
  an invalid state.
- GET for a deleted entity returns a 200 and a stub entity, not a 404
- to PUT a redirect, or to "revert" an entity to point at a specific
  pre-existing revision, PUT a stub entity. things are getting messy
  here... to detect this state, ensure the 'state' field is blank/none
  (this is for API usage ergonomics, where results from a GET are often
  re-used in a PUT or even POST)
- rustfmt
- maybe even more small tweaks along the way? mystery meat!

Tests are in python, not rust (and a future commit)

5 files changed, 272 insertions, 38 deletions

diff --git a/rust/src/api_entity_crud.rs b/rust/src/api_entity_crud.rs
index ccbae26d..605c27ed 100644
--- a/rust/src/api_entity_crud.rs
+++ b/rust/src/api_entity_crud.rs
@@ -44,6 +44,7 @@ where
     type RevRow;
 
     // Generic Methods
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self>;
     fn db_get(conn: &DbConn, ident: FatCatId, hide: HideFlags) -> Result<Self>;
     fn db_get_rev(conn: &DbConn, rev_id: Uuid, hide: HideFlags) -> Result<Self>;
     fn db_expand(&mut self, conn: &DbConn, expand: ExpandFlags) -> Result<()>;
@@ -88,12 +89,26 @@ where
 macro_rules! generic_db_get {
     ($ident_table:ident, $rev_table:ident) => {
         fn db_get(conn: &DbConn, ident: FatCatId, hide: HideFlags) -> Result<Self> {
-            let (ident, rev): (Self::IdentRow, Self::RevRow) = $ident_table::table
+            let res: Option<(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), hide)
+                .first(conn)
+                .optional()?;
+
+            match res {
+                Some((ident, rev)) => {
+                    Self::db_from_row(conn, rev, Some(ident), hide)
+                },
+                None => {
+                    // return a stub (deleted) entity if it's just deleted state
+                    let ident_row: Self::IdentRow = $ident_table::table.find(ident.to_uuid()).first(conn)?;
+                    if ident_row.rev_id.is_none() {
+                        Self::from_deleted_row(ident_row)
+                    } else {
+                        bail!("unexpected condition: entity ident/rev join failed, yet row isn't in deleted state")
+                    }
+                },
+            }
         }
     };
 }
@@ -183,23 +198,77 @@ macro_rules! generic_db_update {
     ($ident_table: ident, $edit_table: ident) => {
         fn db_update(&self, conn: &DbConn, edit_context: &EditContext, ident: FatCatId) -> Result<Self::EditRow> {
             let current: Self::IdentRow = $ident_table::table.find(ident.to_uuid()).first(conn)?;
+            let no_redirect: Option<Uuid> = None;
             // TODO: is this actually true? or should we allow updates in the same editgroup?
             if current.is_live != true {
                 return Err(ErrorKind::InvalidEntityStateTransform(
                     "can't update an entity that doesn't exist yet".to_string()).into());
             }
 
+            if self.state.is_none() {
+                // special case: redirect to another entity
+                if let Some(ref redirect_ident) = self.redirect {
+                    let redirect_ident = FatCatId::from_str(&redirect_ident)?.to_uuid();
+                    if Some(redirect_ident) == current.redirect_id {
+                        return Err(ErrorKind::OtherBadRequest(
+                            "redundantly redirecting entity to it's current target currently isn't supported".to_string()).into());
+                    }
+                    // TODO: if we get a diesel not-found here, should be a special error response?
+                    let target: Self::IdentRow = $ident_table::table.find(redirect_ident).first(conn)?;
+                    if target.is_live == false {
+                        // there is no race condition on this check because WIP -> is_live=true is
+                        // a one-way operation
+                        return Err(ErrorKind::OtherBadRequest(
+                            "attempted to redirect to a WIP entity".to_string()).into());
+                    }
+                    // Note: there is a condition where the target is already a redirect, but we
+                    // don't handle that here because the state of the redirect could change before
+                    // we accept this editgroup
+                    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(target.rev_id),
+                            $edit_table::redirect_id.eq(redirect_ident),
+                            $edit_table::prev_rev.eq(current.rev_id),
+                            $edit_table::extra_json.eq(&self.edit_extra),
+                        ))
+                        .get_result(conn)?;
+                    return Ok(edit)
+                }
+                // special case: revert to point to an existing revision
+                if let Some(ref rev_id) = self.revision {
+                    let rev_id = Uuid::from_str(&rev_id)?;
+                    if Some(rev_id) == current.rev_id {
+                        return Err(ErrorKind::OtherBadRequest(
+                            "reverted entity to it's current state; this isn't currently supported".to_string()).into());
+                    }
+                    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(&rev_id),
+                            $edit_table::redirect_id.eq(no_redirect),
+                            $edit_table::prev_rev.eq(current.rev_id),
+                            $edit_table::extra_json.eq(&self.edit_extra),
+                        ))
+                        .get_result(conn)?;
+                    return Ok(edit)
+                }
+            }
+
+            // regular insert/update
             let rev_id = self.db_insert_rev(conn)?;
             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(&rev_id),
-                    $edit_table::prev_rev.eq(current.rev_id.unwrap()),
-                    $edit_table::extra_json.eq(&self.extra),
+                    $edit_table::redirect_id.eq(no_redirect),
+                    $edit_table::prev_rev.eq(current.rev_id),
+                    $edit_table::extra_json.eq(&self.edit_extra),
                 ))
                 .get_result(conn)?;
-
             Ok(edit)
         }
     }
@@ -215,11 +284,16 @@ macro_rules! generic_db_delete {
             let current: Self::IdentRow = $ident_table::table.find(ident.to_uuid()).first(conn)?;
             if current.is_live != true {
                 return Err(ErrorKind::InvalidEntityStateTransform(
-                    "can't update an entity that doesn't exist yet; delete edit object instead".to_string()).into());
+                    "can't update an entity that doesn't exist yet; delete edit object instead"
+                        .to_string(),
+                )
+                .into());
             }
             if current.rev_id.is_none() {
                 return Err(ErrorKind::InvalidEntityStateTransform(
-                    "entity was already deleted".to_string()).into());
+                    "entity was already deleted".to_string(),
+                )
+                .into());
             }
             let edit: Self::EditRow = insert_into($edit_table::table)
                 .values((
@@ -290,7 +364,9 @@ macro_rules! generic_db_delete_edit {
                 .get_results(conn)?;
             if accepted_rows.len() != 0 {
                 return Err(ErrorKind::EditgroupAlreadyAccepted(
-                    "attempted to delete an already accepted edit".to_string()).into());
+                    "attempted to delete an already accepted edit".to_string(),
+                )
+                .into());
             }
             diesel::delete($edit_table::table.filter($edit_table::id.eq(edit_id))).execute(conn)?;
             Ok(())
@@ -366,6 +442,29 @@ macro_rules! generic_db_accept_edits_batch {
             ))
             .bind::<diesel::sql_types::Uuid, _>(editgroup_id.to_uuid())
             .execute(conn)?;
+            // NOTE: all the following can be skipped for accepts that are all inserts (which I
+            // guess we only know for batch inserts with auto-accept?)
+
+            // update any/all redirects for updated entities
+            let _redir_count = diesel::sql_query(format!(
+                "
+                    UPDATE {entity}_ident
+                    SET
+                        rev_id = {entity}_edit.rev_id
+                    FROM {entity}_edit
+                    WHERE
+                        {entity}_ident.redirect_id = {entity}_edit.ident_id
+                        AND {entity}_edit.editgroup_id = $1",
+                entity = $entity_name_str
+            ))
+            .bind::<diesel::sql_types::Uuid, _>(editgroup_id.to_uuid())
+            .execute(conn)?;
+            // TODO: backwards multiple redirect: if we redirected any entities to targets in this
+            // edit, then we need to update any redirects to *current* entities to *target*
+            // entities
+            // TODO: forward multiple redirect: if we redirected any entities to targets in this
+            // edit, and those targets are themselves redirects, we should point to the "final"
+            // targets
             Ok(count as u64)
         }
     };
@@ -439,6 +538,29 @@ impl EntityCrud for ContainerEntity {
     generic_db_accept_edits_batch!("container");
     generic_db_insert_rev!();
 
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self> {
+        if ident_row.rev_id.is_some() {
+            bail!("called from_deleted_row with a non-deleted-state row")
+        }
+
+        Ok(ContainerEntity {
+            issnl: None,
+            wikidata_qid: None,
+            publisher: None,
+            name: None,
+            abbrev: None,
+            coden: None,
+            state: Some(ident_row.state().unwrap().shortname()),
+            ident: Some(FatCatId::from_uuid(&ident_row.id).to_string()),
+            revision: ident_row.rev_id.map(|u| u.to_string()),
+            redirect: ident_row
+                .redirect_id
+                .map(|u| FatCatId::from_uuid(&u).to_string()),
+            extra: None,
+            edit_extra: None,
+        })
+    }
+
     fn db_from_row(
         _conn: &DbConn,
         rev_row: Self::RevRow,
@@ -523,6 +645,28 @@ impl EntityCrud for CreatorEntity {
     generic_db_accept_edits_batch!("creator");
     generic_db_insert_rev!();
 
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self> {
+        if ident_row.rev_id.is_some() {
+            bail!("called from_deleted_row with a non-deleted-state row")
+        }
+
+        Ok(CreatorEntity {
+            extra: None,
+            edit_extra: None,
+            display_name: None,
+            given_name: None,
+            surname: None,
+            orcid: None,
+            wikidata_qid: None,
+            state: Some(ident_row.state().unwrap().shortname()),
+            ident: Some(FatCatId::from_uuid(&ident_row.id).to_string()),
+            revision: ident_row.rev_id.map(|u| u.to_string()),
+            redirect: ident_row
+                .redirect_id
+                .map(|u| FatCatId::from_uuid(&u).to_string()),
+        })
+    }
+
     fn db_from_row(
         _conn: &DbConn,
         rev_row: Self::RevRow,
@@ -604,6 +748,30 @@ impl EntityCrud for FileEntity {
     generic_db_accept_edits_batch!("file");
     generic_db_insert_rev!();
 
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self> {
+        if ident_row.rev_id.is_some() {
+            bail!("called from_deleted_row with a non-deleted-state row")
+        }
+
+        Ok(FileEntity {
+            sha1: None,
+            sha256: None,
+            md5: None,
+            size: None,
+            urls: None,
+            mimetype: None,
+            releases: None,
+            state: Some(ident_row.state().unwrap().shortname()),
+            ident: Some(FatCatId::from_uuid(&ident_row.id).to_string()),
+            revision: ident_row.rev_id.map(|u| u.to_string()),
+            redirect: ident_row
+                .redirect_id
+                .map(|u| FatCatId::from_uuid(&u).to_string()),
+            extra: None,
+            edit_extra: None,
+        })
+    }
+
     fn db_from_row(
         conn: &DbConn,
         rev_row: Self::RevRow,
@@ -742,6 +910,46 @@ impl EntityCrud for ReleaseEntity {
     generic_db_accept_edits_batch!("release");
     generic_db_insert_rev!();
 
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self> {
+        if ident_row.rev_id.is_some() {
+            bail!("called from_deleted_row with a non-deleted-state row")
+        }
+
+        Ok(ReleaseEntity {
+            title: None,
+            release_type: None,
+            release_status: None,
+            release_date: None,
+            doi: None,
+            pmid: None,
+            pmcid: None,
+            isbn13: None,
+            core_id: None,
+            wikidata_qid: None,
+            volume: None,
+            issue: None,
+            pages: None,
+            files: None,
+            container: None,
+            container_id: None,
+            publisher: None,
+            language: None,
+            work_id: None,
+            refs: None,
+            contribs: None,
+            abstracts: None,
+
+            state: Some(ident_row.state().unwrap().shortname()),
+            ident: Some(FatCatId::from_uuid(&ident_row.id).to_string()),
+            revision: ident_row.rev_id.map(|u| u.to_string()),
+            redirect: ident_row
+                .redirect_id
+                .map(|u| FatCatId::from_uuid(&u).to_string()),
+            extra: None,
+            edit_extra: None,
+        })
+    }
+
     fn db_expand(&mut self, conn: &DbConn, expand: ExpandFlags) -> Result<()> {
         if expand.files {
             let ident = match &self.ident {
@@ -1166,6 +1374,23 @@ impl EntityCrud for WorkEntity {
     generic_db_accept_edits_batch!("work");
     generic_db_insert_rev!();
 
+    fn from_deleted_row(ident_row: Self::IdentRow) -> Result<Self> {
+        if ident_row.rev_id.is_some() {
+            bail!("called from_deleted_row with a non-deleted-state row")
+        }
+
+        Ok(WorkEntity {
+            state: Some(ident_row.state().unwrap().shortname()),
+            ident: Some(FatCatId::from_uuid(&ident_row.id).to_string()),
+            revision: ident_row.rev_id.map(|u| u.to_string()),
+            redirect: ident_row
+                .redirect_id
+                .map(|u| FatCatId::from_uuid(&u).to_string()),
+            extra: None,
+            edit_extra: None,
+        })
+    }
+
     fn db_from_row(
         _conn: &DbConn,
         rev_row: Self::RevRow,
diff --git a/rust/src/api_server.rs b/rust/src/api_server.rs
index 2278f64f..0961194b 100644
--- a/rust/src/api_server.rs
+++ b/rust/src/api_server.rs
@@ -53,7 +53,11 @@ pub struct Server {
     pub db_pool: ConnectionPool,
 }
 
-pub fn get_release_files(id: FatCatId, hide_flags: HideFlags, conn: &DbConn) -> Result<Vec<FileEntity>> {
+pub fn get_release_files(
+    id: FatCatId,
+    hide_flags: HideFlags,
+    conn: &DbConn,
+) -> Result<Vec<FileEntity>> {
     let rows: Vec<(FileRevRow, FileIdentRow, FileReleaseRow)> = file_rev::table
         .inner_join(file_ident::table)
         .inner_join(file_release::table)
diff --git a/rust/src/api_wrappers.rs b/rust/src/api_wrappers.rs
index a2357fe1..c8f6b390 100644
--- a/rust/src/api_wrappers.rs
+++ b/rust/src/api_wrappers.rs
@@ -664,30 +664,31 @@ impl Api for Server {
             Some(param) => HideFlags::from_str(&param).unwrap(),
         };
         // No transaction for GET
-        let ret = match self.lookup_file_handler(&md5, &sha1, &sha256, expand_flags, hide_flags, &conn) {
-            Ok(entity) => LookupFileResponse::FoundEntity(entity),
-            Err(Error(ErrorKind::Diesel(::diesel::result::Error::NotFound), _)) => {
-                LookupFileResponse::NotFound(ErrorResponse {
-                    message: format!("Not found: {:?} / {:?} / {:?}", md5, sha1, sha256),
-                })
-            }
-            Err(Error(ErrorKind::MalformedExternalId(e), _)) => {
-                LookupFileResponse::BadRequest(ErrorResponse {
-                    message: e.to_string(),
-                })
-            }
-            Err(Error(ErrorKind::MissingOrMultipleExternalId(e), _)) => {
-                LookupFileResponse::BadRequest(ErrorResponse {
-                    message: e.to_string(),
-                })
-            }
-            Err(e) => {
-                error!("{}", e);
-                LookupFileResponse::BadRequest(ErrorResponse {
-                    message: e.to_string(),
-                })
-            }
-        };
+        let ret =
+            match self.lookup_file_handler(&md5, &sha1, &sha256, expand_flags, hide_flags, &conn) {
+                Ok(entity) => LookupFileResponse::FoundEntity(entity),
+                Err(Error(ErrorKind::Diesel(::diesel::result::Error::NotFound), _)) => {
+                    LookupFileResponse::NotFound(ErrorResponse {
+                        message: format!("Not found: {:?} / {:?} / {:?}", md5, sha1, sha256),
+                    })
+                }
+                Err(Error(ErrorKind::MalformedExternalId(e), _)) => {
+                    LookupFileResponse::BadRequest(ErrorResponse {
+                        message: e.to_string(),
+                    })
+                }
+                Err(Error(ErrorKind::MissingOrMultipleExternalId(e), _)) => {
+                    LookupFileResponse::BadRequest(ErrorResponse {
+                        message: e.to_string(),
+                    })
+                }
+                Err(e) => {
+                    error!("{}", e);
+                    LookupFileResponse::BadRequest(ErrorResponse {
+                        message: e.to_string(),
+                    })
+                }
+            };
         Box::new(futures::done(Ok(ret)))
     }
 
diff --git a/rust/src/database_models.rs b/rust/src/database_models.rs
index a7076e09..82088f28 100644
--- a/rust/src/database_models.rs
+++ b/rust/src/database_models.rs
@@ -14,7 +14,7 @@ use uuid::Uuid;
 pub enum EntityState {
     WorkInProgress,
     Active(Uuid),
-    Redirect(Uuid, Uuid),
+    Redirect(Uuid, Option<Uuid>),
     Deleted,
 }
 
@@ -111,7 +111,7 @@ macro_rules! entity_structs {
                 }
                 match (self.redirect_id, self.rev_id) {
                     (None, None) => Ok(EntityState::Deleted),
-                    (Some(redir), Some(rev)) => Ok(EntityState::Redirect(redir, rev)),
+                    (Some(redir), rev) => Ok(EntityState::Redirect(redir, rev)),
                     (None, Some(rev)) => Ok(EntityState::Active(rev)),
                     _ => bail!("Invalid EntityIdentRow state"),
                 }
diff --git a/rust/src/lib.rs b/rust/src/lib.rs
index ab293fa9..b7f1817a 100644
--- a/rust/src/lib.rs
+++ b/rust/src/lib.rs
@@ -1,5 +1,5 @@
 #![allow(proc_macro_derive_resolution_fallback)]
-#![recursion_limit="128"]
+#![recursion_limit = "128"]
 
 extern crate chrono;
 extern crate fatcat_api_spec;
@@ -67,6 +67,10 @@ pub mod errors {
                 description("Invalid Entity State Transform")
                 display("tried to mutate an entity which was not in an appropriate state: {}", message)
             }
+            OtherBadRequest(message: String) {
+                description("catch-all error for bad or unallowed requests")
+                display("broke a constraint or made an otherwise invalid request: {}", message)
+            }
         }
     }
 }