Add support for room v11 #987
No reviewers
Labels
No labels
Android
CS::needs customer feedback
CS::needs follow up
CS::needs on prem installation
CS::waiting
Chrome
Design:: Ready
Design:: in progress
Design::UX
E2EE
Edge
Firefox
GDPR
Iteration 13 IM
Linux
MacOS
Need::Discussion
Need::Steps to reproduce
Need::Upstream fix
Needs:: Planning
Needs::Dev-Team
Needs::More information
Needs::Priority
Needs::Product
Needs::Refinement
Needs::Severity
Priority::1-Critical
Priority::2-Max
Priority::3-Impending
Priority::4-High
Priority::5-Medium
Priority::6-Low
Priority::7-None
Progress::Backlog
Progress::Review
Progress::Started
Progress::Testing
Progress::Triage
Progress::Waiting
Reporter::Sentry
Safari
Target::Community
Target::Customer
Target::Internal
Target::PoC
Target::Security
Team:Customer-Success
Team:Design
Team:Infrastructure
Team:Instant-Messaging
Team:Product
Team:Workflows
Type::Bug
Type::Design
Type::Documentation
Type::Feature
Type::Improvement
Type::Support
Type::Tests
Windows
blocked
blocked-by-spec
cla-signed
conduit
contribution::advanced
contribution::easy
contribution::help needed
from::review
iOS
p::ti-tenant
performance
product::triage
proposal
refactor
release-blocker
s: dart_openapi_codegen
s::Famedly-Patient
s::Org-Directory
s::Passport-Generator
s::Requeuest
s:CRM
s:Famedly-App
s:Famedly-Web
s:Fhiroxide
s:Fhiroxide-cli
s:Fhiroxide-client
s:Fhirs
s:Hedwig
s:LISA
s:Matrix-Dart-SDK
s:Role-Manager
s:Synapse
s:User-Directory
s:WFS-Matrix
s:Workflow Engine
s:dtls
s:famedly-error
s:fcm-shared-isolate
s:matrix-api-lite
s:multiple-tab-detector
s:native-imaging
severity::1
severity::2
severity::3
severity::4
technical-debt
voip
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Matthias/conduit#987
Loading…
Reference in a new issue
No description provided.
Delete branch "room-v11"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Resolves https://gitlab.com/famedly/conduit/-/issues/408
Spec: https://spec.matrix.org/v1.9/rooms/v11/
changed title from {-R-}oom v11 to {+Add support for r+}oom v11
changed the description
added 4 commits
a3b8eea9
- Move "redacts" key to "content" in redaction events in v11 rooms18bfd79e
- Remove "creator" key when upgrading rooms to v11fac99503
- create_hash_and_sign_event: Use actual version of RoomCreate events, instead of the default9646439a
- Enable support for room v11Compare with previous version
v11 actually allow all keys here, and there are other changes in the redaction rules. I am wondering why this doesn't use the methods from Ruma, like
ruma::canonical_json::redact_content_in_place
, is it a type issue?Ah you're right, I misunderstood the spec.
changed this line in version 3 of the diff
added 1 commit
Compare with previous version
resolved all threads
I switched to
redact_content_in_place
, it seems to work (tested only onm.room.message
). Thanks!resolved all threads
added 1 commit
520806d4
- Use Ruma's redact_content_in_place instead of custom implementationCompare with previous version
Is this
panic!
intended? This seems highly unusual.panic!
is only used in 1 other place in this entire codebase and it's very early on during Router init, andpanic!
was not used here before.If it is, it needs to be documented why very clearly.
A
panic!
isn't very different from the various.expect()
in this function, but point taken. I'll return an error instead.The expects should still probably be replaced to avoid another https://gitlab.com/famedly/conduit/-/issues/396 where possible
changed this line in version 5 of the diff
added 1 commit
5a7bb1e8
- Return error instead of panic when first event is not m.room.createCompare with previous version
resolved all threads
mentioned in commit girlbossceo/conduwuit@f62f641545
Possibly an unpopular opinion, but if it's presumably an unreachable state, you should add
panic!()
andassert!()
s. Bugs and logic problems aren't handlable errors and should fail loudly rather than continuing in an unknown state or returning a "quiet" error.If it's guaranteed unreachable, then yes. However, here, this can happen due to bugs in the function's caller, and who knows what it may be doing. And even if it's correct now, we don't know for sure future versions will be.
is the MR ready?
I believe so, but it needs to be carefully reviewed by someone who understands Conduit better than I do
Can you write RoomVersionId::V11 to be explicit here? I think it's useful to do a full match in these places so the code is reviewed again for future versions.
We already have this function in src/service/rooms/state/mod.rs
Have you tested this in a real room already?
Can we just use baddatabase for these? That makes sure they get picked up correctly by the error handling code later in this file.
Here's my initial review, I still need to compare this to the spec
changed this line in version 6 of the diff
changed this line in version 6 of the diff
added 2 commits
eb7ac91c
- Reuse existing get_room_version8175bc12
- Explicitly match RoomVersionId::V11Compare with previous version
It would make the errors harder to debug as their only argument would be
&'static str
It's slightly different because it returns an error instead of
None
when the version is not found; I hope my change is okayyes
Some code still relies on the top level redacts field, for example in src/service/rooms/timeline/mod.rs, around line 385 there is
TimelineEventType::RoomRedaction
.I also saw that the spec mentions this:
So I think when we parse m.room.encryption events to the PduEvent type in v11, we should add a top-level redacts key if it is missing. This will not mess with signatures, because the redacts key will be redacted away anyway.
The spec also says this, but I think we can ignore it for now:
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.