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

Remove de.gematik.tim.room.name and de.gematik.tim.room.topic in a future version #272

Open
nico-famedly opened this issue Oct 9, 2024 · 5 comments

Comments

@nico-famedly
Copy link

The state events de.gematik.tim.room.name and de.gematik.tim.room.topic described in https://gemspec.gematik.de/docs/gemSpec/gemSpec_TI-Messenger-Client/gemSpec_TI-Messenger-Client_V1.1.2/#5.4.18 currently are just exact copies of m.room.name and m.room.topic. Currently they don't seem to provide any additional value for TIM clients or servers, however they complicate implementing the spec a lot since they have additional requirements (like requiring m.room.name to be empty) and don't integrate with existing parts of the Matrix specification (shortcuts to specify a room name during room creation, displaying a name for spaces or public rooms using their respective APIs, etc). It also complicates integration with Matrix based solutions since SDKs and clients need to be modified to be compatible on basic features like room names and topics.

Considering that my suggestion would be to remove them from the TIM specification (with an appropriate migration path possibly?). This should simplify developing TIM compliant solutions going forward. I think the idea behind them was to allow encrypting them in the future, however that would already be a breaking change and such custom event types could just be introduced at that point. Alternatively if the idea behind them was extensibility by adding custom fields, that can be more easily served by adding such custom fields in a namespaced manner to existing event types.

Possibly there is a different reason for these events, that I am unaware of however. If such a reason exists, it might be a good idea to document it on this issue and then close it to serve as a future reference.

@Johennes
Copy link
Contributor

Yes, the idea was to start encrypting them, once that is possible. Additional room-specific data is meant to go into separate state events, again, once they can actually be encrypted.

I agree that in their current form the custom name and topic fields are entirely useless and I've raised this topic internally when I joined the team six months ago. The issue is we already have specifications that require the custom fields out there and implementations that comply with them. If we now remove them, there'll be a compatibility problem. If we then reintroduce them later once state can be encrypted, there'll be another compatibility problem. So I'm not sure how we could feasibly remove the custom events again without causing even more pain. 😕

@nico-famedly
Copy link
Author

nico-famedly commented Oct 11, 2024

The intent to be able to encrypt them is very likely to break compatibility once that is possible, which is why imo it should be removed now. If you encrypt the event by keeping the event type plain text and the same, then the event will suddenly have a different content. That is a breaking change. And if you encrypt it and encrypt the event type as well, then the event type doesn't matter, since you will need to implement a new field as well.

The problem is that having those events right now, while they "do nothing", has a permanent cost associated with it, since Matrix APIs are not designed with them in mind. This means if the TIM specification wants to support spaces, all homeservers need to be patched to also return the name field when using the /hierarchy APIs. Similarly for the room directory it is not possible to see the names for public rooms. And a lot of other edge cases, where the TIM spec doesn't provide consistent solutions for them. Removing those event types (with a migration over several steps) is imo easier than specifying all those edge cases. One could as a first step allow using both (m.room.name and de.gematik.tim.room.name), where they should always have the same value with the intent to remove them in a future version. This does cause friction, but I think that is easier than fixing all the cases where those events are causing problems right now.

And once encrypted state events are possible, a lot more stuff will have to change, so the current solution is exactly as future proof (imo) as m.room.name is.

@Johennes
Copy link
Contributor

And a lot of other edge cases, where the TIM spec doesn't provide consistent solutions for them.

Chapter 5 has been streamlined a little in the latest draft release. The idea was that TIM is not going to list all the various cases in which servers need to evaluate the custom events rather than the standard events. Instead A_25820 just makes it a blanket SHOULD requirement.

One could as a first step allow using both (m.room.name and de.gematik.tim.room.name), where they should always have the same value with the intent to remove them in a future version. This does cause friction, but I think that is easier than fixing all the cases where those events are causing problems right now.

Let's assume in the next release (probably some time in 2025) we add a new requirement for clients to always send both types of events with equal content. We already have implementations out in the wild that only send the custom events. So new implementations would still have to support the case where only the custom events are filled with content. Once the old implementations exceed their certification (3+ years from today), we can then make another release and remove the obligation to send and evaluate the custom events.

This seems fine per se. But then since the transition period is 3 years you're still going to have to fix all the cases where the event types interfere on the server. We'd only actually improve things for people starting an implementation in 3 years. Maybe that is still worth it. I'm just trying to call out that the current problems would still have to be solved in the meantime.

@Johennes
Copy link
Contributor

I've re-raised this topic internally and am waiting for feedback and opinions.

@Johennes
Copy link
Contributor

Against all odds, I've been able to get the first step (sending the custom and standard events with identical values) into this week's patch release: https://gemspec.gematik.de/docs/gemSpec/gemSpec_TI-M_Basis/gemSpec_TI-M_Basis_V1.1.1/#5.5.1.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants