Adding `report_combined` table. #5231

dessalines · 2024-11-26T14:27:20Z

Context: #2444

dessalines

Before I continue, I'd like @Nutomic , @dullbananas input on this:

This PR currently only adds the report_combined table, and I'm thinking the other ones should be separate PRs. Doing a combined table for modlog is going to be really large, and it might be better to do each combined table one PR at a time.

dessalines · 2024-11-27T21:16:27Z

crates/api_common/src/comment.rs

-#[derive(Debug, Serialize, Deserialize, Clone, Default)]
-#[cfg_attr(feature = "full", derive(TS))]
-#[cfg_attr(feature = "full", ts(export))]
-/// Report a comment.
-pub struct CreateCommentReport {
-  pub comment_id: CommentId,
-  pub reason: String,
-}


Just moved these to a reports specific mod.

dessalines · 2024-11-27T21:18:27Z

crates/api_common/src/person.rs

 pub struct GetReportCountResponse {
-  #[cfg_attr(feature = "full", ts(optional))]
-  pub community_id: Option<CommunityId>,
-  pub comment_reports: i64,
-  pub post_reports: i64,
-  #[cfg_attr(feature = "full", ts(optional))]
-  pub private_message_reports: Option<i64>,
+  pub count: i64,


Since we now have a ReportsCombinedInternal::get_report_count(), it seems pointless to get each specific count, and make the front-ends add them, but I can revert this back if necessary.

crates/db_schema/replaceable_schema/triggers.sql

dessalines · 2024-11-27T21:22:34Z

crates/db_views/src/report_combined_view.rs

Main changes are in here, and I've added tests for it.

dessalines · 2024-11-27T21:24:54Z

crates/db_views/src/structs.rs

+#[cfg_attr(feature = "full", derive(Queryable))]
+#[cfg_attr(feature = "full", diesel(check_for_backend(diesel::pg::Pg)))]
+/// A combined report view
+pub struct ReportCombinedViewInternal {


I've tried to keep this struct as organized as possible, with the specific items first, then the shared last.

src/api_routes_http.rs

dessalines · 2024-11-27T21:35:09Z

migrations/2024-11-26-115042_add_combined_tables/up.sql

+-- Creates combined tables for the following:
+--
+-- Reports: (comment, post, and private_message)
+-- Inbox: (Comment replies, post replies, comment mentions, post mentions, private messages)
+-- Modlog: (lots of types)
+-- Search: (community, post, comment, user, url)
+-- TODO not sure about these two:
+-- Home: (comment, post)
+-- Community: (comment, post)
+CREATE TABLE report_combined (


These are all the places I can think of, to do a combined enum view: Reports, Inbox, Modlog, Profile, and Search.

I'm leaning towards not doing one for Home and Community, since showing new comments next to posts isn't going to be too useful (and the front page sorts are basically useless for comments, these combined tables can only sort by published.)

dessalines · 2024-11-27T21:42:04Z

One other thing: @dullbananas can I get your assistance in using cursor pagination (using your library) instead of page / limit? It'd be a good idea to start using cursor pagination for these new combined responses.

Nutomic · 2024-11-28T14:51:54Z

I agree its better to make small changes one at a time.

migrations/2024-11-26-115042_add_combined_tables/up.sql

dullbananas · 2024-12-02T04:11:28Z

migrations/2024-11-26-115042_add_combined_tables/up.sql

+    post_report_id int REFERENCES post_report ON UPDATE CASCADE ON DELETE CASCADE,
+    comment_report_id int REFERENCES comment_report ON UPDATE CASCADE ON DELETE CASCADE,
+    private_message_report_id int REFERENCES private_message_report ON UPDATE CASCADE ON DELETE CASCADE,


Foreign key columns should always have indexes

Is that necessary here? Shouldn't we only index for read patterns, IE published desc ?

Indexes on foreign key columns speed up the operations caused by deleting referenced rows or updating the id values. The PaginationCursorData query also needs those indexes.

I'm not sure how to do that for the pagination cursor query, but I'll do that for the other columns now.

I just realized it can also be done by adding UNIQUE individually to each column, which automatically creates the indexes. I don't know if that's better.

Also there's no different index needed for the PaginationCursorData query. The indexes you added are all it needs.

I just realized it can also be done by adding UNIQUE individually to each column, which automatically creates the indexes. I don't know if that's better.

I probably can't do that here since there are a lot of nulls repeated for each row. But I've added the indexes manually anyway.

Unique constraint allows multiple null values by default https://www.postgresql.org/docs/current/ddl-constraints.html#DDL-CONSTRAINTS-UNIQUE-CONSTRAINTS

K I'll try that instead.

dullbananas · 2024-12-02T05:41:12Z

One other thing: @dullbananas can I get your assistance in using cursor pagination (using your library) instead of page / limit? It'd be a good idea to start using cursor pagination for these new combined responses.

see #5244

* add pagination cursor * store timestamp instead of id in cursor (partial) * Revert "store timestamp instead of id in cursor (partial)" This reverts commit 89359dd. * use paginated query builder

…into combined_tables_2

dessalines · 2024-12-02T21:37:30Z

crates/db_views/src/comment_report_view.rs

-> {
-  let all_joins = |query: comment_report::BoxedQuery<'a, Pg>, my_person_id: PersonId| {
-    query
+impl CommentReportView {


I simplified the specific report views to only reads now, not list. The report combined view should always be used for listing reports.

Are these read functions really needed? At least lemmy-ui doesnt seem to use them. Would also be possible to move read into ReportCombinedView, with a single /report/read endpoint which takes ReportCombinedId

Yes they are needed IMO, the reportombinedid is pretty useless, we could even skip serializing it because it doesn't help at all. (actually I think we don't serialize it, the specific views are all that come back).

The main reason its important to keep the specific reads, is that when you ask for a comment report, you might as well get a CommentReportView back, rather than having to get multiple types and coerce them. There's no reason to do that.

dessalines · 2024-12-02T21:38:36Z

crates/db_views/src/comment_report_view.rs

-
-  #[tokio::test]
-  #[serial]
-  async fn test_crud() -> LemmyResult<()> {


I simplified and moved all the tests to ReportCombinedView, since they need to call ReportCombinedView::list

dessalines · 2024-12-02T21:39:41Z

crates/db_views/src/report_combined_view.rs

The main changes are in this file.

I made a new combined reports test, and copied all the tests from the previous views, and refactored them to work with the combined list, so everything should be fine.

Nutomic · 2024-12-04T10:15:45Z

src/api_routes_http.rs

@@ -277,8 +267,12 @@ pub fn config(cfg: &mut ServiceConfig, rate_limit: &RateLimitCell) {
          .route("/save", put().to(save_comment))
          .route("/list", get().to(list_comments))
          .route("/report", post().to(create_comment_report))
-          .route("/report/resolve", put().to(resolve_comment_report))
-          .route("/report/list", get().to(list_comment_reports)),
+          .route("/report/resolve", put().to(resolve_comment_report)),


How about removing these separate report endpoints and making a single endpoint /report/resolve which takes ReportCombinedId?

Probably a not a good idea, since you still need to create report for the specific item anyway. And the reportcombinedid isn't useful for resolving.

migrations/2024-12-02-181601_add_report_combined_table/up.sql

crates/db_views/src/structs.rs

crates/db_views/src/report_combined_view.rs

api_tests/src/comment.spec.ts

dullbananas · 2024-12-07T22:39:00Z

crates/db_schema/src/newtypes.rs

+#[cfg_attr(feature = "full", derive(DieselNewType, TS))]
+#[cfg_attr(feature = "full", ts(export))]
+/// The report combined id
+pub struct ReportCombinedId(i32);


IDs for this table should be an implementation detail, so don't include TS in derive

I'll also add serde skip to that. Er maybe that won't be necessary since it never gets serialized.

Then you can remove derive(Serialize, Deserialize)?

crates/db_schema/src/source/combined/report.rs

crates/db_views/src/report_combined_view.rs

dullbananas · 2024-12-08T01:03:21Z

crates/db_views/src/report_combined_view.rs

+  } else {
+    None
+  }
+}


If the last else is supposed to be unreachable, then make this conversion happen before the vec is built by moving this function's code into a custom implementation of Queryable for ReportCombinedView that calls ReportCombinedViewInternal::build

That seems a bit hacky, since as far as sql is concerned, those columns are nullable. I'd rather keep this conversion simple, and rely on the tests to catch any issues.

Silently failing is more hacky and is important to not do in this case. Changing this to a Queryable implementation is a simple enough way to fix that, and it will also make deserialization separate from the load function. Do it like this:

impl<ST> Queryable<ST, Pg> for ReportCombinedView where ReportCombinedViewInternal: Queryable<ST, Pg>, { type Row = ReportCombinedViewInternal::Row; fn build(row: Self::Row) -> deserialize::Result<Self> { let v = ReportCombinedViewInternal::build(row)?; // body of map_to_enum function goes here, with Some and None replaced with Ok and Err(Box::new(UnexpectedNullError)) } }

migrations/2024-12-02-181601_add_report_combined_table/up.sql

Co-authored-by: dullbananas <[email protected]>

dessalines · 2024-12-09T13:24:26Z

With all these combined_tables PR's I'm working on, I'm stacking them because they use a lot of conflicting code space anyway, so I'll mark these as ready to review as each one gets finished.

api_tests/src/post.spec.ts

Nutomic · 2024-12-12T10:28:47Z

crates/db_views/src/structs.rs

+  pub item_creator_banned_from_community: bool,
+  pub item_creator_is_moderator: bool,
+  pub item_creator_blocked: bool,
+}


Can you simplify this similar to ModlogCombinedViewInternal in the other PR, or does that not work with the db query?

pub struct ReportCombinedViewInternal { pub post: Option<PostReportView>, pub comment: Option<CommentReportView>, pub private_message: Option<PrivateMessageReportView>, }

That can't work for either one. The Internal is the actual DB query that selects everything necessary, then a mapping function maps that data to the enum, which is a container for the different types.

Nutomic

Looks good to merge.

@dullbananas can you approve as well if theres nothing else?

Combined tables try 2

39b3ec2

dessalines changed the title ~~Combined tables try 2~~ WIP: Combined tables try 2 Nov 26, 2024

dessalines added 3 commits November 26, 2024 16:53

Finishing up combined report table.

1ff8ae0

Fix ts optionals.

2351c7a

Adding tests, triggers, and history updates for report_combined.

0ba961f

dessalines commented Nov 27, 2024

View reviewed changes

dessalines changed the title ~~WIP: Combined tables try 2~~ Adding report_combined table. Nov 27, 2024

dessalines commented Nov 27, 2024

View reviewed changes

Adding profile.

d8dda44

Nutomic reviewed Nov 28, 2024

View reviewed changes

migrations/2024-11-26-115042_add_combined_tables/up.sql Outdated Show resolved Hide resolved

Nutomic reviewed Nov 28, 2024

View reviewed changes

migrations/2024-11-26-115042_add_combined_tables/up.sql Outdated Show resolved Hide resolved

dessalines added 3 commits November 28, 2024 17:44

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

d68c0a6

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

a9f28af

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

3adadf9

dullbananas mentioned this pull request Dec 1, 2024

Add cursor pagination to report_combined view #5244

Merged

dullbananas reviewed Dec 2, 2024

View reviewed changes

dullbananas and others added 9 commits December 2, 2024 12:56

Add cursor pagination to report_combined view (#5244)

588e1f6

* add pagination cursor * store timestamp instead of id in cursor (partial) * Revert "store timestamp instead of id in cursor (partial)" This reverts commit 89359dd. * use paginated query builder

Merge branch 'combined_tables_2' of https://github.com/LemmyNet/lemmy …

f25d346

…into combined_tables_2

Fixing migration and paged API.

7fdbb58

Using dullbananas trigger procedure

00beccd

Removing pointless list routes, reorganizing tests.

1776de2

Fixing column XOR check.

612c2e9

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

5a8b9de

Forgot to remove list report actions.

0c854d2

Cleanup.

802a8a6

dessalines commented Dec 2, 2024

View reviewed changes

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

4674821

Nutomic reviewed Dec 4, 2024

View reviewed changes

migrations/2024-12-02-181601_add_report_combined_table/up.sql Outdated Show resolved Hide resolved

Nutomic reviewed Dec 4, 2024

View reviewed changes

crates/db_views/src/structs.rs Show resolved Hide resolved

Nutomic reviewed Dec 4, 2024

View reviewed changes

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

Nutomic reviewed Dec 4, 2024

View reviewed changes

api_tests/src/comment.spec.ts Show resolved Hide resolved

dessalines added 2 commits December 4, 2024 12:39

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

e1affa8

Addressing PR comments.

921d532

dessalines mentioned this pull request Dec 7, 2024

Adding combined person content and person saved tables. #5251

Draft

dessalines added 2 commits December 7, 2024 17:54

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

a940ce3

Fixing api tests.

67d72e3

dullbananas requested changes Dec 8, 2024

View reviewed changes

dessalines and others added 8 commits December 8, 2024 08:40

Update crates/db_views/src/report_combined_view.rs

165c7f4

Co-authored-by: dullbananas <[email protected]>

Update crates/db_views/src/report_combined_view.rs

dc0ec15

Co-authored-by: dullbananas <[email protected]>

Update crates/db_views/src/report_combined_view.rs

22d8697

Co-authored-by: dullbananas <[email protected]>

Update migrations/2024-12-02-181601_add_report_combined_table/up.sql

fa31fc3

Co-authored-by: dullbananas <[email protected]>

Update migrations/2024-12-02-181601_add_report_combined_table/up.sql

10f5e37

Co-authored-by: dullbananas <[email protected]>

Fixing import and fmt.

02bd2f6

Fixing null types in postgres.

2d482b3

Comment out err.

3e31e1c

dessalines requested review from dullbananas and Nutomic December 9, 2024 13:23

Nutomic reviewed Dec 12, 2024

View reviewed changes

api_tests/src/post.spec.ts Outdated Show resolved Hide resolved

Nutomic reviewed Dec 12, 2024

View reviewed changes

api_tests/src/post.spec.ts Outdated Show resolved Hide resolved

Nutomic reviewed Dec 12, 2024

View reviewed changes

dessalines added 4 commits December 12, 2024 09:39

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

1e6b3fb

Addressing PR comments.

c72d5e8

Removing serialization

7c962d6

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

ae9da4d

Nutomic approved these changes Dec 13, 2024

View reviewed changes

Adding report_combined table. #5231

Are you sure you want to change the base?

Adding report_combined table. #5231

Conversation

dessalines commented Nov 26, 2024 • edited Loading

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

dessalines Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

dessalines commented Nov 27, 2024 • edited Loading

Nutomic commented Nov 28, 2024

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 Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dullbananas commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dessalines Dec 10, 2024 • 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

dessalines Dec 8, 2024 • 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

dessalines commented Dec 9, 2024

Nutomic Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nutomic left a comment

Choose a reason for hiding this comment

Adding `report_combined` table. #5231

Adding `report_combined` table. #5231

dessalines commented Nov 26, 2024 •

edited

Loading

dessalines left a comment •

edited

Loading

dessalines Nov 27, 2024 •

edited

Loading

dessalines commented Nov 27, 2024 •

edited

Loading

dessalines Dec 2, 2024 •

edited

Loading

dessalines Dec 10, 2024 •

edited

Loading

dessalines Dec 8, 2024 •

edited

Loading

Nutomic Dec 12, 2024 •

edited

Loading