#133 WIP: Moving to ruma-monorepo

Open
DevinR528 wants to merge 5 commits from DevinR528/conduit:ruma-mono into master
DevinR528 commented 1 week ago

Since this is using my branch of ruma this is mostly just to show you what changed (not expecting this to be merged until the actual ruma crate/repo can be used? I would probably start from a fresh PR anyways). If you have questions or ideas just let me know!

P.S. dang rocket gets annoying with all those warnings from the #[get(... macro 😆.

- [x] I agree to release my code and all other changes of this PR under the AGPL-3.0 license Since this is using my branch of `ruma` this is mostly just to show you what changed (not expecting this to be merged until the actual ruma crate/repo can be used? I would probably start from a fresh PR anyways). If you have questions or ideas just let me know! P.S. dang rocket gets annoying with all those warnings from the `#[get(...` macro 😆.
timo requested changes 1 week ago
Cargo.toml
@@ -42,2 +40,4 @@
# ruma-serde = { git = "https://github.com/ruma/ruma", rev = "baa87104569b45dc07a9a7a16d3c7592ab8f4d6b" }
# ruma-identifiers = { git = "https://github.com/ruma/ruma", rev = "baa87104569b45dc07a9a7a16d3c7592ab8f4d6b" }
#ruma-common = { path = "../ruma/ruma-common" }
#ruma-serde = { path = "../ruma/ruma-serde" }
timo commented 1 week ago

Can this section be removed completely now?

