Community post tags (part 2: API methods) #5389

phiresky · 2025-02-04T22:01:21Z

This is the second part of post tags, implementing the API methods.

Database PR (merged): #4997
Original RFC: LemmyNet/rfcs#4

src/api_routes_v4.rs

crates/api_common/src/post.rs

crates/api/src/post/tags.rs

phiresky · 2025-02-07T21:16:01Z

Ready for review. the API tests pass locally for me, but require merging LemmyNet/lemmy-js-client#483

dessalines

What's the slug thing about? The tag already has a name and an ap_id.

crates/api/src/community/tag.rs

crates/api_common/src/community.rs

crates/api/src/post/tags.rs

crates/api_common/src/post.rs

crates/api/src/community/tag.rs

dessalines · 2025-02-11T20:29:55Z

Do the re-request review when you want me to have another look. I thought about going through it but I'm guessing its still WIP.

phiresky · 2025-02-18T20:50:27Z

I've updated the PR

comments fixed
removed separate API methods, post create and update now do replacement of all tags (if tags given)
added tagupdateform

I'll merge main after reviews are green

Nutomic · 2025-02-20T15:30:40Z

Api tests are failing, looks like you need to merge changes from main branch here.

Nutomic · 2025-02-21T14:05:49Z

crates/db_schema/src/impls/community.rs

+    let tag_name_lower = tag_name.to_lowercase();
+    let id_slug = VALID_ID_SLUG.replace_all(&tag_name_lower, "-");
+    // limit length to 40 chars
+    let id_slug: String = id_slug.chars().take(40).collect();


You should limit length of the tag itself when it is created.

I still don't know why id_slug needs to be separate from name. Aren't tags always just simple strings? If that's so, then its the tag name that needs to be validated before inserted.

how would you propose to construct the AP URL then?

{domain}/tag/{name}, just like community and person.

name is a human readable and updateable name, with spaces, special characters, etc. not an id.

Still it doesnt need a separate length limit, tag create/edit already has a limit.

name is a human readable and updateable name, with spaces, special characters, etc. not an id.

Then we should adopt what's done for either person or community, on the tag table.

Have an unchanging, lowercase name column.

Have a title / display_name column, that can have spaces and other funky characters.

This reverts commit 62f17de.

phiresky · 2025-02-26T16:35:43Z

i've temporarily set the lemmy-js-client lib to point to this pr LemmyNet/lemmy-js-client#483 so that the tests pass. please re-review

dessalines · 2025-02-27T16:33:32Z

crates/api/src/community/tag.rs

+  if !(3..=100).contains(&length) {
+    return Err(LemmyErrorType::InvalidBodyField.into());
+  }


This check should be externalized like all the others, and a specific error made for it. Check site_name_length_check for an example

Also there is no length check in update.

dessalines · 2025-02-27T16:34:29Z

crates/api/src/community/tag.rs

+    published: None, // defaults to now
+    updated: None,
+    deleted: false,


..Default::default(), or better, the insert form shouldn't need these at all, they already have defaults.

Check how PostInsertForm uses derive_new to do this.

dessalines · 2025-02-27T16:38:02Z

crates/api/src/community/tag.rs

+  // Update the tag
+  let tag_form = TagUpdateForm {
+    name: Some(data.name.clone()),
+    updated: Some(Some(chrono::Utc::now())),


Would be better to import this, so you can just use Utc::now() like all the other examples.

dessalines · 2025-02-27T16:43:24Z

crates/db_schema/src/impls/post_tag.rs

+    delete(post_tag::table.filter(post_tag::post_id.eq(post_id)))
+      .execute(conn)
+      .await?;
+    Ok(())


have the output of this be Result<usize, diesel::result::Error>, and delete this Ok(()) line.

dessalines · 2025-02-27T16:44:28Z

crates/db_schema/src/impls/post_tag.rs

+      .values(forms)
+      .execute(conn)
+      .await?;
+    Ok(())


Same, delete this Ok(()) line, and have the output be Result<Vec<Self>, ..

dessalines · 2025-02-27T17:20:55Z

crates/db_schema/src/impls/tag.rs

+    use crate::schema::tag::dsl::*;
+    let conn = &mut get_conn(pool).await?;
+    tag
+      .filter(community_id.eq(search_community_id))


tag::community_id

dessalines · 2025-02-27T17:20:59Z

crates/db_schema/src/impls/tag.rs

+  ) -> Result<Vec<Self>, Error> {
+    use crate::schema::tag::dsl::*;
+    let conn = &mut get_conn(pool).await?;
+    tag


dessalines · 2025-02-27T17:21:05Z

crates/db_schema/src/impls/tag.rs

+    let conn = &mut get_conn(pool).await?;
+    tag
+      .filter(community_id.eq(search_community_id))
+      .filter(deleted.eq(false))


tag::deleted

dessalines · 2025-02-27T17:21:12Z

crates/db_schema/src/impls/tag.rs

+    tag
+      .filter(community_id.eq(search_community_id))
+      .filter(deleted.eq(false))
+      .load::<Tag>(conn)


dessalines · 2025-02-27T17:22:50Z

crates/db_views/src/structs.rs

+#[cfg(feature = "full")]
+#[diesel::dsl::auto_type]
+fn tag_fragment() -> _ {
+  let sel: SqlLiteral<Json> = diesel::dsl::sql::<diesel::sql_types::Json>("json_agg(tag.*)");
+  tag::table
+    .select(sel)
+    .filter(tag::community_id.eq(community::id))
+    .filter(tag::deleted.eq(false))
+    .single_value()
+}


Nice, thx that's going to make things a lot easier.

Nutomic · 2025-02-28T09:00:11Z

crates/api_common/src/tags.rs

+      })
+      .collect(),
+  )
+  .await?;


