Merge torrust#293: Do not allow empty or duplicate tag names

7fedf15 feat!: [torrust#289] do not allow duplicate tags (Jose Celano)
01a7d2e feat!: [torrust#289] do not allow empty tag names (Jose Celano)

Pull request description:

  ### Do not allow empty tag names

  It was allowed to use empty strings like "" or " " for a tag name.

  After merging this PR, it will not be allowed anymore.

  If there are some empty names in the database, they are not renamed.

  A migration to rename empty names was not added because there can be more than one tag, for example:

  - ""
  - " "
  - "  "
  - Etcetera

  We could have generated names like "no tag 1", "no tag 2", but it's not likely that admins have created empty tags.

  ### Do not allow duplicate tag names

  It was allowed to use empty strings like "" or " " for a tag name. And It was also allowed to have duplicate tags.

  After merging this PR, it will not be allowed anymore.

  If there are duplicate tag names, the duplication is removed by appending the tag id as a suffix to the tag name.

ACKs for top commit:
  josecelano:
    ACK 7fedf15

Tree-SHA512: 79200e9048a500a5b268a32db43e64f071042dee94f4200d6f7bfc3199120d1b30f2a2fd07d4d1b8ed01a438d4687261cc988cb9c372716579094be4ad43100c

migrations/mysql/20230914155441_torrust_no_duplicate_tags.sql

@@ -0,0 +1,12 @@
+-- Step 1 & 2: Identify and update the duplicate names
+UPDATE torrust_torrent_tags
+JOIN (
+    SELECT name
+    FROM torrust_torrent_tags
+    GROUP BY name
+    HAVING COUNT(*) > 1
+) AS DuplicateNames ON torrust_torrent_tags.name = DuplicateNames.name
+SET torrust_torrent_tags.name = CONCAT(torrust_torrent_tags.name, '_', torrust_torrent_tags.tag_id);
+
+-- Step 3: Add the UNIQUE constraint to the name column
+ALTER TABLE torrust_torrent_tags ADD UNIQUE (name);

migrations/sqlite3/20230914155441_torrust_no_duplicate_tags.sql

@@ -0,0 +1,13 @@
+-- Step 1: Identify and update the duplicate names
+WITH DuplicateNames AS (
+    SELECT name
+    FROM torrust_torrent_tags
+    GROUP BY name
+    HAVING COUNT(*) > 1
+)
+UPDATE torrust_torrent_tags
+SET name = name || '_' || tag_id
+WHERE name IN (SELECT name FROM DuplicateNames);
+
+-- Step 2: Create a UNIQUE index on the name column
+CREATE UNIQUE INDEX idx_unique_name ON torrust_torrent_tags(name);

src/databases/database.rs

@@ -287,7 +287,7 @@ pub trait Database: Sync + Send {
     async fn update_torrent_category(&self, torrent_id: i64, category_id: CategoryId) -> Result<(), Error>;
 
     /// Add a new tag.
-    async fn add_tag(&self, name: &str) -> Result<(), Error>;
+    async fn insert_tag_and_get_id(&self, name: &str) -> Result<i64, Error>;
 
     /// Delete a tag.
     async fn delete_tag(&self, tag_id: TagId) -> Result<(), Error>;

src/databases/mysql.rs

@@ -804,13 +804,23 @@ impl Database for Mysql {
             })
     }
 
-    async fn add_tag(&self, name: &str) -> Result<(), database::Error> {
+    async fn insert_tag_and_get_id(&self, name: &str) -> Result<i64, database::Error> {
         query("INSERT INTO torrust_torrent_tags (name) VALUES (?)")
             .bind(name)
             .execute(&self.pool)
             .await
-            .map(|_| ())
-            .map_err(|err| database::Error::ErrorWithText(err.to_string()))
+            .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::TagAlreadyExists
+                    } else {
+                        database::Error::Error
+                    }
+                }
+                _ => database::Error::Error,
+            })
     }
 
     async fn delete_tag(&self, tag_id: TagId) -> Result<(), database::Error> {

src/databases/sqlite.rs

@@ -794,13 +794,23 @@ impl Database for Sqlite {
             })
     }
 
