Bundle aggregations outside of the serialization method

[clokep]

This is some partial work towards #11611, the main benefit of which is that the event serialization process is now synchronous and does not access the database.

This requires the callers which wish to include bundled aggregations to manually fetch them and pass them in (as opposed to just passing a flag).

Based on #11592

This will be the basis for some future work for #11611:

• Including the bundled aggregations in the response cache for sync -- see Include bundled aggregations in the sync response cache. #11659.
• Fetching the bundled aggregations of multiple events at once (to reduce database round trips) -- see Fetch edits for multiple events in a single query #11660 for doing this with edits.

This should be reviewable commit-by-commit.

[clokep]

Another downside of this is that the cached EventBase values become slightly mutable, which might be unexpected!

[clokep]

Comparing this to the steps in #11611, I think that this would help be moving the calculation of bundled aggregations from step 2 to step 1, thus ending up with them cached. This would still take over 800 seconds the first time, but should now be a no-op once they're cached (instead of taking 224 seconds). I think we can improve the queries there as well to be a bit more efficient!

See #11659 for this change.

[DMRobertson]

At a high level this looks like a sensible way to pull out the computation of aggregations from the serialisation logic. I did find this all a little daunting though!

I think that this would help be moving the calculation of bundled aggregations from step 2 to step 1, thus ending up with them cached.

I don't quite follow this though. Which things are now cached that weren't before?

Another downside of this is that the cached EventBase values become slightly mutable, which might be unexpected!

Where is this happening, sorry?

[DMRobertson]

This bit moved to the top of the helper function in the previous commit.

[DMRobertson]

The changes in this function seem to be:

• the logic which builds aggregations is pulled out to the helper in the previous commit
• aggregations are precalculated and passed into this function
• any side effects from the old code are maintained: read the relevant data from aggregations and kick off those same side effects

[clokep]

My hope is that it is identical logic, yep! This still does the serialization of the aggregations, but that's perfectly reasonable I think.

[DMRobertson]

Do we need this? Ahh yes: there's a mutation on +469.

[clokep]

I wondered if it would make more sense to make the input to this an attrs class so that the types are much clearer, which would mean we would build the dictionary here instead. Would that be clearer? (I think I'd rather do that as a follow-up since it is a decent amount of changes, but could do it here if you'd like!)

[DMRobertson]

I think that'd be nice, but not crucial. Sounds like it'd be best as a separate change to me.

[clokep]

At a high level this looks like a sensible way to pull out the computation of aggregations from the serialisation logic. I did find this all a little daunting though!

I think that this would help be moving the calculation of bundled aggregations from step 2 to step 1, thus ending up with them cached.

I don't quite follow this though. Which things are now cached that weren't before?

They aren't yet, but #11659 does this (and builds on this).

Another downside of this is that the cached EventBase values become slightly mutable, which might be unexpected!

Where is this happening, sorry?

This was an old comment about the prior approach, should have deleted it, sorry about that! You can ignore this comment.

[DMRobertson]

Thanks for explaining, I think this looks good.

OOI how much test coverage do we have of this bundling logic? (I.e. how reassuring is the successful CI run here?)

[clokep]

OOI how much test coverage do we have of this bundling logic? (I.e. how reassuring is the successful CI run here?)

The main test-case for this is

synapse/tests/rest/client/test_relations.py

