Draft: Add proper state and state_groups to historical events so they return state from /context (avatar/displayname) (MSC2716)#10948
Conversation
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
…rom /context See https://gitlab.com/gitterHQ/webapp/-/merge_requests/2229#note_683532091 Also seems to fix #10764
While working on matrix-org/synapse#10948
…ll-historical-events Conflicts: synapse/handlers/message.py
5d12ae1 to
2de32ed
Compare
2de32ed to
cafb1dc
Compare
| event_id_to_get_state_from = last_event_id | ||
|
|
||
| state = await self.state_store.get_state_for_events( | ||
| [last_event_id], state_filter=state_filter |
There was a problem hiding this comment.
For /context, it only gives you the state from the last event in events_after instead of the event_id itself. This means that often times, you end up missing the historical floating state for the historical message and instead get some of the real resolved state which doesn't have any of the member events we want.
I was messing around with changing this to return the state for the event_id specified but using the last event is part of the spec.
I could change the which events are fetched in events_before/events_after when specifying a historical event so it only returns itself 🤷. Then it would be the last event of itself.
| if builder.internal_metadata.is_historical() and full_state_ids_at_event: | ||
| old_state = await self.store.get_events_as_list(full_state_ids_at_event) | ||
|
|
||
| context = await self.state.compute_event_context(event, old_state=old_state) |
There was a problem hiding this comment.
When you pull state from a state_group, it grabs the full resolved state from the all of the state_group deltas.
So we want to define the state for the historical event as all of the state at that point in time(m.room.create, m.room.power_levels, m.room.member, etc), including member events not matching the sender of the event. This way when /messages fetches the state for the first event in the batch, it most likely grabs from some historical event with all of the member events necessary for that batch.
This does have the flaw where if you just insert a single historical event somewhere, it probably won't resolve the state correctly from /messages or /context since it will grab a non historical event above or below with resolved state which never included the historical state back then.
Might be good to look at /mesages and /context to additionally add the state for any historical messages in that batch.
Also pretty inefficient not to share the same state_group for every historical event in the batch 🤔. This will currently create a new state_group for every historical event because compute_event_context creates a new state_group with old_state every time.
There was a problem hiding this comment.
Sharing state_groups and resolving state in v2, #10975
| event=temp_event, | ||
| current_state_ids=auth_event_state_map, | ||
| for_verification=False, | ||
| ) |
There was a problem hiding this comment.
Moved this auth state stripping into create_new_client_event so we can copy full_state_ids_at_event before we strip and use it for the old_state for state_group calculation during compute_event_context
|
Closing in favor of #10975 |
Add proper state and state_groups to historical events so they return state from
/context. The end-goal is to see the appropriate avatar/displayname in Element.See https://gitlab.com/gitterHQ/webapp/-/merge_requests/2229#note_683532091
Also seems to fix #10764
v2 -> #10975
Dev note
/messagesget_messagesget_state_ids_for_eventget_state_ids_for_events/contextget_event_contextget_state_for_eventsPull Request Checklist
EventStoretoEventWorkerStore.".code blocks.