Skip to content

Commit

Permalink
Merge #376: fix: rework error handling for add_category
Browse files Browse the repository at this point in the history
5dc0017 fix: add comments (pcarles)
5f96cce fix: update doc (pcarles)
c535652 fix: rework error handling for add_category Fixes #253 (pcarles)

Pull request description:

  Hello !

  This PR fixes [#253](#253).
  I think that we can keep the unique constraint in the DB on top of this change as proposed by @josecelano in the original issue

  Following this change, I think that we should move some of the error logic that is inside the databases crate to the services one.
  All of these errors should not be raised at database level but rather at the service level:
  - `database::Error::UsernameTaken`
  - `database::Error::EmailTaken`
  - `database::Error::TagAlreadyExists`
  - `database::Error::TorrentAlreadyExists`
  - `database::Error::TorrentTitleAlreadyExists`

  This will permit to avoid using database error message parsing which is implementation specific and error prone (like [here](https://github.com/torrust/torrust-index/blob/41c80f359490a156e61bc5670874708f350ca75b/src/databases/sqlite.rs#L612C43-L612C43) or [here](https://github.com/torrust/torrust-index/blob/41c80f359490a156e61bc5670874708f350ca75b/src/databases/mysql.rs#L831))

  I'd be happy to open another PR to rework the listed errors if you aggree with this new approach
  That's my first contribution here, and I am also pretty new to the Rust ecosystem so don't hesitate to tell me if anything's wrong

ACKs for top commit:
  josecelano:
    ACK 5dc0017

Tree-SHA512: 6eea5cb5e29db8fb987bb8aafd31e9e1cbc8d6f1260c04b63268af8e2d1d63078c737f6a501925ef78d4ccb322549f69da5e4f434d8af30f6a5a1ff09768e029
  • Loading branch information
josecelano committed Nov 13, 2023
2 parents 41c80f3 + 5dc0017 commit fe9e9f2
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 37 deletions.
1 change: 0 additions & 1 deletion src/databases/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ pub enum Error {
UsernameTaken,
EmailTaken,
UserNotFound,
CategoryAlreadyExists,
CategoryNotFound,
TagAlreadyExists,
TagNotFound,
Expand Down
12 changes: 1 addition & 11 deletions src/databases/mysql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,17 +249,7 @@ impl Database for Mysql {
.execute(&self.pool)
.await
.map(|v| i64::try_from(v.last_insert_id()).expect("last ID is larger than i64"))
.map_err(|e| match e {
sqlx::Error::Database(err) => {
log::error!("DB error: {:?}", err);
if err.message().contains("Duplicate entry") && err.message().contains("name") {
database::Error::CategoryAlreadyExists
} else {
database::Error::Error
}
}
_ => database::Error::Error,
})
.map_err(|_| database::Error::Error)
}

async fn get_category_from_id(&self, category_id: i64) -> Result<Category, database::Error> {
Expand Down
12 changes: 1 addition & 11 deletions src/databases/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,17 +239,7 @@ impl Database for Sqlite {
.execute(&self.pool)
.await
.map(|v| v.last_insert_rowid())
.map_err(|e| match e {
sqlx::Error::Database(err) => {
log::error!("DB error: {:?}", err);
if err.message().contains("UNIQUE") && err.message().contains("name") {
database::Error::CategoryAlreadyExists
} else {
database::Error::Error
}
}
_ => database::Error::Error,
})
.map_err(|_| database::Error::Error)
}

async fn get_category_from_id(&self, category_id: i64) -> Result<Category, database::Error> {
Expand Down
1 change: 0 additions & 1 deletion src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ pub fn map_database_error_to_service_error(error: &database::Error) -> ServiceEr
database::Error::UsernameTaken => ServiceError::UsernameTaken,
database::Error::EmailTaken => ServiceError::EmailTaken,
database::Error::UserNotFound => ServiceError::UserNotFound,
database::Error::CategoryAlreadyExists => ServiceError::CategoryAlreadyExists,
database::Error::CategoryNotFound => ServiceError::InvalidCategory,
database::Error::TagAlreadyExists => ServiceError::TagAlreadyExists,
database::Error::TagNotFound => ServiceError::InvalidTag,
Expand Down
14 changes: 11 additions & 3 deletions src/services/category.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ impl Service {
/// It returns an error if:
///
/// * The user does not have the required permissions.
/// * The category name is empty.
/// * The category already exists.
/// * There is a database error.
pub async fn add_category(&self, category_name: &str, user_id: &UserId) -> Result<i64, ServiceError> {
let user = self.user_repository.get_compact(user_id).await?;
Expand All @@ -44,10 +46,16 @@ impl Service {
return Err(ServiceError::CategoryNameEmpty);
}

match self.category_repository.add(trimmed_name).await {
Ok(id) => Ok(id),
// Try to get the category by name to check if it already exists
match self.category_repository.get_by_name(trimmed_name).await {
// Return ServiceError::CategoryAlreadyExists if the category exists
Ok(_) => Err(ServiceError::CategoryAlreadyExists),
Err(e) => match e {
DatabaseError::CategoryAlreadyExists => Err(ServiceError::CategoryAlreadyExists),
// Otherwise try to create it
DatabaseError::CategoryNotFound => match self.category_repository.add(trimmed_name).await {
Ok(id) => Ok(id),
Err(_) => Err(ServiceError::DatabaseError),
},
_ => Err(ServiceError::DatabaseError),
},
}
Expand Down
11 changes: 1 addition & 10 deletions src/upgrades/from_v1_0_0_to_v2_0_0/databases/sqlite_v2_0_0.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,16 +116,7 @@ impl SqliteDatabaseV2_0_0 {
.execute(&self.pool)
.await
.map(|v| v.last_insert_rowid())
.map_err(|e| match e {
sqlx::Error::Database(err) => {
if err.message().contains("UNIQUE") && err.message().contains("name") {
database::Error::CategoryAlreadyExists
} else {
database::Error::Error
}
}
_ => database::Error::Error,
})
.map_err(|_| database::Error::Error)
}

pub async fn insert_category(&self, category: &CategoryRecordV2) -> Result<i64, sqlx::Error> {
Expand Down

0 comments on commit fe9e9f2

Please sign in to comment.