From 1407c219f9f83d2a7cac4e86a373d760205ea48b Mon Sep 17 00:00:00 2001 From: amirRamirfatahi <30861397+amirRamirfatahi@users.noreply.github.com> Date: Wed, 22 Jan 2025 18:43:32 -0500 Subject: [PATCH] Fix: user search duplicate bug (#260) * Fix user search * fix file urls serde --- src/db/kv/index/json.rs | 18 +++--- src/models/file/details.rs | 24 ++++---- src/models/user/details.rs | 5 +- src/models/user/search.rs | 118 ++++++++++++++++++++++++++++++++++++- 4 files changed, 142 insertions(+), 23 deletions(-) diff --git a/src/db/kv/index/json.rs b/src/db/kv/index/json.rs index cff370cb..597d1e99 100644 --- a/src/db/kv/index/json.rs +++ b/src/db/kv/index/json.rs @@ -358,22 +358,24 @@ pub async fn get_multiple( let results: Vec> = if indexed_values.is_empty() { (0..keys.len()).map(|_| None).collect() } else { - deserialize_values(indexed_values) + deserialize_values(indexed_values)? }; Ok(results) } // Helper function to deserialize JSON strings to Vec> -fn deserialize_values(values: Vec>) -> Vec> { +fn deserialize_values( + values: Vec>, +) -> Result>, DynError> { values .into_iter() - .map(|opt| { - opt.and_then(|value_str| { - serde_json::from_str::>(&value_str) - .ok() - .and_then(|vec| vec.into_iter().next()) - }) + .map(|value_str| match value_str { + Some(value) => { + let value: Vec = serde_json::from_str(&value)?; + Ok(value.into_iter().next()) + } + None => Ok(None), }) .collect() } diff --git a/src/models/file/details.rs b/src/models/file/details.rs index 787fb459..dab210ab 100644 --- a/src/models/file/details.rs +++ b/src/models/file/details.rs @@ -16,26 +16,24 @@ pub struct FileUrls { } mod json_string { - use serde::{self, Deserialize, Deserializer, Serialize, Serializer}; + use serde::{self, Deserialize, Deserializer, Serializer}; - // Deserialize function: convert the JSON string into a struct - pub fn deserialize<'de, D, T>(deserializer: D) -> Result + pub fn serialize(value: &T, serializer: S) -> Result where - D: Deserializer<'de>, - T: Deserialize<'de>, + S: Serializer, + T: serde::Serialize, { - let json_str: &'de str = <&str>::deserialize(deserializer)?; - serde_json::from_str(json_str).map_err(serde::de::Error::custom) + let json_string = serde_json::to_string(value).map_err(serde::ser::Error::custom)?; + serializer.serialize_str(&json_string) } - // Serialize function: convert the struct back into a JSON string - pub fn serialize(value: &T, serializer: S) -> Result + pub fn deserialize<'de, D, T>(deserializer: D) -> Result where - S: Serializer, - T: Serialize, + D: Deserializer<'de>, + T: serde::de::DeserializeOwned, { - let json_str = serde_json::to_string(value).map_err(serde::ser::Error::custom)?; - serializer.serialize_str(&json_str) + let json_string = String::deserialize(deserializer)?; + serde_json::from_str(&json_string).map_err(serde::de::Error::custom) } } diff --git a/src/models/user/details.rs b/src/models/user/details.rs index 2d806ac3..540afff1 100644 --- a/src/models/user/details.rs +++ b/src/models/user/details.rs @@ -71,7 +71,10 @@ where .map_err(serde::de::Error::custom)?; Ok(Some(urls)) } - _ => Err(serde::de::Error::custom("Expected a string or an array")), + serde_json::Value::Null => Ok(None), + _ => Err(serde::de::Error::custom( + "Expected either a string, an array or null", + )), } } diff --git a/src/models/user/search.rs b/src/models/user/search.rs index 5c941e02..5030da23 100644 --- a/src/models/user/search.rs +++ b/src/models/user/search.rs @@ -1,6 +1,6 @@ use super::UserDetails; -use crate::types::DynError; use crate::RedisOps; +use crate::{models::traits::Collection, types::DynError}; use serde::{Deserialize, Serialize}; use utoipa::ToSchema; @@ -57,6 +57,16 @@ impl UserSearch { /// /// This method takes a list of `UserDetails` and adds them all to the sorted set at once. pub async fn put_to_index(details_list: &[&UserDetails]) -> Result<(), DynError> { + // ensure existing records are deleted + Self::delete_existing_records( + details_list + .iter() + .map(|details| details.id.0.as_str()) + .collect::>() + .as_slice(), + ) + .await?; + // Collect all the `username:user_id` pairs and their corresponding scores let mut items: Vec<(f64, String)> = Vec::with_capacity(details_list.len()); @@ -87,4 +97,110 @@ impl UserSearch { ) .await } + + async fn delete_existing_records(user_ids: &[&str]) -> Result<(), DynError> { + if user_ids.is_empty() { + return Ok(()); + } + let mut records_to_delete: Vec = Vec::with_capacity(user_ids.len()); + let keys = user_ids + .iter() + .map(|&id| vec![id]) + .collect::>>(); + let users = UserDetails::get_from_index(keys.iter().map(|item| item.as_slice()).collect()) + .await? + .into_iter() + .flatten() + .collect::>(); + for user_id in user_ids { + let existing_username = users + .iter() + .find(|user| user.id.0 == *user_id) + .map(|user| user.name.to_lowercase()); + if let Some(existing_record) = existing_username { + let search_key = format!("{}:{}", existing_record, user_id); + records_to_delete.push(search_key); + } + } + + Self::remove_from_index_sorted_set( + &USER_NAME_KEY_PARTS, + records_to_delete + .iter() + .map(|item| item.as_str()) + .collect::>() + .as_slice(), + ) + .await?; + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use chrono::Utc; + + use crate::{ + models::{ + traits::Collection, + user::{UserDetails, UserSearch}, + }, + setup, + types::{DynError, PubkyId}, + Config, RedisOps, + }; + + #[tokio_shared_rt::test(shared)] + async fn test_put_to_index_no_duplicates() -> Result<(), DynError> { + let config = Config::from_env(); + setup(&config).await; + // Test that the `put_to_index` method does not add duplicate records to the index + // when called with the same `UserDetails` multiple times. + + // Create a `UserDetails` object + let user_id = "user_id"; + let user_name = "Test User Duplicate"; + let user_details = UserDetails { + id: PubkyId(user_id.to_string()), + name: user_name.to_string(), + bio: None, + status: None, + links: None, + image: None, + indexed_at: Utc::now().timestamp_millis(), + }; + + user_details.put_to_graph().await?; + user_details + .put_index_json(vec![user_id].as_slice(), None) + .await?; + + // Call `put_to_index` with the same `UserDetails` object + UserSearch::put_to_index(&[&user_details]).await?; + + // Check that the index contains only one record for the user + let search_result = UserSearch::get_by_name(&user_name, None, None).await?; + assert_eq!(search_result.unwrap().0, vec![user_id.to_string()]); + + let new_user_name = "Some Other User Name"; + let new_user_details = UserDetails { + id: PubkyId(user_id.to_string()), + name: new_user_name.to_string(), + bio: None, + status: None, + links: None, + image: None, + indexed_at: Utc::now().timestamp_millis(), + }; + + // Call `put_to_index` with new user details + UserSearch::put_to_index(&[&new_user_details]).await?; + + // Check the previous record is deleted + // Check that the index contains only one record for the user + let search_result = UserSearch::get_by_name(&user_name, None, None).await?; + assert_eq!(search_result.is_none(), true); + + Ok(()) + } }