From a46bf76d742840053fcabc8efa39f774a59bf157 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Thu, 14 Dec 2023 14:15:01 +0100 Subject: [PATCH] test(base): Improve `test_media_content`. This patch improves `test_media_content` to ensure that, in case of multiple medias, only the expected ones are removed. Previously, the test wasn't testing _other_ medias that should be kept in case of removals. This patch continues to improve `test_media_content` to ensure that the content of the media are the expected ones. Finally, this patch updates the `MemoryStore` implementation to make tests happy. --- .../src/store/integration_tests.rs | 60 +++++++++++++++---- .../matrix-sdk-base/src/store/memory_store.rs | 22 ++++--- 2 files changed, 64 insertions(+), 18 deletions(-) diff --git a/crates/matrix-sdk-base/src/store/integration_tests.rs b/crates/matrix-sdk-base/src/store/integration_tests.rs index 610d254d1..345c002d7 100644 --- a/crates/matrix-sdk-base/src/store/integration_tests.rs +++ b/crates/matrix-sdk-base/src/store/integration_tests.rs @@ -200,11 +200,8 @@ impl StateStoreIntegrationTests for DynStateStore { async fn test_media_content(&self) { let uri = mxc_uri!("mxc://localhost/media"); - let content: Vec = "somebinarydata".into(); - let request_file = MediaRequest { source: MediaSource::Plain(uri.to_owned()), format: MediaFormat::File }; - let request_thumbnail = MediaRequest { source: MediaSource::Plain(uri.to_owned()), format: MediaFormat::Thumbnail(MediaThumbnailSize { @@ -214,6 +211,17 @@ impl StateStoreIntegrationTests for DynStateStore { }), }; + let other_uri = mxc_uri!("mxc://localhost/media-other"); + let request_other_file = MediaRequest { + source: MediaSource::Plain(other_uri.to_owned()), + format: MediaFormat::File, + }; + + let content: Vec = "hello".into(); + let thumbnail_content: Vec = "world".into(); + let other_content: Vec = "foo".into(); + + // Media isn't present in the cache. assert!( self.get_media_content(&request_file).await.unwrap().is_none(), "unexpected media found" @@ -223,35 +231,63 @@ impl StateStoreIntegrationTests for DynStateStore { "media not found" ); + // Let's add the media. self.add_media_content(&request_file, content.clone()).await.expect("adding media failed"); - assert!( - self.get_media_content(&request_file).await.unwrap().is_some(), + + // Media is present in the cache. + assert_eq!( + self.get_media_content(&request_file).await.unwrap().as_ref(), + Some(&content), "media not found though added" ); + // Let's remove the media. self.remove_media_content(&request_file).await.expect("removing media failed"); + + // Media isn't present in the cache. assert!( self.get_media_content(&request_file).await.unwrap().is_none(), "media still there after removing" ); + // Let's add the media again. self.add_media_content(&request_file, content.clone()) .await .expect("adding media again failed"); - assert!( - self.get_media_content(&request_file).await.unwrap().is_some(), + + assert_eq!( + self.get_media_content(&request_file).await.unwrap().as_ref(), + Some(&content), "media not found after adding again" ); - self.add_media_content(&request_thumbnail, content.clone()) + // Let's add the thumbnail media. + self.add_media_content(&request_thumbnail, thumbnail_content.clone()) .await .expect("adding thumbnail failed"); - assert!( - self.get_media_content(&request_thumbnail).await.unwrap().is_some(), + + // Media's thumbnail is present. + assert_eq!( + self.get_media_content(&request_thumbnail).await.unwrap().as_ref(), + Some(&thumbnail_content), "thumbnail not found" ); + // Let's add another media with a different URI. + self.add_media_content(&request_other_file, other_content.clone()) + .await + .expect("adding other media failed"); + + // Other file is present. + assert_eq!( + self.get_media_content(&request_other_file).await.unwrap().as_ref(), + Some(&other_content), + "other file not found" + ); + + // Let's remove media based on URI. self.remove_media_content_for_uri(uri).await.expect("removing all media for uri failed"); + assert!( self.get_media_content(&request_file).await.unwrap().is_none(), "media wasn't removed" @@ -260,6 +296,10 @@ impl StateStoreIntegrationTests for DynStateStore { self.get_media_content(&request_thumbnail).await.unwrap().is_none(), "thumbnail wasn't removed" ); + assert!( + self.get_media_content(&request_other_file).await.unwrap().is_some(), + "other media was removed" + ); } async fn test_topic_redaction(&self) -> Result<()> { diff --git a/crates/matrix-sdk-base/src/store/memory_store.rs b/crates/matrix-sdk-base/src/store/memory_store.rs index 41bd41f05..de659eb10 100644 --- a/crates/matrix-sdk-base/src/store/memory_store.rs +++ b/crates/matrix-sdk-base/src/store/memory_store.rs @@ -78,7 +78,7 @@ pub struct MemoryStore { HashMap<(String, Option), HashMap>>, >, >, - media: StdRwLock)>>, + media: StdRwLock)>>, custom: StdRwLock, Vec>>, } @@ -703,16 +703,19 @@ impl StateStore for MemoryStore { } async fn add_media_content(&self, request: &MediaRequest, data: Vec) -> Result<()> { - self.media.write().unwrap().push((request.uri().to_owned(), data)); + // Avoid duplication. Let's try to remove it first. + self.remove_media_content(request).await?; + // Now, let's add it. + self.media.write().unwrap().push((request.uri().to_owned(), request.unique_key(), data)); Ok(()) } async fn get_media_content(&self, request: &MediaRequest) -> Result>> { let media = self.media.read().unwrap(); - let expected_key = request.uri().to_owned(); + let expected_key = request.unique_key(); - for (media_key, media_content) in media.iter() { + for (_media_uri, media_key, media_content) in media.iter() { if media_key == &expected_key { return Ok(Some(media_content.to_owned())); } @@ -723,8 +726,11 @@ impl StateStore for MemoryStore { async fn remove_media_content(&self, request: &MediaRequest) -> Result<()> { let mut media = self.media.write().unwrap(); - let expected_key = request.uri().to_owned(); - let Some(index) = media.iter().position(|(media_key, _)| media_key == &expected_key) else { + let expected_key = request.unique_key(); + let Some(index) = media + .iter() + .position(|(_media_uri, media_key, _media_content)| media_key == &expected_key) + else { return Ok(()); }; @@ -739,8 +745,8 @@ impl StateStore for MemoryStore { let positions = media .iter() .enumerate() - .filter_map(|(position, (media_key, _))| { - (media_key == &expected_key).then_some(position) + .filter_map(|(position, (media_uri, _media_key, _media_content))| { + (media_uri == &expected_key).then_some(position) }) .collect::>();