From 43b2be9764bf244122f25a0f2bf8ec8c761f5cad Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 7 Feb 2020 13:08:34 +0000 Subject: [PATCH 1/3] Replace _event_dict_property with DictProperty this amounts to the same thing, but replaces `_event_dict` with `_dict`, and removes some of the function layers generated by `property`. --- synapse/events/__init__.py | 144 ++++++++++++++++++-------------- tests/storage/test_redaction.py | 2 +- 2 files changed, 80 insertions(+), 66 deletions(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index a842661a90..512254f65d 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -37,6 +37,65 @@ from synapse.util.frozenutils import freeze USE_FROZEN_DICTS = strtobool(os.environ.get("SYNAPSE_USE_FROZEN_DICTS", "0")) +class DictProperty: + """An object property which delegates to the `_dict` within its parent object.""" + + __slots__ = ["key"] + + def __init__(self, key: str): + self.key = key + + def __get__(self, instance, owner=None): + # if the property is accessed as a class property rather than an instance + # property, return the property itself rather than the value + if instance is None: + return self + try: + return instance._dict[self.key] + except KeyError as e1: + # We want this to look like a regular attribute error (mostly so that + # hasattr() works correctly), so we convert the KeyError into an + # AttributeError. + # + # To exclude the KeyError from the traceback, we explicitly + # 'raise from e1.__context__' (which is better than 'raise from None', + # becuase that would omit any *earlier* exceptions). + # + raise AttributeError( + "'%s' has no '%s' property" % (type(instance), self.key) + ) from e1.__context__ + + def __set__(self, instance, v): + instance._dict[self.key] = v + + def __delete__(self, instance): + try: + del instance._dict[self.key] + except KeyError as e1: + raise AttributeError( + "'%s' has no '%s' property" % (type(instance), self.key) + ) from e1.__context__ + + +class DefaultDictProperty(DictProperty): + """An extension of DictProperty which provides a default if the property is + not present in the parent's _dict. + + Note that this means that hasattr() on the property always returns True. + """ + + __slots__ = ["default"] + + def __init__(self, key, default): + super().__init__(key) + self.default = default + + def __get__(self, instance, owner=None): + if instance is None: + return self + return instance._dict.get(self.key, self.default) + + class _EventInternalMetadata(object): def __init__(self, internal_metadata_dict): self.__dict__ = dict(internal_metadata_dict) @@ -117,51 +176,6 @@ class _EventInternalMetadata(object): return getattr(self, "redacted", False) -_SENTINEL = object() - - -def _event_dict_property(key, default=_SENTINEL): - """Creates a new property for the given key that delegates access to - `self._event_dict`. - - The default is used if the key is missing from the `_event_dict`, if given, - otherwise an AttributeError will be raised. - - Note: If a default is given then `hasattr` will always return true. - """ - - # We want to be able to use hasattr with the event dict properties. - # However, (on python3) hasattr expects AttributeError to be raised. Hence, - # we need to transform the KeyError into an AttributeError - - def getter_raises(self): - try: - return self._event_dict[key] - except KeyError: - raise AttributeError(key) - - def getter_default(self): - return self._event_dict.get(key, default) - - def setter(self, v): - try: - self._event_dict[key] = v - except KeyError: - raise AttributeError(key) - - def delete(self): - try: - del self._event_dict[key] - except KeyError: - raise AttributeError(key) - - if default is _SENTINEL: - # No default given, so use the getter that raises - return property(getter_raises, setter, delete) - else: - return property(getter_default, setter, delete) - - class EventBase(object): def __init__( self, @@ -175,23 +189,23 @@ class EventBase(object): self.unsigned = unsigned self.rejected_reason = rejected_reason - self._event_dict = event_dict + self._dict = event_dict self.internal_metadata = _EventInternalMetadata(internal_metadata_dict) - auth_events = _event_dict_property("auth_events") - depth = _event_dict_property("depth") - content = _event_dict_property("content") - hashes = _event_dict_property("hashes") - origin = _event_dict_property("origin") - origin_server_ts = _event_dict_property("origin_server_ts") - prev_events = _event_dict_property("prev_events") - redacts = _event_dict_property("redacts", None) - room_id = _event_dict_property("room_id") - sender = _event_dict_property("sender") - state_key = _event_dict_property("state_key") - type = _event_dict_property("type") - user_id = _event_dict_property("sender") + auth_events = DictProperty("auth_events") + depth = DictProperty("depth") + content = DictProperty("content") + hashes = DictProperty("hashes") + origin = DictProperty("origin") + origin_server_ts = DictProperty("origin_server_ts") + prev_events = DictProperty("prev_events") + redacts = DefaultDictProperty("redacts", None) + room_id = DictProperty("room_id") + sender = DictProperty("sender") + state_key = DictProperty("state_key") + type = DictProperty("type") + user_id = DictProperty("sender") @property def event_id(self) -> str: @@ -205,13 +219,13 @@ class EventBase(object): return hasattr(self, "state_key") and self.state_key is not None def get_dict(self) -> JsonDict: - d = dict(self._event_dict) + d = dict(self._dict) d.update({"signatures": self.signatures, "unsigned": dict(self.unsigned)}) return d def get(self, key, default=None): - return self._event_dict.get(key, default) + return self._dict.get(key, default) def get_internal_metadata_dict(self): return self.internal_metadata.get_dict() @@ -233,16 +247,16 @@ class EventBase(object): raise AttributeError("Unrecognized attribute %s" % (instance,)) def __getitem__(self, field): - return self._event_dict[field] + return self._dict[field] def __contains__(self, field): - return field in self._event_dict + return field in self._dict def items(self): - return list(self._event_dict.items()) + return list(self._dict.items()) def keys(self): - return six.iterkeys(self._event_dict) + return six.iterkeys(self._dict) def prev_event_ids(self): """Returns the list of prev event IDs. The order matches the order diff --git a/tests/storage/test_redaction.py b/tests/storage/test_redaction.py index b9ee6ec1ec..db3667dc43 100644 --- a/tests/storage/test_redaction.py +++ b/tests/storage/test_redaction.py @@ -240,7 +240,7 @@ class RedactionTestCase(unittest.HomeserverTestCase): built_event = yield self._base_builder.build(prev_event_ids) built_event._event_id = self._event_id - built_event._event_dict["event_id"] = self._event_id + built_event._dict["event_id"] = self._event_id assert built_event.event_id == self._event_id return built_event From 9551911f88b558d20e2fd9c2ae791fe5b732db02 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 7 Feb 2020 13:22:17 +0000 Subject: [PATCH 2/3] Rewrite _EventInternalMetadata to back it with a _dict Mostly, this gives mypy an easier time. --- synapse/events/__init__.py | 55 +++++++++++++++++++++++++++----------- 1 file changed, 39 insertions(+), 16 deletions(-) diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index 512254f65d..7307116556 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- # Copyright 2014-2016 OpenMarket Ltd # Copyright 2019 New Vector Ltd +# Copyright 2020 The Matrix.org Foundation C.I.C. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -97,33 +98,55 @@ class DefaultDictProperty(DictProperty): class _EventInternalMetadata(object): - def __init__(self, internal_metadata_dict): - self.__dict__ = dict(internal_metadata_dict) + __slots__ = ["_dict"] - def get_dict(self): - return dict(self.__dict__) + def __init__(self, internal_metadata_dict: JsonDict): + # we have to copy the dict, because it turns out that the same dict is + # reused. TODO: fix that + self._dict = dict(internal_metadata_dict) - def is_outlier(self): - return getattr(self, "outlier", False) + outlier = DictProperty("outlier") # type: bool + out_of_band_membership = DictProperty("out_of_band_membership") # type: bool + send_on_behalf_of = DictProperty("send_on_behalf_of") # type: str + recheck_redaction = DictProperty("recheck_redaction") # type: bool + soft_failed = DictProperty("soft_failed") # type: bool + proactively_send = DictProperty("proactively_send") # type: bool + redacted = DictProperty("redacted") # type: bool + txn_id = DictProperty("txn_id") # type: str + token_id = DictProperty("token_id") # type: str + stream_ordering = DictProperty("stream_ordering") # type: int - def is_out_of_band_membership(self): + # XXX: These are set by StreamWorkerStore._set_before_and_after. + # I'm pretty sure that these are never persisted to the database, so shouldn't + # be here + before = DictProperty("before") # type: str + after = DictProperty("after") # type: str + order = DictProperty("order") # type: int + + def get_dict(self) -> JsonDict: + return dict(self._dict) + + def is_outlier(self) -> bool: + return self._dict.get("outlier", False) + + def is_out_of_band_membership(self) -> bool: """Whether this is an out of band membership, like an invite or an invite rejection. This is needed as those events are marked as outliers, but they still need to be processed as if they're new events (e.g. updating invite state in the database, relaying to clients, etc). """ - return getattr(self, "out_of_band_membership", False) + return self._dict.get("out_of_band_membership", False) - def get_send_on_behalf_of(self): + def get_send_on_behalf_of(self) -> Optional[str]: """Whether this server should send the event on behalf of another server. This is used by the federation "send_join" API to forward the initial join event for a server in the room. returns a str with the name of the server this event is sent on behalf of. """ - return getattr(self, "send_on_behalf_of", None) + return self._dict.get("send_on_behalf_of") - def need_to_check_redaction(self): + def need_to_check_redaction(self) -> bool: """Whether the redaction event needs to be rechecked when fetching from the database. @@ -136,9 +159,9 @@ class _EventInternalMetadata(object): Returns: bool """ - return getattr(self, "recheck_redaction", False) + return self._dict.get("recheck_redaction", False) - def is_soft_failed(self): + def is_soft_failed(self) -> bool: """Whether the event has been soft failed. Soft failed events should be handled as usual, except: @@ -150,7 +173,7 @@ class _EventInternalMetadata(object): Returns: bool """ - return getattr(self, "soft_failed", False) + return self._dict.get("soft_failed", False) def should_proactively_send(self): """Whether the event, if ours, should be sent to other clients and @@ -162,7 +185,7 @@ class _EventInternalMetadata(object): Returns: bool """ - return getattr(self, "proactively_send", True) + return self._dict.get("proactively_send", True) def is_redacted(self): """Whether the event has been redacted. @@ -173,7 +196,7 @@ class _EventInternalMetadata(object): Returns: bool """ - return getattr(self, "redacted", False) + return self._dict.get("redacted", False) class EventBase(object): From 5a78f47f6e21505f84490c3b8da49e96ac8e3483 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 7 Feb 2020 13:27:58 +0000 Subject: [PATCH 3/3] changelog --- changelog.d/6872.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/6872.misc diff --git a/changelog.d/6872.misc b/changelog.d/6872.misc new file mode 100644 index 0000000000..215a0c82c3 --- /dev/null +++ b/changelog.d/6872.misc @@ -0,0 +1 @@ +Refactor _EventInternalMetadata object to improve type safety.