feat: add post_replies source in user stream #239

amirRamirfatahi · 2024-12-05T12:48:18Z

This PR fixes #228

Pre-submission Checklist

For tests to work you need a working neo4j and redis instance with the example dataset in docker/db-graph

Testing: Implement and pass new tests for the new features/fixes, cargo test.
Performance: Ensure new code has relevant performance benchmarks, cargo bench

SHAcollision

The PR looks good. There is a few things we can improve.

This PR still needs at least 1 more test and it would be very nice if we also add a benchmark for this new stream of users.

SHAcollision · 2025-01-02T07:51:03Z

src/models/user/stream.rs

+                let post_id = post_id.unwrap();
+                let author_id = author_id.unwrap();


Needs proper handling instead of unwrap()

let post_id = post_id.ok_or_else(|| { anyhow!("Post ID should be provided for user streams with source 'post_replies'") })?; let author_id = author_id.ok_or_else(|| { anyhow!("Author ID should be provided for user streams with source 'post_replies'") })?;

SHAcollision · 2025-01-02T08:01:55Z

src/models/user/stream.rs

+                let unique_user_ids: HashSet<String> = replies
+                    .map(|replies| {
+                        replies
+                            .into_iter()
+                            .map(|reply| reply.0.split(":").next().unwrap().to_string())
+                            .collect::<Vec<String>>()
+                    })
+                    .into_iter()
+                    .flatten()
+                    .collect();
+                Some(unique_user_ids.into_iter().collect())


Avoid unwrap()

Untested, maybe something like.

