Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/18786.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix invalidation of storage cache that was broken in 1.135.0.
2 changes: 1 addition & 1 deletion synapse/api/auth/internal.py
Original file line number Diff line number Diff line change
Expand Up @@ -296,4 +296,4 @@ async def is_server_admin(self, requester: Requester) -> bool:
Returns:
True if the user is an admin
"""
return await self.store.is_server_admin(requester.user)
return await self.store.is_server_admin(requester.user.to_string())
2 changes: 1 addition & 1 deletion synapse/handlers/room_member.py
Original file line number Diff line number Diff line change
Expand Up @@ -1921,7 +1921,7 @@ async def _remote_join(
check_complexity
and self.hs.config.server.limit_remote_rooms.admins_can_join
):
check_complexity = not await self.store.is_server_admin(user)
check_complexity = not await self.store.is_server_admin(user.to_string())

if check_complexity:
# Fetch the room complexity
Expand Down
2 changes: 1 addition & 1 deletion synapse/module_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ async def is_user_admin(self, user_id: str) -> bool:
Returns:
True if the user is a server admin, False otherwise.
"""
return await self._store.is_server_admin(UserID.from_string(user_id))
return await self._store.is_server_admin(user_id)

async def set_user_admin(self, user_id: str, admin: bool) -> None:
"""Sets if a user is a server admin.
Expand Down
2 changes: 1 addition & 1 deletion synapse/rest/admin/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -1000,7 +1000,7 @@ async def on_GET(
"Only local users can be admins of this homeserver",
)

is_admin = await self.store.is_server_admin(target_user)
is_admin = await self.store.is_server_admin(target_user.to_string())

return HTTPStatus.OK, {"admin": is_admin}

Expand Down
4 changes: 2 additions & 2 deletions synapse/storage/databases/main/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ async def delete_account_validity_for_user(self, user_id: str) -> None:
)

@cached(max_entries=100000)
async def is_server_admin(self, user: UserID) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why didn't our custom mypy plugin catch this?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

async def is_server_admin(self, user: str) -> bool:
"""Determines if a user is an admin of this homeserver.

Args:
Expand All @@ -685,7 +685,7 @@ async def is_server_admin(self, user: UserID) -> bool:
"""
res = await self.db_pool.simple_select_one_onecol(
table="users",
keyvalues={"name": user.to_string()},
keyvalues={"name": user},
retcol="admin",
allow_none=True,
desc="is_server_admin",
Expand Down
5 changes: 1 addition & 4 deletions synapse/visibility.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
RetentionPolicy,
StateMap,
StrCollection,
UserID,
get_domain_from_id,
)
from synapse.types.state import StateFilter
Expand Down Expand Up @@ -120,9 +119,7 @@ async def filter_events_for_client(
# Default case is to *exclude* soft-failed events
events = [e for e in events if not e.internal_metadata.is_soft_failed()]
client_config = await storage.main.get_admin_client_config_for_user(user_id)
if filter_send_to_client and await storage.main.is_server_admin(
UserID.from_string(user_id)
):
if filter_send_to_client and await storage.main.is_server_admin(user_id):
if client_config.return_soft_failed_events:
# The user has requested that all events be included, so do that.
# We copy the list for mutation safety.
Expand Down
Loading