Lines 455 to 582 in 6c68e87

	@unittest.override_config({"experimental_features": {"msc3440_enabled": True}})
	def test_bundled_aggregations(self):
	"""Test that annotations, references, and threads get correctly bundled."""
	# Setup by sending a variety of relations.
	channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "a")
	self.assertEquals(200, channel.code, channel.json_body)
	channel = self._send_relation(
	RelationTypes.ANNOTATION, "m.reaction", "a", access_token=self.user2_token
	)
	self.assertEquals(200, channel.code, channel.json_body)
	channel = self._send_relation(RelationTypes.ANNOTATION, "m.reaction", "b")
	self.assertEquals(200, channel.code, channel.json_body)
	channel = self._send_relation(RelationTypes.REFERENCE, "m.room.test")
	self.assertEquals(200, channel.code, channel.json_body)
	reply_1 = channel.json_body["event_id"]
	channel = self._send_relation(RelationTypes.REFERENCE, "m.room.test")
	self.assertEquals(200, channel.code, channel.json_body)
	reply_2 = channel.json_body["event_id"]
	channel = self._send_relation(RelationTypes.THREAD, "m.room.test")
	self.assertEquals(200, channel.code, channel.json_body)
	channel = self._send_relation(RelationTypes.THREAD, "m.room.test")
	self.assertEquals(200, channel.code, channel.json_body)
	thread_2 = channel.json_body["event_id"]
	def assert_bundle(actual):
	"""Assert the expected values of the bundled aggregations."""
	# Ensure the fields are as expected.
	self.assertCountEqual(
	actual.keys(),
	(
	RelationTypes.ANNOTATION,
	RelationTypes.REFERENCE,
	RelationTypes.THREAD,
	),
	)
	# Check the values of each field.
	self.assertEquals(
	{
	"chunk": [
	{"type": "m.reaction", "key": "a", "count": 2},
	{"type": "m.reaction", "key": "b", "count": 1},
	]
	},
	actual[RelationTypes.ANNOTATION],
	)
	self.assertEquals(
	{"chunk": [{"event_id": reply_1}, {"event_id": reply_2}]},
	actual[RelationTypes.REFERENCE],
	)
	self.assertEquals(
	2,
	actual[RelationTypes.THREAD].get("count"),
	)
	# The latest thread event has some fields that don't matter.
	self.assert_dict(
	{
	"content": {
	"m.relates_to": {
	"event_id": self.parent_id,
	"rel_type": RelationTypes.THREAD,
	}
	},
	"event_id": thread_2,
	"room_id": self.room,
	"sender": self.user_id,
	"type": "m.room.test",
	"user_id": self.user_id,
	},
	actual[RelationTypes.THREAD].get("latest_event"),
	)
	def _find_and_assert_event(events):
	"""
	Find the parent event in a chunk of events and assert that it has the proper bundled aggregations.
	"""
	for event in events:
	if event["event_id"] == self.parent_id:
	break
	else:
	raise AssertionError(f"Event {self.parent_id} not found in chunk")
	assert_bundle(event["unsigned"].get("m.relations"))
	# Request the event directly.
	channel = self.make_request(
	"GET",
	f"/rooms/{self.room}/event/{self.parent_id}",
	access_token=self.user_token,
	)
	self.assertEquals(200, channel.code, channel.json_body)
	assert_bundle(channel.json_body["unsigned"].get("m.relations"))
	# Request the room messages.
	channel = self.make_request(
	"GET",
	f"/rooms/{self.room}/messages?dir=b",
	access_token=self.user_token,
	)
	self.assertEquals(200, channel.code, channel.json_body)
	_find_and_assert_event(channel.json_body["chunk"])
	# Request the room context.
	channel = self.make_request(
	"GET",
	f"/rooms/{self.room}/context/{self.parent_id}",
	access_token=self.user_token,
	)
	self.assertEquals(200, channel.code, channel.json_body)
	assert_bundle(channel.json_body["event"]["unsigned"].get("m.relations"))
	# Request sync.
	# channel = self.make_request("GET", "/sync", access_token=self.user_token)
	# self.assertEquals(200, channel.code, channel.json_body)
	# room_timeline = channel.json_body["rooms"]["join"][self.room]["timeline"]
	# self.assertTrue(room_timeline["limited"])
	# _find_and_assert_event(room_timeline["events"])
	# Note that /relations is tested separately in test_aggregation_get_event_for_thread
	# since it needs different data configured.

This definitely failed quite a few times until I got it right. 😄