Extracting pagination cursor utils into a trait. #5424

dessalines · 2025-02-13T21:36:00Z

Fixes Make a trait out of PaginationCursor / PaginationCursorData #5275

Note: This does not handle this for post_view, which is a frustrating mess right now. I'm going to open up a new issue for that one and add it to the milestone.

- Fixes #5275

dessalines · 2025-02-13T21:36:44Z

crates/api_common/src/person.rs

+  /// the pagination cursor to use to fetch the next page
+  #[cfg_attr(feature = "full", ts(optional))]
+  pub next_page: Option<PaginationCursor>,


I'd forgotten to add next_page for all of these responses.

dessalines · 2025-02-13T21:37:37Z

crates/apub/src/api/list_person_content.rs

-  // parse pagination token
-  let page_after = if let Some(pa) = &data.page_cursor {
-    Some(pa.read(&mut context.pool()).await?)
-  } else {
-    None
-  };
-  let page_back = data.page_back;
-  let type_ = data.type_;
-
  let content = PersonContentCombinedQuery {
    creator_id: person_details_id,
-    type_,
-    page_after,
-    page_back,


Instead of doing a database read for the page cursor string before, I'm now passing the page_cursor string into the list queries.

Changed my mind on this, as internal queries kept causing stack overflows.

dessalines · 2025-02-13T21:37:47Z

crates/apub/src/api/list_person_content.rs

  }
  .list(&mut context.pool(), &local_user_view)
  .await?;

-  Ok(Json(ListPersonContentResponse { content }))
+  let next_page = content.last().map(PageCursorBuilder::cursor);


This can use the trait function now.

dessalines · 2025-02-13T21:40:02Z

crates/db_schema/src/impls/combined/modlog.rs

