Feature/custom emoji and tagline views #4580

Freakazoid182 · 2024-03-31T15:52:54Z

Add paged endpoints for custom_emoji and tagline, as per #4577

Does not remove the resources from GetSiteResponse.

@dessalines:

How to handle breaking changes to GetSiteResponse? Can custom_emojis and taglines only be removed on api/v4?
Taglines are now created, updated and deleted through the site update endpoint. Do you want complete separation of this resource, like with custom_emoji?

crates/db_views/src/tagline_view.rs

crates/api_crud/src/custom_emoji/list.rs

Nutomic · 2024-04-02T12:54:33Z

How to handle breaking changes to GetSiteResponse? Can custom_emojis and taglines only be removed on api/v4?

We can publish this in 0.19.x, and then remove the GetSiteResponse fields in 0.20.0. That way api clients have some time to do the migration.

dessalines · 2024-04-02T15:09:32Z

How to handle breaking changes to GetSiteResponse? Can custom_emojis and taglines only be removed on api/v4?

I've added the breaking change label, so we'll wait till after this upcoming release to merge this. Its unavoidably a breaking change tho, because taglines and emojis are going to come back with separate endpoints.

@Nutomic I'd rather just push this to 0.20

dessalines · 2024-04-02T15:11:06Z

crates/api_common/src/custom_emoji.rs

+#[cfg_attr(feature = "full", ts(export))]
+/// Fetches a list of registration applications.
+pub struct ListCustomEmojis {
+  pub page: Option<i64>,


@makotech222 look into your emoji library, because these may need a search term, optional category, etc.

I've added a category search parameter. This column is also already indexed

crates/api_crud/src/tagline/list.rs

crates/db_views/src/structs.rs

crates/db_views/src/tagline_view.rs

Freakazoid182 · 2024-04-02T20:30:04Z

How to handle breaking changes to GetSiteResponse? Can custom_emojis and taglines only be removed on api/v4?

I've added the breaking change label, so we'll wait till after this upcoming release to merge this. Its unavoidably a breaking change tho, because taglines and emojis are going to come back with separate endpoints.

@Nutomic I'd rather just push this to 0.20

Confirming: This means we'll remove the custom_emojis and taglines from GetSiteResponse in this PR right?

dessalines · 2024-04-02T21:20:05Z

yes. IMO a single random single tagline can could back with getsiteresponse also, so that front ends can use a random tagline. But proper editing of taglines would be the new endpoints.

Freakazoid182 · 2024-04-02T21:31:36Z

yes. IMO a single random single tagline can could back with getsiteresponse also, so that front ends can use a random tagline. But proper editing of taglines would be the new endpoints.

Understood. I'll also need to add the create, update and delete endpoints for taglines then indeed.

Freakazoid182 · 2024-04-04T21:13:36Z

yes. IMO a single random single tagline can could back with getsiteresponse also, so that front ends can use a random tagline. But proper editing of taglines would be the new endpoints.

I've added new endpoints to edit taglines, and GetSiteResponse returns a random tagline. Please check if you're happy with the solution of getting a single random tagline from the database. Testing it with a handful of taglines seems to work fine.

dessalines

Looking good so far!

Also can you mark this as a draft PR? I'm mainly afraid of one of us prematurely merging it, since it has some breaking changes. After this upcoming release, we'll be doing breaking changes again, and one of us can mark it as ready for review.

crates/db_schema/src/source/tagline.rs

crates/api_crud/src/tagline/update.rs

crates/db_schema/src/impls/tagline.rs

src/api_routes_http.rs

Remove superfluous properties

Nutomic · 2024-04-08T13:50:03Z

crates/utils/src/utils/validation.rs

@@ -22,6 +22,8 @@ const BIO_MAX_LENGTH: usize = 300;
 const ALT_TEXT_MAX_LENGTH: usize = 300;
 const SITE_NAME_MAX_LENGTH: usize = 20;
 const SITE_NAME_MIN_LENGTH: usize = 1;
+const TAGLINE_CONTENT_MIN_LENGTH: usize = 1;
+const TAGLINE_CONTENT_MAX_LENGTH: usize = 50000;


This seems extremely long, same as max post body. I think 200 or so would be enough.

Btw only is_valid_post_title() calls trim() before length check. Would probably make sense to trim inside of min_length_check and min_length_check so that it applies to all strings.

cc @dessalines

Hexbear uses taglines far longer than 200 characters. Lemmygrad as well.

I took 50000 which is the default max body length. Seems like too much indeed. Maybe 500 chars?

I checked hexbear and lemmygrad taglines manually, longest one I noticed is 1170 chars. That seems extremely long but if we want to support it, we can set a limit of 1500 or 2000 chars.

IMO we shouldn't be too opinionated in the back-end about the limits. If sites want to take up an entire screen with a tagline, that's up to them.

Agreed @dessalines. Shall we keep it as is, or remove the max limit completely?

As is seems fine to me. You've externalized it so we can always tweak it later easily.

crates/utils/src/error.rs

Nutomic · 2024-04-08T13:55:56Z

crates/db_views/src/custom_emoji_view.rs

+      )
+      .into_boxed();
+
+    query = query.filter(custom_emoji::local_site_id.eq(for_local_site_id));


