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

Redacted events: Use Any*Event enums for redacted events #114

Merged
merged 14 commits into from
Jul 11, 2020

Conversation

DevinR528
Copy link
Member

Sorry if opening all these is annoying, implementing it even if it's a bit off is the best way for me to understand something and I'm hoping having something to see and talk about helps everyone.

commit 1: Add most of the logic to generate the correct AnyRedactedEvent enums.
commit 2: Add the needed event content types for the 5 state events that have a content field. Create full, stub, and stripped versions of state events and full and stub versions of message event structs. Uncomment and fix redacted event tests.

Add the needed event content types for the 5 state events that have a
content field. Create full, stub and stripped versions of state events
and full and stub versions of message event structs. Uncomment and fix
redacted event tests.
@jplatte
Copy link
Member

jplatte commented Jul 7, 2020

implementing it even if it's a bit off is the best way for me to understand something and I'm hoping having something to see and talk about helps everyone

It definitely helps, no worries!

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.

First impressions are positive ^^

ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_enum.rs Show resolved Hide resolved
ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
ruma-events/src/custom.rs Outdated Show resolved Hide resolved
Add two ruma_event attributes to optionally emit the redacted event
code. Conditionally generate a unit struct to represent the redacted
events content.
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Show resolved Hide resolved
ruma-events-macros/src/event_enum.rs Show resolved Hide resolved
All redacted event code is now generated by the *EventContent derive
macro. The exception are any content structs with the custom_redaction
attribute. This leaves implementing up to the user.
ruma-events-macros/src/event_enum.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_enum.rs Show resolved Hide resolved
ruma-events/src/custom.rs Outdated Show resolved Hide resolved
ruma-events/src/custom.rs Outdated Show resolved Hide resolved
ruma-events/src/room/redaction.rs Outdated Show resolved Hide resolved
ruma-events/src/room/redaction.rs Show resolved Hide resolved
ruma-events/tests/redacted.rs Outdated Show resolved Hide resolved
ruma-events/src/lib.rs Outdated Show resolved Hide resolved
ruma-events/src/lib.rs Outdated Show resolved Hide resolved
@jplatte
Copy link
Member

jplatte commented Jul 10, 2020

I think I mentioned this before, but the (currently only) case I actually think we should have custom_redacted for (although it could be solved differently too) is m.room.aliases: its redact function actually needs a parameter for whether to redact the content or not. Thinking about this more, this also means the associated has_fields function I thought of doesn't work! We actually need a method, taking &self, to determine whether to write the content field. That shouldn't make things any more complicated, though.

@DevinR528
Copy link
Member Author

We also need redact methods on the Any*Event enums and something to deal with the fact that redacted_because is only a RedactedEvent so doesn't allow stub or stripped events. I tried out what we have so far in matrix-sdk and these were the only awkward bits.

I actually think we should have custom_redacted for (although it could be solved differently too) is m.room.aliases

You have said that before but a good reminder, I was thinking that implementations could redact their own made-up events but this can't actually happen can it (a server has no way to know what a client is up to and vice-versa)? If the only reason we have custom redacted events is aliases should we maybe remove the custom and add a RedactedV1AliasesEventContent or something like that? Do we need to have a type for the old redacted aliases event, if we got sent the JSON for an old aliases event it would (de)serialize correctly? Having the catch all and semitry is nice.

I'll start working on the RedactedEventContent and see if that opens any posibillities that would make custom easier to implement.

@jplatte
Copy link
Member

jplatte commented Jul 10, 2020

We also need redact methods on the Any*Event enums

This PR is already quite big, let's do this separately.

and something to deal with the fact that redacted_because is only a RedactedEvent so doesn't allow stub or stripped events

I don't understand what you mean, can you expand on that?

I was thinking that implementations could redact their own made-up events but this can't actually happen can it (a server has no way to know what a client is up to and vice-versa)?

Again not sure what you mean, sorry 😅

If the only reason we have custom redacted events is aliases should we maybe remove the custom and add a RedactedV1AliasesEventContent or something like that? Do we need to have a type for the old redacted aliases event, if we got sent the JSON for an old aliases event it would (de)serialize correctly? Having the catch all and semitry is nice.

I don't fully understand what your idea is here, but if you want, try it out.

@DevinR528
Copy link
Member Author

and something to deal with the fact that redacted_because is only a RedactedEvent so doesn't allow stub or stripped events

According to spec the redaction event should be inserted into the redacted_because field of unsigned, what I was getting at is that we should have a way (an enum maybe see example below) that allows this to be a stub or full event (are there stripped redaction event?)

enum AnyRedactionEvent {
    Full(RedactionEvent),
    Stub(RedactionEventStub),
    // stripped if needed
}
// then
struct UnsignedData {
    redacted_because: Option<AnyRedactionEvent>,
}

I was thinking that implementations could redact their own made-up events but this can't actually happen can it (a server has no way to know what a client is up to and vice-versa)?

I see now you weren't misunderstanding I read your comment completely wrong you were just talking about the attribute not custom events in general. So disregard that paragraph.

@jplatte
Copy link
Member

jplatte commented Jul 10, 2020

According to spec the redaction event should be inserted into the redacted_because field of unsigned, what I was getting at is that we should have a way (an enum maybe see example below) that allows this to be a stub or full event (are there stripped redaction event?)

I see! I guess the spec could be clearer about this. It definitely makes sense in my mind that redacted_because could itself contain a redacted (redaction) event. I'm not so sure about the others though. I will ask on #matrix-spec:matrix.org.

The *EventContent derive now generates the RedactedEventContent
and EventContent trait implementations.
ruma-events-macros/src/event_content.rs Show resolved Hide resolved
ruma-events/src/lib.rs Outdated Show resolved Hide resolved
ruma-events/src/room/redaction.rs Outdated Show resolved Hide resolved
ruma-events/src/room/aliases.rs Outdated Show resolved Hide resolved
ruma-events/src/lib.rs Outdated Show resolved Hide resolved
@DevinR528
Copy link
Member Author

Ah sorry about that I squashed a commit the wrong way, I should have squashed the last one into the one I ended up squashing.

oldest commit
middle // I squashed this into the commit above it
new // I should have squashed this and rewrote the commit?

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.

So the only missing thing now is the aliases field for RedactedAliasesEventContent and corresponding RedactedEventContent implementation changes, right?

ruma-events-macros/src/event_content.rs Show resolved Hide resolved
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.

So I did another full review and found a few more issues 😅

ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Outdated Show resolved Hide resolved
ruma-events-macros/src/event_content.rs Show resolved Hide resolved
ruma-events/src/room/redaction.rs Outdated Show resolved Hide resolved
ruma-events/src/room/member.rs Outdated Show resolved Hide resolved

/// A stripped-down redacted state event.
#[derive(Clone, Debug, Event)]
pub struct RedactedStrippedStateEventStub<C: RedactedStateEventContent> {
Copy link
Member

Choose a reason for hiding this comment

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

No RedactedStrippedStateEvent?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there supposed to also be StrippedStateEvent? If so there is no regular event struct for this either.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right. 😄

Yeah no, stripped state events are only ever received as part of the sync response ^^

Copy link
Member Author

Choose a reason for hiding this comment

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

So I don't need to change anything right?

Copy link
Member

Choose a reason for hiding this comment

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

No ^^

/// Any message event stub that has been redacted.
RedactedMessage(AnyRedactedMessageEventStub),
/// Any state event stub that has been redacted.
RedactedState(AnyRedactedStateEventStub),
}

Copy link
Member

@jplatte jplatte Jul 10, 2020

Choose a reason for hiding this comment

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

Maybe something for a followup PR too, but we do now need enums over AnyStateEvent | AnyRedactedStateEvent, AnyStateEventStub | AnyRedactedStateEventStub and AnyStrippedStateEventStub | AnyRedactedStrippedStateEventStub. Maybe we actually need to use a Regular prefix for the generated Message and State enums and reuse their previous named for these new enums.

Copy link
Member Author

@DevinR528 DevinR528 Jul 11, 2020

Choose a reason for hiding this comment

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

I think the rename makes sense. Probably be best to do it as a new PR, and maybe after the format_ident and fixing of the brittle enum name generation (fixing this stuff ident.to_string().replace... and making the event_enum! macro only take certain name/kind: Message)?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

ruma-events/src/custom.rs Outdated Show resolved Hide resolved
@jplatte jplatte merged commit 5e428ac into ruma:master Jul 11, 2020
@DevinR528 DevinR528 deleted the redacted2-event branch May 3, 2021 11:52
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