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

Room Versioning #1425

Closed
richvdh opened this issue Jul 17, 2018 · 24 comments
Closed

Room Versioning #1425

richvdh opened this issue Jul 17, 2018 · 24 comments
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal

Comments

@richvdh
Copy link
Member

richvdh commented Jul 17, 2018

Documentation: https://docs.google.com/document/d/1urKgReoHqxX8R_XtySB17dPi-DZcKhqTEL2_s895Wz0
PRs: #1516

supercedes #1196.

@richvdh richvdh added proposal-in-review proposal A matrix spec change proposal labels Jul 17, 2018
@richvdh
Copy link
Member Author

richvdh commented Jul 20, 2018

Last call for comments on this before we go ahead and start implementation next week.

@erikjohnston
Copy link
Member

Are we using integers or strings for room versions? Benefits of strings is that it would make it a) clearer that you can't use ranges, and b) makes it easier to have versions like test-version-foo

OTOH, I'm not sure we want to be encouraging weird room versions in the wild

@turt2live
Copy link
Member

The error schema implies integers, which I think is a good thing. It's much easier to determine if a room is outdated when it's an integer.

@non-Jedi
Copy link
Contributor

non-Jedi commented Jul 20, 2018

The wording in this document makes it sound like the actual spec PR may have different "mechanics" than outlined in this document. Specifically, the phrase, "might look like". I just want to be clear that if the actual way that room versions work ends up being different than outlined in the mechanics section of this document, a new spec proposal will be made before a spec PR is merged, right?

Other than that, here's some points I'd like clarification on:

  • Room versions are supposed to mark changes in mechanics for both clients and servers, right? But the way the proposal is worded, it seems to say that servers get to set the default version of rooms. Does this mean that outdated clients will no longer be able to create rooms that they can use on servers that set a default room version other than 1?
  • Will clients be able to override the default room version when creating rooms (this only partially helps with previous point since many clients may not be aware of room version at all)?
  • Will the spec continue to include sections about old room mechanics even as version 2 gets rolled out? It seems that it should since homeservers at least must implement both version 1 and version 2 room mechanics. Clients for a while probably ought to as well.
  • In theory, could the version of a room affect how a client interacts with it before they've joined the room? For example if we implement different semantics for peeking into a room. If so, do we need to add an endpoint for checking room version or something that must be called before a client attempts to interact with the room?

For the record, I really like the general approach of this proposal. Worrying about room versioning before worrying about upgrade path seems like the right way to go about things.

@KitsuneRal
Copy link
Member

KitsuneRal commented Jul 22, 2018

There's not a word about exposing the room version to clients; exposing the join error is only mentioned. Given that the room version may (potentially greatly, in case of event id formats etc.) affect the clients, should this proposal mention where to find the room version in CS API? Or are we deliberately keeping it a server-server thing for now, given that there no actually immediate changes that would need the clients to know about the room version?

@richvdh
Copy link
Member Author

richvdh commented Jul 23, 2018

@erikjohnston / @turt2live : let's define them as integers.

@richvdh
Copy link
Member Author

richvdh commented Jul 23, 2018

@non-Jedi:

The wording in this document makes it sound like the actual spec PR may have different "mechanics" than outlined in this document. Specifically, the phrase, "might look like"

Sorry, that phrasing was really just an artefact of the text being copied from an earlier proposal where things were less certain. Now updated.

I just want to be clear that if the actual way that room versions work ends up being different than outlined in the mechanics section of this document, a new spec proposal will be made before a spec PR is merged, right?

It's likely that, as I implement this, I'll discover things that don't work out as planned. If that happens, I'll update the doc and highlight the changes here.

Room versions are supposed to mark changes in mechanics for both clients and servers, right? But the way the proposal is worded, it seems to say that servers get to set the default version of rooms.

Correct.

Room versions are supposed to mark changes in mechanics for both clients and servers, right?

Yes, it's likely that some changes that might be supported by a room version bump will require client-side changes to support (an example would be: changing the way bans work so that bans are stored in a separate m.room.ban event rather than an m.room.member event. In order to figure out who has been banned from a room, a client would have to look at a different sort of event).

Does this mean that outdated clients will no longer be able to create rooms that they can use on servers that set a default room version other than 1?

Will clients be able to override the default room version when creating rooms (this only partially helps with previous point since many clients may not be aware of room version at all)?

These are interesting questions which I suspect we haven't thought about hard enough.

I think that, for testing future room versions if nothing else, the ability to override the default version is going to be important.

I'm hesitant to have an "I only support v1" flag on createRoom (or the inverse: "I do support v2"), for two reasons:

  • I think "rooms they can use" overstates the case here. For many changes, there will be no need for clients to be changed at all. For many others, 90% of the functionality of a room will continue to work. That would lead to a lot of rooms being created on old room versions when there is no need.
  • Obviously, it's not just the client that creates a room that needs to worry about the version of a room. Any other client that belongs to a user who has joined a v2 room needs to behave sanely in the presence of v2 rooms.

An extreme solution to the latter would be to extend the format of /sync so that v2 rooms are listed separately to v1 rooms, so that v2-unaware clients don't see the v2 rooms at all. We'd also need support on the /join API and others so that v2-unaware clients don't end up joining a v1 room by mistake.

Right now, I'm minded to hold fire on this, and make it more of a case-by-case judgement based on exactly what changes are happening: some changes will require significant changes to the C-S API, and others will be invisible to clients, and we should pick a solution accordingly. I'm certainly open to discussion here, though.

Will the spec continue to include sections about old room mechanics even as version 2 gets rolled out? It seems that it should since homeservers at least must implement both version 1 and version 2 room mechanics. Clients for a while probably ought to as well.

Absolutely. I think @turt2live is working on some refactoring of the spec to make this a more tractable thing to do.

In theory, could the version of a room affect how a client interacts with it before they've joined the room? ...

In theory, yes. In practice: it's just a particular case of the more general questions about client interaction above.

@richvdh
Copy link
Member Author

richvdh commented Jul 23, 2018

@KitsuneRal: clients can observe the room version by looking at the m.room.create event, should they so wish.

@richvdh
Copy link
Member Author

richvdh commented Jul 23, 2018

All: thanks for feedback so far. Having it in here where there is a record of it is useful, but I've also created a matrix room (#msc-1425:sw1v.org) if anyone has any /quick/ questions.

@KitsuneRal
Copy link
Member

Will clients have to explicitly request m.room.create to find out how they should read events for that room already coming through sync to them? Doesn't sound client-friendly.

@turt2live
Copy link
Member

Will the spec continue to include sections about old room mechanics even as version 2 gets rolled out? It seems that it should since homeservers at least must implement both version 1 and version 2 room mechanics. Clients for a while probably ought to as well.

Absolutely. I think @turt2live is working on some refactoring of the spec to make this a more tractable thing to do.

I'm not sure what the requirements are for this yet, although it is on the list (under r0 and above making it pretty).

@richvdh
Copy link
Member Author

richvdh commented Jul 24, 2018

In discussion in #msc-1425:sw1v.org, I became convinced of the merits of making room_version a string. I've updated the doc accordingly, and added a section discussing the allowable grammar. Feedback welcome.

[I've also been thinking a bit more about client-side support. I'll update the doc accordingly in due course].

@richvdh
Copy link
Member Author

richvdh commented Jul 24, 2018

I've now added some thoughts on client interaction at https://docs.google.com/document/d/1urKgReoHqxX8R_XtySB17dPi-DZcKhqTEL2_s895Wz0#heading=h.b398exru3iu5. It needs more discussion; feedback once again welcome.

@KitsuneRal
Copy link
Member

I don't see much harm in trying to join an incompatible room - after all, I might still be able to read it on another client. On the other hand, filtering the response from /sync by supported versions looks useful for some cases. Adding the room version to list_public_rooms.yaml would certainly be nice but not a showstopper. Assuming these things are in place, the proposal looks good to me.

@richvdh
Copy link
Member Author

richvdh commented Aug 10, 2018

Basic support for this has now landed in synapse, in matrix-org/synapse#3654.

I think it was largely as originally specced, except for some minor tweaks:

  • room_version is an opaque string, as per Room Versioning #1425 (comment).
  • /make_join now takes a series of separate ?ver=1&ver=2&ver=3 parameters, rather than a single ?room_versions=1,2,3 parameter. This is more consistent with what has been done elsewhere in Matrix.
  • /make_join does not return the room_version (instead the joining server has to await the arrival of the m.room.create event in the send_join response)