This is unnecessary because all emojis have the same local site id. Honestly you can remove the local_site_id column, it doesnt seem to have any use.

I made a similar point regarding taglines.

I've removed the local_site_id from both custom_emoji and tagline. It's all done in a single commit, so it should be easy to revert if it turns out to be needed some reason.

crates/api_common/src/tagline.rs

crates/api_crud/src/tagline/create.rs

crates/db_schema/src/source/tagline.rs

crates/db_schema/src/impls/tagline.rs

SleeplessOne1917 · 2024-04-08T17:04:27Z

crates/db_views/src/custom_emoji_view.rs

+      )
+      .into_boxed();
+
+    query = query.filter(custom_emoji::local_site_id.eq(for_local_site_id));


I made a similar point regarding taglines.

dessalines

We also need feedback from @makotech222 , because we need to tie in how async loading of emojis, and searching, will work in the front ends.

crates/db_schema/src/source/tagline.rs

makotech222 · 2024-04-10T14:59:52Z

We also need feedback from @makotech222 , because we need to tie in how async loading of emojis, and searching, will work in the front ends.

Like i said, I don't think separating out the emoji list into pages is a worthwhile endeavor. Taglines is fine, because we technically only need a random one on page load and then you can paginate for admin page.

Its up to you guys how you want to do the frontend side if you paginate emojis. You'd have to make sure Tribute and EmojiMart can both support it. Just remember its going to require a network call + a db call to handle each and every search anytime emojis are used.

Freakazoid182 · 2024-04-11T19:55:29Z

We also need feedback from @makotech222 , because we need to tie in how async loading of emojis, and searching, will work in the front ends.

Like i said, I don't think separating out the emoji list into pages is a worthwhile endeavor. Taglines is fine, because we technically only need a random one on page load and then you can paginate for admin page.

Its up to you guys how you want to do the frontend side if you paginate emojis. You'd have to make sure Tribute and EmojiMart can both support it. Just remember its going to require a network call + a db call to handle each and every search anytime emojis are used.

I only gave the Tribute and EmojiMart code a quick look. It does seem that for EmojiMart all emojis needs to be loaded when constructing the Picker. In any case, we can put back the emojis on the GetSiteResponse, or all Emojis need to be retrieved on site render through an separate API call. Or we keep it like now, and another solution will have to be found.

I think having the paged API for custom emojis is fine to have for the admin functionality.

dessalines · 2024-04-11T20:51:39Z

It does seem that for EmojiMart all emojis needs to be loaded when constructing the Picker. In any case, we can put back the emojis on the GetSiteResponse, or all Emojis need to be retrieved on site render through an separate API call. Or we keep it like now, and another solution will have to be found.

Dang. Well let's allow an override on the paging limits on the new ListCustomEmojis endpoint you added, so that other requests to GetSite aren't hit with such a massive emoji list. There are a few examples of this override in the code, lmk if you need help finding them.

In the future tho we should look to either add async to emojimart, or find a new library, because fetching thousands of emojis all at once isn't going to scale up well.

makotech222 · 2024-04-12T06:11:17Z

Just want to note, hexbear probably has the most expansive emoji and tagline list in lemmyverse. Our getsiteresponse in its entirety is about 1.7mb. Not that bad

EmojiPicker needs to be able to retrieve all emojis in 1 call

Freakazoid182 · 2024-04-14T10:48:21Z

There are a few examples of this override in the code, lmk if you need help finding them.

