feat(matrix/space): implemented `space.board` events #16

avdb13 · 2024-01-26T17:27:58Z

Stumbled upon some difficulties here since I wanted to do stuff correct without changing the API later.
I chose to separate post from reply to prevent having a separate topic event. Votes for reply are calculated through m.reaction but since it can form a relation with any event we can't enforce votes on this event type only.
I was not sure if we needed an enum for post since we might consider having different body types in the future such as a poll or media content.

Stumbled upon some difficulties here since I wanted to do stuff correct without changing the API later. I chose to separate `post` from `reply` to prevent having a separate `topic` event. Votes for `reply` are calculated through `m.reaction` but since it can form a relation with any event we can't enforce votes on this event type only. I was not sure if we needed an enum for `post` since we might consider having different `body` types in the future such as a poll or media content.

Access token is still absent since I'm still not sure how we will store them. Another consideration to make is whether to require `room_id` for replies since we *could* retrieve for the event being replied to and extract it from there.

crates/core/src/db/entities/legacy.rs

EstebanBorai · 2024-01-28T21:06:37Z

crates/core/src/events/service.rs

+    pub async fn all_events(&self) -> Result<Vec<LegacyEntity>> {
+        let statement = "
+            SELECT
+                e.event_id,
+                js.json,
+                ms.display_name,
+                ms.avatar_url,
+                ra.room_alias,
+                RIGHT(e.event_id, 11) AS slug,
+                js.json :: json ->> 'origin_server_ts' AS edited_on,
+                0 AS replies,
+            FROM
+                events e
+                LEFT JOIN event_json js USING(event_id)
+                LEFT JOIN room_aliases ra USING(room_id)
+                LEFT JOIN event_relations er ON er.relates_to_id = e.event_id
+                LEFT JOIN redactions ON redacts = e.event_id
+                LEFT JOIN membership_state ms ON ms.user_id = e.sender
+                AND ms.room_id = e.room_id
+            WHERE
+                e.origin_server_ts < $1
+                AND e.type = 'space.board.post'
+                AND redacts IS NULL
+                AND aliases.room_alias IS NOT NULL
+        ";


Is there a way to achieve this via API? I would rather avoid querying database directly.

This is possible for Matrix events but our problem is that the response is paginated (see below).
I also decided to avoid plain SQL and go with sea_query instead as we wanna support SQLite and RocksDB in the future.
https://matrix-org.github.io/synapse/latest/admin_api/rooms.html

I think we can use filters and search terms for this perhaps?

EstebanBorai · 2024-01-28T21:06:58Z

crates/core/src/events/service.rs

+    pub async fn all_replies(&self) -> Result<Vec<()>> {
+        unimplemented!()
+    }


Please avoid committing unimplemented functions.

EstebanBorai · 2024-01-28T21:07:45Z

crates/core/src/lib.rs

@@ -1,15 +1,19 @@
 pub mod account;
 pub mod auth;
+pub mod db;


Direct database access is not encouraged, unless core contains an owned DB I suggest using the Synapse API.

I would use SQL Read Ops as last resort, its safer to use API as DAL.

No direct database access should be happening.

…rrors

avdb13 · 2024-02-02T22:37:34Z

Excuses for not writing any tests yet by the way!

EstebanBorai · 2024-02-03T02:50:03Z

Cargo.toml

@@ -9,6 +9,7 @@ default-members = ["crates/server"]
 resolver = "1"

 [workspace.dependencies]
+assert_matches2 = "0.1.2"


Why is this required?

This is used for tests.
See: https://docs.rs/assert_matches2/latest/assert_matches2

EstebanBorai · 2024-02-03T02:51:07Z

crates/core/src/events/service.rs

+        let mut tmp = (*self.admin).clone();
+        tmp.set_token(access_token.to_string()).map_err(|err| {
+            tracing::error!(?err, "Failed to set access token");
+            Error::Unknown
+        })?;
+
+        tmp.put_json(path, &body).await.map_err(|err| {
+            tracing::error!(?err, "Failed to make http request");
+            Error::Unknown
+        })
+    }


Please specify error cause. Context is defined in the API

Could you elaborate on context? The first error can only happen when the token is of the wrong format or empty, while the second one can be related to a wrong access token or malformed body.
Returning Error::Unknown here is not ideal, is that what you want to be changed?

EstebanBorai · 2024-02-03T02:52:05Z

crates/core/src/events/service.rs

