Matthias/conduit
1
0
Fork 0
Code Issues 179 Pull requests 44 Projects Releases 5 Packages Wiki Activity

Add support for room v11 #987

Open
_ZN3val wants to merge 9 commits from room-v11 into next
pull from: room-v11
merge into: Matthias:next
Conversation 34 Commits 9 Files changed 6 +158 -74
_ZN3val commented 2023-12-01 17:18:30 +00:00 (Migrated from gitlab.com)
Copy link

Resolves https://gitlab.com/famedly/conduit/-/issues/408

Spec: https://spec.matrix.org/v1.9/rooms/v11/

  • I agree to release my code and all other changes of this MR under the Apache-2.0 license
Resolves https://gitlab.com/famedly/conduit/-/issues/408 Spec: https://spec.matrix.org/v1.9/rooms/v11/ ----------------------------------------------------------------------------- - [x] I agree to release my code and all other changes of this MR under the Apache-2.0 license
_ZN3val commented 2023-12-01 17:18:48 +00:00 (Migrated from gitlab.com)
Copy link

changed title from {-R-}oom v11 to {+Add support for r+}oom v11

changed title from **{-R-}oom v11** to **{+Add support for r+}oom v11**
_ZN3val commented 2023-12-01 17:18:48 +00:00 (Migrated from gitlab.com)
Copy link

changed the description

changed the description
_ZN3val commented 2023-12-01 17:29:24 +00:00 (Migrated from gitlab.com)
Copy link

added 4 commits

  • a3b8eea9 - Move "redacts" key to "content" in redaction events in v11 rooms
  • 18bfd79e - Remove "creator" key when upgrading rooms to v11
  • fac99503 - create_hash_and_sign_event: Use actual version of RoomCreate events, instead of the default
  • 9646439a - Enable support for room v11

Compare with previous version

added 4 commits <ul><li>a3b8eea9 - Move &quot;redacts&quot; key to &quot;content&quot; in redaction events in v11 rooms</li><li>18bfd79e - Remove &quot;creator&quot; key when upgrading rooms to v11</li><li>fac99503 - create_hash_and_sign_event: Use actual version of RoomCreate events, instead of the default</li><li>9646439a - Enable support for room v11</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861015501&start_sha=abeb1a160fcc2d86e4bd2e00d1504e4f6bf7d814)
zecakeh commented 2023-12-01 17:45:31 +00:00 (Migrated from gitlab.com)
Copy link

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?

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?
_ZN3val commented 2023-12-01 17:52:36 +00:00 (Migrated from gitlab.com)
Copy link

Ah you're right, I misunderstood the spec.

Ah you're right, I misunderstood the spec.
_ZN3val commented 2023-12-01 18:08:29 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861047521&start_sha=9646439a94f35a7ab46274c1465ee1192215724a#8738328442e0fca2cfb67ccff0d0c38d65989a4d_72_60)
_ZN3val commented 2023-12-01 18:08:29 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • ecc9dfb4 - Use Ruma's redact_content_in_place instead of custom implementation

Compare with previous version

added 1 commit <ul><li>ecc9dfb4 - Use Ruma&#39;s redact_content_in_place instead of custom implementation</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861047521&start_sha=9646439a94f35a7ab46274c1465ee1192215724a)
_ZN3val commented 2023-12-01 18:08:42 +00:00 (Migrated from gitlab.com)
Copy link

resolved all threads

resolved all threads
_ZN3val commented 2023-12-01 18:11:05 +00:00 (Migrated from gitlab.com)
Copy link

I switched to redact_content_in_place, it seems to work (tested only on m.room.message). Thanks!

I switched to `redact_content_in_place`, it seems to work (tested only on `m.room.message`). Thanks!
_ZN3val commented 2023-12-01 18:11:58 +00:00 (Migrated from gitlab.com)
Copy link

resolved all threads

resolved all threads
_ZN3val commented 2023-12-01 18:12:29 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 520806d4 - Use Ruma's redact_content_in_place instead of custom implementation

Compare with previous version

added 1 commit <ul><li>520806d4 - Use Ruma&#39;s redact_content_in_place instead of custom implementation</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861050680&start_sha=ecc9dfb4bf1a29dd70325f139b8d2a54bee434ff)
girlbossceo commented 2023-12-02 14:24:18 +00:00 (Migrated from gitlab.com)
Copy link

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, and panic! was not used here before.

If it is, it needs to be documented why very clearly.

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, and `panic!` was not used here before. If it is, it needs to be documented why very clearly.
_ZN3val commented 2023-12-02 14:31:52 +00:00 (Migrated from gitlab.com)
Copy link

A panic! isn't very different from the various .expect() in this function, but point taken. I'll return an error instead.