+    query = match prefix {
+      'A' => query.filter(modlog_combined::admin_allow_instance_id.eq(id)),
+      'B' => query.filter(modlog_combined::admin_block_instance_id.eq(id)),
+      'C' => query.filter(modlog_combined::admin_purge_comment_id.eq(id)),


For the modlog, which has like 17 tables, I changed these from strings to chars.

dessalines · 2025-02-13T21:43:53Z

crates/db_schema/src/newtypes.rs

+/// A pagination cursor
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "full", derive(TS))]
+#[cfg_attr(feature = "full", ts(export))]
+pub struct PaginationCursor(pub String);
+
+impl PaginationCursor {
+  pub fn create(prefix: char, id: i32) -> Self {
+    // hex encoding to prevent ossification
+    Self(format!("{prefix}{id:x}"))
+  }
+
+  pub fn prefix_and_id(&self) -> LemmyResult<(char, i32)> {
+    let (prefix_str, id_str) = self
+      .0
+      .split_at_checked(1)
+      .ok_or(LemmyErrorType::CouldntParsePaginationToken)?;
+    let prefix = prefix_str
+      .chars()
+      .next()
+      .ok_or(LemmyErrorType::CouldntParsePaginationToken)?;
+    let id = i32::from_str_radix(id_str, 16)?;
+
+    Ok((prefix, id))


Added some functions here to help create a cursor, and extract a prefix and id from a given one.

- Must come after #5424 - Fixes #2847

crates/db_schema/src/newtypes.rs

SleeplessOne1917 · 2025-02-15T03:34:33Z

crates/db_schema/src/newtypes.rs

+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[cfg_attr(feature = "full", derive(TS))]
+#[cfg_attr(feature = "full", ts(export))]
+pub struct PaginationCursor(pub String);


I was about to make a whole detailed comment about how validation should be done at the time an instance of a struct is created and this this should have probably been pub struct PaginaitonCursor(char, i32) and strings should be parsed into this type via something like impl TryFrom<String> for PaginationCursor so that it's impossible to construct an invalid cursor, but then I noticed that this struct needs to be (de)serializable. Not sure how to use the "parse, don't validate" approach with this in mind.

I also ran into a case later, where the cursor id needs to be a string. The local_image table needs a cursor that uses the pictrs_alias as its cursor id.

I dont see where this is used for local_image so it should be possible to make the String field private.

It's part of a different pr I have in draft, where I remove the rest of page / limits.

crates/db_schema/src/traits.rs

dessalines · 2025-02-15T17:29:51Z

crates/db_views/src/combined/search_combined_view.rs

+    let mut query = PaginatedQueryBuilder::new(query);

    if self.page_back.unwrap_or_default() {
-      query = query.before(page_after).limit_and_offset_from_end();
+      query = query.before(self.cursor_data).limit_and_offset_from_end();
    } else {
-      query = query.after(page_after);
+      query = query.after(self.cursor_data);


@dullbananas is this correct? Is no limit required?

It does need a limit. The fix should be in a separate PR.

Thx. I'm trying to add the rest of cursor pagination in #5429 , when its ready I'll give you a ping.

dessalines · 2025-02-16T16:32:14Z

@Nutomic I'll wait on merging this, since we should probably merge your aggregates removal PR first.

Nutomic · 2025-02-17T13:07:12Z

crates/db_views/src/combined/modlog_combined_view.rs

+      ModlogCombinedView::ModRemoveComment(v) => ('N', v.mod_remove_comment.id.0),
+      ModlogCombinedView::ModRemoveCommunity(v) => ('O', v.mod_remove_community.id.0),
+      ModlogCombinedView::ModRemovePost(v) => ('P', v.mod_remove_post.id.0),
+      ModlogCombinedView::ModTransferCommunity(v) => ('Q', v.mod_transfer_community.id.0),


You are currently defining the mapping of db type to letter twice for each view (in case of modlog its here and in crates/db_schema/src/impls/combined/modlog.rs). This way its easy to have a mistake where the definitions are different in both places, so it should only be defined once. But to be honest I dont see any way to do that.

Anyway I would definitely prefer to have both of these next to each other, then its harder to make a mistake by changing one conversion but not the other. So PaginationCursorReader and PaginationCursorBuilder should be merged into a single trait with two methods to_cursor() and from_cursor().

Also it looks like youre missing pagination for viewing all modlog types (or all person content, all inbox types etc).

Yeah I couldn't really figure out a way to have back and forth mappings in one function, because they work on different objects.

I originally tried to use a single trait, but removed it after I realized it didn't make sense. The from_cursor should be implemented by a single db table, while the to_cursor has to exist on the view.

I'll play around a bit more and see if there's a better way to do it.

You can have both impls on the view but return a single db table anyway.

This is done now. I couldn't really figure out what to do for the All cases, since then no specific content row type ids are provided.

@dullbananas will have to give input on how to properly handle cursor pagination, because a lot of things are still missing, and I'm not sure I'm using the library properly.

For All you need to add a separate letter, and use the combined table id. In fact it makes sense to use the combined table id also when a specific type is selected.

I recommend not using the id from combined tables anywhere in the api, including in pagination cursor strings. If we use something other than combined tables in the future, then the combined id will stop working.

In that case I don't know what id we'd use for All. IMO these should all probably be fixed in a later PR anyway when you have time to help with cursor pagination. Even just having one correct example outside of post_view would help.

No change here is needed for handling All. This handles variants of ModlogCombinedView (which corresponds to a single item), not ListingType.

Nutomic · 2025-02-17T13:19:56Z

crates/api/src/local_user/notifications/list_inbox.rs

-  let page_after = if let Some(pa) = &data.page_cursor {
-    Some(pa.read(&mut context.pool()).await?)
+  let cursor_data = if let Some(cursor) = &data.page_cursor {
+    Some(InboxCombined::from_cursor(cursor, &mut context.pool()).await?)
  } else {
    None
  };


This logic belongs to the database, so change it here to pass the unchanged page_cursor as query paramter, and call from_cursor inside the query method. Same for all other types. Generally all the pagination logic for eg SearchCombinedView should be contained within search_combined_view.rs, not spread over various files.

I tried that initially, (and wanted to do it that way) but that can't be done unfortunately. If you try to run multiple sync queries inside of the list functions, you end up getting untraceable stack overflows.

So all of the pre-fetched data (like extracting cursor data from a cursor string to be able to paginate), has to be done outside of, or before, the list functions.

To get working stack traces you have to comment out this line. And I dont see why it would fail, there should be no difference calling a method outside of list or inside. Maybe you need to move the method call further to the top, before doing joins.

I tried putting it in both places, but it still failed. As long as the calls to convert from a pagination string to pagination data struct are pretty simple, then IMO its fine to have them outside of the list function, and have the list function take in the pagination data.

crates/db_schema/src/traits.rs

Extracting pagination cursor utils into a trait.

ad75528

- Fixes #5275

dessalines commented Feb 13, 2025

View reviewed changes

dessalines mentioned this pull request Feb 13, 2025

Clean up pagination and other code in post_view.rs #5425

Open

12 tasks

dessalines added 4 commits February 13, 2025 23:08

Merge remote-tracking branch 'origin/main' into pagination_cursor_trait

2109461

Refactoring to avoid stack overflows.

40e4883

Merge remote-tracking branch 'origin/main' into pagination_cursor_trait

47b893c

Fixing api_common feature.

9ec468f

dessalines marked this pull request as ready for review February 14, 2025 15:55

dessalines requested review from Nutomic, phiresky, dullbananas and SleeplessOne1917 as code owners February 14, 2025 15:55

dessalines added a commit that referenced this pull request Feb 14, 2025

Adding pagination for GetBannedPersons.

e9ecc92

- Must come after #5424 - Fixes #2847

dessalines mentioned this pull request Feb 14, 2025

Adding pagination for GetBannedPersons #5428

Draft

SleeplessOne1917 reviewed Feb 15, 2025

View reviewed changes

crates/db_schema/src/newtypes.rs Outdated Show resolved Hide resolved

SleeplessOne1917 reviewed Feb 15, 2025

View reviewed changes

crates/db_schema/src/traits.rs Outdated Show resolved Hide resolved

Rename the traits and paginationcursor::new

d96e9db

dessalines commented Feb 15, 2025

View reviewed changes

dullbananas approved these changes Feb 16, 2025

View reviewed changes

dullbananas mentioned this pull request Feb 16, 2025

Make forward pagination cursor strings different from backward #5434

Open

5 tasks

Nutomic reviewed Feb 17, 2025

View reviewed changes

crates/db_schema/src/traits.rs Show resolved Hide resolved

dessalines added 3 commits February 18, 2025 11:18

Merge remote-tracking branch 'origin/main' into pagination_cursor_trait

dc81051

Using combined trait.

902c4dd

Merge remote-tracking branch 'origin/main' into pagination_cursor_trait

e67828e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extracting pagination cursor utils into a trait. #5424

Extracting pagination cursor utils into a trait. #5424

dessalines commented Feb 13, 2025

dessalines Feb 13, 2025

dessalines Feb 13, 2025

dessalines Feb 14, 2025

dessalines Feb 13, 2025

dessalines Feb 13, 2025

dessalines Feb 13, 2025

SleeplessOne1917 Feb 15, 2025

dessalines Feb 15, 2025

Nutomic Feb 17, 2025

dessalines Feb 17, 2025

dessalines Feb 15, 2025

dullbananas Feb 15, 2025

dessalines Feb 16, 2025

dessalines commented Feb 16, 2025

Nutomic Feb 17, 2025 •

edited

Loading

Nutomic Feb 17, 2025 •

edited

Loading

dessalines Feb 17, 2025

Nutomic Feb 17, 2025

dessalines Feb 18, 2025

Nutomic Feb 21, 2025

dullbananas Feb 21, 2025

dessalines Feb 22, 2025

dullbananas Feb 22, 2025

Nutomic Feb 17, 2025 •

edited

Loading

dessalines Feb 18, 2025

Nutomic Feb 20, 2025

dessalines Feb 20, 2025

Extracting pagination cursor utils into a trait. #5424

Are you sure you want to change the base?

Extracting pagination cursor utils into a trait. #5424

Conversation

dessalines commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines commented Feb 16, 2025

Nutomic Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Nutomic Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic Feb 17, 2025 •

edited

Loading

Nutomic Feb 17, 2025 •

edited

Loading

Nutomic Feb 17, 2025 •

edited

Loading