+    pub async fn send_post<S: AsRef<str>>(
+        &self,
+        content: Raw<BoardPostEventContent>,
+        board_id: S,


Suggested change

board_id: S,

board_id: impl AsRef<str>,

Given that this is the single param that uses generic value

EstebanBorai

Thanks for giving this a shot!
I see this PR involves many features all at once in different layers of abstraction. I suggest split it into different PRs to address each concern at its own level.

I suggest using API calls over DB conns for DAL as well.

And please add tests, its important code is validated and that we can ensure it doesn't breaks with future changes!

I can see theres many types of events addressed all at once, perhaps adding the event abstraction to matrix crate is a good start for a PR, then introduce one of the event types and finally introduce it to the core logic so its presented (this could be a second PR) at this point testing against matrix and core logic its easy (refer to test crate for examples).

Keep it up!!

EstebanBorai · 2024-02-03T02:52:27Z

crates/core/src/events/service.rs

+        let resp = self
+            .put_json(endpoint, &content, access_token)
+            .await
+            .map_err(|err| {
+                tracing::error!(?err, "Failed to forward custom event");
+                Error::Unknown
+            })?;


Please map API error codes

EstebanBorai · 2024-02-03T02:53:32Z

crates/core/src/events/service.rs

+        let data: CreateEventResponse = resp.json().await.map_err(|err| {
+            tracing::error!(?err, "Failed deserialize Synapse response");
+            Error::Unknown
+        })?;


Given that API version is pinned this shouldnt happen. Can you add tests for response bodies ad deserialization?

EstebanBorai · 2024-02-03T02:54:54Z

crates/core/src/lib.rs

+    pub const POSTGRES_HOST: &str = "POSTGRES_HOST";
+    pub const POSTGRES_USER: &str = "POSTGRES_USER";
+    pub const POSTGRES_PASSWORD: &str = "POSTGRES_PASSWORD";
+    pub const POSTGRES_DB: &str = "POSTGRES_DB";


DB connection is not recommended. I suggest using API please.

EstebanBorai · 2024-02-03T02:55:50Z

crates/matrix/src/client/resources/events.rs

+use anyhow::Result;
+use ruma_common::serde::Raw;
+use ruma_common::{OwnedEventId, OwnedRoomId, TransactionId};
+


Suggested change

We should have CI/CD set up for formatting.

EstebanBorai · 2024-02-03T02:56:11Z

crates/matrix/src/client/resources/events.rs

+
+#[derive(Serialize, Deserialize)]
+pub struct CreateEventResponse {
+    event_id: String,


Use newtype please

EstebanBorai · 2024-02-03T02:57:33Z

crates/server/src/router/api/v1/events/legacy/mod.rs

Can you elaborate a bit on why is this called legacy, please?

I already explained this ... it was to keep backward compatibility. Seems like it doesn't matter since it's only making implementation harder than necessary.

EstebanBorai · 2024-02-03T02:58:14Z

crates/server/src/router/api/v1/events/legacy/mod.rs

+        let event = Router::new()
+            .route("/", get(unimplemented))
+            .route("/thread", get(unimplemented))
+            .route("/replies", get(unimplemented))
+            .merge(protected);
+
+        let events = Router::new()
+            .route("/", get(unimplemented))
+            .route("/:room", get(unimplemented));


Avoid pushing unimplemented code please.

Suggested change

let event = Router::new()

.route("/", get(unimplemented))

.route("/thread", get(unimplemented))

.route("/replies", get(unimplemented))

.merge(protected);

let events = Router::new()

.route("/", get(unimplemented))

.route("/:room", get(unimplemented));

avdb13 added 2 commits January 26, 2024 22:23

feat(core/db): initialize database on startup

fe33e2b

avdb13 force-pushed the events branch 2 times, most recently from c73b837 to b0fab96 Compare January 27, 2024 18:47

avdb13 force-pushed the events branch from cdb3ee1 to a6aa659 Compare January 28, 2024 00:04

EstebanBorai reviewed Jan 28, 2024

View reviewed changes

avdb13 force-pushed the events branch 2 times, most recently from f9271c6 to ef21bd9 Compare January 30, 2024 14:30

chore: fix room endpoints for Admin API

9147534

avdb13 force-pushed the events branch from ef21bd9 to 9147534 Compare January 30, 2024 14:39

avdb13 and others added 12 commits January 30, 2024 21:29

chore: cleanup for space events

2e27104

chore: test deserializing for space events

a40925b

refactor: make EventsService parameter types more strict

aeb63f5

Merge branch 'main' into events

29ffbb1

chore: move type validation to the service layer and provide useful e…

cdbd29f

…rrors

fix: make AccessToken convertable to Secret

ad4c374

fix: complete convenience constructors and deserialization tests

2520e07

fix: remove state events we might not need

71d255a

feat: generic handler to retrieve room messages

883fb23

feat: set access token for client requests

1e4c31d

feat: complete events service layer

c9b63ac

feat: add event creation to the router

dd15ada

EstebanBorai reviewed Feb 3, 2024

View reviewed changes

EstebanBorai requested changes Feb 3, 2024

View reviewed changes

avdb13 closed this Feb 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(matrix/space): implemented `space.board` events #16

feat(matrix/space): implemented `space.board` events #16

avdb13 commented Jan 26, 2024

EstebanBorai Jan 28, 2024

avdb13 Jan 29, 2024

EstebanBorai Feb 3, 2024

EstebanBorai Jan 28, 2024

EstebanBorai Jan 28, 2024

EstebanBorai Feb 3, 2024

avdb13 Feb 5, 2024 •

edited

Loading

avdb13 commented Feb 2, 2024

EstebanBorai Feb 3, 2024

avdb13 Feb 5, 2024

EstebanBorai Feb 3, 2024

avdb13 Feb 5, 2024

EstebanBorai Feb 3, 2024

EstebanBorai left a comment

EstebanBorai Feb 3, 2024

EstebanBorai Feb 3, 2024

EstebanBorai Feb 3, 2024

EstebanBorai Feb 3, 2024

avdb13 Feb 5, 2024 •

edited

Loading

EstebanBorai Feb 3, 2024

EstebanBorai Feb 3, 2024

avdb13 Feb 5, 2024

EstebanBorai Feb 3, 2024

feat(matrix/space): implemented space.board events #16

feat(matrix/space): implemented space.board events #16

Conversation

avdb13 commented Jan 26, 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

Choose a reason for hiding this comment

avdb13 Feb 5, 2024 • edited Loading

Choose a reason for hiding this comment

avdb13 commented Feb 2, 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

EstebanBorai 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

avdb13 Feb 5, 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

feat(matrix/space): implemented `space.board` events #16

feat(matrix/space): implemented `space.board` events #16

avdb13 Feb 5, 2024 •

edited

Loading

avdb13 Feb 5, 2024 •

edited

Loading