Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

events: Keep data of unknown relations #1581

Merged
merged 2 commits into from
Jul 6, 2023
Merged

Conversation

zecakeh
Copy link
Contributor

@zecakeh zecakeh commented Jun 17, 2023

So I thought that being able to catch the data of unknown relations would be as simple as adding an untagged variant, thanks to the latest release, but it seems like it doesn't work.

For some reason the relation field of RelatesToJsonRepr is None with an unknown rel_type (see tests)… I'm putting the code here to make sure this is a serde issue rather than me not seeing something with our code. Maybe it's an issue with having a flatten above…


Preview: https://pr-1581--ruma-docs.surge.sh

@zecakeh zecakeh requested a review from a team June 17, 2023 14:00
@zecakeh zecakeh force-pushed the zecakeh/custom-relation branch 3 times, most recently from 17814cb to 761771a Compare June 24, 2023 12:47
@zecakeh
Copy link
Contributor Author

zecakeh commented Jun 24, 2023

So it turns out untagged doesn't work in our cases for deserialization. I ended up using JsonValue instead of Box<RawJsonValue> for deserializing relations of the room::message module though.

@zecakeh zecakeh force-pushed the zecakeh/custom-relation branch 2 times, most recently from de9a5a6 to 38294bd Compare June 27, 2023 07:59
@zecakeh
Copy link
Contributor Author

zecakeh commented Jun 27, 2023

Rebased on main to solve CI issue.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Re. usage of serde_json::Value, can you explain why it's necessary? Haven't actually found the place it's used yet from skimming. It's quite bad perf-wise so I'd like to avoid it where possible.

crates/ruma-appservice-api/Cargo.toml Outdated Show resolved Hide resolved
crates/ruma-common/src/events/encrypted.rs Outdated Show resolved Hide resolved
@zecakeh
Copy link
Contributor Author

zecakeh commented Jul 4, 2023

Re. usage of serde_json::Value, can you explain why it's necessary? Haven't actually found the place it's used yet from skimming. It's quite bad perf-wise so I'd like to avoid it where possible.

I don't know if it is necessary per se, but I can't find another way to deserialize it and keep custom relation data. The untagged variant attribute doesn't work and neither does deserializing m.relates_to in a Box<RawJsonValue>, for some reason.

@zecakeh
Copy link
Contributor Author

zecakeh commented Jul 4, 2023

Rebased on main

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Haven't reviewed thoroughly, but AFAICT we have new tests, and the JsonObject deserialization only happens for custom relations, not for known ones. Based on that, I'm happy to merge.

@zecakeh
Copy link
Contributor Author

zecakeh commented Jul 5, 2023

Alright I finally found a way to do it without JsonValue. I got some inspiration from the description of serde-rs/serde#2403 (the PR that added the untagged attributes to enum variants): separate the enum between a tagged enum for known relations an untagged one to get the custom relation if the tagged ones don't match, and it works.

My guess is that this doesn't work with the attribute because the rel_type field is consumed trying to match the tagged variants, so we can't get it in the custom variant and deserialization fails.

crates/ruma-common/src/events/room/encrypted.rs Outdated Show resolved Hide resolved
Comment on lines +96 to +98
#[derive(Deserialize)]
#[serde(untagged)]
enum RelationDeHelper {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, this isn't much better than serde_json::Value I think (does internal buffering). I have a different idea..

#[derive(Deserialize)]
struct EventWithRelatesToDeHelper<C> {
#[serde(rename = "m.relates_to")]
relates_to: Option<RelatesToDeHelper>,
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a boxed RawJsonValue and try different deserializers on that?

Copy link
Member

@jplatte jplatte Jul 5, 2023

Choose a reason for hiding this comment

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

If you don't feel like it though we can also merge now and I'll try to revisit myself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have been trying to do that in different ways, and for some reason whenever I try to deserialize something into a Box<RawJsonValue>, it always triggers this error on types that are not RoomMessageEventContent (so the extensible events types):

Error("invalid type: newtype struct, expected any valid JSON value", line: 1, column: 296)

Copy link
Member

@jplatte jplatte Jul 6, 2023

Choose a reason for hiding this comment

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

Sounds like we're using internal buffering elsewhere. I guess that makes sense. Let's merge.

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Nice!

@zecakeh zecakeh enabled auto-merge (rebase) July 6, 2023 15:01
@zecakeh zecakeh merged commit 07bc060 into main Jul 6, 2023
21 checks passed
@zecakeh zecakeh deleted the zecakeh/custom-relation branch July 6, 2023 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants