feat(send_queue): send redactions via the send queue#6250
feat(send_queue): send redactions via the send queue#6250Johennes wants to merge 21 commits intomatrix-org:mainfrom
Conversation
Johennes
force-pushed
the
johannes/send-queue-redactions
branch
2 times, most recently
from
48f5ec6 to
01a4c00
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6250 +/- ##
==========================================
- Coverage 89.86% 89.80% -0.06%
==========================================
Files 377 377
Lines 103167 103389 +222
Branches 103167 103389 +222
==========================================
+ Hits 92710 92851 +141
- Misses 6893 6966 +73
- Partials 3564 3572 +8 ☔ View full report in Codecov by Sentry. |
|
Sorry for the ping @bnjbvr. 🙈 Since we briefly spoke about this in the dev room, I was curious if you had any high-level thoughts on this change? I'm basically just trying to verify that this is moving into the right direction architecturally before spending more time on it. Any thoughts on this would be highly appreciated. |
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks for the ping. Proposed a slightly different implementation at the timeline level, so that users don't have to check two booleans to know if something's been redacted. Looks like you're on the right track, though!
crates/matrix-sdk-ui/src/timeline/controller/decryption_retry_task.rs
Outdated
Show resolved
Hide resolved
Johennes
force-pushed
the
johannes/send-queue-redactions
branch
3 times, most recently
from
76726b6 to
0ceef26
Compare
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Johennes
force-pushed
the
johannes/send-queue-redactions
branch
from
0ceef26 to
ba8f1bb
Compare
|
Thanks for the initial feedback. I think this is now ready for review. I've included sending redactions in the send queue and handling their local echoes in the timeline in two separate commits. There will have to be a follow-up PR to change the entry paths in the FFI and UI layer to actually use the send queue when sending redactions. I thought it might be good to exclude this here. |
Johennes
requested review from
bnjbvr and
stefanceriu
and removed request for
a team
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks, makes sense to me to keep this PR as minimal as possible, considering it's already 600LOC; thanks for thinking about this! I've posted a comment which might be more a question/a thing to double-check, with respect to the shape of the saved event in the DB (might be different based on the room version). Let me know if I'm completely wrong here :)
Inline is_local_redacted Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Remove unredacted_content ctor parameter Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Switch from then_some to then to make the operation lazy Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Add documentation for the unredact method Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Regroup match arms Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Use a room-version specific format when caching local redaction events Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
Thanks! Sorry, I spotted a few more things, including one non-trivial question that might turn into an interesting experiment…
| error: Arc::new(matrix_sdk::Error::SendQueueWedgeError(Box::new( | ||
| send_error, | ||
| ))), | ||
| is_recoverable: false, |
There was a problem hiding this comment.
shouldn't this bool be based on a field of send_error somehow?
There was a problem hiding this comment.
Hm, I think not. This is actually copied from the LocalEchoContent::Event match-arm a few lines above. It seems that send_error is of type QueueWedgeError which appears to always be unrecoverable.
/// Represents a failed to send unrecoverable error of an event sent via the
/// send queue.
| /// If a redaction for this event is currently being sent but the server | ||
| /// hasn't yet acknowledged it via its remote echo, the original content | ||
| /// before redaction. Otherwise, None. | ||
| pub(super) unredacted_content: Option<TimelineItemContent>, |
There was a problem hiding this comment.
I realize I forgot to ask the previous time, but could we store the unredacted_content in MsgLikeKind::Redacted, or would it make the code super hard to read, or expose an internal implementation detail to the consumers of these APIs? I think it would be nice in that it would make it impossible to represent impossible states, but maybe it complicates things a bit too much, WDYT?
There was a problem hiding this comment.
Hm, I'm probably entirely misunderstanding but does unredacted_content not necessarily need to hold just a normal TimelineItemContent with any MsgLikeKind because we want to be able to restore the original content upon unredaction?
There was a problem hiding this comment.
Or wait, you mean move unredacted_content from here into MsgLikeKind::Redacted? But then we'd have a TimelineItemContent inside of MsgLikeKind?
Sorry, I'm most likely just being dense. 🙈
There was a problem hiding this comment.
Yes, sorry, my comment was about moving the unredacted content field inside MsgLikeKind::Redacted, if that's possible? We might need to box it, to avoid a recursive type here…
There was a problem hiding this comment.
Ok, I see. I gave it a shot in 4560ac8 but I'm not sure how well it turned out. A part of the change bled through to the FFI layer and it looks like the recursive type might cause some more issues there since neither Box nor an enum inside an Arc seem to work over FFI. I've left things unchanged there for now meaning local redactions will still look like remote redactions over the FFI.
Lazily allocate string Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Extract bindings to simplify format statement Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Fix line breaks in comments Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Move the unredacted content into MsgLikeKind::Redacted Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
| pub(in crate::timeline) fn redact(&self, rules: &RedactionRules) -> Self { | ||
| pub(in crate::timeline) fn redact(&self, rules: &RedactionRules, is_local: bool) -> Self { | ||
| match self { | ||
| Self::MsgLike(_) | Self::CallInvite | Self::RtcNotification => { |
There was a problem hiding this comment.
I assume we should handle the other match arms below, too. I'm not sure how to apply MsgLikeKind::Redacted to them though given that they currently use variants other than TimelineItemContent::MsgLike.
There was a problem hiding this comment.
Uhh, that sounds like a good argument to keep the unredacted_content out of the MsgLike::Redacted variant. So mayyyyyybe we could revert the commit that moves the unredacted content there. Sorry for the back and forth on this, I didn't have all the requirements in my head at the time of reviewing. What do you think? Does that sound sensible to you? (If so, we could add a big doc comment next to unredacted_content meaning that it might be there for all kinds of event items, notably state events etc.)
There was a problem hiding this comment.
No worries at all. I also only realized this issue once I hit it when making the changes.
I do think the previous variant was a bit easier to understand, if maybe not as elegant. Have reverted the commit and extended the doc comment in 8e60190.
| pub(in crate::timeline) fn redact(&self, rules: &RedactionRules) -> Self { | ||
| pub(in crate::timeline) fn redact(&self, rules: &RedactionRules, is_local: bool) -> Self { | ||
| match self { | ||
| Self::MsgLike(_) | Self::CallInvite | Self::RtcNotification => { |
There was a problem hiding this comment.
Uhh, that sounds like a good argument to keep the unredacted_content out of the MsgLike::Redacted variant. So mayyyyyybe we could revert the commit that moves the unredacted content there. Sorry for the back and forth on this, I didn't have all the requirements in my head at the time of reviewing. What do you think? Does that sound sensible to you? (If so, we could add a big doc comment next to unredacted_content meaning that it might be there for all kinds of event items, notably state events etc.)
This reverts commit 4560ac8.
Extend doc comment Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
LGTM! Minor comments, feel free to address them and squash your commits as you'd like (or I'll squash-merge upon merge 👍)). Thanks for doing this!
| let Some(content) = &self.unredacted_content else { return self.clone() }; | ||
| let kind = match &self.kind { | ||
| EventTimelineItemKind::Local(l) => EventTimelineItemKind::Local(l.clone()), | ||
| EventTimelineItemKind::Remote(r) => EventTimelineItemKind::Remote(r.redact()), |
There was a problem hiding this comment.
Can you add a comment that we can't unredact a remote item, that's why we're calling redact() here? (if that's the correct reasoning behind it)
There was a problem hiding this comment.
Hm, actually I think you've found a bug here. I just copied this from the redact method above. I'm now realizing that RemoteEventTimelineItem::redact also clears some things irreversibly that we may want to revert if a local redaction of such an item is reverted. I guess we may need an unredacted_kind member?
There was a problem hiding this comment.
Ouch. Makes sense to add such fields. Maybe we can group all the unredacted_ fields together, as an Option<UnredactedItem> or so? (If you can add a that shows what would have been missing, that'd be great!)
Change redaction aggregation from local to remote when it was sent Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Move changelog entry to the correct section Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
Rename entry-point method to redact Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
2 participants
This is a first step towards #4162 and adds a way to send redactions (including their local echoes) via the send queue.
I had to introduce new variants for
SentRequestKeyandLocalEchoContentbecause in some room versions the redacted event ID sits at the top-level of the event rather than incontent.At the timeline level redactions are handled via a new boolean flag in
AggregationKind::Redaction. Local echoes of redactions merely set a flag on the timeline event whereas remote echoes of redactions lead to actual redactions as before.The FFI bindings will be updated in a follow-up PR.
CHANGELOG.mdfiles.