Move logic to avoid duplicate categories from database to services #253
Description
I'm working on changing the pipeline to run the E2E test with MySQL, not only with SQLite. See #252
I found a bug because the endpoint to add new categories was not returning the same HTPP status code. When using MySQL it returns 500 and for SQLite, it returns 400. The right behaviour is the 400.
The reason why it was returning 500 it's that we do not detect the error because the error message is different:
SQLite:
async fn insert_category_and_get_id(&self, category_name: &str) -> Result<i64, database::Error> {
query("INSERT INTO torrust_categories (name) VALUES (?)")
.bind(category_name)
.execute(&self.pool)
.await
.map(|v| v.last_insert_rowid())
.map_err(|e| match e {
sqlx::Error::Database(err) => {
if err.message().contains("UNIQUE") {
database::Error::CategoryAlreadyExists
} else {
database::Error::Error
}
}
_ => database::Error::Error,
})
}MySQL (the fixed version):
async fn insert_category_and_get_id(&self, category_name: &str) -> Result<i64, database::Error> {
query("INSERT INTO torrust_categories (name) VALUES (?)")
.bind(category_name)
.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) => {
if err.message().contains("Duplicate entry") {
// Example error message when you try to insert a duplicate category:
// Error: Duplicate entry 'category name SAMPLE_NAME' for key 'torrust_categories.name'
database::Error::CategoryAlreadyExists
} else {
database::Error::Error
}
}
_ => database::Error::Error,
})
}- We should not rely on concrete error messages.
- We should move the logic to services here. We can check if the category already exists before adding it. We can keep the DB constraint or remove it. If we remove it, maybe we need an aggregate for the whole category list. For now, I would keep it, but I would add the guard clause to the service to make it more explicit in the code.