A `panic!` isn't very different from the various `.expect()` in this function, but point taken. I'll return an error instead.
girlbossceo commented 2023-12-02 14:48:11 +00:00 (Migrated from gitlab.com)
Copy link

The expects should still probably be replaced to avoid another https://gitlab.com/famedly/conduit/-/issues/396 where possible

The expects should still probably be replaced to avoid another https://gitlab.com/famedly/conduit/-/issues/396 where possible
_ZN3val commented 2023-12-02 16:51:26 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 5 of the diff

changed this line in [version 5 of the diff](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861412998&start_sha=520806d41385cddee20ec8d8a83bc1c491a61280#3f7d009329014f29e5cd9c98898ddcedcbe38d6b_701_699)
_ZN3val commented 2023-12-02 16:51:26 +00:00 (Migrated from gitlab.com)
Copy link

added 1 commit

  • 5a7bb1e8 - Return error instead of panic when first event is not m.room.create

Compare with previous version

added 1 commit <ul><li>5a7bb1e8 - Return error instead of panic when first event is not m.room.create</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/562/diffs?diff_id=861412998&start_sha=520806d41385cddee20ec8d8a83bc1c491a61280)
_ZN3val commented 2023-12-02 17:50:04 +00:00 (Migrated from gitlab.com)
Copy link

resolved all threads

resolved all threads
girlbossceo commented 2023-12-03 02:34:45 +00:00 (Migrated from gitlab.com)
Copy link

mentioned in commit girlbossceo/conduwuit@f62f641545

mentioned in commit girlbossceo/conduwuit@f62f6415450e1ef6326b3ac38406021fff12f0e1
tezlm commented 2023-12-03 03:24:39 +00:00 (Migrated from gitlab.com)
Copy link

Possibly an unpopular opinion, but if it's presumably an unreachable state, you should add panic!() and assert!()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.

Possibly an unpopular opinion, but if it's presumably an unreachable state, you should add `panic!()` and `assert!()`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.
_ZN3val commented 2023-12-03 07:06:39 +00:00 (Migrated from gitlab.com)
Copy link

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.

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.
okias commented 2023-12-22 10:53:10 +00:00 (Migrated from gitlab.com)
Copy link

is the MR ready?

is the MR ready?
_ZN3val commented 2023-12-22 11:54:10 +00:00 (Migrated from gitlab.com)
Copy link

I believe so, but it needs to be carefully reviewed by someone who understands Conduit better than I do

I believe so, but it needs to be carefully reviewed by someone who understands Conduit better than I do
timokoesters commented 2023-12-24 16:22:51 +00:00 (Migrated from gitlab.com)
Copy link

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.

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.
timokoesters commented 2023-12-24 16:22:52 +00:00 (Migrated from gitlab.com)
Copy link

We already have this function in src/service/rooms/state/mod.rs

We already have this function in src/service/rooms/state/mod.rs
timokoesters commented 2023-12-24 16:22:52 +00:00 (Migrated from gitlab.com)
Copy link

Have you tested this in a real room already?

Have you tested this in a real room already?
timokoesters commented 2023-12-24 16:22:52 +00:00 (Migrated from gitlab.com)
Copy link

Can we just use baddatabase for these? That makes sure they get picked up correctly by the error handling code later in this file.

Can we just use baddatabase for these? That makes sure they get picked up correctly by the error handling code later in this file.
timokoesters commented 2023-12-24 16:22:53 +00:00 (Migrated from gitlab.com)
Copy link

Here's my initial review, I still need to compare this to the spec

Here's my initial review, I still need to compare this to the spec
_ZN3val commented 2023-12-24 18:04:57 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/famedly/conduit/-/merge_requests/562/diffs?diff_id=880565959&start_sha=5a7bb1e8f104b66e65a16e447d930b478640383a#cd91a7743a53129a76df61ce74064141dae4eaa1_149_149)
_ZN3val commented 2023-12-24 18:04:58 +00:00 (Migrated from gitlab.com)
Copy link

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/famedly/conduit/-/merge_requests/562/diffs?diff_id=880565959&start_sha=5a7bb1e8f104b66e65a16e447d930b478640383a#3f7d009329014f29e5cd9c98898ddcedcbe38d6b_132_131)
_ZN3val commented 2023-12-24 18:04:58 +00:00 (Migrated from gitlab.com)
Copy link

added 2 commits

  • eb7ac91c - Reuse existing get_room_version
  • 8175bc12 - Explicitly match RoomVersionId::V11

Compare with previous version

added 2 commits <ul><li>eb7ac91c - Reuse existing get_room_version</li><li>8175bc12 - Explicitly match RoomVersionId::V11</li></ul> [Compare with previous version](/famedly/conduit/-/merge_requests/562/diffs?diff_id=880565959&start_sha=5a7bb1e8f104b66e65a16e447d930b478640383a)
_ZN3val commented 2023-12-24 18:07:10 +00:00 (Migrated from gitlab.com)
Copy link

