Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Draft: Add proper state and state_groups to historical events so they return state from /context (avatar/displayname) (MSC2716) #10948

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Sep 29, 2021

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

  • /messages
    • get_messages
    • get_state_ids_for_event
    • get_state_ids_for_events
  • /context
    • get_event_context
    • get_state_for_events

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct (run the linters)

MadLittleMods and others added 6 commits September 28, 2021 22:00
Fix event context for outlier causing failures in all of the MSC2716
Complement tests.

The `EventContext.for_outlier` refactor happened in #10883
and this spot was left out.
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>
@MadLittleMods MadLittleMods marked this pull request as draft September 29, 2021 23:55
MadLittleMods added a commit to matrix-org/complement that referenced this pull request Sep 30, 2021
…ll-historical-events

Conflicts:
	synapse/handlers/message.py
@MadLittleMods MadLittleMods changed the base branch from madlittlemods/fix-msc2716-outlier-events-failing-tests to develop October 1, 2021 06:37
@MadLittleMods MadLittleMods force-pushed the madlittlemods/msc2716-resolve-state-for-all-historical-events branch from 5d12ae1 to 2de32ed Compare October 1, 2021 06:44
@MadLittleMods MadLittleMods force-pushed the madlittlemods/msc2716-resolve-state-for-all-historical-events branch from 2de32ed to cafb1dc Compare October 1, 2021 06:46

state = await self.state_store.get_state_for_events(
[last_event_id], state_filter=state_filter
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sharing state_groups and resolving state in v2, #10975

event=temp_event,
current_state_ids=auth_event_state_map,
for_verification=False,
)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MadLittleMods
Copy link
Contributor Author

Closing in favor of #10975

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

Successfully merging this pull request may close these issues.

Backfill should gracefully handle the case where no one has the complete state for a given event
1 participant