From 4dd1cd0783a83965c3e67798e0653f48f58ab0b5 Mon Sep 17 00:00:00 2001 From: dullbananas Date: Mon, 11 Dec 2023 18:05:47 -0700 Subject: [PATCH 1/6] Update post_view.rs --- crates/db_views/src/post_view.rs | 332 +++++++++++++++++++------------ 1 file changed, 201 insertions(+), 131 deletions(-) diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index bd768cc382..8cfe5251fa 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -40,7 +40,7 @@ use lemmy_db_schema::{ post_read, post_saved, }, - utils::{fuzzy_search, get_conn, limit_and_offset, DbConn, DbPool, ListFn, Queries, ReadFn}, + utils::{fuzzy_search, get_conn, limit_and_offset, now, DbConn, DbPool, ListFn, Queries, ReadFn}, ListingType, SortType, }; @@ -48,53 +48,32 @@ use tracing::debug; sql_function!(fn coalesce(x: sql_types::Nullable, y: sql_types::BigInt) -> sql_types::BigInt); -fn order_and_page_filter_desc( - query: Q, - column: C, - options: &PostQuery, - getter: impl Fn(&PostAggregates) -> T, -) -> Q -where - Q: diesel::query_dsl::methods::ThenOrderDsl, Output = Q> - + diesel::query_dsl::methods::ThenOrderDsl, Output = Q> - + diesel::query_dsl::methods::FilterDsl, Output = Q> - + diesel::query_dsl::methods::FilterDsl, Output = Q>, - C: Expression + Copy, - C::SqlType: SingleValue + SqlType, - T: AsExpression, -{ - let mut query = query.then_order_by(column.desc()); - if let Some(before) = &options.page_before_or_equal { - query = query.filter(column.ge(getter(&before.0))); - } - if let Some(after) = &options.page_after { - query = query.filter(column.le(getter(&after.0))); - } - query +#[derive(Clone, Copy)] +enum Ord { + Desc, + Asc, } -fn order_and_page_filter_asc( - query: Q, - column: C, - options: &PostQuery, - getter: impl Fn(&PostAggregates) -> T, -) -> Q -where - Q: diesel::query_dsl::methods::ThenOrderDsl, Output = Q> - + diesel::query_dsl::methods::FilterDsl, Output = Q> - + diesel::query_dsl::methods::FilterDsl, Output = Q>, - C: Expression + Copy, - C::SqlType: SingleValue + SqlType, - T: AsExpression, -{ - let mut query = query.then_order_by(column.asc()); - if let Some(before) = &options.page_before_or_equal { - query = query.filter(column.le(getter(&before.0))); - } - if let Some(after) = &options.page_after { - query = query.filter(column.ge(getter(&after.0))); - } - query +struct PaginationCursorField { + then_order_by_desc: fn(Q) -> Q, + then_order_by_asc: fn(Q) -> Q, + le: fn(&PostAggregates) -> BoxExpr, + ge: fn(&PostAggregates) -> BoxExpr, + ne: fn(&PostAggregates) -> BoxExpr, +} + +/// Returns `PaginationCursorField<_, _>` for the given name +macro_rules! field { + ($name:ident) => { + // Type inference doesn't work if normal method call syntax is used + PaginationCursorField { + then_order_by_desc: |query| QueryDsl::then_order_by(query, post_aggregates::$name.desc()), + then_order_by_asc: |query| QueryDsl::then_order_by(query, post_aggregates::$name.asc()), + le: |e| Box::new(post_aggregates::$name.le(e.$name)), + ge: |e| Box::new(post_aggregates::$name.ge(e.$name)), + ne: |e| Box::new(post_aggregates::$name.ne(e.$name)), + } + }; } fn queries<'a>() -> Queries< @@ -334,16 +313,6 @@ fn queries<'a>() -> Queries< .filter(community::removed.eq(false)) .filter(post::removed.eq(false)); } - if options.community_id.is_none() || options.community_id_just_for_prefetch { - query = order_and_page_filter_desc(query, post_aggregates::featured_local, &options, |e| { - e.featured_local - }); - } else { - query = - order_and_page_filter_desc(query, post_aggregates::featured_community, &options, |e| { - e.featured_community - }); - } if let Some(community_id) = options.community_id { query = query.filter(post_aggregates::community_id.eq(community_id)); } @@ -481,85 +450,99 @@ fn queries<'a>() -> Queries< ))); query = query.filter(not(is_creator_blocked(person_id))); } - let now = diesel::dsl::now.into_sql::(); - { - use post_aggregates::{ - comments, - controversy_rank, - hot_rank, - hot_rank_active, - published, - scaled_rank, - score, - }; - match options.sort.as_ref().unwrap_or(&SortType::Hot) { - SortType::Active => { - query = - order_and_page_filter_desc(query, hot_rank_active, &options, |e| e.hot_rank_active); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); - } - SortType::Hot => { - query = order_and_page_filter_desc(query, hot_rank, &options, |e| e.hot_rank); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); + let featured_field = if options.community_id.is_none() || options.community_id_just_for_prefetch { + field!(featured_local) + } else { + field!(featured_community) + }; + + let (main_sort, top_sort_interval) = match sort { + SortType::Active => ((Ord::Desc, field!(hot_rank_active)), None), + SortType::Hot => ((Ord::Desc, field!(hot_rank)), None), + SortType::Scaled => ((Ord::Desc, field!(scaled_rank)), None), + SortType::Controversial => ((Ord::Desc, field!(controversy_rank)), None), + SortType::New => ((Ord::Desc, field!(published)), None), + SortType::Old => ((Ord::Asc, field!(published)), None), + SortType::NewComments => ((Ord::Desc, field!(newest_comment_time)), None), + SortType::MostComments => ((Ord::Desc, field!(comments)), None), + SortType::TopAll => ((Ord::Desc, field!(score)), None), + SortType::TopYear => ((Ord::Desc, field!(score)), Some(1.years())), + SortType::TopMonth => ((Ord::Desc, field!(score)), Some(1.months())), + SortType::TopWeek => ((Ord::Desc, field!(score)), Some(1.weeks())), + SortType::TopDay => ((Ord::Desc, field!(score)), Some(1.days())), + SortType::TopHour => ((Ord::Desc, field!(score)), Some(1.hours())), + SortType::TopSixHour => ((Ord::Desc, field!(score)), Some(6.hours())), + SortType::TopTwelveHour => ((Ord::Desc, field!(score)), Some(12.hours())), + SortType::TopThreeMonths => ((Ord::Desc, field!(score)), Some(3.months())), + SortType::TopSixMonths => ((Ord::Desc, field!(score)), Some(6.months())), + SortType::TopNineMonths => ((Ord::Desc, field!(score)), Some(9.months())), + }; + + if let Some(interval) = top_sort_interval { + query = query.filter(post_aggregates::published.gt(now() - interval)); } - SortType::Scaled => { - query = order_and_page_filter_desc(query, scaled_rank, &options, |e| e.scaled_rank); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); - } - SortType::Controversial => { - query = - order_and_page_filter_desc(query, controversy_rank, &options, |e| e.controversy_rank); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); - } - SortType::New => { - query = order_and_page_filter_desc(query, published, &options, |e| e.published) - } - SortType::Old => { - query = order_and_page_filter_asc(query, published, &options, |e| e.published) - } - SortType::NewComments => { - query = order_and_page_filter_desc(query, newest_comment_time, &options, |e| { - e.newest_comment_time - }) - } - SortType::MostComments => { - query = order_and_page_filter_desc(query, comments, &options, |e| e.comments); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); - } - SortType::TopAll => { - query = order_and_page_filter_desc(query, score, &options, |e| e.score); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); - } - o @ (SortType::TopYear - | SortType::TopMonth - | SortType::TopWeek - | SortType::TopDay - | SortType::TopHour - | SortType::TopSixHour - | SortType::TopTwelveHour - | SortType::TopThreeMonths - | SortType::TopSixMonths - | SortType::TopNineMonths) => { - let interval = match o { - SortType::TopYear => 1.years(), - SortType::TopMonth => 1.months(), - SortType::TopWeek => 1.weeks(), - SortType::TopDay => 1.days(), - SortType::TopHour => 1.hours(), - SortType::TopSixHour => 6.hours(), - SortType::TopTwelveHour => 12.hours(), - SortType::TopThreeMonths => 3.months(), - SortType::TopSixMonths => 6.months(), - SortType::TopNineMonths => 9.months(), - _ => return Err(Error::NotFound), + + let tie_breaker = match sort { + // A second time-based sort would not be very useful + SortType::New | SortType::Old | SortType::NewComments => None, + _ => Some((Ord::Desc, field!(published))), + }; + + let sorts = [ + Some((Ord::Desc, featured_field)), + Some(main_sort), + tie_breaker, + ]; + let sorts_iter = sorts.iter().flatten(); + + // This loop does almost the same thing as sorting by and comparing tuples. If the rows were + // only sorted by 1 field called `foo` in descending order, then it would be like this: + // + // ``` + // query = query.then_order_by(foo.desc()); + // if let Some(first) = &options.page_after { + // query = query.filter(foo.le(first.foo)); + // } + // if let Some(last) = &page_before_or_equal { + // query = query.filter(foo.ge(last.foo)); + // } + // ``` + // + // If multiple rows have the same value for a sorted field, then they are + // grouped together, and the rows in that group are sorted by the next fields. + // When checking if a row is within the range determined by the cursors, a field + // that's sorted after other fields is only compared if the row and the cursor + // are in the same group created by the previous sort, which is checked by using + // `or` to skip the comparison if any previously sorted field is not equal. + for (i, (order, field)) in sorts_iter.clone().enumerate() { + // Both cursors are treated as inclusive here. `page_after` is made exclusive + // by adding `1` to the offset. + let (then_order_by_field, compare_first, compare_last) = match order { + Ord::Desc => (field.then_order_by_desc, field.le, field.ge), + Ord::Asc => (field.then_order_by_asc, field.ge, field.le), }; - query = query.filter(post_aggregates::published.gt(now - interval)); - query = order_and_page_filter_desc(query, score, &options, |e| e.score); - query = order_and_page_filter_desc(query, published, &options, |e| e.published); + + query = then_order_by_field(query); + + for (cursor_data, compare) in [ + (&options.page_after, compare_first), + (&page_before_or_equal, compare_last), + ] { + let Some(cursor_data) = cursor_data else { + continue; + }; + let mut condition: BoxExpr<_, sql_types::Bool> = Box::new(compare(&cursor_data.0)); + + // For each field that was sorted before the current one, skip the filter by changing + // `condition` to `true` if the row's value doesn't equal the cursor's value. + for (_, other_field) in sorts_iter.clone().take(i) { + condition = Box::new(condition.or((other_field.ne)(&cursor_data.0))); + } + + query = query.filter(condition); + } } - } - }; let (limit, mut offset) = limit_and_offset(options.page, options.limit)?; if options.page_after.is_some() { @@ -1428,6 +1411,93 @@ mod tests { cleanup(data, pool).await; } + #[tokio::test] + #[serial] + async fn pagination_includes_each_post_once() -> LemmyResult<()> { + let pool = &build_db_pool().await?; + let pool = &mut pool.into(); + let data = init_data(pool).await?; + + // TODO make new community instead + let pre_existing_post_ids = data + .default_post_query() + .list(pool) + .await? + .into_iter() + .map(|p| p.post.id) + .collect::>(); + + let mut expected_post_ids = vec![]; + let mut comment_ids = vec![]; + + // Create 15 posts for each amount of comments from 0 to 9 + for comments in 0..10 { + for _ in 0..15 { + let post = Post::create( + pool, + &PostInsertForm::builder() + .name("keep Christ in Christmas".to_owned()) + .creator_id(data.local_user_view.person.id) + .community_id(data.inserted_community.id) + .featured_local(Some((comments % 2) == 0)) + .featured_community(Some((comments % 2) == 0)) + .published(Some(Utc::now() - Duration::from_secs(comments % 3))) + .build(), + ) + .await?; + expected_post_ids.push(post.id); + for _ in 0..comments { + let comment = Comment::create( + pool, + &CommentInsertForm::builder() + .creator_id(data.local_user_view.person.id) + .post_id(post.id) + .content("hi".to_owned()) + .build(), + None, + ) + .await?; + comment_ids.push(comment.id); + } + } + } + + let mut post_ids = vec![]; + let mut page_after = None; + loop { + let posts = PostQuery { + sort: Some(SortType::MostComments), + page_after, + limit: Some(10), + ..data.default_post_query() + } + .list(pool) + .await?; + + post_ids.extend( + posts + .iter() + .map(|p| p.post.id) + .filter(|id| !pre_existing_post_ids.contains(&id)) + ); + + if let Some(p) = posts.into_iter().last() { + page_after = Some(PaginationCursorData(p.counts)); + } else { + break; + } + } + + cleanup(data, pool).await?; + + expected_post_ids.sort_unstable_by_key(|id| id.0); + post_ids.sort_unstable_by_key(|id| id.0); + assert_eq!(expected_post_ids, post_ids); + + Ok(()) + } + + async fn cleanup(data: Data, pool: &mut DbPool<'_>) { let num_deleted = Post::delete(pool, data.inserted_post.id).await.unwrap(); Community::delete(pool, data.inserted_community.id) From 9884bdf8a579ab0005cb667697f491e701b96479 Mon Sep 17 00:00:00 2001 From: dullbananas Date: Mon, 11 Dec 2023 19:32:39 -0700 Subject: [PATCH 2/6] Update post_view.rs --- crates/db_views/src/post_view.rs | 103 ++++++++++++++----------------- 1 file changed, 47 insertions(+), 56 deletions(-) diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index 8cfe5251fa..f17dd5fc4b 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -721,15 +721,17 @@ mod tests { #![allow(clippy::indexing_slicing)] use crate::{ - post_view::{PostQuery, PostView}, + post_view::{PaginationCursorData, PostQuery, PostView}, structs::LocalUserView, }; + use chrono::Utc; use lemmy_db_schema::{ aggregates::structs::PostAggregates, impls::actor_language::UNDETERMINED_ID, newtypes::LanguageId, source::{ actor_language::LocalUserLanguage, + comment::{Comment, CommentInsertForm}; community::{Community, CommunityInsertForm, CommunityModerator, CommunityModeratorForm}, community_block::{CommunityBlock, CommunityBlockForm}, instance::Instance, @@ -1413,88 +1415,77 @@ mod tests { #[tokio::test] #[serial] - async fn pagination_includes_each_post_once() -> LemmyResult<()> { - let pool = &build_db_pool().await?; + async fn pagination_includes_each_post_once() { + let pool = &build_db_pool().await.unwrap(); let pool = &mut pool.into(); - let data = init_data(pool).await?; + let data = init_data(pool).await.unwrap(); - // TODO make new community instead - let pre_existing_post_ids = data - .default_post_query() - .list(pool) - .await? - .into_iter() - .map(|p| p.post.id) - .collect::>(); + let community_form = CommunityInsertForm::builder() + .name("yes".to_string()) + .title("yes".to_owned()) + .public_key("pubkey".to_string()) + .instance_id(data.inserted_instance.id) + .build(); + let inserted_community = Community::create(pool, &community_form).await.unwrap(); - let mut expected_post_ids = vec![]; - let mut comment_ids = vec![]; + let mut inserted_post_ids = vec![]; + let mut inserted_comment_ids = vec![]; - // Create 15 posts for each amount of comments from 0 to 9 + // Create 150 posts with varying non-correlating values for publish date, number of comments, and featured for comments in 0..10 { for _ in 0..15 { - let post = Post::create( - pool, - &PostInsertForm::builder() - .name("keep Christ in Christmas".to_owned()) - .creator_id(data.local_user_view.person.id) - .community_id(data.inserted_community.id) - .featured_local(Some((comments % 2) == 0)) - .featured_community(Some((comments % 2) == 0)) - .published(Some(Utc::now() - Duration::from_secs(comments % 3))) - .build(), - ) - .await?; - expected_post_ids.push(post.id); + let post_form = PostInsertForm::builder() + .name("keep Christ in Christmas".to_owned()) + .creator_id(data.local_user_view.person.id) + .community_id(inserted_community.id) + .featured_local(Some((comments % 2) == 0)) + .featured_community(Some((comments % 2) == 0)) + .published(Some(Utc::now() - Duration::from_secs(comments % 3))) + .build(); + let inserted_post = Post::create(pool, &post_form).await.unwrap(); + inserted_post_ids.push(inserted_post.id); + for _ in 0..comments { - let comment = Comment::create( - pool, - &CommentInsertForm::builder() - .creator_id(data.local_user_view.person.id) - .post_id(post.id) - .content("hi".to_owned()) - .build(), - None, - ) - .await?; - comment_ids.push(comment.id); + let comment_form = CommentInsertForm::builder() + .creator_id(data.local_user_view.person.id) + .post_id(inserted_post.id) + .content("yes".to_owned()) + .build(); + let inserted_comment = Comment::create(pool, &comment_form, None).await.unwrap(); + inserted_comment_ids.push(inserted_comment.id); } } } - let mut post_ids = vec![]; + let mut listed_post_ids = vec![]; let mut page_after = None; loop { - let posts = PostQuery { + let post_listings = PostQuery { + community_id: Some(inserted_community.id), sort: Some(SortType::MostComments), - page_after, limit: Some(10), - ..data.default_post_query() + page_after, } .list(pool) - .await?; + .await + .unwrap(); - post_ids.extend( - posts - .iter() - .map(|p| p.post.id) - .filter(|id| !pre_existing_post_ids.contains(&id)) - ); + listed_post_ids.extend(post_listings.iter().map(|p| p.post.id)); - if let Some(p) = posts.into_iter().last() { + if let Some(p) = post_listings.into_iter().last() { page_after = Some(PaginationCursorData(p.counts)); } else { break; } } - cleanup(data, pool).await?; + inserted_post_ids.sort_unstable_by_key(|id| id.0); + listed_post_ids.sort_unstable_by_key(|id| id.0); - expected_post_ids.sort_unstable_by_key(|id| id.0); - post_ids.sort_unstable_by_key(|id| id.0); - assert_eq!(expected_post_ids, post_ids); + assert_eq!(inserted_post_ids, listed_post_ids); - Ok(()) + Community::delete(pool, inserted_community.id).await.unwrap(); + cleanup(data, pool).await; } From cc2457d3fb8238aa9bfe77a3ebf618b269c93b30 Mon Sep 17 00:00:00 2001 From: dullbananas Date: Mon, 11 Dec 2023 19:34:58 -0700 Subject: [PATCH 3/6] Update Cargo.toml --- crates/db_views/Cargo.toml | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/db_views/Cargo.toml b/crates/db_views/Cargo.toml index 847b743921..5c8fd21ebc 100644 --- a/crates/db_views/Cargo.toml +++ b/crates/db_views/Cargo.toml @@ -41,3 +41,4 @@ actix-web = { workspace = true, optional = true } [dev-dependencies] serial_test = { workspace = true } tokio = { workspace = true } +chrono = { workspace = true } From b65e011727a7db4378a1c8c35a33246fdefac28c Mon Sep 17 00:00:00 2001 From: dullbananas Date: Mon, 11 Dec 2023 19:38:55 -0700 Subject: [PATCH 4/6] Update post_view.rs --- crates/db_views/src/post_view.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index f17dd5fc4b..b206528952 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -57,9 +57,9 @@ enum Ord { struct PaginationCursorField { then_order_by_desc: fn(Q) -> Q, then_order_by_asc: fn(Q) -> Q, - le: fn(&PostAggregates) -> BoxExpr, - ge: fn(&PostAggregates) -> BoxExpr, - ne: fn(&PostAggregates) -> BoxExpr, + le: fn(&PostAggregates) -> Box>, + ge: fn(&PostAggregates) -> Box>, + ne: fn(&PostAggregates) -> Box>, } /// Returns `PaginationCursorField<_, _>` for the given name @@ -532,7 +532,7 @@ fn queries<'a>() -> Queries< let Some(cursor_data) = cursor_data else { continue; }; - let mut condition: BoxExpr<_, sql_types::Bool> = Box::new(compare(&cursor_data.0)); + let mut condition: Box> = Box::new(compare(&cursor_data.0)); // For each field that was sorted before the current one, skip the filter by changing // `condition` to `true` if the row's value doesn't equal the cursor's value. From cfa9c9bab2c4df7a2fddd953434180faecb06a09 Mon Sep 17 00:00:00 2001 From: dull b Date: Tue, 12 Dec 2023 02:47:59 +0000 Subject: [PATCH 5/6] fix --- Cargo.lock | 1 + crates/db_views/src/post_view.rs | 195 ++++++++++++++++--------------- 2 files changed, 100 insertions(+), 96 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 98c9fe8068..9deff7aaba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2693,6 +2693,7 @@ name = "lemmy_db_views" version = "0.19.0-rc.13" dependencies = [ "actix-web", + "chrono", "diesel", "diesel-async", "diesel_ltree", diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index b206528952..e41c230780 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -1,15 +1,13 @@ use crate::structs::{LocalUserView, PaginationCursor, PostView}; use diesel::{ debug_query, - dsl::{self, exists, not, IntervalDsl}, - expression::AsExpression, + dsl::{exists, not, IntervalDsl}, pg::Pg, result::Error, sql_function, - sql_types::{self, SingleValue, SqlType, Timestamptz}, + sql_types, BoolExpressionMethods, BoxableExpression, - Expression, ExpressionMethods, IntoSql, JoinOnDsl, @@ -35,7 +33,7 @@ use lemmy_db_schema::{ person_block, person_post_aggregates, post, - post_aggregates::{self, newest_comment_time}, + post_aggregates, post_like, post_read, post_saved, @@ -451,99 +449,101 @@ fn queries<'a>() -> Queries< query = query.filter(not(is_creator_blocked(person_id))); } - let featured_field = if options.community_id.is_none() || options.community_id_just_for_prefetch { - field!(featured_local) - } else { - field!(featured_community) - }; + let featured_field = if options.community_id.is_none() || options.community_id_just_for_prefetch + { + field!(featured_local) + } else { + field!(featured_community) + }; - let (main_sort, top_sort_interval) = match sort { - SortType::Active => ((Ord::Desc, field!(hot_rank_active)), None), - SortType::Hot => ((Ord::Desc, field!(hot_rank)), None), - SortType::Scaled => ((Ord::Desc, field!(scaled_rank)), None), - SortType::Controversial => ((Ord::Desc, field!(controversy_rank)), None), - SortType::New => ((Ord::Desc, field!(published)), None), - SortType::Old => ((Ord::Asc, field!(published)), None), - SortType::NewComments => ((Ord::Desc, field!(newest_comment_time)), None), - SortType::MostComments => ((Ord::Desc, field!(comments)), None), - SortType::TopAll => ((Ord::Desc, field!(score)), None), - SortType::TopYear => ((Ord::Desc, field!(score)), Some(1.years())), - SortType::TopMonth => ((Ord::Desc, field!(score)), Some(1.months())), - SortType::TopWeek => ((Ord::Desc, field!(score)), Some(1.weeks())), - SortType::TopDay => ((Ord::Desc, field!(score)), Some(1.days())), - SortType::TopHour => ((Ord::Desc, field!(score)), Some(1.hours())), - SortType::TopSixHour => ((Ord::Desc, field!(score)), Some(6.hours())), - SortType::TopTwelveHour => ((Ord::Desc, field!(score)), Some(12.hours())), - SortType::TopThreeMonths => ((Ord::Desc, field!(score)), Some(3.months())), - SortType::TopSixMonths => ((Ord::Desc, field!(score)), Some(6.months())), - SortType::TopNineMonths => ((Ord::Desc, field!(score)), Some(9.months())), - }; + let (main_sort, top_sort_interval) = match options.sort.unwrap_or(SortType::Hot) { + SortType::Active => ((Ord::Desc, field!(hot_rank_active)), None), + SortType::Hot => ((Ord::Desc, field!(hot_rank)), None), + SortType::Scaled => ((Ord::Desc, field!(scaled_rank)), None), + SortType::Controversial => ((Ord::Desc, field!(controversy_rank)), None), + SortType::New => ((Ord::Desc, field!(published)), None), + SortType::Old => ((Ord::Asc, field!(published)), None), + SortType::NewComments => ((Ord::Desc, field!(newest_comment_time)), None), + SortType::MostComments => ((Ord::Desc, field!(comments)), None), + SortType::TopAll => ((Ord::Desc, field!(score)), None), + SortType::TopYear => ((Ord::Desc, field!(score)), Some(1.years())), + SortType::TopMonth => ((Ord::Desc, field!(score)), Some(1.months())), + SortType::TopWeek => ((Ord::Desc, field!(score)), Some(1.weeks())), + SortType::TopDay => ((Ord::Desc, field!(score)), Some(1.days())), + SortType::TopHour => ((Ord::Desc, field!(score)), Some(1.hours())), + SortType::TopSixHour => ((Ord::Desc, field!(score)), Some(6.hours())), + SortType::TopTwelveHour => ((Ord::Desc, field!(score)), Some(12.hours())), + SortType::TopThreeMonths => ((Ord::Desc, field!(score)), Some(3.months())), + SortType::TopSixMonths => ((Ord::Desc, field!(score)), Some(6.months())), + SortType::TopNineMonths => ((Ord::Desc, field!(score)), Some(9.months())), + }; - if let Some(interval) = top_sort_interval { - query = query.filter(post_aggregates::published.gt(now() - interval)); - } + if let Some(interval) = top_sort_interval { + query = query.filter(post_aggregates::published.gt(now() - interval)); + } + + let tie_breaker = match options.sort.unwrap_or(SortType::Hot) { + // A second time-based sort would not be very useful + SortType::New | SortType::Old | SortType::NewComments => None, + _ => Some((Ord::Desc, field!(published))), + }; + + let sorts = [ + Some((Ord::Desc, featured_field)), + Some(main_sort), + tie_breaker, + ]; + let sorts_iter = sorts.iter().flatten(); + + // This loop does almost the same thing as sorting by and comparing tuples. If the rows were + // only sorted by 1 field called `foo` in descending order, then it would be like this: + // + // ``` + // query = query.then_order_by(foo.desc()); + // if let Some(first) = &options.page_after { + // query = query.filter(foo.le(first.foo)); + // } + // if let Some(last) = &page_before_or_equal { + // query = query.filter(foo.ge(last.foo)); + // } + // ``` + // + // If multiple rows have the same value for a sorted field, then they are + // grouped together, and the rows in that group are sorted by the next fields. + // When checking if a row is within the range determined by the cursors, a field + // that's sorted after other fields is only compared if the row and the cursor + // are in the same group created by the previous sort, which is checked by using + // `or` to skip the comparison if any previously sorted field is not equal. + for (i, (order, field)) in sorts_iter.clone().enumerate() { + // Both cursors are treated as inclusive here. `page_after` is made exclusive + // by adding `1` to the offset. + let (then_order_by_field, compare_first, compare_last) = match order { + Ord::Desc => (field.then_order_by_desc, field.le, field.ge), + Ord::Asc => (field.then_order_by_asc, field.ge, field.le), + }; - let tie_breaker = match sort { - // A second time-based sort would not be very useful - SortType::New | SortType::Old | SortType::NewComments => None, - _ => Some((Ord::Desc, field!(published))), + query = then_order_by_field(query); + + for (cursor_data, compare) in [ + (&options.page_after, compare_first), + (&options.page_before_or_equal, compare_last), + ] { + let Some(cursor_data) = cursor_data else { + continue; }; + let mut condition: Box> = + Box::new(compare(&cursor_data.0)); - let sorts = [ - Some((Ord::Desc, featured_field)), - Some(main_sort), - tie_breaker, - ]; - let sorts_iter = sorts.iter().flatten(); - - // This loop does almost the same thing as sorting by and comparing tuples. If the rows were - // only sorted by 1 field called `foo` in descending order, then it would be like this: - // - // ``` - // query = query.then_order_by(foo.desc()); - // if let Some(first) = &options.page_after { - // query = query.filter(foo.le(first.foo)); - // } - // if let Some(last) = &page_before_or_equal { - // query = query.filter(foo.ge(last.foo)); - // } - // ``` - // - // If multiple rows have the same value for a sorted field, then they are - // grouped together, and the rows in that group are sorted by the next fields. - // When checking if a row is within the range determined by the cursors, a field - // that's sorted after other fields is only compared if the row and the cursor - // are in the same group created by the previous sort, which is checked by using - // `or` to skip the comparison if any previously sorted field is not equal. - for (i, (order, field)) in sorts_iter.clone().enumerate() { - // Both cursors are treated as inclusive here. `page_after` is made exclusive - // by adding `1` to the offset. - let (then_order_by_field, compare_first, compare_last) = match order { - Ord::Desc => (field.then_order_by_desc, field.le, field.ge), - Ord::Asc => (field.then_order_by_asc, field.ge, field.le), - }; - - query = then_order_by_field(query); - - for (cursor_data, compare) in [ - (&options.page_after, compare_first), - (&page_before_or_equal, compare_last), - ] { - let Some(cursor_data) = cursor_data else { - continue; - }; - let mut condition: Box> = Box::new(compare(&cursor_data.0)); - - // For each field that was sorted before the current one, skip the filter by changing - // `condition` to `true` if the row's value doesn't equal the cursor's value. - for (_, other_field) in sorts_iter.clone().take(i) { - condition = Box::new(condition.or((other_field.ne)(&cursor_data.0))); - } - - query = query.filter(condition); - } + // For each field that was sorted before the current one, skip the filter by changing + // `condition` to `true` if the row's value doesn't equal the cursor's value. + for (_, other_field) in sorts_iter.clone().take(i) { + condition = Box::new(condition.or((other_field.ne)(&cursor_data.0))); } + query = query.filter(condition); + } + } + let (limit, mut offset) = limit_and_offset(options.page, options.limit)?; if options.page_after.is_some() { // always skip exactly one post because that's the last post of the previous page @@ -731,7 +731,7 @@ mod tests { newtypes::LanguageId, source::{ actor_language::LocalUserLanguage, - comment::{Comment, CommentInsertForm}; + comment::{Comment, CommentInsertForm}, community::{Community, CommunityInsertForm, CommunityModerator, CommunityModeratorForm}, community_block::{CommunityBlock, CommunityBlockForm}, instance::Instance, @@ -748,6 +748,7 @@ mod tests { SubscribedType, }; use serial_test::serial; + use std::time::Duration; struct Data { inserted_instance: Instance, @@ -1416,9 +1417,9 @@ mod tests { #[tokio::test] #[serial] async fn pagination_includes_each_post_once() { - let pool = &build_db_pool().await.unwrap(); + let pool = &build_db_pool_for_tests().await; let pool = &mut pool.into(); - let data = init_data(pool).await.unwrap(); + let data = init_data(pool).await; let community_form = CommunityInsertForm::builder() .name("yes".to_string()) @@ -1465,6 +1466,7 @@ mod tests { sort: Some(SortType::MostComments), limit: Some(10), page_after, + ..Default::default() } .list(pool) .await @@ -1484,11 +1486,12 @@ mod tests { assert_eq!(inserted_post_ids, listed_post_ids); - Community::delete(pool, inserted_community.id).await.unwrap(); + Community::delete(pool, inserted_community.id) + .await + .unwrap(); cleanup(data, pool).await; } - async fn cleanup(data: Data, pool: &mut DbPool<'_>) { let num_deleted = Post::delete(pool, data.inserted_post.id).await.unwrap(); Community::delete(pool, data.inserted_community.id) From c5904c3876c2762bc21cab5d1eb2d8f0501fae09 Mon Sep 17 00:00:00 2001 From: dullbananas Date: Wed, 13 Dec 2023 14:56:34 -0700 Subject: [PATCH 6/6] Update post_view.rs --- crates/db_views/src/post_view.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/db_views/src/post_view.rs b/crates/db_views/src/post_view.rs index 3319c06987..1d15c7c41d 100644 --- a/crates/db_views/src/post_view.rs +++ b/crates/db_views/src/post_view.rs @@ -46,7 +46,7 @@ use tracing::debug; sql_function!(fn coalesce(x: sql_types::Nullable, y: sql_types::BigInt) -> sql_types::BigInt); -#[derive(Clone, Copy)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] enum Ord { Desc, Asc,