It would make the errors harder to debug as their only argument would be &'static str

It would make the errors harder to debug as their only argument would be `&'static str`
_ZN3val commented 2023-12-24 18:07:11 +00:00 (Migrated from gitlab.com)
Copy link

It's slightly different because it returns an error instead of None when the version is not found; I hope my change is okay

It's slightly different because it returns an error instead of `None` when the version is not found; I hope my change is okay
_ZN3val commented 2023-12-24 18:07:11 +00:00 (Migrated from gitlab.com)
Copy link

yes

yes
timokoesters commented 2024-01-24 17:16:47 +00:00 (Migrated from gitlab.com)
Copy link

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:

For backwards-compatibility with older clients, servers should add a redacts property to the top level of m.room.redaction events in when serving such events over the Client-Server API.

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:

For improved compatibility with newer clients, servers should add a redacts property to the content of m.room.redaction events in older room versions when serving such events over the Client-Server API.

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: > For backwards-compatibility with older clients, servers should add a redacts property to the top level of m.room.redaction events in when serving such events over the Client-Server API. 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: > For improved compatibility with newer clients, servers should add a redacts property to the content of m.room.redaction events in older room versions when serving such events over the Client-Server API.
This pull request is broken due to missing fork information.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin room-v11:room-v11
git checkout room-v11

Merge

Merge the changes and update on Forgejo.
git checkout next
git merge --no-ff room-v11
git checkout next
git merge --ff-only room-v11
git checkout room-v11
git rebase next
git checkout next
git merge --no-ff room-v11
git checkout next
git merge --squash room-v11
git checkout next
git merge --ff-only room-v11
git checkout next
git merge room-v11
git push origin next
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Android

This label indicates that the issue affects Android devices.

  
CS::needs customer feedback

Issue has been finished and needs feedback from the customer if everything is okay OR the customer needs to answer questions before issue can be worked on.

  
CS::needs follow up

We have received questions or new information from the customer that needs discussing in the meeting with Product and Sales

  
CS::needs on prem installation

This Issue needs to be installed on prem at the customer after closing

  
CS::waiting

This Issue is in progress in other teams (prod/tech)

  
Chrome

This label indicates that the issue affects the chrome browser. You should always combine it with other platforms like Android, if it only affects Android.

  
Design:: Ready

   
Design:: in progress

   
Design::UX

@Famedly: Do not use this label

  
E2EE

   
Edge

This label indicates that the issue affects Edge devices.

  
Firefox

This label indicates that the issue affects Firefox devices.

  
GDPR

   
Iteration 13 IM

   
Linux

This label indicates that the issue affects Linux devices.

  
MacOS

This label indicates that the issue affects MacOS devices.

  
Need::Discussion

Topic needs more discussion

  
Need::Steps to reproduce

   
Need::Upstream fix

   
Needs:: Planning

   
Needs::Dev-Team

This label indicates that the issue needs to be assigned to a dev team.

  
Needs::More information

   
Needs::Priority

This label should be set if there is no priority on feature or improvement requests.

  
Needs::Product

   
Needs::Refinement

the issue is triaged and needs to go to the refinement meeting to be able to set story points to the issue

  
Needs::Severity

This label should be set if bug reports need a severity.

  
Priority::1-Critical

The Priority 1 label indicates a critically dire issue such as a regression, or failure in existing features. We look at issues with this label frequently. If you find yourself assigning a Prio 1 label to an issue, please be sure that there's a positive handoff between filing and a prospective owner for the issue. Prio 1 issues should be rare. Normally there should be zero open P1 issues. "The world is on fire."

  
Priority::2-Max

The Priority 2 label indicates that the issue deserves immediate attention, such as affecting a top-tier customer. There are generally only one or two Prio 2 bugs at a time, and we aim for there to be none. "A customer is on fire."

  
Priority::3-Impending

A bug with Priority 3 flag indicates that a top-tier customer is about to encounter this bug. Generally, there are less than ten Priority 3 bugs. Issues may not be handled immediately (sometimes some Prio 4 issues are handled first), but are top-of-mind and we explicitly consider them weekly. "A customer is about to be on fire."

  
Priority::4-High

The Priority 4 label indicates high-priority issues that are at the top of the work list. This is the highest priority level an issue can have if it isn't affecting a top-tier customer or breaking the app. Bugs marked Priority 4 are generally actively being worked on unless the assignee is dealing with a P1-P3 bug(or another P4 issue).

  
Priority::5-Medium

The Priority 5 label indicates issues that we agree are important to work on, but not at the top of the work list. This is the default level for a bug. An issue at this priority level may not be worked on for a long time. Often an issue at this level will first migrate to Prio 4 before we work on them.

  
Priority::6-Low