let unique_user_ids: HashSet<String> = replies .map(|replies_batch| { replies_batch .into_iter() .filter_map(|(raw_id, _some_other_data)| { // Split on ":" and take the first piece; avoid `unwrap()` raw_id.split(':').next().map(ToString::to_string) }) .collect::<Vec<String>>() }) .flatten() .collect();

SHAcollision · 2025-01-02T08:04:58Z

src/models/user/stream.rs

+            UserStreamSource::PostReplies => {
+                let post_id = post_id.unwrap();
+                let author_id = author_id.unwrap();
+                let key_parts = [
+                    &POST_REPLIES_PER_POST_KEY_PARTS[..],
+                    &[author_id.as_str(), post_id.as_str()],
+                ]
+                .concat();
+                let replies = PostStream::try_from_index_sorted_set(
+                    &key_parts,
+                    None,
+                    None,
+                    None,
+                    None,
+                    SortOrder::Descending,
+                    None,
+                )
+                .await?;
+                let unique_user_ids: HashSet<String> = replies
+                    .map(|replies| {
+                        replies
+                            .into_iter()
+                            .map(|reply| reply.0.split(":").next().unwrap().to_string())
+                            .collect::<Vec<String>>()
+                    })
+                    .into_iter()
+                    .flatten()
+                    .collect();
+                Some(unique_user_ids.into_iter().collect())
+            }


Too much logic inside of this arm of the match statement.

We should do the same as for the UserStreamSource::Recommended arm. Move the logic to a new get_post_replies_ids(post_id: &str, author_id: &str) -> Result<Option<Vec<String>>, DynError> function.

SHAcollision · 2025-01-02T08:08:15Z

This PR solves #228

Please use the keyword Fixes , e.g., This PR fixes #228 to automatically link this PR to the issue.

SHAcollision · 2025-01-21T16:58:47Z

Is this PR ready for review?

SHAcollision

This PR is basically ready to merge I only left a few minor NIT comments that I think are worth adressing.

SHAcollision · 2025-02-11T16:52:47Z

src/models/user/stream.rs

+    PostReplies,
+}
+
+pub struct UserStreamInput {


NIT: IMO we are creating this Struct only to get around clippy warning but we are not improving clarity nor saving on verbosity. On the contrary, we are constructing and deconstructing this object immediately only so the number of args in the function is no more than 7. Offering alternatives in the next comment...

SHAcollision · 2025-02-11T16:58:20Z

src/models/user/stream.rs

-        depth: Option<u8>,
-    ) -> Result<Option<Self>, DynError> {
-        let user_ids = Self::get_user_list_from_source(user_id, source, skip, limit).await?;
+    pub async fn get_by_id(input: UserStreamInput) -> Result<Option<Self>, DynError> {


NIT part 2: I propose 2 solutions that I think are a bit more elegant than these adhoc structs . We can either:

#[allow(clippy::too_many_arguments)] on top of this function.

Reuse our Pagination type so skip and limit become a single argument.

SHAcollision · 2025-02-11T17:06:15Z

src/models/user/stream.rs

+                    .map(|reply| {
+                        reply
+                            .0
+                            .split(":")


I'm not 100% sure on this one. I think reply.0.split(":").map(ToString::to_string).collect::<Vec<String>>()[0].clone() could potentially panic if the reply string does not contain a colon (I know, it should contain it...). But instead, we could consider using iterator methods like .next() with proper error handling.

SHAcollision · 2025-02-11T17:10:01Z

src/models/user/stream.rs

+            .into_iter()
+            .flatten()
+            .collect();
+        Ok(Some(unique_user_ids.into_iter().collect()))


NIT: There is a lot going on in the 2 last statements of this function. Maybe we can divide them into more lines for clarity.

NIT: It might be nice to document (small comment right here //) that we are using HashSet to enforce uniqueness, but the order of the repliers is not guaranteed. We could also sort the vector before returning (not worth it IMO, no requirement for ordering AFAIK).

SHAcollision · 2025-02-11T17:19:09Z

src/models/user/stream.rs

+        let unique_user_ids: HashSet<String> = replies
+            .map(|replies| {
+                replies
+                    .into_iter()
+                    .map(|reply| {
+                        reply
+                            .0
+                            .split(":")
+                            .map(ToString::to_string)
+                            .collect::<Vec<String>>()[0]
+                            .clone()
+                    })
+                    .collect::<Vec<String>>()
+            })
+            .into_iter()
+            .flatten()
+            .collect();
+        Ok(Some(unique_user_ids.into_iter().collect()))


Maybe something like this is clearer and panic safe.

Suggested change

let unique_user_ids: HashSet<String> = replies

.map(|replies| {

replies

.into_iter()

.map(|reply| {

reply

.0

.split(":")

.map(ToString::to_string)

.collect::<Vec<String>>()[0]

.clone()

})

.collect::<Vec<String>>()

})

.into_iter()

.flatten()

.collect();

Ok(Some(unique_user_ids.into_iter().collect()))

// If there are replies, extract unique user IDs using a HashSet.

let unique_user_ids: HashSet<String> = if let Some(replies) = replies {

replies

.into_iter()

.filter_map(|reply| reply.0.split(':').next().map(|s| s.to_string()))

.collect()

} else {

// If no replies are found, return None.

return Ok(None);

};

// Convert the HashSet to a Vec. (Note: the ordering will be arbitrary.)

Ok(Some(unique_user_ids.into_iter().collect()))

Add post_replies

80c674c

amirRamirfatahi changed the title ~~Add post_replies~~ Add post_replies source in user stream Dec 5, 2024

tipogi linked an issue Dec 6, 2024 that may be closed by this pull request

Feat (service): user stream of reply participants #228

Open

SHAcollision requested changes Jan 2, 2025

View reviewed changes

amirRamirfatahi added 5 commits January 13, 2025 08:46

Merge branch 'main' into feature/post-replies-participants

43042a6

Merge branch 'main' into feature/post-replies-participants

9797b0f

perform cleanups

0a1eae2

Add benchmark for new stream

f060d91

Add a test case for post replies

17d5240

amirRamirfatahi force-pushed the feature/post-replies-participants branch from 47d5dcb to 17d5240 Compare January 15, 2025 07:30

amirRamirfatahi marked this pull request as ready for review January 22, 2025 22:14

amirRamirfatahi changed the title ~~Add post_replies source in user stream~~ feat: add post_replies source in user stream Jan 22, 2025

amirRamirfatahi self-assigned this Jan 28, 2025

SHAcollision reviewed Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add post_replies source in user stream #239

feat: add post_replies source in user stream #239

amirRamirfatahi commented Dec 5, 2024 •

edited

Loading

SHAcollision left a comment

SHAcollision Jan 2, 2025

SHAcollision Jan 2, 2025

SHAcollision Jan 2, 2025 •

edited

Loading

SHAcollision commented Jan 2, 2025 •

edited

Loading

SHAcollision commented Jan 21, 2025

SHAcollision left a comment

SHAcollision Feb 11, 2025 •

edited

Loading

SHAcollision Feb 11, 2025

SHAcollision Feb 11, 2025 •

edited

Loading

SHAcollision Feb 11, 2025

SHAcollision Feb 11, 2025

		let post_id = post_id.unwrap();
		let author_id = author_id.unwrap();

feat: add post_replies source in user stream #239

Are you sure you want to change the base?

feat: add post_replies source in user stream #239

Conversation

amirRamirfatahi commented Dec 5, 2024 • edited Loading

Pre-submission Checklist

SHAcollision left a comment

Choose a reason for hiding this comment

SHAcollision Jan 2, 2025

Choose a reason for hiding this comment

SHAcollision Jan 2, 2025

Choose a reason for hiding this comment

SHAcollision Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

SHAcollision commented Jan 2, 2025 • edited Loading

SHAcollision commented Jan 21, 2025

SHAcollision left a comment

Choose a reason for hiding this comment

SHAcollision Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

SHAcollision Feb 11, 2025

Choose a reason for hiding this comment

SHAcollision Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

SHAcollision Feb 11, 2025

Choose a reason for hiding this comment

SHAcollision Feb 11, 2025

Choose a reason for hiding this comment

amirRamirfatahi commented Dec 5, 2024 •

edited

Loading

SHAcollision Jan 2, 2025 •

edited

Loading

SHAcollision commented Jan 2, 2025 •

edited

Loading

SHAcollision Feb 11, 2025 •

edited

Loading

SHAcollision Feb 11, 2025 •

edited

Loading