#133 Moving to ruma-monorepo

Merged
timo merged 15 commits from DevinR528/conduit:ruma-mono into master 2 months 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 3 months 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 3 months 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 3 months ago

Can you fix the EventJson types on your ruma branch?

Can you fix the EventJson types on your ruma branch?
DevinR528 commented 3 months 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 2 months 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 3 months 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 3 months 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 3 months 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 3 months 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 3 months ago

Why flat_map?

Why flat_map?
DevinR528 commented 3 months 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 3 months 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 3 months 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 3 months 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 3 months 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 3 months 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 3 months 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 3 months 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 3 months ago

Can you make this function take ServerNameRef as an argument?

Can you make this function take ServerNameRef as an argument?
DevinR528 commented 3 months 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 3 months 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 3 months 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 3 months ago

Is it easy to tweak this to fix #130?

Is it easy to tweak this to fix #130?
DevinR528 commented 3 months 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 3 months 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 3 months ago

I prefer not using .map each time

I prefer not using .map each time
DevinR528 commented 3 months 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 3 months 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 3 months 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.
DevinR528 commented 2 months ago
Poster

This PR is blocked until work on the ruma-client-api Outgoing trait happens. This will most likely allow servers to send non EventJson wrapped events while clients can still receive EventJson wrapped events.

@jplatte

This PR is blocked until work on the ruma-client-api `Outgoing` trait happens. This will most likely allow servers to send non `EventJson` wrapped events while clients can still receive `EventJson` wrapped events. @jplatte
jplatte commented 2 months ago
Collaborator

So I know I wrote somewhere that we could re-remove EventJson wrapping on the sending side, but I take that back.

The reason for this is that it would otherwise become impossible to implement federation – unknown fields in known events would get removed by passing through a homeserver implemented using Ruma.

So I know I wrote somewhere that we could re-remove `EventJson` wrapping on the sending side, but I take that back. The reason for this is that it would otherwise become impossible to implement federation – unknown fields in known events would get removed by passing through a homeserver implemented using Ruma.
DevinR528 changed title from WIP: Moving to ruma-monorepo to Moving to ruma-monorepo 2 months ago
DevinR528 commented 2 months ago
Poster

The only issue left is that this uses my fork of ruma/ruma where I rebased your backups and cross-singing branches into master at that time. Nothing has changed API wise for servers but I’d imagine you want the features.

The only issue left is that this uses my fork of ruma/ruma where I rebased your backups and cross-singing branches into master at that time. Nothing has changed API wise for servers but I'd imagine you want the features.
DevinR528 commented 2 months ago
Poster

This uses the new ruma features that we added to allow unspec’ed stuff in. I would imagine this is basically the API ruma will stick with for the crates.io release but @jplatte would know for sure.

This uses the new ruma features that we added to allow unspec'ed stuff in. I would imagine this is basically the API ruma will stick with for the crates.io release but @jplatte would know for sure.
timo requested changes 2 months ago
Cargo.toml
@@ -44,1 +32,3 @@
#ruma-identifiers = { path = "../ruma/ruma-identifiers" }
git = "https://github.com/ruma/ruma"
features = ["rand", "client-api", "federation-api", "unstable-pre-spec", "unstable-synapse-quirks"]
rev = "848b225"
timo commented 2 months ago

Can you make this dependency a oneliner, like with rocket?

Can you make this dependency a oneliner, like with rocket?
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -66,3 +65,3 @@
topic,
},
EventJson, EventType,
AnyBasicEvent, AnyEphemeralRoomEvent, AnyEvent as EduEvent, EventJson, EventType,
timo commented 2 months ago

Maybe we should avoid the ‘as’, it might be

Maybe we should avoid the 'as', it might be
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -287,3 +286,2 @@
access_token: token,
home_server: Some(db.globals.server_name().to_owned()),
device_id,
home_server: Some(db.globals.server_name().to_string()),
timo commented 2 months ago

Why doesn’t this take the ServerName type?

Why doesn't this take the ServerName type?
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -288,2 +287,2 @@
home_server: Some(db.globals.server_name().to_owned()),
device_id,
home_server: Some(db.globals.server_name().to_string()),
device_id: device_id.to_string(),
timo commented 2 months ago