The Priority 6 label indicates issues we think are valid but not important. This is the default level for new feature requests. We are unlikely to work on such issues unless they form part of a long-term strategic plan (see the Roadmap).

  
Priority::7-None

The Priority 7 label indicates valid issues that are unlikely to ever be worked on. We use "thumbs-up" on these issues to promote them to Priority 6 or higher based on demand.

  
Progress::Backlog

The Refinement of the issue is completed and DoR for the Squad is fulfilled

  
Progress::Review

Merge Request is ready for review. By definition MR review needs to be done by a second developer.

  
Progress::Started

The issue has been picked up for work by a developer in the team and the developer started to work on actively on the issue.

  
Progress::Testing

In testing

  
Progress::Triage

This is the first step of the process. Issue has been recently opened. Product team will identify/evaluate the following: - Clarify questions if required / verify DoR for Product is met. - It’s been identified if Design work is required or not. - Technical Feasibility - Components involved. - Team to work on that issue. - Severity (in case the issue refers to a bug)

  
Progress::Waiting

A blocker has been identified and work cannot continue.

  
Reporter::Sentry

   
Safari

This label indicates that the issue affects Safari devices.

  
Target::Community

   
Target::Customer

   
Target::Internal

   
Target::PoC

Proof of Concept - issues necessary for potential customer acquisition. Can be scoped by deadline for planned presentations

  
Target::Security

   
Team:Customer-Success

   
Team:Design

Issues which are in the competence area of the frontend chapter. It is not a scoped label anymore as issues can be part of different chapters. Before we can start implementing/changing this, we need a UI/UX design first.

  
Team:Infrastructure

Issues which are in the competence area of the infrastructure chapter. It is not a scoped label anymore as issues can be part of different chapters.

  
Team:Instant-Messaging

   
Team:Product

Issues which are in the responsibility of the product team.

  
Team:Workflows

   
Type::Bug

A software bug is an error, flaw or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.

  
Type::Design

A task about creating or improving the UI/UX design

  
Type::Documentation

   
Type::Feature

Nothing broken; Request for a new feature.

  
Type::Improvement

An improvement or enhancement to an existing feature, technically solution or code. It makes a working functionality faster, more secure, add some additional value, refactors code etc. or helps to design a better UX and UI.

  
Type::Support

   
Type::Tests

A task about writing or improving automated or manual tests.

  
Windows

This label indicates that the issue affects Windows devices.

  
blocked

   
blocked-by-spec

   
cla-signed

   
conduit

   
contribution::advanced

   
contribution::easy

Easy task to start with a contribution

  
contribution::help needed

   
from::review

Reported by famedly review

  
iOS

This label indicates that the issue affects iOS devices.

  
p::ti-tenant

This label indicates that the issues is a requirement of the ti-tenant

  
performance

   
product::triage

   
proposal

A feature propsal.

  
refactor

   
release-blocker

This issue needs to be fixed before the next release.

  
s: dart_openapi_codegen

   
s::Famedly-Patient

Service: Famedly-Patient App

  
s::Org-Directory

For issues that relate to the organisation directory https://gitlab.com/famedly/company/backend/services/organisation-directory/

  
s::Passport-Generator

Service: PASSporT-Generator

  
s::Requeuest

For anything relevant to the requeuest library

  
s:CRM

Service: CRM

  
s:Famedly-App

Service: Famedly-App

  
s:Famedly-Web

Service: Famedly-Web

  
s:Fhiroxide

Service: Fhiroxide

  
s:Fhiroxide-cli

Fhir(oxide) maintenance and debug CLI

  
s:Fhiroxide-client

Fhiroxide client library

  
s:Fhirs

Service: FHIRS

  
s:Hedwig

Service: Hedwig

  
s:LISA

Service: LISA

  
s:Matrix-Dart-SDK

Service: Matrix-Dart-SDK

  
s:Role-Manager

Service: Role-Manager

  
s:Synapse

Service: Synapse

  
s:User-Directory

Service: User-Directory

  
s:WFS-Matrix

Service: Workflow-Service-Matrix

  
s:Workflow Engine

Service: Workflow-Engine

  
s:dtls

   
s:famedly-error

   
s:fcm-shared-isolate

   
s:matrix-api-lite

   
s:multiple-tab-detector

   
s:native-imaging

   
severity::1

Blocker: Broken feature with no workaround or any data-loss. It makes the feature unusable!

  
severity::2

Critical: Broken feature with an unacceptably complex workaround.

  
severity::3

Major: Broken feature with a workaround.

  
severity::4

Low: Functionality is inconvenient.

  
technical-debt

This Issue is a technical debt. Mark issues with this label to have an overview of the technical debts in a repository or group.

  
voip
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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Matthias/conduit#987
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?