Skip to content

Commit

Permalink
feat!: [#289] do not allow duplicate tags
Browse files Browse the repository at this point in the history
Duplicate tag names were allowed. This introduce unique constrains.
If there were duyplicate tags they are updated appeding the id as a suffix.
For example, having two tags:

```json
[
  {
    tag_id: 1,
    name: "fractal"
  },
  {
    tag_id: 2,
    name: "fractal"
  }
]
```

They would be renamed to:

```json
[
  {
    tag_id: 1,
    name: "fractal_1"
  },
  {
    tag_id: 2,
    name: "fractal_2"
  }
]
```

From now on, duplicate tags are not allowed.
  • Loading branch information
josecelano committed Sep 14, 2023
1 parent 01a7d2e commit 7fedf15
Show file tree
Hide file tree
Showing 10 changed files with 63 additions and 19 deletions.
12 changes: 12 additions & 0 deletions migrations/mysql/20230914155441_torrust_no_duplicate_tags.sql
Original file line number Diff line number Diff line change
@@ -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);
13 changes: 13 additions & 0 deletions migrations/sqlite3/20230914155441_torrust_no_duplicate_tags.sql
Original file line number Diff line number Diff line change
@@ -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);
2 changes: 1 addition & 1 deletion src/databases/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Expand Down
16 changes: 13 additions & 3 deletions src/databases/mysql.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
18 changes: 14 additions & 4 deletions src/databases/sqlite.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand Down
8 changes: 4 additions & 4 deletions src/services/tag.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -45,7 +45,7 @@ impl Service {
}

match self.tag_repository.add(trimmed_name).await {
Ok(()) => Ok(()),
Ok(id) => Ok(id),
Err(e) => match e {
DatabaseError::TagAlreadyExists => Err(ServiceError::TagAlreadyExists),
_ => Err(ServiceError::DatabaseError),
Expand Down Expand Up @@ -95,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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down
2 changes: 1 addition & 1 deletion src/web/api/v1/contexts/tag/handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}
Expand Down
7 changes: 3 additions & 4 deletions tests/e2e/web/api/v1/contexts/tag/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -117,7 +115,8 @@ 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]
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/web/api/v1/contexts/user/steps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down

0 comments on commit 7fedf15

Please sign in to comment.