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

Consistently use collections.abc.Mapping to match frozendict. #12564

Merged
merged 2 commits into from
Apr 27, 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/12564.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Consistently check if an object is a `frozendict`.
7 changes: 4 additions & 3 deletions synapse/events/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
)

import attr
from frozendict import frozendict

from synapse.api.constants import EventContentFields, EventTypes, RelationTypes
from synapse.api.errors import Codes, SynapseError
Expand Down Expand Up @@ -204,7 +203,9 @@ def _copy_field(src: JsonDict, dst: JsonDict, field: List[str]) -> None:
key_to_move = field.pop(-1)
sub_dict = src
for sub_field in field: # e.g. sub_field => "content"
if sub_field in sub_dict and type(sub_dict[sub_field]) in [dict, frozendict]:
Copy link
Member Author

Choose a reason for hiding this comment

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

This one is slightly different, but I didn't see why it also shouldn't use isinstance?

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 would reject sub_dict[sub_fields] which were instances of a class that inherits from either dict or frozendict. I don't think we have a particularly good reason to do that 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.

This should all be data from JSON, so I can't see how we would get anything besides dict or frozendict.

The code is from 2016 (70a2157 and 6d4e6d4) so I think it just didn't use isinstance(...).

if sub_field in sub_dict and isinstance(
sub_dict[sub_field], collections.abc.Mapping
):
sub_dict = sub_dict[sub_field]
else:
return
Expand Down Expand Up @@ -622,7 +623,7 @@ def validate_canonicaljson(value: Any) -> None:
# Note that Infinity, -Infinity, and NaN are also considered floats.
raise SynapseError(400, "Bad JSON value: float", Codes.BAD_JSON)

elif isinstance(value, (dict, frozendict)):
elif isinstance(value, collections.abc.Mapping):
for v in value.values():
validate_canonicaljson(v)

Expand Down
4 changes: 2 additions & 2 deletions synapse/handlers/relations.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import collections.abc
import logging
from typing import (
TYPE_CHECKING,
Expand All @@ -24,7 +25,6 @@
)

import attr
from frozendict import frozendict

from synapse.api.constants import RelationTypes
from synapse.api.errors import SynapseError
Expand Down Expand Up @@ -380,7 +380,7 @@ async def get_bundled_aggregations(
# Do not bundle aggregations for an event which represents an edit or an
# annotation. It does not make sense for them to have related events.
relates_to = event.content.get("m.relates_to")
if isinstance(relates_to, (dict, frozendict)):
if isinstance(relates_to, collections.abc.Mapping):
relation_type = relates_to.get("rel_type")
if relation_type in (RelationTypes.ANNOTATION, RelationTypes.REPLACE):
continue
Expand Down
5 changes: 2 additions & 3 deletions synapse/storage/databases/main/state.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,10 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import collections.abc
import logging
from typing import TYPE_CHECKING, Collection, Dict, Iterable, Optional, Set, Tuple

from frozendict import frozendict

from synapse.api.constants import EventTypes, Membership
from synapse.api.errors import NotFoundError, UnsupportedRoomVersionError
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
Expand Down Expand Up @@ -160,7 +159,7 @@ async def get_room_predecessor(self, room_id: str) -> Optional[JsonMapping]:
predecessor = create_event.content.get("predecessor", None)

# Ensure the key is a dictionary
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this comment could now be confusing in the future?

if not isinstance(predecessor, (dict, frozendict)):
if not isinstance(predecessor, collections.abc.Mapping):
return None

# The keys must be strings since the data is JSON.
Expand Down
3 changes: 2 additions & 1 deletion synapse/util/frozenutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import collections.abc
from typing import Any

from frozendict import frozendict
Expand All @@ -35,7 +36,7 @@ def freeze(o: Any) -> Any:


def unfreeze(o: Any) -> Any:
Copy link
Contributor

Choose a reason for hiding this comment

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

I note that:

  • this doesn't unfreeze a frozenset to a set
  • maybe the try-except could be replaced with an if isinstance(o, collections.abc.Iterable)?
  • unfreezing a string bytestring still leaves you with an immutable object!

Judging from where we use it, it looks like this is only used to unfreeze event content rather than a generic unfreeze function.

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 don't believe it is meant to be a generic unfreeze function. 👍

if isinstance(o, (dict, frozendict)):
if isinstance(o, collections.abc.Mapping):
return {k: unfreeze(v) for k, v in o.items()}

if isinstance(o, (bytes, str)):
Expand Down