Refactor state module to support multiple room versions #3673
Conversation
We split out the actual state resolution algorithm to prepare for having multiple versions.
erikjohnston
force-pushed
the
erikj/refactor_state_handler
branch
from
035d4c4 to
5075e44
Compare
synapse/handlers/room_member.py
Outdated
| @@ -341,9 +341,10 @@ def _update_membership( | |||
| prev_events_and_hashes = yield self.store.get_prev_events_for_room( | |||
| room_id, | |||
| ) | |||
| latest_event_ids = ( | |||
| latest_event_ids = [ | |||
There was a problem hiding this comment.
Ah, this used to be a required change but no longer. Will change it back for simplicity of PR
synapse/state/__init__.py
Outdated
| @@ -365,8 +378,13 @@ def resolve_state_groups_for_events(self, room_id, event_ids): | |||
| delta_ids=delta_ids, | |||
| )) | |||
|
|
|||
| room_version = explicit_room_version | |||
| if not room_version: | |||
There was a problem hiding this comment.
surely we should never get here for an m.room.create, because we should never have to do state resolution?
In other words: explicit_room_version seems redundant.
synapse/state/__init__.py
Outdated
| @@ -390,14 +409,18 @@ def resolve_events(self, state_sets, event): | |||
| for ev in st | |||
| } | |||
|
|
|||
| room_version = yield self.store.get_room_version(event.room_id) | |||
There was a problem hiding this comment.
this looks redundant, since it's passed as an arg.
|
I'm assuming that the stuff in synapse/state/v1.py has just moved out of synapse/state.py - it's hard to check this. Generally I'd have preferred this to be split into one PR which makes the state handler aware of room versions, and another to move the v1 stuff out. |
erikjohnston
force-pushed
the
erikj/refactor_state_handler
branch
from
34982f4 to
3f6013d
Compare
erikjohnston
force-pushed
the
erikj/refactor_state_handler
branch
from
3f6013d to
4d66427
Compare
Yup, its just a copy and paste.
Ah sorry. FWIW I did the split up in the first commit and then changed it to make it version aware. |
synapse/state/__init__.py
Outdated
| @@ -262,14 +262,9 @@ def compute_event_context(self, event, old_state=None): | |||
| defer.returnValue(context) | |||
|
|
|||
| logger.debug("calling resolve_state_groups from compute_event_context") | |||
| if event.type == EventTypes.Create: | |||
There was a problem hiding this comment.
I guess you could explicitly assert that event.type != EventTypes.Create here to give a slightly more comprehensible failure if/when it happens.
There was a problem hiding this comment.
Creation events will happily reach that point
There was a problem hiding this comment.
oh. why would we ever be doing state res on create events?
There was a problem hiding this comment.
We don't, but we do call compute_event_context, and that gets nooped in resolve_state_groups_for_events since it has not prev events
This lays the ground work for supporting different state resolution algorithms depending on room version.
The two big changes are:
state.pyintostate/__init__.pyandstate/v1.py, where the latter contains only the v1 state resolution algorithm.The reason for checking the room version so late is because large amounts of code assume that if the state handler is given a bunch of event_ids it doesn't have, it'll return an empty state rather than throwing an error. (In particular, a lot of client APIs don't check if the room exists before trying to create the events, and instead rely on the event auth to throw a 403 on empty state). This is not great and should be fixed, but I don't really want to try and block implementing v2 state resolution on fixing that.
There should be no behavioural change here.