Merge these into a single method PostTag::set() which deletes old tags and sets new ones in a transaction.

Nutomic · 2025-02-28T09:00:40Z

crates/api_common/src/tags.rs

+    .await?;
+  }
+  // validate tags
+  let valid_tags: std::collections::HashSet<TagId> =


Import Hashset

Nutomic · 2025-02-28T09:01:42Z

crates/api/src/community/tag.rs

+  Ok(Json(tag))
+}
+
+#[tracing::instrument(skip(context))]


Remove these macros, see #5368

Nutomic · 2025-02-28T09:02:41Z

api_tests/src/tags.spec.ts

+
+  // List tags
+  let listRes = await alpha.getCommunity({ id: communityId });
+  expect(listRes.community_view.post_tags.length).toBeGreaterThan(0);


Suggested change

expect(listRes.community_view.post_tags.length).toBeGreaterThan(0);

expect(listRes.community_view.post_tags.length).toBe(1);

Nutomic · 2025-02-28T09:03:28Z

api_tests/src/tags.spec.ts

+  expect(
+    listRes.community_view.post_tags.find(t => t.id === createRes.id),
+  ).toBeUndefined();


Suggested change

expect(

listRes.community_view.post_tags.find(t => t.id === createRes.id),

).toBeUndefined();

expect(listRes.community_view.post_tags.length).toBe(0);

Nutomic · 2025-02-28T09:09:31Z

@phiresky The discussion at the top about table names is not resolved yet. If you have better ideas how to name them thats fine, but right now the names dont make it clear what these tables and structs are for.

#5389 (comment)

phiresky · 2025-02-28T10:30:36Z

@Nutomic the discussion you linked is about the API path, which is resolved. Tables are not even part of this PR.

Nutomic

Yes the tables were in #4997 which is part of the same feature. In that PR it wasnt clear yet how it all works, only now is it possible to review properly. So Im repeating my request again:

Yes it all needs to be made more consistent. For the main tags table community_tags makes sense, while the relation tag-post can be represented by a table community_tags_for_post.

dessalines · 2025-02-28T20:31:20Z

Here's that discussion, and I believe that we agreed on leaving a generic tag table, and validating that its an allowed tag in the back-end. (And possibly adding a tag_type enum in the future). Any specific items then get a _tag table, like here for post_tag.

So I don't believe any tag table changes (or new tables) are necessary, outside of possibly adding a display_name column.

phiresky added 4 commits February 4, 2025 22:01

post tags part 2

d69ccc8

Merge remote-tracking branch 'origin/main' into post-tags-2

ee045ad

post tags part 2 (api methods)

1f4c451

re-remove tracing

55e71b9

Nutomic reviewed Feb 5, 2025

View reviewed changes

src/api_routes_v4.rs Outdated Show resolved Hide resolved

Nutomic reviewed Feb 5, 2025

View reviewed changes

src/api_routes_v4.rs Outdated Show resolved Hide resolved

Nutomic reviewed Feb 5, 2025

View reviewed changes

crates/api_common/src/post.rs Outdated Show resolved Hide resolved

Nutomic reviewed Feb 5, 2025

View reviewed changes

crates/api/src/post/tags.rs Outdated Show resolved Hide resolved

Nutomic reviewed Feb 5, 2025

View reviewed changes

crates/api/src/post/tags.rs Outdated Show resolved Hide resolved

phiresky mentioned this pull request Feb 7, 2025

add post tags functions LemmyNet/lemmy-js-client#483

Draft

phiresky marked this pull request as ready for review February 7, 2025 21:15

phiresky requested review from dessalines, dullbananas and SleeplessOne1917 as code owners February 7, 2025 21:15

api tests

c13feef

phiresky force-pushed the post-tags-2 branch from 339d2f4 to c13feef Compare February 7, 2025 21:18

dessalines reviewed Feb 9, 2025

View reviewed changes

dullbananas approved these changes Feb 10, 2025

View reviewed changes

Nutomic reviewed Feb 10, 2025

View reviewed changes

crates/api/src/community/tag.rs Outdated Show resolved Hide resolved

phiresky added 3 commits February 11, 2025 17:06

update according to comments

5335294

update lints and api package

475f2fd

Merge remote-tracking branch 'origin/main' into post-tags-2

f920ca5

phiresky added 2 commits February 18, 2025 21:43

validate tags, fix compile

ecaa70f

insert many simultaneously

1b3e8b0

phiresky force-pushed the post-tags-2 branch from 222d18f to 1b3e8b0 Compare February 18, 2025 20:50

phiresky requested review from dessalines and Nutomic February 18, 2025 20:50

o

90038a2

temp revert for tests

62f17de

Nutomic reviewed Feb 21, 2025

View reviewed changes

phiresky added 5 commits February 26, 2025 16:13

Merge remote-tracking branch 'origin/main' into post-tags-2

f53f528

update dep

977ef8f

Revert "temp revert for tests"

b5c6d3f

This reverts commit 62f17de.

fix deps

537dcbd

limit tag length

8d8a2ad

phiresky force-pushed the post-tags-2 branch from b6013a1 to 8d8a2ad Compare February 26, 2025 15:43

phiresky requested a review from Nutomic February 26, 2025 16:27

dessalines reviewed Feb 27, 2025

View reviewed changes

Nutomic reviewed Feb 28, 2025

View reviewed changes

crates/api_common/src/tags.rs

.await?;

}

// validate tags

let valid_tags: std::collections::HashSet<TagId> =

Copy link

Member

Nutomic Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import Hashset

Nutomic reviewed Feb 28, 2025

View reviewed changes

crates/api/src/community/tag.rs

Ok(Json(tag))

}

#[tracing::instrument(skip(context))]

Copy link

Member

Nutomic Feb 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these macros, see #5368

Nutomic reviewed Feb 28, 2025

View reviewed changes

Nutomic requested changes Feb 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Community post tags (part 2: API methods) #5389

Community post tags (part 2: API methods) #5389

phiresky commented Feb 4, 2025 •

edited

Loading

phiresky commented Feb 7, 2025 •

edited

Loading

dessalines left a comment

dessalines commented Feb 11, 2025

phiresky commented Feb 18, 2025 •

edited

Loading

Nutomic commented Feb 20, 2025

Nutomic Feb 21, 2025

dessalines Feb 27, 2025

phiresky Feb 27, 2025

dessalines Feb 27, 2025 •

edited

Loading

phiresky Feb 27, 2025

Nutomic Feb 28, 2025

dessalines Feb 28, 2025

phiresky commented Feb 26, 2025

dessalines Feb 27, 2025

Nutomic Feb 28, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

dessalines Feb 27, 2025

Nutomic Feb 28, 2025

Nutomic Feb 28, 2025

Nutomic Feb 28, 2025

Nutomic Feb 28, 2025

Nutomic Feb 28, 2025

Nutomic commented Feb 28, 2025 •

edited

Loading

phiresky commented Feb 28, 2025 •

edited

Loading

Nutomic left a comment

dessalines commented Feb 28, 2025 •

edited

Loading

	expect(listRes.community_view.post_tags.length).toBeGreaterThan(0);
	expect(listRes.community_view.post_tags.length).toBe(1);

Community post tags (part 2: API methods) #5389

Are you sure you want to change the base?

Community post tags (part 2: API methods) #5389

Conversation

phiresky commented Feb 4, 2025 • edited Loading

phiresky commented Feb 7, 2025 • edited Loading

dessalines left a comment

Choose a reason for hiding this comment

dessalines commented Feb 11, 2025

phiresky commented Feb 18, 2025 • edited Loading

Nutomic commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Feb 27, 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

phiresky commented Feb 26, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic commented Feb 28, 2025 • edited Loading

phiresky commented Feb 28, 2025 • edited Loading

Nutomic left a comment

Choose a reason for hiding this comment

dessalines commented Feb 28, 2025 • edited Loading

phiresky commented Feb 4, 2025 •

edited

Loading

phiresky commented Feb 7, 2025 •

edited

Loading

phiresky commented Feb 18, 2025 •

edited

Loading

dessalines Feb 27, 2025 •

edited

Loading

Nutomic commented Feb 28, 2025 •

edited

Loading

phiresky commented Feb 28, 2025 •

edited

Loading

dessalines commented Feb 28, 2025 •

edited

Loading