Fix a bug where redactions were not being sent over federation if we did not have the original event.

[H-Shay]

Prior to this change, redactions were not sent over federation if the sending server hadn't received the original event. Fixes #12795. Per the discussion on #12795, this is achieved by making the function EventsWorkerStore._get_events_from_cache_or_db public and using that rather than EventsWorkerStore.get_events_as_list, which drops redaction events if the original event isn't present. I wrote a test to verify that this change works, you can see it here: https://github.com/matrix-org/complement/actions/runs/3055371487/jobs/4929450057

[DMRobertson]

Complement test is matrix-org/complement#462

[DMRobertson]

The changes look good, but coming back to this with fresher eyes I'm a bit worried about the bigger picture.

We now have get_events_from_cache_or_db and get_events_as_list which both sound like they do the same thing but have different behaviour with regard to redaction events. I think we now have two duplicates of this snippet of source too? I'm just worried that the fix might make it harder to reason about the source in the future.

synapse/synapse/storage/databases/main/events_worker.py

Lines 576 to 586 in d3d9ca1

	if get_prev_content:
	if "replaces_state" in event.unsigned:
	prev = await self.get_event(
	event.unsigned["replaces_state"],
	get_prev_content=False,
	allow_none=True,
	)
	if prev:
	event.unsigned = dict(event.unsigned)
	event.unsigned["prev_content"] = prev.content
	event.unsigned["prev_sender"] = prev.sender

Perhaps @richvdh can advise when he gets back?

[richvdh]

We now have get_events_from_cache_or_db and get_events_as_list which both sound like they do the same thing but have different behaviour with regard to redaction events.

I don't disagree, and it's basically the point I made on #12795:

having lots of similar-but-different entry points makes a component hard to use correctly.

Still, I can't think of a better solution here. What we could do is try to find names that better describe the functionality (maybe s/get_events_from_cache_or_db/get_unredacted_events? other suggestions welcome) and spell out the differences between the entry points in the docstrings.

[richvdh]

looks good to me in general, but a few comments

[richvdh]

Nearly!

(with all this talk of relying on the ordering of event_entries, I'm wondering if we should have a test that checks that behaviour is correct if get_unredacted_events_from_cache_or_db returns them in a different order. But... it looks like we don't have any useful tests for this bit of FederationSender currently, so that sounds like a bit of a big job, so I guess we better leave it.)

[richvdh]

this is still assuming an ordering in event_entries. You could use event_ids here instead. Alternatively, you could maintain a last_event_ts as you iterate over event_ids above.

(Aside: for this operation, it's nice to avoid copying the whole thing to a list just to get the last entry. next(reversed(event_entries.keys())) will do the job I think.)

[H-Shay]

Thanks for that insight, I knew that lists are a little expensive so I appreciate the workaround.

[richvdh]

lgtm!

(sorry for the glacial review speed!)

[H-Shay]

Merged #13813 into develop, thanks for the explanations and the review!