Why to string here? What type is it?

Why to string here? What type is it?
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -607,0 +606,4 @@
.deserialize()
.map_err(|_| Error::bad_database("Deserialization of account data failed"));

if let EduEvent::Basic(data) = data? {
timo commented 2 months ago

Put the ? at the end of the ‘let data’

Put the ? at the end of the 'let data'
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -607,0 +608,4 @@

if let EduEvent::Basic(data) = data? {
Ok(get_global_account_data::Response {
account_data: EventJson::from(data),
timo commented 2 months ago

.into() here?

.into() here?
DevinR528 commented 2 months ago

That’s how every other Response is, I’m assuming it converts it to the rocket type? The compiler fails without it.

That's how every other `Response` is, I'm assuming it converts it to the rocket type? The compiler fails without it.
timo commented 2 months ago

Sorry, I asw asking if you could replace the EventJson::from with .

Sorry, I asw asking if you could replace the EventJson::from with .
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -1206,0 +1220,4 @@
.creation_content
.as_ref()
.and_then(|c| c.predecessor.clone());
content.room_version = RoomVersionId::version_6();
timo commented 2 months ago

Why this instead of the struct syntax?

Why this instead of the struct syntax?
DevinR528 commented 2 months ago

All the constructors are eventually moving to #[non_exaustive] but at some point there will be a dedicated way to do this (a macro I think).

All the constructors are eventually moving to `#[non_exaustive]` but at some point there will be a dedicated way to do this (a macro I think).
timo commented 2 months ago

Hmm... it’s okay I guess. I’d like my code to exhaustively match though, so I can’t forget fields by accident. I’m okay with my code breaking when II update the major or minor version of ruma

Hmm... it's okay I guess. I'd like my code to exhaustively match though, so I can't forget fields by accident. I'm okay with my code breaking when II update the major or minor version of ruma
DevinR528 commented 2 months ago

Hmm that’s a good point, @jplatte is moving to the non_exaustive to avoid breaking changes if/when fields are changed which would be nice. But the safety provided by knowing nothing was missed is good, maybe he has some thoughts?

Hmm that's a good point, @jplatte is moving to the non_exaustive to avoid breaking changes if/when fields are changed which would be nice. But the safety provided by knowing nothing was missed is good, maybe he has some thoughts?
jplatte commented 2 months ago

@timo I’m not sure that can be supported in a nice way. We have caret dependencies (the default) and so you wouldn’t have to pin just ruma, but also ruma-events and ruma-*-api.

@timo I'm not sure that can be supported in a nice way. We have caret dependencies (the default) and so you wouldn't have to pin just `ruma`, but also `ruma-events` and `ruma-*-api`.
src/client_server.rs
@@ -1517,3 +1524,3 @@
Ok(get_alias::Response {
room_id,
servers: vec![db.globals.server_name().to_owned()],
servers: vec![db.globals.server_name().to_string()],
timo commented 2 months ago

Why does Ruma take strings here? Is this todo?

Why does Ruma take strings here? Is this todo?
src/client_server.rs
@@ -2541,0 +2548,4 @@
if let Ok(EduEvent::Basic(account_event)) = v.deserialize() {
Some(EventJson::from(account_event))
} else {
None
timo commented 2 months ago

Call Error::bad_database here to make it emit a warning

Call Error::bad_database here to make it emit a warning
DevinR528 marked this conversation as resolved
src/database.rs
@@ -62,6 +62,7 @@ impl Database {
.to_owned())
})?;

println!("{:?}", path);
timo commented 2 months ago

Remove this

Remove this
DevinR528 marked this conversation as resolved
src/database/globals.rs
@@ -30,1 +32,3 @@
.to_owned(),
.to_string()
.try_into()
.map_err(|_| crate::Error::bad_database("Private or public keys are invalid."))?,
timo commented 2 months ago

import crate::Error

import crate::Error
DevinR528 marked this conversation as resolved
src/database/rooms.rs
@@ -516,3 +516,3 @@
room_id: room_id.clone(),
sender: sender.clone(),
origin: globals.server_name().to_owned(),
origin: globals.server_name().to_string(),
timo commented 2 months ago

You could change the pdu definition to take ServerName

You could change the pdu definition to take ServerName
DevinR528 marked this conversation as resolved
src/database/rooms/edus.rs
@@ -236,3 +237,3 @@
Ok(ruma::events::typing::TypingEvent {
content: ruma::events::typing::TypingEventContent { user_ids },
room_id: None, // Can be inferred
room_id: room_id.clone(), // Can be inferred
timo commented 2 months ago

Comment is outdated (and I’d still like this to be None)

Comment is outdated (and I'd still like this to be None)
timo commented 2 months ago

Actually maybe this should be a stub and not have that field at all

Actually maybe this should be a stub and not have that field at all
DevinR528 marked this conversation as resolved
src/pdu.rs
@@ -92,0 +94,4 @@
serde_json::from_str::<EventJson<AnyStateEvent>>(&json)
.expect("EventJson::from_str always works")
}
pub fn to_state_event_stub(&self) -> EventJson<AnySyncStateEvent> {
timo commented 2 months ago

Can we rename these methods a bit? For example to_sync_state_event() instead of stub?

Can we rename these methods a bit? For example to_sync_state_event() instead of stub?
DevinR528 marked this conversation as resolved
DevinR528 commented 2 months ago
Poster

So after changing roomactives_all to return the sync (no room_id) version the entire sync_route function is using AnySyncEphemeralRoomEvent enum variants which is the correct type according to the ruma::api::sync_events::Response. The last review still open just needs a PR to ruma, but since your pinned to a rev I’m hoping your ready to just merge this and I/someone can take care of the get_alias::Response field type change to ServerName in another PR 🥺 . ;D

So after changing `roomactives_all` to return the sync (no room_id) version the entire sync_route function is using `AnySyncEphemeralRoomEvent` enum variants which is the correct type according to the `ruma::api::sync_events::Response`. The last review still open just needs a PR to ruma, but since your pinned to a rev I'm hoping your ready to just merge this and I/someone can take care of the `get_alias::Response` field type change to `ServerName` in another PR 🥺 . ;D
timo requested changes 2 months ago
timo left a comment

Looks good!

src/client_server.rs
@@ -604,3 +604,3 @@
.ok_or(Error::BadRequest(ErrorKind::NotFound, "Data not found."))?;

Ok(get_global_account_data::Response { account_data: data }.into())
let data: AnyEvent = data
timo commented 2 months ago

Can we avoid shadowing datahere by chaining them?

Can we avoid shadowing `data`here by chaining them?
DevinR528 marked this conversation as resolved
src/client_server.rs
@@ -2542,0 +2550,4 @@
if let Ok(AnyEvent::Basic(account_event)) = v.deserialize() {
Ok(EventJson::from(account_event))
} else {
Err(Error::bad_database("found invalid event"))
timo commented 2 months ago

Change the error to “Invalid account event in database.”

Change the error to "Invalid account event in database."
timo commented 2 months ago

Also use filter_map here (and change ok and err to some and none) to remove the Result<> in the collect

Also use filter_map here (and change ok and err to some and none) to remove the Result<> in the collect
timo commented 2 months ago

Maybe also use map_err().ok() or something to avoid branching

Maybe also use map_err().ok() or something to avoid branching
src/client_server.rs
@@ -2684,0 +2697,4 @@
Err(Error::bad_database("found invalid event"))
}
})
.collect::<Result<Vec<_>, _>>()?,
timo commented 2 months ago

same here

same here
DevinR528 marked this conversation as resolved
src/database/globals.rs
@@ -30,1 +32,3 @@
.to_owned(),
.to_string()
.try_into()
.map_err(|_| Error::bad_database("Private or public keys are invalid."))?,
timo commented 2 months ago

I think this is a wrong error message? Should be a BadConfig too

I think this is a wrong error message? Should be a BadConfig too
DevinR528 marked this conversation as resolved
timo approved these changes 2 months ago
timo merged commit 678f33acf9 into master 2 months ago
DevinR528 deleted branch ruma-mono 2 months ago

Reviewers

timo approved these changes 2 months ago
The pull request has been merged as 678f33acf9.
Sign in to join this conversation.
No reviewers
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.