Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Correct check_username_for_spam annotations and docs #12246

Merged
merged 8 commits into from
Mar 18, 2022
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/12246.doc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Correct `check_username_for_spam` annotations and docs.
10 changes: 6 additions & 4 deletions docs/modules/spam_checker_callbacks.md
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ any of the subsequent implementations of this callback.
_First introduced in Synapse v1.37.0_

```python
async def check_username_for_spam(user_profile: Dict[str, str]) -> bool
async def check_username_for_spam(user_profile: synapse.module_api.UserProfile) -> bool
```

Called when computing search results in the user directory. The module must return a
Expand All @@ -182,9 +182,11 @@ search results; otherwise return `False`.

The profile is represented as a dictionary with the following keys:

* `user_id`: The Matrix ID for this user.
* `display_name`: The user's display name.
* `avatar_url`: The `mxc://` URL to the user's avatar.
* `user_id: str`. The Matrix ID for this user.
* `display_name: Optional[str]`. The user's display name, or `None` if this user
has not set a display name.
* `avatar_url: Optional[str]`. The `mxc://` URL to the user's avatar, or `None`
if this user has not set an avatar.

The module is given a copy of the original dictionary, so modifying it from within the
module cannot modify a user's profile when included in user directory search results.
Expand Down
7 changes: 3 additions & 4 deletions synapse/events/spamcheck.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
Awaitable,
Callable,
Collection,
Dict,
List,
Optional,
Tuple,
Expand All @@ -31,7 +30,7 @@
from synapse.rest.media.v1._base import FileInfo
from synapse.rest.media.v1.media_storage import ReadableFileWrapper
from synapse.spam_checker_api import RegistrationBehaviour
from synapse.types import RoomAlias
from synapse.types import RoomAlias, UserProfile
from synapse.util.async_helpers import maybe_awaitable

if TYPE_CHECKING:
Expand All @@ -50,7 +49,7 @@
USER_MAY_CREATE_ROOM_CALLBACK = Callable[[str], Awaitable[bool]]
USER_MAY_CREATE_ROOM_ALIAS_CALLBACK = Callable[[str, RoomAlias], Awaitable[bool]]
USER_MAY_PUBLISH_ROOM_CALLBACK = Callable[[str, str], Awaitable[bool]]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[Dict[str, str]], Awaitable[bool]]
CHECK_USERNAME_FOR_SPAM_CALLBACK = Callable[[UserProfile], Awaitable[bool]]
LEGACY_CHECK_REGISTRATION_FOR_SPAM_CALLBACK = Callable[
[
Optional[dict],
Expand Down Expand Up @@ -383,7 +382,7 @@ async def user_may_publish_room(self, userid: str, room_id: str) -> bool:

return True

async def check_username_for_spam(self, user_profile: Dict[str, str]) -> bool:
async def check_username_for_spam(self, user_profile: UserProfile) -> bool:
"""Checks if a user ID or display name are considered "spammy" by this server.

If the server considers a username spammy, then it will not be included in
Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
from synapse.api.constants import EventTypes, HistoryVisibility, JoinRules, Membership
from synapse.handlers.state_deltas import MatchChange, StateDeltasHandler
from synapse.metrics.background_process_metrics import run_as_background_process
from synapse.storage.databases.main.user_directory import SearchResult
from synapse.storage.roommember import ProfileInfo
from synapse.types import JsonDict
from synapse.util.metrics import Measure

if TYPE_CHECKING:
Expand Down Expand Up @@ -78,7 +78,7 @@ def __init__(self, hs: "HomeServer"):

async def search_users(
self, user_id: str, search_term: str, limit: int
) -> JsonDict:
) -> SearchResult:
"""Searches for users in directory

Returns:
Expand Down
2 changes: 2 additions & 0 deletions synapse/module_api/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@
StateMap,
UserID,
UserInfo,
UserProfile,
create_requester,
)
from synapse.util import Clock
Expand Down Expand Up @@ -150,6 +151,7 @@
"EventBase",
"StateMap",
"ProfileInfo",
"UserProfile",
]

logger = logging.getLogger(__name__)
Expand Down
4 changes: 2 additions & 2 deletions synapse/rest/client/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from synapse.http.server import HttpServer
from synapse.http.servlet import RestServlet, parse_json_object_from_request
from synapse.http.site import SynapseRequest
from synapse.types import JsonDict
from synapse.types import JsonMapping

from ._base import client_patterns

Expand All @@ -38,7 +38,7 @@ def __init__(self, hs: "HomeServer"):
self.auth = hs.get_auth()
self.user_directory_handler = hs.get_user_directory_handler()

