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

Define StateMap as immutable and add a MutableStateMap type. #8183

Merged
merged 5 commits into from
Aug 28, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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/8183.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add type hints to `synapse.state`.
20 changes: 14 additions & 6 deletions synapse/handlers/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,13 @@
from synapse.replication.http.membership import ReplicationUserJoinedLeftRoomRestServlet
from synapse.state import StateResolutionStore, resolve_events_with_store
from synapse.storage.databases.main.events_worker import EventRedactBehaviour
from synapse.types import JsonDict, StateMap, UserID, get_domain_from_id
from synapse.types import (
JsonDict,
MutableStateMap,
StateMap,
UserID,
get_domain_from_id,
)
from synapse.util.async_helpers import Linearizer, concurrently_execute
from synapse.util.distributor import user_joined_room
from synapse.util.retryutils import NotRetryingDestination
Expand All @@ -96,7 +102,7 @@ class _NewEventInfo:

event = attr.ib(type=EventBase)
state = attr.ib(type=Optional[Sequence[EventBase]], default=None)
auth_events = attr.ib(type=Optional[StateMap[EventBase]], default=None)
auth_events = attr.ib(type=Optional[MutableStateMap[EventBase]], default=None)


class FederationHandler(BaseHandler):
Expand Down Expand Up @@ -2053,7 +2059,7 @@ async def _prep_event(
origin: str,
event: EventBase,
state: Optional[Iterable[EventBase]],
auth_events: Optional[StateMap[EventBase]],
auth_events: Optional[MutableStateMap[EventBase]],
backfilled: bool,
) -> EventContext:
context = await self.state_handler.compute_event_context(event, old_state=state)
Expand Down Expand Up @@ -2137,7 +2143,9 @@ async def _check_for_soft_fail(
current_states = await self.state_handler.resolve_events(
room_version, state_sets, event
)
current_state_ids = {k: e.event_id for k, e in current_states.items()}
current_state_ids = {
k: e.event_id for k, e in current_states.items()
} # type: StateMap[str]
else:
current_state_ids = await self.state_handler.get_current_state_ids(
event.room_id, latest_event_ids=extrem_ids
Expand Down Expand Up @@ -2223,7 +2231,7 @@ async def do_auth(
origin: str,
event: EventBase,
context: EventContext,
auth_events: StateMap[EventBase],
auth_events: MutableStateMap[EventBase],
) -> EventContext:
"""

Expand Down Expand Up @@ -2274,7 +2282,7 @@ async def _update_auth_events_and_context_for_auth(
origin: str,
event: EventBase,
context: EventContext,
auth_events: StateMap[EventBase],
auth_events: MutableStateMap[EventBase],
) -> EventContext:
"""Helper for do_auth. See there for docs.

Expand Down
3 changes: 2 additions & 1 deletion synapse/handlers/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from synapse.storage.state import StateFilter
from synapse.types import (
JsonDict,
MutableStateMap,
Requester,
RoomAlias,
RoomID,
Expand Down Expand Up @@ -814,7 +815,7 @@ async def _send_events_for_new_room(
room_id: str,
preset_config: str,
invite_list: List[str],
initial_state: StateMap,
initial_state: MutableStateMap,
creation_content: JsonDict,
room_alias: Optional[RoomAlias] = None,
power_level_content_override: Optional[JsonDict] = None,
Expand Down
5 changes: 3 additions & 2 deletions synapse/handlers/sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from synapse.types import (
Collection,
JsonDict,
MutableStateMap,
RoomStreamToken,
StateMap,
StreamToken,
Expand Down Expand Up @@ -588,7 +589,7 @@ async def compute_summary(
room_id: str,
sync_config: SyncConfig,
batch: TimelineBatch,
state: StateMap[EventBase],
state: MutableStateMap[EventBase],
now_token: StreamToken,
) -> Optional[JsonDict]:
""" Works out a room summary block for this room, summarising the number
Expand Down Expand Up @@ -736,7 +737,7 @@ async def compute_state_delta(
since_token: Optional[StreamToken],
now_token: StreamToken,
full_state: bool,
) -> StateMap[EventBase]:
) -> MutableStateMap[EventBase]:
""" Works out the difference in state between the start of the timeline
and the previous sync.

Expand Down
15 changes: 9 additions & 6 deletions synapse/state/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
from synapse.state import v1, v2
from synapse.storage.databases.main.events_worker import EventRedactBehaviour
from synapse.storage.roommember import ProfileInfo
from synapse.types import Collection, StateMap
from synapse.types import Collection, MutableStateMap, StateMap
from synapse.util import Clock
from synapse.util.async_helpers import Linearizer
from synapse.util.caches.expiringcache import ExpiringCache
Expand Down Expand Up @@ -205,7 +205,7 @@ async def get_current_state_ids(

logger.debug("calling resolve_state_groups from get_current_state_ids")
ret = await self.resolve_state_groups_for_events(room_id, latest_event_ids)
return dict(ret.state)
return ret.state

async def get_current_users_in_room(
self, room_id: str, latest_event_ids: Optional[List[str]] = None
Expand Down Expand Up @@ -302,7 +302,7 @@ async def compute_event_context(
# if we're given the state before the event, then we use that
state_ids_before_event = {
(s.type, s.state_key): s.event_id for s in old_state
}
} # type: StateMap[str]
state_group_before_event = None
state_group_before_event_prev_group = None
deltas_to_state_group_before_event = None
Expand All @@ -315,7 +315,7 @@ async def compute_event_context(
event.room_id, event.prev_event_ids()
)

state_ids_before_event = dict(entry.state)
state_ids_before_event = entry.state
state_group_before_event = entry.state_group
state_group_before_event_prev_group = entry.prev_group
deltas_to_state_group_before_event = entry.delta_ids
Expand Down Expand Up @@ -540,7 +540,7 @@ async def resolve_state_groups(
#
# XXX: is this actually worthwhile, or should we just let
# resolve_events_with_store do it?
new_state = {}
new_state = {} # type: MutableStateMap[str]
conflicted_state = False
for st in state_groups_ids.values():
for key, e_id in st.items():
Expand All @@ -554,7 +554,10 @@ async def resolve_state_groups(
if conflicted_state:
logger.info("Resolving conflicted state for %r", room_id)
with Measure(self.clock, "state._resolve_events"):
new_state = await resolve_events_with_store(
# mypy doesn't like that new_state was previously mutable,
# and now isn't. We don't mutate past this point though, so
# tell it to go away.
new_state = await resolve_events_with_store( # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

If there's something better to do here let me know. I think we could do it with temporary variables, but it would make the code harder to follow?

Copy link
Contributor

Choose a reason for hiding this comment

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

cast to MutableStateMap might be more preferable,

though I'd be concerned about taking something immutable, falsely declaring it as mutable and then passing it to a function taking the mutable variant (that function 'could' change in the future, if we're being general).

I'd say that you should be preferring to lock things down (take the prior mutable and say it is now immutable).

Not sure about the specific risk in this scenario though.

Copy link
Member Author

Choose a reason for hiding this comment

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

That seems reasonable. I think it is OK to falsely declare it as mutable here, see a7f2152.

self.clock,
room_id,
room_version,
Expand Down
10 changes: 5 additions & 5 deletions synapse/state/v1.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
from synapse.api.errors import AuthError
from synapse.api.room_versions import RoomVersions
from synapse.events import EventBase
from synapse.types import StateMap
from synapse.types import MutableStateMap, StateMap

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -131,7 +131,7 @@ async def resolve_events_with_store(

def _seperate(
state_sets: Iterable[StateMap[str]],
) -> Tuple[StateMap[str], StateMap[Set[str]]]:
) -> Tuple[MutableStateMap[str], MutableStateMap[Set[str]]]:
"""Takes the state_sets and figures out which keys are conflicted and
which aren't. i.e., which have multiple different event_ids associated
with them in different state sets.
Expand All @@ -152,7 +152,7 @@ def _seperate(
"""
state_set_iterator = iter(state_sets)
unconflicted_state = dict(next(state_set_iterator))
conflicted_state = {} # type: StateMap[Set[str]]
conflicted_state = {} # type: MutableStateMap[Set[str]]

for state_set in state_set_iterator:
for key, value in state_set.items():
Expand Down Expand Up @@ -208,7 +208,7 @@ def _create_auth_events_from_maps(


def _resolve_with_state(
unconflicted_state_ids: StateMap[str],
unconflicted_state_ids: MutableStateMap[str],
conflicted_state_ids: StateMap[Set[str]],
auth_event_ids: StateMap[str],
state_map: Dict[str, EventBase],
Expand Down Expand Up @@ -241,7 +241,7 @@ def _resolve_with_state(


def _resolve_state_events(
conflicted_state: StateMap[List[EventBase]], auth_events: StateMap[EventBase]
conflicted_state: StateMap[List[EventBase]], auth_events: MutableStateMap[EventBase]
) -> StateMap[EventBase]:
""" This is where we actually decide which of the conflicted state to
use.
Expand Down
6 changes: 3 additions & 3 deletions synapse/state/v2.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
from synapse.api.errors import AuthError
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS
from synapse.events import EventBase
from synapse.types import StateMap
from synapse.types import MutableStateMap, StateMap
from synapse.util import Clock

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -414,7 +414,7 @@ async def _iterative_auth_checks(
base_state: StateMap[str],
event_map: Dict[str, EventBase],
state_res_store: "synapse.state.StateResolutionStore",
) -> StateMap[str]:
) -> MutableStateMap[str]:
"""Sequentially apply auth checks to each event in given list, updating the
state as it goes along.

Expand All @@ -430,7 +430,7 @@ async def _iterative_auth_checks(
Returns:
Returns the final updated state
"""
resolved_state = base_state.copy()
resolved_state = dict(base_state)
Copy link
Member Author

Choose a reason for hiding this comment

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

MutableMapping doesn't have a copy() method (although Dict does). This seemed equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree — both do shallow copies.

room_version_obj = KNOWN_ROOM_VERSIONS[room_version]

for idx, event_id in enumerate(event_ids, start=1):
Expand Down
7 changes: 4 additions & 3 deletions synapse/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
import string
import sys
from collections import namedtuple
from typing import Any, Dict, Tuple, Type, TypeVar
from typing import Any, Dict, Mapping, MutableMapping, Tuple, Type, TypeVar

import attr
from signedjson.key import decode_verify_key_bytes
Expand All @@ -41,8 +41,9 @@ class Collection(Iterable[T_co], Container[T_co], Sized): # type: ignore
# Define a state map type from type/state_key to T (usually an event ID or
# event)
T = TypeVar("T")
StateMap = Dict[Tuple[str, str], T]

StateKey = Tuple[str, str]
StateMap = Mapping[StateKey, T]
MutableStateMap = MutableMapping[StateKey, T]

# the type of a JSON-serialisable dict. This could be made stronger, but it will
# do for now.
Expand Down