From 4fe69134ac8520dc16f4a84c6bc3b2e9ba36a482 Mon Sep 17 00:00:00 2001 From: Leo Valais Date: Thu, 26 Dec 2024 17:26:34 +0100 Subject: [PATCH] editoast: change Model trait Create to use editoast_models::Error Signed-off-by: Leo Valais --- editoast/Cargo.lock | 1 + editoast/Cargo.toml | 3 +- .../src/model/codegen/create_impl.rs | 2 +- editoast/editoast_models/Cargo.toml | 1 + editoast/editoast_models/src/model/error.rs | 49 ++++++++++++++++++- editoast/openapi.yaml | 20 ++++++++ editoast/src/models/prelude/create.rs | 9 ++-- .../src/models/stdcm_search_environment.rs | 12 +++-- editoast/src/views/rolling_stock.rs | 35 ++++++++++++- editoast/src/views/temporary_speed_limits.rs | 36 +++++++++----- editoast/src/views/work_schedules.rs | 39 +++++++++------ front/public/locales/fr/errors.json | 1 + 12 files changed, 170 insertions(+), 38 deletions(-) diff --git a/editoast/Cargo.lock b/editoast/Cargo.lock index e162f3bb31b..bf126e7da05 100644 --- a/editoast/Cargo.lock +++ b/editoast/Cargo.lock @@ -1453,6 +1453,7 @@ dependencies = [ "opentelemetry-semantic-conventions", "postgis_diesel", "postgres-openssl", + "regex", "thiserror 2.0.9", "tokio", "tokio-postgres", diff --git a/editoast/Cargo.toml b/editoast/Cargo.toml index a1fc2fffdcf..140fb65ca5d 100644 --- a/editoast/Cargo.toml +++ b/editoast/Cargo.toml @@ -63,6 +63,7 @@ postgres-openssl = "0.5.0" pretty_assertions = "1.4.1" rand = "0.8.5" rangemap = "1.5.1" +regex = "1.11.1" # 0.12.0 to 0.12.4 have weird timeout issues https://github.com/seanmonstar/reqwest/issues/2283 # This bug was introduced between 0.12.0 and 0.12.3. reqwest = { version = "0.11.27", features = ["json"] } @@ -162,7 +163,7 @@ redis = { version = "0.27", default-features = false, features = [ "tokio-comp", "tokio-native-tls-comp", ] } -regex = "1.11.1" +regex.workspace = true reqwest.workspace = true serde = { workspace = true, features = ["derive"] } serde_json.workspace = true diff --git a/editoast/editoast_derive/src/model/codegen/create_impl.rs b/editoast/editoast_derive/src/model/codegen/create_impl.rs index 26c9e47a173..95ed97538d9 100644 --- a/editoast/editoast_derive/src/model/codegen/create_impl.rs +++ b/editoast/editoast_derive/src/model/codegen/create_impl.rs @@ -28,7 +28,7 @@ impl ToTokens for CreateImpl { async fn create( self, conn: &mut editoast_models::DbConnection, - ) -> crate::error::Result<#model> { + ) -> std::result::Result<#model, editoast_models::model::Error> { use diesel_async::RunQueryDsl; use #table_mod::dsl; use std::ops::DerefMut; diff --git a/editoast/editoast_models/Cargo.toml b/editoast/editoast_models/Cargo.toml index c98c61521f6..63157406b03 100644 --- a/editoast/editoast_models/Cargo.toml +++ b/editoast/editoast_models/Cargo.toml @@ -17,6 +17,7 @@ openssl.workspace = true opentelemetry-semantic-conventions.workspace = true postgis_diesel.workspace = true postgres-openssl.workspace = true +regex.workspace = true thiserror.workspace = true tokio.workspace = true tokio-postgres.workspace = true diff --git a/editoast/editoast_models/src/model/error.rs b/editoast/editoast_models/src/model/error.rs index cf59cb7335c..1ba087c9196 100644 --- a/editoast/editoast_models/src/model/error.rs +++ b/editoast/editoast_models/src/model/error.rs @@ -1,11 +1,58 @@ +use std::sync::LazyLock; + +use diesel::result::DatabaseErrorKind; +use regex::Regex; + #[derive(Debug, thiserror::Error)] pub enum Error { + #[error("unique constraint violation: \"{constraint}\"")] + UniqueViolation { constraint: String }, + #[error("check constraint violation on relation \"{relation}\": \"{constraint}\"")] + CheckViolation { + relation: String, + constraint: String, + }, #[error(transparent)] DatabaseError(#[from] crate::DatabaseError), } impl From for Error { fn from(e: diesel::result::Error) -> Self { - Self::DatabaseError(e.into()) + match &e { + diesel::result::Error::DatabaseError(DatabaseErrorKind::UniqueViolation, inner) => { + static RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"duplicate key value violates unique constraint "([0-9a-zA-Z_-]+)""#, + ) + .unwrap() + }); + if let Some(captures) = RE.captures((*inner).message()) { + Self::UniqueViolation { + constraint: captures.get(1).unwrap().as_str().to_string(), + } + } else { + tracing::error!(?RE, %e, "failed to parse PostgreSQL error message"); + Self::DatabaseError(e.into()) + } + } + diesel::result::Error::DatabaseError(DatabaseErrorKind::CheckViolation, inner) => { + static RE: LazyLock = LazyLock::new(|| { + Regex::new( + r#"new row for relation "([0-9a-zA-Z_-]+)" violates check constraint "([0-9a-zA-Z_-]+)""#, + ) + .unwrap() + }); + if let Some(captures) = RE.captures((*inner).message()) { + Self::CheckViolation { + relation: captures.get(1).unwrap().as_str().to_string(), + constraint: captures.get(2).unwrap().as_str().to_string(), + } + } else { + tracing::error!(?RE, %e, "failed to parse PostgreSQL error message"); + Self::DatabaseError(e.into()) + } + } + _ => Self::DatabaseError(e.into()), + } } } diff --git a/editoast/openapi.yaml b/editoast/openapi.yaml index 2eb5e425cfc..28bef2adf42 100644 --- a/editoast/openapi.yaml +++ b/editoast/openapi.yaml @@ -4763,6 +4763,7 @@ components: - $ref: '#/components/schemas/EditoastStudyErrorDatabase' - $ref: '#/components/schemas/EditoastStudyErrorNotFound' - $ref: '#/components/schemas/EditoastStudyErrorStartDateAfterEndDate' + - $ref: '#/components/schemas/EditoastTemporarySpeedLimitErrorDatabase' - $ref: '#/components/schemas/EditoastTemporarySpeedLimitErrorNameAlreadyUsed' - $ref: '#/components/schemas/EditoastTimetableErrorDatabase' - $ref: '#/components/schemas/EditoastTimetableErrorInfraNotFound' @@ -6102,6 +6103,25 @@ components: type: string enum: - editoast:study:StartDateAfterEndDate + EditoastTemporarySpeedLimitErrorDatabase: + type: object + required: + - type + - status + - message + properties: + context: + type: object + message: + type: string + status: + type: integer + enum: + - 500 + type: + type: string + enum: + - editoast:temporary_speed_limit:Database EditoastTemporarySpeedLimitErrorNameAlreadyUsed: type: object required: diff --git a/editoast/src/models/prelude/create.rs b/editoast/src/models/prelude/create.rs index dfa5d583bf9..e4509d4e6ae 100644 --- a/editoast/src/models/prelude/create.rs +++ b/editoast/src/models/prelude/create.rs @@ -1,5 +1,6 @@ use std::fmt::Debug; +use editoast_models::model; use editoast_models::DbConnection; use crate::error::EditoastError; @@ -13,17 +14,17 @@ use crate::error::Result; pub trait Create: Sized { /// Creates a new row in the database with the values of the changeset and /// returns the created model instance - async fn create(self, conn: &mut DbConnection) -> Result; + async fn create(self, conn: &mut DbConnection) -> std::result::Result; /// Just like [Create::create] but discards the error if any and returns `Err(fail())` instead - async fn create_or_fail E + Send>( + async fn create_or_fail, F: FnOnce() -> E + Send>( self, conn: &'async_trait mut DbConnection, fail: F, - ) -> Result { + ) -> std::result::Result { match self.create(conn).await { Ok(obj) => Ok(obj), - Err(_) => Err(fail().into()), + Err(_) => Err(fail()), } } } diff --git a/editoast/src/models/stdcm_search_environment.rs b/editoast/src/models/stdcm_search_environment.rs index 7d837807e9f..8dad4c7f103 100644 --- a/editoast/src/models/stdcm_search_environment.rs +++ b/editoast/src/models/stdcm_search_environment.rs @@ -3,13 +3,14 @@ use diesel::ExpressionMethods; use diesel::QueryDsl; use diesel_async::RunQueryDsl; use editoast_derive::Model; +use editoast_models::model; use editoast_models::DbConnection; use serde::Serialize; use std::ops::DerefMut; +use std::result::Result; use utoipa::ToSchema; -use crate::error::Result; -use crate::models::prelude::{Create, Row}; +use crate::models::prelude::*; #[cfg(test)] use serde::Deserialize; @@ -47,7 +48,7 @@ impl StdcmSearchEnvironment { .ok() } - pub async fn delete_all(conn: &mut DbConnection) -> Result<()> { + pub async fn delete_all(conn: &mut DbConnection) -> Result<(), model::Error> { use editoast_models::tables::stdcm_search_environment::dsl::*; diesel::delete(stdcm_search_environment) .execute(conn.write().await.deref_mut()) @@ -57,7 +58,10 @@ impl StdcmSearchEnvironment { } impl StdcmSearchEnvironmentChangeset { - pub async fn overwrite(self, conn: &mut DbConnection) -> Result { + pub async fn overwrite( + self, + conn: &mut DbConnection, + ) -> Result { StdcmSearchEnvironment::delete_all(conn).await?; self.create(conn).await } diff --git a/editoast/src/views/rolling_stock.rs b/editoast/src/views/rolling_stock.rs index 43a1293e2dd..e6942b2bacb 100644 --- a/editoast/src/views/rolling_stock.rs +++ b/editoast/src/views/rolling_stock.rs @@ -2,6 +2,7 @@ pub mod form; pub mod light; mod towed; +use editoast_models::model; pub use form::RollingStockForm; use std::io::Cursor; @@ -147,7 +148,7 @@ pub enum RollingStockError { #[error(transparent)] #[editoast_error(status = 500)] - Database(#[from] editoast_models::model::Error), + Database(model::Error), } #[derive(Debug, Error)] @@ -178,6 +179,7 @@ pub(crate) enum LiveryMultipartError { }, } +// Still used to parse the error of `Update` and `Save`. Will be removed soon. pub fn map_diesel_error(e: InternalError, name: impl AsRef) -> InternalError { if e.message .contains(r#"duplicate key value violates unique constraint "rolling_stock_name_key""#) @@ -190,6 +192,35 @@ pub fn map_diesel_error(e: InternalError, name: impl AsRef) -> InternalErro } } +impl From for RollingStockError { + fn from(e: model::Error) -> Self { + match e { + model::Error::UniqueViolation { constraint } + if constraint == "rolling_stock_name_key" => + { + Self::NameAlreadyUsed { + name: String::default(), + } + } + model::Error::CheckViolation { constraint, .. } + if constraint == "base_power_class_null_or_non_empty" => + { + Self::BasePowerClassEmpty + } + e => Self::Database(e), + } + } +} + +impl RollingStockError { + fn with_name(self, name: String) -> Self { + match self { + Self::NameAlreadyUsed { .. } => Self::NameAlreadyUsed { name }, + e => e, + } + } +} + #[derive(IntoParams)] #[allow(unused)] pub struct RollingStockIdParam { @@ -334,7 +365,7 @@ async fn create( .version(0) .create(conn) .await - .map_err(|e| map_diesel_error(e, rolling_stock_name))?; + .map_err(|e| RollingStockError::from(e).with_name(rolling_stock_name))?; Ok(Json(rolling_stock)) } diff --git a/editoast/src/views/temporary_speed_limits.rs b/editoast/src/views/temporary_speed_limits.rs index 0b0f1d27571..57bc78dd849 100644 --- a/editoast/src/views/temporary_speed_limits.rs +++ b/editoast/src/views/temporary_speed_limits.rs @@ -4,6 +4,7 @@ use axum::Extension; use chrono::NaiveDateTime; use chrono::Utc; use editoast_derive::EditoastError; +use editoast_models::model; use editoast_models::DbConnectionPoolV2; use editoast_schemas::infra::DirectionalTrackRange; use serde::de::Error as SerdeError; @@ -12,7 +13,6 @@ use std::result::Result as StdResult; use thiserror::Error; use utoipa::ToSchema; -use crate::error::InternalError; use crate::error::Result; use crate::models::temporary_speed_limits::TemporarySpeedLimit; use crate::models::temporary_speed_limits::TemporarySpeedLimitGroup; @@ -111,18 +111,32 @@ enum TemporarySpeedLimitError { #[error("Name '{name}' already used")] #[editoast_error(status = 400)] NameAlreadyUsed { name: String }, + #[error(transparent)] + #[editoast_error(status = 500)] + Database(model::Error), } -fn map_diesel_error(e: InternalError, name: impl AsRef) -> InternalError { - if e.message.contains( - r#"duplicate key value violates unique constraint "temporary_speed_limit_group_name_key""#, - ) { - TemporarySpeedLimitError::NameAlreadyUsed { - name: name.as_ref().to_string(), +impl From for TemporarySpeedLimitError { + fn from(e: model::Error) -> Self { + match e { + model::Error::UniqueViolation { constraint } + if constraint == "temporary_speed_limit_group_name_key" => + { + Self::NameAlreadyUsed { + name: "unknown".to_string(), + } + } + e => Self::Database(e), + } + } +} + +impl TemporarySpeedLimitError { + fn with_name(self, name: String) -> Self { + match self { + Self::NameAlreadyUsed { .. } => Self::NameAlreadyUsed { name }, + e => e, } - .into() - } else { - e } } @@ -158,7 +172,7 @@ async fn create_temporary_speed_limit_group( .creation_date(Utc::now().naive_utc()) .create(conn) .await - .map_err(|e| map_diesel_error(e, speed_limit_group_name))?; + .map_err(|e| TemporarySpeedLimitError::from(e).with_name(speed_limit_group_name))?; // Create the speed limits let speed_limits_changesets = speed_limits diff --git a/editoast/src/views/work_schedules.rs b/editoast/src/views/work_schedules.rs index 8dfc5217145..8ba3e7bf15c 100644 --- a/editoast/src/views/work_schedules.rs +++ b/editoast/src/views/work_schedules.rs @@ -1,6 +1,5 @@ use super::pagination::PaginatedList; use crate::core::pathfinding::TrackRange as CoreTrackRange; -use crate::error::InternalError; use crate::error::Result; use crate::models::prelude::*; use crate::models::work_schedules::WorkSchedule; @@ -24,6 +23,7 @@ use chrono::Utc; use derivative::Derivative; use editoast_authz::BuiltinRole; use editoast_derive::EditoastError; +use editoast_models::model; use editoast_models::DbConnectionPoolV2; use editoast_schemas::infra::Direction; use editoast_schemas::infra::TrackRange; @@ -75,19 +75,30 @@ enum WorkScheduleError { WorkScheduleGroupNotFound { id: i64 }, #[error(transparent)] #[editoast_error(status = 500)] - Database(#[from] editoast_models::model::Error), + Database(model::Error), } -pub fn map_diesel_error(e: InternalError, name: impl AsRef) -> InternalError { - if e.message.contains( - r#"duplicate key value violates unique constraint "work_schedule_group_name_key""#, - ) { - WorkScheduleError::NameAlreadyUsed { - name: name.as_ref().to_string(), +impl From for WorkScheduleError { + fn from(e: model::Error) -> Self { + match e { + model::Error::UniqueViolation { constraint } + if constraint == "work_schedule_group_name_key" => + { + Self::NameAlreadyUsed { + name: String::default(), + } + } + e => Self::Database(e), + } + } +} + +impl WorkScheduleError { + fn with_name(self, name: String) -> Self { + match self { + Self::NameAlreadyUsed { .. } => Self::NameAlreadyUsed { name }, + e => e, } - .into() - } else { - e } } @@ -339,8 +350,8 @@ async fn create_group( .name(group_name.clone()) .creation_date(Utc::now()) .create(conn) - .await; - let work_schedule_group = work_schedule_group.map_err(|e| map_diesel_error(e, group_name))?; + .await + .map_err(|e| WorkScheduleError::from(e).with_name(group_name))?; Ok(Json(WorkScheduleGroupCreateResponse { work_schedule_group_id: work_schedule_group.id, })) @@ -611,7 +622,7 @@ pub mod tests { let work_schedule_response = app .fetch(request) .assert_status(StatusCode::BAD_REQUEST) - .json_into::(); + .json_into::(); assert_eq!( &work_schedule_response.error_type, diff --git a/front/public/locales/fr/errors.json b/front/public/locales/fr/errors.json index fcf54c4c7f3..498d39bd5b2 100644 --- a/front/public/locales/fr/errors.json +++ b/front/public/locales/fr/errors.json @@ -234,6 +234,7 @@ "WorkScheduleGroupNotFound": "Le groupe de planche travaux avec l'id {{id}} n'existe pas" }, "temporary_speed_limit": { + "Database": "Erreur interne (base de données)", "NameAlreadyUsed": "Un groupe de limites temporaires de vitesse avec le nom '{{name}} existe déjà" }, "app_health": {