Allow selecting "prejoin" events by state keys

[DMRobertson]

Fixes #14640.

Recommend commitwise review.

See https://pr14642--synapse-docs-previews.netlify.app/usage/configuration/config_documentation.html#room_prejoin_state for docs

[DMRobertson]

This wasn't particularly neat, but it gets the job done.

[reivilibre]

not sure about the need for a new type of StateFilter — note that the normal StateFilter is usable for querying only the required state from the database as well, but I trust you've considered it and have good reason

[reivilibre]

can state_key be None here? do we need to guard against that?

[DMRobertson]

can state_key be None here?

Yes, if someone specifies an entry consisting only of an event type.

do we need to guard against that?

No, it's a valid input that's explicitly handled by .add(None), see

synapse/synapse/config/api.py

Lines 50 to 52 in c801ce7

	def add(self, state_key: Optional[str]) -> None:
	if state_key is None:
	self.options = set()

[DMRobertson]

not sure about the need for a new type of StateFilter

FWIW I don't intend for this to get used outside the config class.

I trust you've considered it and have good reason

I admit it's somewhat ideological and bordering on overkill.

The thinking: for each configured event type T, we either have

• a nonempty set of accepted state key strings, or
• a dummy value meaning "accept any state key".

It's tempting to represent the dummy value with the empty set. But that's a bit a footgun, because the semantics don't match. E.g.

• set{} | {"hello"} is {"hello"}, meaning only the "hello" state key is allowed,
• but "no restriction on state key" union "state key hello is permitted" is "no restriction on state key".

so this wrapper was an attempt to avoid footgunning yourself.

---

Happy to rework this to e.g. use a different representation or to make the type more obviously private, especially if you think it's unclear as is. WDYT?

[reivilibre]

I think StateFilter.from_types does closely enough to what you want? — you give it (type, state_key) tuples (or (type, None) for all state keys of the given type).

I don't know if tests back this up, but to my eye it looks like giving it [("type1", "sk1"), ("type1", None)] will do the right thing regarding union behaviour and expand to [("type1", None)] — with None meaning 'ALL' or 'no restriction'...

I believe StateFilter.filter_state should also be useful to you in doing the filtering, rather than doing it yourself with should_include.

However it's very possible I haven't spotted something subtle which means I'm wrong

[DMRobertson]

Oh ISWYM. I think I thought StateFilter was a no-go for some reason... but I'm not sure I thought about it properly. I'll take a proper look

[DMRobertson]

Ahh, I think I've messed up merging in with #14668. Might have to rebase this, sorry

[DMRobertson]

Cheekily marking this as release-blocker because I'd like to get it in before I go on holiday.

StateFilter changes

[reivilibre]

LGTM, thanks for entertaining me on switching to StateFilter!

[reivilibre]

hmph. Sounds footgunny. Might be worth looking to see if it's worth pushing StateFilter.filter down into get_current_state_ids?
Possibly it may not be (if not every caller does that for some reason).

(N.B. you don't have to — merely a suggestion if it's an easy improvement whilst in the area)

[DMRobertson]

I considered it, but I'm pretty scared of the StateFilter machinery. I'll have a quick look at the call sites tomorrow

[DMRobertson]

Quick search for call sites that provide a state filter:

synapse/synapse/handlers/federation_event.py

Lines 1986 to 1990 in b5b5f66

	current_state_ids = (
	await self._state_storage_controller.get_current_state_ids(
	event.room_id, StateFilter.from_types(auth_types)
	)
	)

synapse/synapse/handlers/message.py

Lines 232 to 235 in b5b5f66

	state_ids = await self._state_storage_controller.get_current_state_ids(
	room_id, state_filter=state_filter
	)
	room_state = await self.store.get_events(state_ids.values())

synapse/synapse/handlers/register.py

Lines 541 to 543 in b5b5f66

	state = await self._storage_controllers.state.get_current_state_ids(
	room_id, StateFilter.from_types([(EventTypes.JoinRules, "")])
	)

synapse/synapse/handlers/room.py

Line 306 in b5b5f66

old_room_state = await tombstone_context.get_current_state_ids(state_filter)

synapse/synapse/handlers/room.py

Lines 484 to 490 in b5b5f66

	old_room_state_ids = (
	await self._storage_controllers.state.get_current_state_ids(
	old_room_id, StateFilter.from_types(types_to_copy)
	)
	)
	# map from event_id to BaseEvent
	old_room_state_events = await self.store.get_events(old_room_state_ids.values())

synapse/synapse/handlers/room.py

Lines 562 to 567 in b5b5f66

	# Transfer membership events
	old_room_member_state_ids = (
	await self._storage_controllers.state.get_current_state_ids(
	old_room_id, StateFilter.from_types([(EventTypes.Member, None)])
	)
	)

There are more. I couldn't see anywhere that does anything like this.

But I also noticed this comment:

synapse/synapse/storage/controllers/state.py

Lines 405 to 417 in b5b5f66

	@cancellable
	async def get_current_state_ids(
	self,
	room_id: str,
	state_filter: Optional[StateFilter] = None,
	await_full_state: bool = True,
	on_invalidate: Optional[Callable[[], None]] = None,
	) -> StateMap[str]:
	"""Get the current state event ids for a room based on the
	current_state_events table.
	If a state filter is given (that is not `StateFilter.all()`) the query
	result is *not* cached.

so maybe using a StateFilter is counterproductive if it bypasses caching? (I guess it depends on how likely it is that the room's current state exists in memory, how much effort a filter saves when making the db query etc)

Anyway I think this is one to leave for a rainy day!

[reivilibre]

(I pressed ctrl+enter rather than approve)