-    async fn add_tag(&self, name: &str) -> Result<(), database::Error> {
+    async fn insert_tag_and_get_id(&self, tag_name: &str) -> Result<i64, database::Error> {
         query("INSERT INTO torrust_torrent_tags (name) VALUES (?)")
-            .bind(name)
+            .bind(tag_name)
             .execute(&self.pool)
             .await
-            .map(|_| ())
-            .map_err(|err| database::Error::ErrorWithText(err.to_string()))
+            .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::TagAlreadyExists
+                    } else {
+                        database::Error::Error
+                    }
+                }
+                _ => database::Error::Error,
+            })
     }
 
     async fn delete_tag(&self, tag_id: TagId) -> Result<(), database::Error> {

src/errors.rs

@@ -136,6 +136,9 @@ pub enum ServiceError {
     #[display(fmt = "Tag already exists.")]
     TagAlreadyExists,
 
+    #[display(fmt = "Tag name cannot be empty.")]
+    TagNameEmpty,
+
     #[display(fmt = "Category not found.")]
     CategoryNotFound,
 
@@ -240,6 +243,7 @@ pub fn http_status_code_for_service_error(error: &ServiceError) -> StatusCode {
         ServiceError::TrackerOffline => StatusCode::INTERNAL_SERVER_ERROR,
         ServiceError::CategoryNameEmpty => StatusCode::BAD_REQUEST,
         ServiceError::CategoryAlreadyExists => StatusCode::BAD_REQUEST,
+        ServiceError::TagNameEmpty => StatusCode::BAD_REQUEST,
         ServiceError::TagAlreadyExists => StatusCode::BAD_REQUEST,
         ServiceError::InternalServerError => StatusCode::INTERNAL_SERVER_ERROR,
         ServiceError::EmailMissing => StatusCode::NOT_FOUND,

src/services/tag.rs

@@ -29,7 +29,7 @@ impl Service {
     ///
     /// * The user does not have the required permissions.
     /// * There is a database error.
-    pub async fn add_tag(&self, tag_name: &str, user_id: &UserId) -> Result<(), ServiceError> {
+    pub async fn add_tag(&self, tag_name: &str, user_id: &UserId) -> Result<TagId, ServiceError> {
         let user = self.user_repository.get_compact(user_id).await?;
 
         // Check if user is administrator
@@ -38,8 +38,14 @@ impl Service {
             return Err(ServiceError::Unauthorized);
         }
 
-        match self.tag_repository.add(tag_name).await {
-            Ok(()) => Ok(()),
+        let trimmed_name = tag_name.trim();
+
+        if trimmed_name.is_empty() {
+            return Err(ServiceError::TagNameEmpty);
+        }
+
+        match self.tag_repository.add(trimmed_name).await {
+            Ok(id) => Ok(id),
             Err(e) => match e {
                 DatabaseError::TagAlreadyExists => Err(ServiceError::TagAlreadyExists),
                 _ => Err(ServiceError::DatabaseError),
@@ -89,8 +95,8 @@ impl DbTagRepository {
     /// # Errors
     ///
     /// It returns an error if there is a database error.
-    pub async fn add(&self, tag_name: &str) -> Result<(), Error> {
-        self.database.add_tag(tag_name).await
+    pub async fn add(&self, tag_name: &str) -> Result<TagId, Error> {
+        self.database.insert_tag_and_get_id(tag_name).await
     }
 
     /// It returns all the tags.

src/upgrades/from_v1_0_0_to_v2_0_0/databases/sqlite_v2_0_0.rs

@@ -280,7 +280,7 @@ impl SqliteDatabaseV2_0_0 {
             query(&format!("DELETE FROM {table};"))
                 .execute(&self.pool)
                 .await
-                .expect("table {table} should be deleted");
+                .unwrap_or_else(|_| panic!("table {table} should be deleted"));
         }
 
         Ok(())

src/web/api/v1/contexts/tag/handlers.rs

@@ -52,7 +52,7 @@ pub async fn add_handler(
     };
 
     match app_data.tag_service.add_tag(&add_tag_form.name, &user_id).await {
-        Ok(()) => added_tag(&add_tag_form.name).into_response(),
+        Ok(_) => added_tag(&add_tag_form.name).into_response(),
         Err(error) => error.into_response(),
     }
 }

tests/e2e/web/api/v1/contexts/tag/contract.rs

@@ -104,9 +104,7 @@ async fn it_should_allow_admins_to_add_new_tags() {
 }
 
 #[tokio::test]
-async fn it_should_allow_adding_duplicated_tags() {
-    // code-review: is this an intended behavior?
-
+async fn it_should_not_allow_adding_duplicated_tags() {
     let mut env = TestEnv::new();
     env.start(api::Version::V1).await;
 
@@ -117,19 +115,22 @@ async fn it_should_allow_adding_duplicated_tags() {
 
     // Try to add the same tag again
     let response = add_tag(&random_tag_name, &env).await;
-    assert_eq!(response.status, 200);
+
+    assert_eq!(response.status, 400);
 }
 
 #[tokio::test]
-async fn it_should_allow_adding_a_tag_with_an_empty_name() {
-    // code-review: is this an intended behavior?
-
+async fn it_should_not_allow_adding_a_tag_with_an_empty_name() {
     let mut env = TestEnv::new();
     env.start(api::Version::V1).await;
 
-    let empty_tag_name = String::new();
-    let response = add_tag(&empty_tag_name, &env).await;
-    assert_eq!(response.status, 200);
+    let invalid_tag_names = vec![String::new(), " ".to_string()];
+
+    for invalid_name in invalid_tag_names {
+        let response = add_tag(&invalid_name, &env).await;
+
+        assert_eq!(response.status, 400);
+    }
 }
 
 #[tokio::test]

tests/e2e/web/api/v1/contexts/user/steps.rs

@@ -20,7 +20,7 @@ pub async fn new_logged_in_admin(env: &TestEnv) -> LoggedInUserData {
     let user_profile = database
         .get_user_profile_from_username(&user.username)
         .await
-        .unwrap_or_else(|_| panic!("user {user:#?} should  have a profile."));
+        .unwrap_or_else(|_| panic!("no user profile for the user: {user:#?}."));
 
     database.grant_admin_role(user_profile.user_id).await.unwrap();