Set type of user_id on is_server_admin to str

[anoadragon453]

And updates the few calling sites.

This allows the cache of this function to be invalidated. As we were calling _invalidate_cache_and_stream with a str instead of a UserID as this function expected, the cache was not actually being invalidated.

We can't just pass UserID to _invalidate_cache_and_stream, as that function puts the args in a DB column and does not deserialise types when pulling them out.

Once again fixes #18738. Broke in #18747.

Fix #18738
Fix #18783

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.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Code style is correct (run the linters)

[AndrewFerr]

Confirmed that this fixes #18783.

[MadLittleMods]

Why didn't our custom mypy plugin catch this?

synapse/scripts-dev/mypy_synapse_plugin.py

Lines 215 to 230 in 6514381

	def get_method_signature_hook(
	self, fullname: str
	) -> Optional[Callable[[MethodSigContext], CallableType]]:
	if fullname.startswith(
	(
	"synapse.util.caches.descriptors.CachedFunction.__call__",
	"synapse.util.caches.descriptors._LruCachedFunction.__call__",
	)
	):
	return cached_function_method_signature
	if fullname in (
	"synapse.util.caches.descriptors._CachedFunctionDescriptor.__call__",
	"synapse.util.caches.descriptors._CachedListFunctionDescriptor.__call__",
	):
	return check_is_cacheable_wrapper

[anoadragon453]

I believe our plugin currently allows proper type-checking of callers of cached functions, but it doesn't help with _invalidate_cache_and_stream. The arguments in that case are completely different. _invalidate_cache_and_stream takes in the function name and a list of parameters (which are typed as Any). The cached decorated takes the parameters directly.

I think what we actually want to do here is to make use of ParamSpec to "forward" the types from an inner to an outer function. Then mypy could warn that we were using str instead of UserID.

But we'd ideally go one step further and specify that _invalidate_cache_and_stream can only take in primitive types (because they are dropped into the DB, and then pulled out again without deserialisation). Either that, or store the types in the DB as well/pickle them so we can store things like UserID in there.

[MadLittleMods]

Ahh, so UserID is actually is_cacheable. And the only problem was mis-aligning str and UserID in the lookup to invalidate.

[anoadragon453]

Yes, and we can tell the cache is working by seeing #18783 😁

[anoadragon453]

Base changed to release-v1.136 to include the fix in the RC.