The proposal doc has been updated accordingly.

@richvdh richvdh added spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec and removed proposal-in-review labels Aug 10, 2018
@KitsuneRal
Copy link
Member

/me still hopes for the room version in /sync response (maybe as a part of room summaries?)

dbkr added a commit to matrix-org/matrix-js-sdk that referenced this issue Aug 14, 2018
To get the version number of the room as per matrix-org/matrix-spec-proposals#1425
@turt2live turt2live added this to To do: proposals (prioritized) in August 2018 r0 Aug 14, 2018
@neilisfragile neilisfragile moved this from To do: proposals (not prioritized) to To do: server-server (prioritized) in August 2018 r0 Aug 15, 2018
turt2live added a commit to turt2live/matrix-doc that referenced this issue Aug 15, 2018
This is the spec PR for matrix-org#1425

Room version upgrades are not part of MSC1425.

Documented aspects:
* room_version on the create event
* creating a room with a specific version (useful for testing)
* make_join behaviour
* error code documentation
* grammar of room versions

Based upon https://docs.google.com/document/d/1urKgReoHqxX8R_XtySB17dPi-DZcKhqTEL2_s895Wz0/edit
@turt2live turt2live added spec-pr-in-review A proposal which has been PR'd against the spec and is in review and removed spec-pr-missing Proposal has been implemented and is being used in the wild but hasn't yet been added to the spec labels Aug 15, 2018
@turt2live turt2live moved this from To do: server-server (prioritized) to In review in August 2018 r0 Aug 15, 2018
@richvdh
Copy link
Member Author

richvdh commented Aug 16, 2018

@KitsuneRal: why don't you just get the create event, if you want to know the room version?

(I certainly don't think the summary is the right place)

@KitsuneRal
Copy link
Member

KitsuneRal commented Aug 16, 2018

Well, basically it's yet another call for me, for EACH room in the roomlist - at best, I have to do something like that:

  1. Do the first sync.
  2. Collect the list of room ids from it.
  3. Retrieve m.room.create for all rooms (by one more "service" /sync?).
  4. Match up the response from this request with rooms from the first one.
  5. Finally figure, which rooms I can read.

That looks a bit too involved for the initial sync when I really want to show things to the user as soon as possible. And there's never been a case so far when I needed to issue a network request right from processing the response from another network request. Not that libQMatrixClient couldn't do that, but I don't want to mess with special cases due to race conditions between the sync in step 1 and the sync in step 3 (what if the user left some room right before me sending a request for m.room.create events, e.g.?).

@richvdh
Copy link
Member Author

richvdh commented Aug 16, 2018

  1. Do the first sync.
  2. Collect the list of room ids from it.
  3. Retrieve m.room.create for all rooms (by one more "service" /sync?).

I don't quite understand why you wouldn't get the m.room.create in the first sync?

@KitsuneRal
Copy link
Member

Because it would be beyond the latest N events I'm getting in the first sync?

@ara4n
Copy link
Member

ara4n commented Aug 19, 2018

you always receive all state events in the first sync (ignoring lazy-loading members). the limit is only applied to the events in the timeline.

@KitsuneRal
Copy link
Member

I suspected that I didn't notice an elephant in the room :) thanks, and sorry for the time taken.

@turt2live turt2live added merged A proposal whose PR has merged into the spec! and removed spec-pr-in-review A proposal which has been PR'd against the spec and is in review labels Aug 24, 2018
@turt2live
Copy link
Member

Merged 🎉

August 2018 r0 automation moved this from In review to Done (this list will be incomplete) Aug 24, 2018
@richvdh
Copy link
Member Author

richvdh commented Jan 17, 2019

the spec pr was #1516

@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Apr 21, 2020
RiotTranslateBot pushed a commit to RiotTranslateBot/matrix-doc that referenced this issue Aug 22, 2023
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature MSC for not-core and not-maintenance stuff merged A proposal whose PR has merged into the spec! proposal A matrix spec change proposal
Projects
No open projects
August 2018 r0
  
Done (this list will be incomplete)
Development

No branches or pull requests

6 participants