async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonDict]:
async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonMapping]:
"""Searches for users in directory

Returns:
Expand Down
23 changes: 19 additions & 4 deletions synapse/storage/databases/main/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
cast,
)

from typing_extensions import TypedDict

from synapse.api.errors import StoreError

if TYPE_CHECKING:
Expand All @@ -40,7 +42,12 @@
from synapse.storage.databases.main.state import StateFilter
from synapse.storage.databases.main.state_deltas import StateDeltasStore
from synapse.storage.engines import PostgresEngine, Sqlite3Engine
from synapse.types import JsonDict, get_domain_from_id, get_localpart_from_id
from synapse.types import (
JsonDict,
UserProfile,
get_domain_from_id,
get_localpart_from_id,
)
from synapse.util.caches.descriptors import cached

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -591,6 +598,11 @@ async def update_user_directory_stream_pos(self, stream_id: Optional[int]) -> No
)


class SearchResult(TypedDict):
limited: bool
results: List[UserProfile]


class UserDirectoryStore(UserDirectoryBackgroundUpdateStore):
# How many records do we calculate before sending it to
# add_users_who_share_private_rooms?
Expand Down Expand Up @@ -777,7 +789,7 @@ async def get_user_directory_stream_pos(self) -> Optional[int]:

async def search_user_dir(
self, user_id: str, search_term: str, limit: int
) -> JsonDict:
) -> SearchResult:
"""Searches for users in directory

Returns:
Expand Down Expand Up @@ -910,8 +922,11 @@ async def search_user_dir(
# This should be unreachable.
raise Exception("Unrecognized database engine")

results = await self.db_pool.execute(
"search_user_dir", self.db_pool.cursor_to_dict, sql, *args
results = cast(
List[UserProfile],
await self.db_pool.execute(
"search_user_dir", self.db_pool.cursor_to_dict, sql, *args
),
)

limited = len(results) > limit
Expand Down
11 changes: 11 additions & 0 deletions synapse/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import attr
from frozendict import frozendict
from signedjson.key import decode_verify_key_bytes
from typing_extensions import TypedDict
from unpaddedbase64 import decode_base64
from zope.interface import Interface

Expand Down Expand Up @@ -63,6 +64,10 @@
# JSON types. These could be made stronger, but will do for now.
# A JSON-serialisable dict.
JsonDict = Dict[str, Any]
# A JSON-serialisable mapping; roughly speaking an immutable JSONDict.
# Useful when you have a TypedDict which isn't going to be mutated and you don't want
# to cast to JsonDict everywhere.
JsonMapping = Mapping[str, Any]
Comment on lines +67 to +70
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth noting that Mappings aren't necessarily JSON-serialisable, unless they're really dicts or frozendicts. Essentially when we use JsonMapping, we're pinky promising that it's really a (frozen)dict.

The same kind of thing applies to JsonSerializable below I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't 100% sold on this either. I wanted to see if there was a way to make more use of TypedDicts without having to cast those back to JsonDicts everywhere. Maybe I could just make the return type of this function SearchResult? That feels a bit off though---it's exposing a type from storage.databases.main in rest.client.

async def on_POST(self, request: SynapseRequest) -> Tuple[int, JsonMapping]:

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I'm not opposed to making the return type some sort of TypedDict. I think we really ought to have a file somewhere full of TypedDict definitions that come from the spec, since that's the source of truth for the return type here (and not the database layer).

In any case I'm happy with the JsonMapping as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I broadly sympathise, but I don't think that'd be fool proof: e.g. given d: JsonMapping = {"a": [1,2,3]} I think d["a"].append(4) would still type check?

I think we really ought to have a file somewhere full of TypedDict definitions that come from the spec, since that's the source of truth for the return type here (and not the database layer).

I like the sound of this; e.g. synapse.types.spec.UserDirectorySearchResult. I guess the dream would be to automatically generate such a file from the openapi definition in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think there is a case for having a JsonMapping type, to signal that functions aren't allowed to modify the contents of a dict. Especially around caches.

I broadly sympathise, but I don't think that'd be fool proof: e.g. given d: JsonMapping = {"a": [1,2,3]} I think d["a"].append(4) would still type check?

True! Unless we manage to define JsonMapping = Mapping[str, ImmutableJsonSerializable] recursively, which mypy won't let us do. (grumble grumble this would be possible in typescript)

# A JSON-serialisable object.
JsonSerializable = object

Expand Down Expand Up @@ -791,3 +796,9 @@ class UserInfo:
is_deactivated: bool
is_guest: bool
is_shadow_banned: bool


class UserProfile(TypedDict):
user_id: str
display_name: Optional[str]
avatar_url: Optional[str]