From d0e9ea523093f19ba1d1b0e4ffb446fb8a205152 Mon Sep 17 00:00:00 2001 From: Brendan O'Connell Date: Wed, 25 Sep 2024 16:22:53 +0200 Subject: [PATCH] Refactored update_canonical_location into update function in crud --- thoth-api/src/graphql/model.rs | 36 +------ thoth-api/src/model/location/crud.rs | 144 ++++++++++++++++++++------- 2 files changed, 111 insertions(+), 69 deletions(-) diff --git a/thoth-api/src/graphql/model.rs b/thoth-api/src/graphql/model.rs index 8eeffb8b8..ad2e068e8 100644 --- a/thoth-api/src/graphql/model.rs +++ b/thoth-api/src/graphql/model.rs @@ -1991,39 +1991,9 @@ impl MutationRoot { if data.canonical { data.canonical_record_complete(&context.db)?; } - // if changes to a location don't change its canonical or non-canonical status, perform a regular update. - if data.canonical == location.canonical { - location - .update(&context.db, &data, &account_id) - .map_err(|e| e.into()) - // trying to change canonical location to non-canonical results in error. - } else if location.canonical && (data.canonical != location.canonical) { - Err(ThothError::CanonicalLocationError.into()) - // if user changes a non-canonical location to canonical, perform two simultaneous updates: - // change the old canonical location to non-canonical, and change the old non-canonical location to canonical - } else { - let canonical_location = data.get_canonical_location(&context.db); - - let final_canonical_location = match canonical_location { - Ok(location) => location, - Err(e) => { - return Err(ThothError::from(e).into()); - } - }; - - let old_canonical_location = PatchLocation { - location_id: final_canonical_location.location_id, - publication_id: final_canonical_location.publication_id, - landing_page: final_canonical_location.landing_page.clone(), - full_text_url: final_canonical_location.full_text_url.clone(), - location_platform: final_canonical_location.location_platform.clone(), - canonical: false, - }; - - location - .update_canonical_location(&context.db, data, &old_canonical_location, old_canonical_location.location_id, &account_id) - .map_err(|e| e.into()) - } + location + .update(&context.db, &data, &account_id) + .map_err(|e| e.into()) } #[graphql(description = "Update an existing price with the specified values")] diff --git a/thoth-api/src/model/location/crud.rs b/thoth-api/src/model/location/crud.rs index 8092432d4..c95250099 100644 --- a/thoth-api/src/model/location/crud.rs +++ b/thoth-api/src/model/location/crud.rs @@ -2,11 +2,11 @@ use super::{ Location, LocationField, LocationHistory, LocationOrderBy, LocationPlatform, NewLocation, NewLocationHistory, PatchLocation, }; +use crate::db_insert; use crate::graphql::utils::Direction; use crate::model::{Crud, DbInsert, HistoryEntry}; use crate::schema::{location, location_history}; -use crate::{crud_methods, db_insert}; -use diesel::{ExpressionMethods, QueryDsl, RunQueryDsl}; +use diesel::{Connection, ExpressionMethods, QueryDsl, RunQueryDsl}; use thoth_errors::{ThothError, ThothResult}; use uuid::Uuid; @@ -126,7 +126,109 @@ impl Crud for Location { crate::model::publication::Publication::from_id(db, &self.publication_id)?.publisher_id(db) } - crud_methods!(location::table, location::dsl::location); + // `crud_methods!` cannot be used for update(), because + // we sometimes need to execute multiple statements in the same transaction + // for changing a non-canonical location to canonical. + // These functions recreate the `crud_methods!` logic. + fn from_id(db: &crate::db::PgPool, entity_id: &Uuid) -> ThothResult { + let mut connection = db.get()?; + match location::table + .find(entity_id) + .get_result::(&mut connection) + { + Ok(t) => Ok(t), + Err(e) => Err(ThothError::from(e)), + } + } + + fn create(db: &crate::db::PgPool, data: &NewLocation) -> ThothResult { + let mut connection = db.get()?; + + connection.transaction(|connection| { + diesel::insert_into(location::table) + .values(data) + .get_result::(connection) + .map_err(|e| e.into()) + }) + } + + fn update( + &self, + db: &crate::db::PgPool, + data: &PatchLocation, + account_id: &Uuid, + ) -> ThothResult { + // if changes to a location don't change its canonical or non-canonical status, perform a single update to that location. + if &data.canonical == &self.canonical { + let mut connection = db.get()?; + connection.transaction(|connection| { + match diesel::update(location::table.find(&self.location_id)) + .set(data) + .get_result::(connection) + { + // On success, create a new history table entry. + Ok(l) => match self.new_history_entry(account_id).insert(connection) { + Ok(_) => Ok(l), + Err(e) => Err(e), + }, + Err(e) => Err(ThothError::from(e)), + } + }) + // trying to change canonical location to non-canonical results in error. + } else if self.canonical && (data.canonical != self.canonical) { + Err(ThothError::CanonicalLocationError.into()) + // if user changes a non-canonical location to canonical, perform two simultaneous updates: + // change the old canonical location to non-canonical, and change the old non-canonical location to canonical + } else { + let canonical_location = self.get_canonical_location(db); + let final_canonical_location = match canonical_location { + Ok(location) => location, + Err(e) => { + return Err(ThothError::from(e).into()); + } + }; + + let old_canonical_location = PatchLocation { + location_id: final_canonical_location.location_id, + publication_id: final_canonical_location.publication_id, + landing_page: final_canonical_location.landing_page.clone(), + full_text_url: final_canonical_location.full_text_url.clone(), + location_platform: final_canonical_location.location_platform.clone(), + canonical: false, + }; + let mut connection = db.get()?; + connection.transaction(|connection| { + // Update the currently canonical location to non-canonical + diesel::update(location::table.find(&old_canonical_location.location_id.clone())) + .set(old_canonical_location) + .execute(connection)?; + match diesel::update(location::table.find(&self.location_id)) + // Update the data from the currently non-canonical location to canonical + .set(data) + .get_result::(connection) + { + // On success, create a new history table entry. + Ok(t) => match self.new_history_entry(account_id).insert(connection) { + Ok(_) => Ok(t), + Err(e) => Err(e), + }, + Err(e) => Err(ThothError::from(e)), + } + }) + } + } + + fn delete(self, db: &crate::db::PgPool) -> ThothResult { + let mut connection = db.get()?; + connection.transaction(|connection| { + match diesel::delete(location::table.find(self.location_id)).execute(connection) { + Ok(_) => Ok(self), + Err(e) => Err(ThothError::from(e)), + } + }) + } + + } impl HistoryEntry for Location { @@ -182,38 +284,6 @@ impl NewLocation { } impl Location { - pub fn update_canonical_location( - &self, - db: &crate::db::PgPool, - data: PatchLocation, - old_canonical_location: &PatchLocation, - old_canonical_location_id: Uuid, - account_id: &Uuid, - ) -> ThothResult { - use crate::diesel::Connection; - let mut connection = db.get()?; - connection.transaction(|connection| { - // Update the currently canonical location to non-canonical - diesel::update(location::table.find(old_canonical_location_id)) - .set(old_canonical_location) - .execute(connection)?; - match diesel::update(location::table.find(&self.location_id)) - // Update the data from the currently non-canonical location to canonical - .set(data) - .get_result::(connection) - { - // On success, create a new history table entry. - Ok(t) => match self.new_history_entry(account_id).insert(connection) { - Ok(_) => Ok(t), - Err(e) => Err(e), - }, - Err(e) => Err(ThothError::from(e)), - } - }) - } -} - -impl PatchLocation { pub fn get_canonical_location( &self, db: &crate::db::PgPool @@ -226,7 +296,9 @@ impl PatchLocation { .expect("Error loading canonical location for publication"); Ok(canonical_location) } - +} + +impl PatchLocation { pub fn canonical_record_complete(&self, db: &crate::db::PgPool) -> ThothResult<()> { location_canonical_record_complete( self.publication_id,