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

Fix /initialSync error due to unhashable RoomStreamToken #10827

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e4eb0fa
Fix /initialSync error due to unhashable `RoomStreamToken`
Sep 15, 2021
79bf045
fix newsfile
Sep 15, 2021
fc7d054
add notes to docstrings and fix attribute type
Sep 17, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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
11 changes: 6 additions & 5 deletions synapse/types.py
View file
Open in desktop
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 @@ -466,12 +467,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=Mapping[str, int],
Copy link
Contributor

Choose a reason for hiding this comment

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

I was about to say 'huh, I never thought of just calling frozendict a Mapping!', but I'm not sure this type annotation tells the whole story.

dict is a subclass of Mapping, yet this field very clearly needs the Mapping to be hashable, so the type annotation is too loose here.

I think this really does need frozendict[str, int] — which I think you have to put in double quotes (:sob: — there is a fork of frozendict which came in after generics emerged and fixes that, but we don't use it in Synapse).

Copy link
Member

Choose a reason for hiding this comment

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

I think this really does need frozendict[str, int] — which I think you have to put in double quotes (😭 — there is a fork of frozendict which came in after generics emerged and fixes that, but we don't use it in Synapse).

We have a stub for this in our repo.

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've updated the type to be "frozendict[str, int]" with the quotes. It does feel slightly off since the frozendict-ness is really an implementation detail. The type required is Mapping[str, int] & Hashable except there's no way to express that in Python that I know of.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this really does need frozendict[str, int] — which I think you have to put in double quotes (sob — there is a fork of frozendict which came in after generics emerged and fixes that, but we don't use it in Synapse).

We have a stub for this in our repo.

I don't think stubs are used at runtime, so you still have to put in the quotes otherwise you get the type subscripting error. (am I missing something? I hope so :))

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 don't think stubs are used at runtime, so you still have to put in the quotes otherwise you get the type subscripting error. (am I missing something? I hope so :))

Yep, without the quotes I get a type subscripting error.

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 +508,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 +541,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