Can this section be removed completely now?
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -481,0 +481,4 @@
let data: Result<EduEvent, Error> = data.deserialize().map_err(Into::into);
match data? {
EduEvent::Basic(data) => Ok(get_global_account_data::Response {
account_data: EventJson::from(data),
timo commented 1 week ago

Can you fix the EventJson types on your ruma branch?

Can you fix the EventJson types on your ruma branch?
DevinR528 commented 1 week ago

The EventJson is still helpful for client implementations, I just meant that anywhere in conduit we could get rid of it. Should I start working on that, could be a seperate PR but whatever works for you.

The `EventJson` is still helpful for client implementations, I just meant that anywhere in conduit we could get rid of it. Should I start working on that, could be a seperate PR but whatever works for you.
timo commented 5 days ago

Could we use the Response -> IncomingResponse magic to get rid of EventJsons?

Could we use the Response -> IncomingResponse magic to get rid of EventJsons?
src/client_server.rs
@@ -481,0 +484,4 @@
account_data: EventJson::from(data),
}
.into()),
_ => panic!("timo what do i do here"),
timo commented 1 week ago

Use if let and return Err(Error::bad_database(...)) in the else case (see line ~371)

Use if let and return Err(Error::bad_database(...)) in the else case (see line ~371)
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -876,0 +880,4 @@
EduEvent::Ephemeral(AnyEphemeralRoomEvent::Receipt(
ruma::events::receipt::ReceiptEvent {
content: ruma::events::receipt::ReceiptEventContent(receipt_content),
room_id: body.room_id.clone(),
timo commented 1 week ago

Can the room_id stay None? We don’t need to send it in this case.

Can the room_id stay None? We don't need to send it in this case.
jplatte commented 1 week ago

room_ids are no longer optional, instead there’s AnyWhateverEventStub enums that don’t contain room_ids (soon to be renamed AnySyncWhateverEvent).

`room_id`s are no longer optional, instead there's `AnyWhateverEventStub` enums that don't contain `room_id`s (soon to be renamed `AnySyncWhateverEvent`).
src/client_server.rs
@@ -2218,0 +2227,4 @@
.filter_map(|r| {
if let Ok(EduEvent::Ephemeral(ev)) = r.deserialize() {
// TODO we could get rid of EventJson?
Some(EventJson::from(ev))
timo commented 1 week ago

^

^
src/client_server.rs
@@ -2241,3 +2258,3 @@
.changes_since(Some(&room_id), &user_id, since)?
.into_iter()
.map(|(_, v)| v)
.flat_map(|(_, v)| {
timo commented 1 week ago

Why flat_map?

Why flat_map?
DevinR528 commented 1 week ago

This removes anything that isn’t a basic event, account_data is a vec of AnyEvent::Basic.

The layout of the events is, starting from the top:

/// The full event group, full as in they have room_id's
AnyEvent {
    Basic(AnyBasicEvent),
    Message(AnyMessageEvent),
    State(AnyStateEvent),
    Ephemeral(...),
}
AnyRoomEvent {
    Message(AnyMessageEvent),
    State(AnyStateEvent).
}
/// the room_id less version
AnyRoomEventStub {
    Message(AnyMessageEventStub),
    State(AnyStateEventStub),
}

// the Any*Event[Stub] are like the old Event,
// StateEvent and RoomEvent enums
AnyStateEvent[Stub] {
    RoomAliases(StateEvent[Stub]<AliasesEventContent>),
    RoomAvatar(...),
    RoomJoinRules(...),
    // ...
}

Is there another way to do this that would be better?

This removes anything that isn't a basic event, `account_data` is a vec of `AnyEvent::Basic`. The layout of the events is, starting from the top: ```rust /// The full event group, full as in they have room_id's AnyEvent { Basic(AnyBasicEvent), Message(AnyMessageEvent), State(AnyStateEvent), Ephemeral(...), } AnyRoomEvent { Message(AnyMessageEvent), State(AnyStateEvent). } /// the room_id less version AnyRoomEventStub { Message(AnyMessageEventStub), State(AnyStateEventStub), } // the Any*Event[Stub] are like the old Event, // StateEvent and RoomEvent enums AnyStateEvent[Stub] { RoomAliases(StateEvent[Stub]<AliasesEventContent>), RoomAvatar(...), RoomJoinRules(...), // ... } ``` Is there another way to do this that would be better?
DevinR528 commented 1 week ago

Or did you just mean use filter_map like you do everywhere else?

Or did you just mean use `filter_map` like you do everywhere else?
timo commented 1 week ago

Yeah, I prefer filter_map. Btw, should we print an error if the if let fails? Under what circumstances will that happen?

Yeah, I prefer filter_map. Btw, should we print an error if the if let fails? Under what circumstances will that happen?
DevinR528 commented 1 week ago

It would run the else block if deserialization failed but also if an event other than an account_data event (AnyEvent::Basic) is found. If db.account_data.changes_since(... ever returns event json with more than just a content field it will run the else block.

It would run the else block if deserialization failed but also if an event other than an account_data event (`AnyEvent::Basic`) is found. If `db.account_data.changes_since(...` ever returns event json with more than just a content field it will run the else block.
src/client_server.rs
@@ -2564,0 +2603,4 @@
.limit
.try_into()
.map_or(Ok::<_, Error>(10_usize), |l: u32| Ok(l as usize))?;

timo commented 1 week ago

This is the same in both match branches, can you move it before the match to avoid this duplication?

This is the same in both match branches, can you move it before the match to avoid this duplication?
DevinR528 marked this conversation as resolved
src/database/globals.rs
@@ -31,0 +32,4 @@
.unwrap_or("localhost")
.to_owned(),
)
.map_err(|_| Error::bad_database("Invalid server name"))?,
timo commented 1 week ago

Make this BadConfig instead of bad_database

Make this BadConfig instead of bad_database
DevinR528 marked this conversation as resolved
src/database/globals.rs
@@ -62,2 +67,2 @@
pub fn server_name(&self) -> &str {
&self.server_name
pub fn server_name(&self) -> ServerNameRef<'_> {
self.server_name.as_ref()
timo commented 1 week ago

Why is the return type ServerNameRef instead of &ServerName? Is this like &String and &str?

Why is the return type ServerNameRef instead of &ServerName? Is this like &String and &str?
jplatte commented 1 week ago

It’s because custom DSTs are complicated, so currently the ref types are not &IdType but IdType<&str> (which IdTypeRef is an alias for). We found a solution for making them DSTs though, somebody just needs to implement it :)

It's because custom DSTs are complicated, so currently the ref types are not `&IdType` but `IdType<&str>` (which `IdTypeRef` is an alias for). We found a solution for making them DSTs though, somebody just needs to implement it :)
src/database/rooms.rs
@@ -493,3 +493,3 @@
let mut pdu_json = serde_json::to_value(&pdu).expect("event is valid, we just created it");
ruma::signatures::hash_and_sign_event(
globals.server_name(),
globals.server_name().as_str(),
timo commented 1 week ago

Can you make this function take ServerNameRef as an argument?

Can you make this function take ServerNameRef as an argument?
DevinR528 commented 1 week ago

I have little to no understanding of ruma_signatures but looking at the docs for the functions

/// * entity_id: The identifier of the entity creating the signature. Generally this means a
/// homeserver, e.g. “example.com”.

not sure what “generally” implies in this situation?

I have little to no understanding of ruma_signatures but looking at the docs for the functions > /// * entity_id: The identifier of the entity creating the signature. Generally this means a > /// homeserver, e.g. "example.com". not sure what "generally" implies in this situation?
src/database/rooms/edus.rs
@@ -63,2 +63,4 @@
since: u64,
) -> Result<impl Iterator<Item = Result<EventJson<EduEvent>>>> {
// TODO is this ^^^^^^^
// only ever a read receipt could we just return EphemeralRoomEvent here?
timo commented 1 week ago

My initial idea was to keep it general so multiple EDUs can be saved using the same methods. RoomLatests ended up only saving the read receipt. Remove the comments for now, I’ll clean it up later :)

My initial idea was to keep it general so multiple EDUs can be saved using the same methods. RoomLatests ended up only saving the read receipt. Remove the comments for now, I'll clean it up later :)
DevinR528 marked this conversation as resolved
src/error.rs
@@ -28,0 +29,4 @@
SerdeError {
#[from]
source: serde_json::Error,
},
timo commented 1 week ago

Remove this please, we can use .map_err to convert these error types to bad_database or BadRequest

Remove this please, we can use .map_err to convert these error types to bad_database or BadRequest
DevinR528 marked this conversation as resolved
src/pdu.rs
@@ -96,1 +101,4 @@
pub fn to_stripped_state_event(&self) -> EventJson<AnyStrippedStateEventStub> {
let json = serde_json::to_string(&self).expect("PDUs are always valid");
serde_json::from_str::<EventJson<AnyStrippedStateEventStub>>(&json)
.expect("EventJson::from_str always works")
timo commented 1 week ago

Is it easy to tweak this to fix #130?

Is it easy to tweak this to fix #130?
DevinR528 commented 1 week ago

Yes I can take a look at exactly what it would entail but I’m pretty sure it would be easy. ;)

Yes I can take a look at exactly what it would entail but I'm pretty sure it would be easy. ;)
DevinR528 commented 1 week ago
let invited_room = sync_events::InvitedRoom {
    invite_state: sync_events::InviteState {
        events: db
            .rooms
            .room_state(&room_id)?
            .into_iter()
            .map(|(_, pdu)| pdu.to_stripped_state_event())
            .map(EventJson::from)
            .collect(),
    },
};

you could make to_stripped_state_event return AnyStrippedStateEvent and then .map(EventJson::from) then you konw it’s actually correct?

```rust let invited_room = sync_events::InvitedRoom { invite_state: sync_events::InviteState { events: db .rooms .room_state(&room_id)? .into_iter() .map(|(_, pdu)| pdu.to_stripped_state_event()) .map(EventJson::from) .collect(), }, }; ``` you could make `to_stripped_state_event` return `AnyStrippedStateEvent` and then `.map(EventJson::from)` then you konw it's actually correct?
timo commented 1 week ago

I prefer not using .map each time

I prefer not using .map each time
DevinR528 commented 1 week ago

This seems to relate some to removing EventJson from conduit as much as possible, if we want to avoid the map, I don’t see a way around it. Basically, there is no way around a ser/deser cycle to check the type.

This seems to relate some to removing EventJson from conduit as much as possible, if we want to avoid the map, I don't see a way around it. Basically, there is no way around a ser/deser cycle to check the type.
timo commented 1 week ago
Owner

Thanks for your work, there’s some cleanup left to do, but I’m happy that we can use the latest Ruma again soon :)

Thanks for your work, there's some cleanup left to do, but I'm happy that we can use the latest Ruma again soon :)
DevinR528 commented 1 week ago
Poster

Just pushing up my progress that way you can see what i’ve marked resolved I’ve actually done.

Just pushing up my progress that way you can see what i've marked resolved I've actually done.

Reviewers

timo requested changes 1 week ago
This pull request is marked as a work in progress. Remove the WIP: prefix from the title when it's ready
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.