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

Fix /initialSync error due to unhashable RoomStreamToken #10827

Merged
merged 3 commits into from
Sep 22, 2021
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/10827.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix error in deprecated `/initialSync` endpoint when using the undocumented `from` and `to` parameters.
4 changes: 3 additions & 1 deletion synapse/storage/databases/main/stream.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@
from collections import namedtuple
from typing import TYPE_CHECKING, Collection, Dict, List, Optional, Set, Tuple

from frozendict import frozendict

from twisted.internet import defer

from synapse.api.filtering import Filter
Expand Down Expand Up @@ -379,7 +381,7 @@ def get_room_max_token(self) -> RoomStreamToken:
if p > min_pos
}

return RoomStreamToken(None, min_pos, positions)
return RoomStreamToken(None, min_pos, frozendict(positions))

async def get_room_events_stream_for_rooms(
self,
Expand Down
20 changes: 15 additions & 5 deletions synapse/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
)

import attr
from frozendict import frozendict
from signedjson.key import decode_verify_key_bytes
from unpaddedbase64 import decode_base64
from zope.interface import Interface
Expand Down Expand Up @@ -457,6 +458,9 @@ class RoomStreamToken:

Note: The `RoomStreamToken` cannot have both a topological part and an
instance map.

For caching purposes, `RoomStreamToken`s and by extension, all their
attributes, must be hashable.
"""

topological = attr.ib(
Expand All @@ -466,12 +470,12 @@ class RoomStreamToken:
stream = attr.ib(type=int, validator=attr.validators.instance_of(int))

instance_map = attr.ib(
type=Dict[str, int],
factory=dict,
type="frozendict[str, int]",
factory=frozendict,
validator=attr.validators.deep_mapping(
key_validator=attr.validators.instance_of(str),
value_validator=attr.validators.instance_of(int),
mapping_validator=attr.validators.instance_of(dict),
mapping_validator=attr.validators.instance_of(frozendict),
Copy link
Contributor

Choose a reason for hiding this comment

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

oh but you have this validator here ... well. Now I am wondering if this is better or worse. I'm interested to see what others of the team think (or is this something we already do elsewhere?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is a runtime check rather than something Mypy can check, though

),
)

Expand Down Expand Up @@ -507,7 +511,7 @@ async def parse(cls, store: "DataStore", string: str) -> "RoomStreamToken":
return cls(
topological=None,
stream=stream,
instance_map=instance_map,
instance_map=frozendict(instance_map),
)
except Exception:
pass
Expand Down Expand Up @@ -540,7 +544,7 @@ def copy_and_advance(self, other: "RoomStreamToken") -> "RoomStreamToken":
for instance in set(self.instance_map).union(other.instance_map)
}

return RoomStreamToken(None, max_stream, instance_map)
return RoomStreamToken(None, max_stream, frozendict(instance_map))

def as_historical_tuple(self) -> Tuple[int, int]:
"""Returns a tuple of `(topological, stream)` for historical tokens.
Expand Down Expand Up @@ -593,6 +597,12 @@ async def to_string(self, store: "DataStore") -> str:

@attr.s(slots=True, frozen=True)
class StreamToken:
"""A collection of positions within multiple streams.

For caching purposes, `StreamToken`s and by extension, all their attributes,
must be hashable.
"""

room_key = attr.ib(
type=RoomStreamToken, validator=attr.validators.instance_of(RoomStreamToken)
)
Expand Down