The only thing I could find was on the LocalImage db_schema (get_all_helper). There's a ignore_page_limits bool there. I've added this to the ListCustomEmojis query.

Freakazoid182 · 2024-04-14T10:51:30Z

I noticed that on the GetSiteResponse API, there's a short lived cache to limit calls to the database. It it worth it to add this to ListCustomEmojis for requests that use ignore_page_limits?

Nutomic · 2024-04-15T10:31:08Z

Yes it would be much simpler with caching. Caching different pages would be possible with cache key (category, page, limit), but an attacker could bypass it by calling with various different parameters. So I think its better to get rid of pagination entirely for taglines and emojis, so that there is only a single possible response which can be cached very well. This also makes it much easier for clients to implement.

dessalines · 2024-04-16T03:24:51Z

Its not just about the fetch size, its also about limiting the amount of info a front end page has to render and display.

Let's say you're editing emojis and taglines. You don't want to have to render, load images for, and scroll through 800 emojis all at once.

As for caching, I think that's on us to add it if we think its necessary, not you.

dessalines · 2024-04-16T03:27:44Z

crates/db_views/src/custom_emoji_view.rs

@@ -35,16 +35,49 @@ impl CustomEmojiView {
    }
  }

-  pub async fn get_all(
+  pub async fn get_all(pool: &mut DbPool<'_>) -> Result<Vec<Self>, Error> {


You can probably get rid of this one now. Or leave it as a helper function, but have it call `self.list(.... true)

dessalines · 2024-04-16T03:31:47Z

crates/db_views/src/custom_emoji_view.rs

+      query = query.filter(custom_emoji::category.eq(category))
+    }
+
+    let emojis = query
      .order(custom_emoji::category)


The category ordering should probably only happen if they supplied a category, just to be faster. So you could add .order_by(category) into the if let Some(category) above

then add query = query.then_order_by(id) below that block.

Nutomic · 2024-04-16T12:47:21Z

Keeping pagination is also fine. In that case it makes no sense to do caching in Rust, better leave it to nginx. That is already handled by cache-control headers which Lemmy sends for unauthenticated users.

Only keep get_all als helper function calling list with paging ignored Only order on category when filtering on category

Freakazoid182 · 2024-04-16T20:06:16Z

As for caching, I think that's on us to add it if we think its necessary, not you.

In that case it makes no sense to do caching in Rust, better leave it to nginx

Sounds good to me.

Keeping pagination is also fine.

I'll keep the the paging as it is then

Freakazoid182 added 3 commits March 31, 2024 16:49

Add custom_emoji list route

fd27523

Add tagline list route

b4a8615

Apply linting

10a8f01

Nutomic reviewed Apr 2, 2024

View reviewed changes

crates/db_views/src/tagline_view.rs Outdated Show resolved Hide resolved

Nutomic reviewed Apr 2, 2024

View reviewed changes

crates/api_crud/src/custom_emoji/list.rs Outdated Show resolved Hide resolved

dessalines added the breaking change label Apr 2, 2024

dessalines reviewed Apr 2, 2024

View reviewed changes

Remove unecessary TaglineView

17e2834

Add category filter for custom emoji

486c746

Freakazoid182 added 6 commits April 3, 2024 22:14

Add create tagline endpoint

a1a7e2c

Add update tagline endpoint

ab93806

Add delete tagline endpoint

e536d39

Format through lint.sh

5358908

Remove custom_emojis and taglines from site resource

2b1fdf1

Get random tagline on site requets

f1b993f

Freakazoid182 marked this pull request as ready for review April 5, 2024 07:27

Freakazoid182 requested review from phiresky and SleeplessOne1917 as code owners April 5, 2024 07:27

dessalines reviewed Apr 5, 2024

View reviewed changes

dessalines mentioned this pull request Apr 5, 2024

Add .optional() to database fetches that shouldn't throw an error for zero-result single-row-fetches. #4601

Closed

5 tasks

Freakazoid182 marked this pull request as draft April 5, 2024 19:59

Freakazoid182 added 3 commits April 5, 2024 22:36

Impl Crud for Tagline

144c112

Remove superfluous properties

Move tagline endpoints under /admin

5342a22

Impl Crud for CustomEmoji

bab417a

Freakazoid182 force-pushed the feature/custom-emoji-and-tagline-views branch from 2d7272e to 6ffcc1f Compare April 7, 2024 09:09

Nutomic reviewed Apr 8, 2024

View reviewed changes

crates/utils/src/error.rs Outdated Show resolved Hide resolved

Nutomic reviewed Apr 8, 2024

View reviewed changes

SleeplessOne1917 requested changes Apr 8, 2024

View reviewed changes

SleeplessOne1917 and others added 5 commits April 8, 2024 17:07

Merge branch 'main' into feature/custom-emoji-and-tagline-views

5cf15a4

Consolidate Tagline error types

7ce89fb

Remove unecessary clone

8c7b300

Updat misleading comments

562c909

Remove local_site_id from tagline and custom_emoji

6935acb

Nutomic approved these changes Apr 9, 2024

View reviewed changes

dessalines reviewed Apr 10, 2024

View reviewed changes

crates/db_schema/src/source/tagline.rs Show resolved Hide resolved

Update TaglineInserForm and TaglineUpdateForm

999e984

SleeplessOne1917 approved these changes Apr 11, 2024

View reviewed changes

Add ignore_page_limits for custom emojis

1f629cc

EmojiPicker needs to be able to retrieve all emojis in 1 call

dessalines reviewed Apr 16, 2024

View reviewed changes

Update custom_emoji_view

4771085

Only keep get_all als helper function calling list with paging ignored Only order on category when filtering on category

Merge branch 'main' into feature/custom-emoji-and-tagline-views

920adcc

dessalines added this to the 0.20.0 milestone Apr 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/custom emoji and tagline views #4580

Feature/custom emoji and tagline views #4580

Freakazoid182 commented Mar 31, 2024

Nutomic commented Apr 2, 2024

dessalines commented Apr 2, 2024

dessalines Apr 2, 2024

Freakazoid182 Apr 2, 2024

Freakazoid182 commented Apr 2, 2024

dessalines commented Apr 2, 2024 •

edited

Freakazoid182 commented Apr 2, 2024

Freakazoid182 commented Apr 4, 2024

dessalines left a comment

Nutomic Apr 8, 2024

Nutomic Apr 8, 2024

SleeplessOne1917 Apr 8, 2024

Freakazoid182 Apr 8, 2024

Nutomic Apr 9, 2024

dessalines Apr 10, 2024

Freakazoid182 Apr 11, 2024

dessalines Apr 11, 2024

Nutomic Apr 8, 2024

SleeplessOne1917 Apr 8, 2024

Freakazoid182 Apr 8, 2024

SleeplessOne1917 Apr 8, 2024

dessalines left a comment

makotech222 commented Apr 10, 2024

Freakazoid182 commented Apr 11, 2024

dessalines commented Apr 11, 2024

makotech222 commented Apr 12, 2024

Freakazoid182 commented Apr 14, 2024

Freakazoid182 commented Apr 14, 2024

Nutomic commented Apr 15, 2024

dessalines commented Apr 16, 2024

dessalines Apr 16, 2024

dessalines Apr 16, 2024 •

edited

Nutomic commented Apr 16, 2024

Freakazoid182 commented Apr 16, 2024

Feature/custom emoji and tagline views #4580

Are you sure you want to change the base?

Feature/custom emoji and tagline views #4580

Conversation

Freakazoid182 commented Mar 31, 2024

Nutomic commented Apr 2, 2024

dessalines commented Apr 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freakazoid182 commented Apr 2, 2024

dessalines commented Apr 2, 2024 • edited

Freakazoid182 commented Apr 2, 2024

Freakazoid182 commented Apr 4, 2024

dessalines left a 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

Choose a reason for hiding this comment

dessalines left a comment

Choose a reason for hiding this comment

makotech222 commented Apr 10, 2024

Freakazoid182 commented Apr 11, 2024

dessalines commented Apr 11, 2024

makotech222 commented Apr 12, 2024

Freakazoid182 commented Apr 14, 2024

Freakazoid182 commented Apr 14, 2024

Nutomic commented Apr 15, 2024

dessalines commented Apr 16, 2024

Choose a reason for hiding this comment

dessalines Apr 16, 2024 • edited

Choose a reason for hiding this comment

Nutomic commented Apr 16, 2024

Freakazoid182 commented Apr 16, 2024

dessalines commented Apr 2, 2024 •

edited

dessalines Apr 16, 2024 •

edited