From 3a19af52159f6a4f2e41094cefdf271ce208c833 Mon Sep 17 00:00:00 2001 From: Nutomic Date: Tue, 17 Oct 2023 18:35:51 +0200 Subject: [PATCH] Allow marking multiple posts as read in single api call (fixes #3963) (#4048) * Allow marking multiple posts as read in single api call (fixes #3963) * cleanup * limit array length * fix test * review --------- Co-authored-by: Dessalines --- crates/api/src/post/mark_read.rs | 34 ++++---- crates/api_common/src/post.rs | 2 + crates/api_common/src/utils.rs | 27 ++---- crates/apub/src/api/user_settings_backup.rs | 14 +-- crates/db_schema/src/impls/post.rs | 94 ++++++++++++--------- crates/db_schema/src/source/post.rs | 2 +- crates/db_schema/src/traits.rs | 11 --- crates/utils/src/error.rs | 6 +- 8 files changed, 91 insertions(+), 99 deletions(-) diff --git a/crates/api/src/post/mark_read.rs b/crates/api/src/post/mark_read.rs index a248b0196..a377b3c6c 100644 --- a/crates/api/src/post/mark_read.rs +++ b/crates/api/src/post/mark_read.rs @@ -1,30 +1,34 @@ use actix_web::web::{Data, Json}; -use lemmy_api_common::{ - context::LemmyContext, - post::{MarkPostAsRead, PostResponse}, - utils, -}; -use lemmy_db_views::structs::{LocalUserView, PostView}; -use lemmy_utils::error::LemmyError; +use lemmy_api_common::{context::LemmyContext, post::MarkPostAsRead, SuccessResponse}; +use lemmy_db_schema::source::post::PostRead; +use lemmy_db_views::structs::LocalUserView; +use lemmy_utils::error::{LemmyError, LemmyErrorExt, LemmyErrorType, MAX_API_PARAM_ELEMENTS}; +use std::collections::HashSet; #[tracing::instrument(skip(context))] pub async fn mark_post_as_read( data: Json, context: Data, local_user_view: LocalUserView, -) -> Result, LemmyError> { - let post_id = data.post_id; +) -> Result, LemmyError> { + let mut post_ids = data.post_ids.iter().cloned().collect::>(); + post_ids.insert(data.post_id); let person_id = local_user_view.person.id; + if post_ids.len() > MAX_API_PARAM_ELEMENTS { + Err(LemmyErrorType::TooManyItems)?; + } + // Mark the post as read / unread if data.read { - utils::mark_post_as_read(person_id, post_id, &mut context.pool()).await?; + PostRead::mark_as_read(&mut context.pool(), post_ids, person_id) + .await + .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)?; } else { - utils::mark_post_as_unread(person_id, post_id, &mut context.pool()).await?; + PostRead::mark_as_unread(&mut context.pool(), post_ids, person_id) + .await + .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)?; } - // Fetch it - let post_view = PostView::read(&mut context.pool(), post_id, Some(person_id), false).await?; - - Ok(Json(PostResponse { post_view })) + Ok(Json(SuccessResponse::default())) } diff --git a/crates/api_common/src/post.rs b/crates/api_common/src/post.rs index 86f721701..fbb6f1d38 100644 --- a/crates/api_common/src/post.rs +++ b/crates/api_common/src/post.rs @@ -140,7 +140,9 @@ pub struct RemovePost { #[cfg_attr(feature = "full", ts(export))] /// Mark a post as read. pub struct MarkPostAsRead { + /// TODO: deprecated, send `post_ids` instead pub post_id: PostId, + pub post_ids: Vec, pub read: bool, } diff --git a/crates/api_common/src/utils.rs b/crates/api_common/src/utils.rs index 139620b67..df84adc16 100644 --- a/crates/api_common/src/utils.rs +++ b/crates/api_common/src/utils.rs @@ -18,9 +18,9 @@ use lemmy_db_schema::{ password_reset_request::PasswordResetRequest, person::{Person, PersonUpdateForm}, person_block::PersonBlock, - post::{Post, PostRead, PostReadForm}, + post::{Post, PostRead}, }, - traits::{Crud, Readable}, + traits::Crud, utils::DbPool, }; use lemmy_db_views::{comment_view::CommentQuery, structs::LocalUserView}; @@ -39,6 +39,7 @@ use lemmy_utils::{ }; use regex::Regex; use rosetta_i18n::{Language, LanguageId}; +use std::collections::HashSet; use tracing::warn; use url::{ParseError, Url}; @@ -117,25 +118,11 @@ pub async fn mark_post_as_read( person_id: PersonId, post_id: PostId, pool: &mut DbPool<'_>, -) -> Result { - let post_read_form = PostReadForm { post_id, person_id }; - - PostRead::mark_as_read(pool, &post_read_form) +) -> Result<(), LemmyError> { + PostRead::mark_as_read(pool, HashSet::from([post_id]), person_id) .await - .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead) -} - -#[tracing::instrument(skip_all)] -pub async fn mark_post_as_unread( - person_id: PersonId, - post_id: PostId, - pool: &mut DbPool<'_>, -) -> Result { - let post_read_form = PostReadForm { post_id, person_id }; - - PostRead::mark_as_unread(pool, &post_read_form) - .await - .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead) + .with_lemmy_type(LemmyErrorType::CouldntMarkPostAsRead)?; + Ok(()) } pub fn check_user_valid(person: &Person) -> Result<(), LemmyError> { diff --git a/crates/apub/src/api/user_settings_backup.rs b/crates/apub/src/api/user_settings_backup.rs index 3a9b05847..419be280d 100644 --- a/crates/apub/src/api/user_settings_backup.rs +++ b/crates/apub/src/api/user_settings_backup.rs @@ -23,18 +23,12 @@ use lemmy_db_schema::{ }; use lemmy_db_views::structs::LocalUserView; use lemmy_utils::{ - error::{LemmyError, LemmyErrorType, LemmyResult}, + error::{LemmyError, LemmyErrorType, LemmyResult, MAX_API_PARAM_ELEMENTS}, spawn_try_task, }; use serde::{Deserialize, Serialize}; use tracing::info; -/// Maximum number of follow/block URLs which can be imported at once, to prevent server overloading. -/// To import a larger backup, split it into multiple parts. -/// -/// TODO: having the user manually split files will very be confusing -const MAX_URL_IMPORT_COUNT: usize = 1000; - /// Backup of user data. This struct should never be changed so that the data can be used as a /// long-term backup in case the instance goes down unexpectedly. All fields are optional to allow /// importing partial backups. @@ -138,8 +132,8 @@ pub async fn import_settings( + data.blocked_users.len() + data.saved_posts.len() + data.saved_comments.len(); - if url_count > MAX_URL_IMPORT_COUNT { - Err(LemmyErrorType::UserBackupTooLarge)?; + if url_count > MAX_API_PARAM_ELEMENTS { + Err(LemmyErrorType::TooManyItems)?; } spawn_try_task(async move { @@ -434,7 +428,7 @@ mod tests { assert_eq!( imported.err().unwrap().error_type, - LemmyErrorType::UserBackupTooLarge + LemmyErrorType::TooManyItems ); LocalUser::delete(&mut context.pool(), export_user.local_user.id) diff --git a/crates/db_schema/src/impls/post.rs b/crates/db_schema/src/impls/post.rs index 514aceaba..4a719415a 100644 --- a/crates/db_schema/src/impls/post.rs +++ b/crates/db_schema/src/impls/post.rs @@ -28,13 +28,14 @@ use crate::{ PostSavedForm, PostUpdateForm, }, - traits::{Crud, Likeable, Readable, Saveable}, + traits::{Crud, Likeable, Saveable}, utils::{get_conn, naive_now, DbPool, DELETED_REPLACEMENT_TEXT, FETCH_LIMIT_MAX}, }; use ::url::Url; use chrono::{Duration, Utc}; use diesel::{dsl::insert_into, result::Error, ExpressionMethods, QueryDsl, TextExpressionMethods}; use diesel_async::RunQueryDsl; +use std::collections::HashSet; #[async_trait] impl Crud for Post { @@ -302,34 +303,38 @@ impl Saveable for PostSaved { } } -#[async_trait] -impl Readable for PostRead { - type Form = PostReadForm; - async fn mark_as_read( +impl PostRead { + pub async fn mark_as_read( pool: &mut DbPool<'_>, - post_read_form: &PostReadForm, - ) -> Result { - use crate::schema::post_read::dsl::{person_id, post_id, post_read}; + post_ids: HashSet, + person_id: PersonId, + ) -> Result { + use crate::schema::post_read::dsl::post_read; let conn = &mut get_conn(pool).await?; + + let forms = post_ids + .into_iter() + .map(|post_id| PostReadForm { post_id, person_id }) + .collect::>(); insert_into(post_read) - .values(post_read_form) - .on_conflict((post_id, person_id)) - .do_update() - .set(post_read_form) - .get_result::(conn) + .values(forms) + .on_conflict_do_nothing() + .execute(conn) .await } - async fn mark_as_unread( + pub async fn mark_as_unread( pool: &mut DbPool<'_>, - post_read_form: &PostReadForm, + post_id_: HashSet, + person_id_: PersonId, ) -> Result { use crate::schema::post_read::dsl::{person_id, post_id, post_read}; let conn = &mut get_conn(pool).await?; + diesel::delete( post_read - .filter(post_id.eq(post_read_form.post_id)) - .filter(person_id.eq(post_read_form.person_id)), + .filter(post_id.eq_any(post_id_)) + .filter(person_id.eq(person_id_)), ) .execute(conn) .await @@ -352,16 +357,16 @@ mod tests { PostLike, PostLikeForm, PostRead, - PostReadForm, PostSaved, PostSavedForm, PostUpdateForm, }, }, - traits::{Crud, Likeable, Readable, Saveable}, + traits::{Crud, Likeable, Saveable}, utils::build_db_pool_for_tests, }; use serial_test::serial; + use std::collections::HashSet; #[tokio::test] #[serial] @@ -398,6 +403,13 @@ mod tests { let inserted_post = Post::create(pool, &new_post).await.unwrap(); + let new_post2 = PostInsertForm::builder() + .name("A test post 2".into()) + .creator_id(inserted_person.id) + .community_id(inserted_community.id) + .build(); + let inserted_post2 = Post::create(pool, &new_post2).await.unwrap(); + let expected_post = Post { id: inserted_post.id, name: "A test post".into(), @@ -455,19 +467,14 @@ mod tests { }; // Post Read - let post_read_form = PostReadForm { - post_id: inserted_post.id, - person_id: inserted_person.id, - }; - - let inserted_post_read = PostRead::mark_as_read(pool, &post_read_form).await.unwrap(); - - let expected_post_read = PostRead { - id: inserted_post_read.id, - post_id: inserted_post.id, - person_id: inserted_person.id, - published: inserted_post_read.published, - }; + let marked_as_read = PostRead::mark_as_read( + pool, + HashSet::from([inserted_post.id, inserted_post2.id]), + inserted_person.id, + ) + .await + .unwrap(); + assert_eq!(2, marked_as_read); let read_post = Post::read(pool, inserted_post.id).await.unwrap(); @@ -482,11 +489,21 @@ mod tests { let like_removed = PostLike::remove(pool, inserted_person.id, inserted_post.id) .await .unwrap(); + assert_eq!(1, like_removed); let saved_removed = PostSaved::unsave(pool, &post_saved_form).await.unwrap(); - let read_removed = PostRead::mark_as_unread(pool, &post_read_form) - .await - .unwrap(); - let num_deleted = Post::delete(pool, inserted_post.id).await.unwrap(); + assert_eq!(1, saved_removed); + let read_removed = PostRead::mark_as_unread( + pool, + HashSet::from([inserted_post.id, inserted_post2.id]), + inserted_person.id, + ) + .await + .unwrap(); + assert_eq!(2, read_removed); + + let num_deleted = Post::delete(pool, inserted_post.id).await.unwrap() + + Post::delete(pool, inserted_post2.id).await.unwrap(); + assert_eq!(2, num_deleted); Community::delete(pool, inserted_community.id) .await .unwrap(); @@ -498,10 +515,5 @@ mod tests { assert_eq!(expected_post, updated_post); assert_eq!(expected_post_like, inserted_post_like); assert_eq!(expected_post_saved, inserted_post_saved); - assert_eq!(expected_post_read, inserted_post_read); - assert_eq!(1, like_removed); - assert_eq!(1, saved_removed); - assert_eq!(1, read_removed); - assert_eq!(1, num_deleted); } } diff --git a/crates/db_schema/src/source/post.rs b/crates/db_schema/src/source/post.rs index 4fe8e34c6..72c32d4af 100644 --- a/crates/db_schema/src/source/post.rs +++ b/crates/db_schema/src/source/post.rs @@ -162,7 +162,7 @@ pub struct PostRead { #[cfg_attr(feature = "full", derive(Insertable, AsChangeset))] #[cfg_attr(feature = "full", diesel(table_name = post_read))] -pub struct PostReadForm { +pub(crate) struct PostReadForm { pub post_id: PostId, pub person_id: PersonId, } diff --git a/crates/db_schema/src/traits.rs b/crates/db_schema/src/traits.rs index 9fa5a598c..e58319c0b 100644 --- a/crates/db_schema/src/traits.rs +++ b/crates/db_schema/src/traits.rs @@ -140,17 +140,6 @@ pub trait Blockable { Self: Sized; } -#[async_trait] -pub trait Readable { - type Form; - async fn mark_as_read(pool: &mut DbPool<'_>, form: &Self::Form) -> Result - where - Self: Sized; - async fn mark_as_unread(pool: &mut DbPool<'_>, form: &Self::Form) -> Result - where - Self: Sized; -} - #[async_trait] pub trait Reportable { type Form; diff --git a/crates/utils/src/error.rs b/crates/utils/src/error.rs index 7ac42f0bb..3b5da1bc9 100644 --- a/crates/utils/src/error.rs +++ b/crates/utils/src/error.rs @@ -15,6 +15,9 @@ pub struct LemmyError { pub context: SpanTrace, } +/// Maximum number of items in an array passed as API parameter. See [[LemmyErrorType::TooManyItems]] +pub const MAX_API_PARAM_ELEMENTS: usize = 1000; + impl From for LemmyError where T: Into, @@ -215,8 +218,9 @@ pub enum LemmyErrorType { InstanceBlockAlreadyExists, /// `jwt` cookie must be marked secure and httponly AuthCookieInsecure, + /// Thrown when an API call is submitted with more than 1000 array elements, see [[MAX_API_PARAM_ELEMENTS]] + TooManyItems, CommunityHasNoFollowers, - UserBackupTooLarge, Unknown(String), }