-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Use the proper serialization format when bundling aggregations #12090
Changes from 4 commits
bea2c48
c544efb
1212e98
c835577
21c1b26
64cd901
56ed827
3b43c13
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,6 +26,7 @@ | |||||||||
Union, | ||||||||||
) | ||||||||||
|
||||||||||
import attr | ||||||||||
from frozendict import frozendict | ||||||||||
|
||||||||||
from synapse.api.constants import EventContentFields, EventTypes, RelationTypes | ||||||||||
|
@@ -303,29 +304,37 @@ def format_event_for_client_v2_without_room_id(d: JsonDict) -> JsonDict: | |||||||||
return d | ||||||||||
|
||||||||||
|
||||||||||
@attr.s(slots=True, frozen=True, auto_attribs=True) | ||||||||||
class SerializeEventConfig: | ||||||||||
as_client_event: bool = True | ||||||||||
# Function to convert from federation format to client format | ||||||||||
event_format: Callable[[JsonDict], JsonDict] = format_event_for_client_v1 | ||||||||||
# ID of the user's auth token - used for namespacing of transaction IDs | ||||||||||
token_id: Optional[int] = None | ||||||||||
# List of event fields to include. If empty, all fields will be returned. | ||||||||||
only_event_fields: Optional[List[str]] = None | ||||||||||
# Some events can have stripped room state stored in the `unsigned` field. | ||||||||||
# This is required for invite and knock functionality. If this option is | ||||||||||
# False, that state will be removed from the event before it is returned. | ||||||||||
# Otherwise, it will be kept. | ||||||||||
include_stripped_room_state: bool = False | ||||||||||
|
||||||||||
|
||||||||||
_DEFAULT_SERIALIZE_EVENT_CONFIG = SerializeEventConfig() | ||||||||||
|
||||||||||
|
||||||||||
def serialize_event( | ||||||||||
e: Union[JsonDict, EventBase], | ||||||||||
time_now_ms: int, | ||||||||||
*, | ||||||||||
as_client_event: bool = True, | ||||||||||
event_format: Callable[[JsonDict], JsonDict] = format_event_for_client_v1, | ||||||||||
token_id: Optional[str] = None, | ||||||||||
only_event_fields: Optional[List[str]] = None, | ||||||||||
include_stripped_room_state: bool = False, | ||||||||||
config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG, | ||||||||||
) -> JsonDict: | ||||||||||
"""Serialize event for clients | ||||||||||
|
||||||||||
Args: | ||||||||||
e | ||||||||||
time_now_ms | ||||||||||
as_client_event | ||||||||||
event_format | ||||||||||
token_id | ||||||||||
only_event_fields | ||||||||||
include_stripped_room_state: Some events can have stripped room state | ||||||||||
stored in the `unsigned` field. This is required for invite and knock | ||||||||||
functionality. If this option is False, that state will be removed from the | ||||||||||
event before it is returned. Otherwise, it will be kept. | ||||||||||
config: Event serialization config | ||||||||||
|
||||||||||
Returns: | ||||||||||
The serialized event dictionary. | ||||||||||
|
@@ -348,11 +357,13 @@ def serialize_event( | |||||||||
|
||||||||||
if "redacted_because" in e.unsigned: | ||||||||||
d["unsigned"]["redacted_because"] = serialize_event( | ||||||||||
e.unsigned["redacted_because"], time_now_ms, event_format=event_format | ||||||||||
e.unsigned["redacted_because"], | ||||||||||
time_now_ms, | ||||||||||
config=SerializeEventConfig(event_format=config.event_format), | ||||||||||
) | ||||||||||
|
||||||||||
if token_id is not None: | ||||||||||
if token_id == getattr(e.internal_metadata, "token_id", None): | ||||||||||
if config.token_id is not None: | ||||||||||
if config.token_id == getattr(e.internal_metadata, "token_id", None): | ||||||||||
txn_id = getattr(e.internal_metadata, "txn_id", None) | ||||||||||
if txn_id is not None: | ||||||||||
d["unsigned"]["transaction_id"] = txn_id | ||||||||||
|
@@ -361,13 +372,14 @@ def serialize_event( | |||||||||
# that are meant to provide metadata about a room to an invitee/knocker. They are | ||||||||||
# intended to only be included in specific circumstances, such as down sync, and | ||||||||||
# should not be included in any other case. | ||||||||||
if not include_stripped_room_state: | ||||||||||
if not config.include_stripped_room_state: | ||||||||||
d["unsigned"].pop("invite_room_state", None) | ||||||||||
d["unsigned"].pop("knock_room_state", None) | ||||||||||
|
||||||||||
if as_client_event: | ||||||||||
d = event_format(d) | ||||||||||
if config.as_client_event: | ||||||||||
d = config.event_format(d) | ||||||||||
|
||||||||||
only_event_fields = config.only_event_fields | ||||||||||
if only_event_fields: | ||||||||||
if not isinstance(only_event_fields, list) or not all( | ||||||||||
isinstance(f, str) for f in only_event_fields | ||||||||||
|
@@ -390,18 +402,18 @@ def serialize_event( | |||||||||
event: Union[JsonDict, EventBase], | ||||||||||
time_now: int, | ||||||||||
*, | ||||||||||
config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG, | ||||||||||
bundle_aggregations: Optional[Dict[str, "BundledAggregations"]] = None, | ||||||||||
**kwargs: Any, | ||||||||||
) -> JsonDict: | ||||||||||
"""Serializes a single event. | ||||||||||
|
||||||||||
Args: | ||||||||||
event: The event being serialized. | ||||||||||
time_now: The current time in milliseconds | ||||||||||
config: Event serialization config | ||||||||||
bundle_aggregations: Whether to include the bundled aggregations for this | ||||||||||
event. Only applies to non-state events. (State events never include | ||||||||||
bundled aggregations.) | ||||||||||
**kwargs: Arguments to pass to `serialize_event` | ||||||||||
|
||||||||||
Returns: | ||||||||||
The serialized event | ||||||||||
|
@@ -410,7 +422,7 @@ def serialize_event( | |||||||||
if not isinstance(event, EventBase): | ||||||||||
return event | ||||||||||
|
||||||||||
serialized_event = serialize_event(event, time_now, **kwargs) | ||||||||||
serialized_event = serialize_event(event, time_now, config=config) | ||||||||||
|
||||||||||
# Check if there are any bundled aggregations to include with the event. | ||||||||||
if bundle_aggregations: | ||||||||||
|
@@ -419,6 +431,7 @@ def serialize_event( | |||||||||
self._inject_bundled_aggregations( | ||||||||||
event, | ||||||||||
time_now, | ||||||||||
config, | ||||||||||
bundle_aggregations[event.event_id], | ||||||||||
serialized_event, | ||||||||||
) | ||||||||||
|
@@ -456,6 +469,7 @@ def _inject_bundled_aggregations( | |||||||||
self, | ||||||||||
event: EventBase, | ||||||||||
time_now: int, | ||||||||||
config: SerializeEventConfig, | ||||||||||
aggregations: "BundledAggregations", | ||||||||||
serialized_event: JsonDict, | ||||||||||
) -> None: | ||||||||||
|
@@ -466,6 +480,7 @@ def _inject_bundled_aggregations( | |||||||||
time_now: The current time in milliseconds | ||||||||||
aggregations: The bundled aggregation to serialize. | ||||||||||
serialized_event: The serialized event which may be modified. | ||||||||||
config: Event serialization config | ||||||||||
|
||||||||||
""" | ||||||||||
serialized_aggregations = {} | ||||||||||
|
@@ -494,7 +509,10 @@ def _inject_bundled_aggregations( | |||||||||
|
||||||||||
# Don't bundle aggregations as this could recurse forever. | ||||||||||
serialized_latest_event = self.serialize_event( | ||||||||||
thread.latest_event, time_now, bundle_aggregations=None | ||||||||||
thread.latest_event, | ||||||||||
time_now, | ||||||||||
config=config, | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if this is correct or if we should only pass in the format, similar to what we do for redactions: synapse/synapse/events/utils.py Lines 349 to 352 in 45f4540
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was discussed a bit in #synapse-dev:matrix.org and we agree that things should definitely be consistent. @erikjohnston thinks that:
I'm going to make that change here. |
||||||||||
bundle_aggregations=None, | ||||||||||
) | ||||||||||
# Manually apply an edit, if one exists. | ||||||||||
if thread.latest_edit: | ||||||||||
|
@@ -515,20 +533,34 @@ def _inject_bundled_aggregations( | |||||||||
) | ||||||||||
|
||||||||||
def serialize_events( | ||||||||||
self, events: Iterable[Union[JsonDict, EventBase]], time_now: int, **kwargs: Any | ||||||||||
self, | ||||||||||
events: Iterable[Union[JsonDict, EventBase]], | ||||||||||
time_now: int, | ||||||||||
*, | ||||||||||
config: SerializeEventConfig = _DEFAULT_SERIALIZE_EVENT_CONFIG, | ||||||||||
bundle_aggregations: Optional[Dict[str, "BundledAggregations"]] = None, | ||||||||||
) -> List[JsonDict]: | ||||||||||
"""Serializes multiple events. | ||||||||||
|
||||||||||
Args: | ||||||||||
event | ||||||||||
time_now: The current time in milliseconds | ||||||||||
**kwargs: Arguments to pass to `serialize_event` | ||||||||||
config: Event serialization config | ||||||||||
bundle_aggregations: Whether to include the bundled aggregations for this | ||||||||||
event. Only applies to non-state events. (State events never include | ||||||||||
bundled aggregations.) | ||||||||||
|
||||||||||
Returns: | ||||||||||
The list of serialized events | ||||||||||
""" | ||||||||||
return [ | ||||||||||
self.serialize_event(event, time_now=time_now, **kwargs) for event in events | ||||||||||
self.serialize_event( | ||||||||||
event, | ||||||||||
time_now, | ||||||||||
config=config, | ||||||||||
bundle_aggregations=bundle_aggregations, | ||||||||||
) | ||||||||||
for event in events | ||||||||||
] | ||||||||||
|
||||||||||
|
||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of me thinks we should be consistent and use
EventSerializer
here, but it doesn't really matter unless we might bundled aggregations. I dislike that this is two separate methods though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like it might be worthwhile as an extra pass if you have the time and it looks sensible as you're doing it?