From 23b8fcf85e9cb9dadb2b8ba2a29a2f717e19f790 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 May 2026 18:51:42 +0100 Subject: [PATCH] Port `Event.unsigned` field to Rust (#19708) Similar to #19706, let's port the `unsigned` field into a Rust class. This does change things a bit in that we now define exactly what unsigned fields that are allowed to be added to an event, and what actually gets persisted. This should be a noop though, as we carefully filter out what unsigned fields we allow in from federation, for example As a side effect of this cleanup, I think this fixes handling `unsigned.age` on events received over federation. --- changelog.d/19708.misc | 1 + rust/src/events/mod.rs | 2 + rust/src/events/unsigned.rs | 429 ++++++++++++++++++ synapse/events/__init__.py | 26 +- synapse/events/utils.py | 3 +- synapse/federation/federation_base.py | 32 +- synapse/federation/federation_client.py | 7 +- synapse/federation/federation_server.py | 17 +- synapse/handlers/message.py | 1 - synapse/storage/databases/main/events.py | 2 +- .../storage/databases/main/events_worker.py | 16 +- synapse/synapse_rust/events.pyi | 32 +- tests/events/test_utils.py | 32 +- tests/federation/test_federation_server.py | 10 +- 14 files changed, 568 insertions(+), 42 deletions(-) create mode 100644 changelog.d/19708.misc create mode 100644 rust/src/events/unsigned.rs diff --git a/changelog.d/19708.misc b/changelog.d/19708.misc new file mode 100644 index 0000000000..308c2b04d0 --- /dev/null +++ b/changelog.d/19708.misc @@ -0,0 +1 @@ +Port `Event.unsigned` field to Rust. diff --git a/rust/src/events/mod.rs b/rust/src/events/mod.rs index e42eb97739..5f505abb91 100644 --- a/rust/src/events/mod.rs +++ b/rust/src/events/mod.rs @@ -28,12 +28,14 @@ use pyo3::{ pub mod filter; mod internal_metadata; pub mod signatures; +pub mod unsigned; /// Called when registering modules with python. pub fn register_module(py: Python<'_>, m: &Bound<'_, PyModule>) -> PyResult<()> { let child_module = PyModule::new(py, "events")?; child_module.add_class::()?; child_module.add_class::()?; + child_module.add_class::()?; child_module.add_function(wrap_pyfunction!(filter::event_visible_to_server_py, m)?)?; m.add_submodule(&child_module)?; diff --git a/rust/src/events/unsigned.rs b/rust/src/events/unsigned.rs new file mode 100644 index 0000000000..c41ed7e6e1 --- /dev/null +++ b/rust/src/events/unsigned.rs @@ -0,0 +1,429 @@ +/* + * This file is licensed under the Affero General Public License (AGPL) version 3. + * + * Copyright (C) 2026 Element Creations Ltd + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as + * published by the Free Software Foundation, either version 3 of the + * License, or (at your option) any later version. + * + * See the GNU Affero General Public License for more details: + * . + * + */ + +use std::sync::{Arc, RwLock, RwLockReadGuard}; + +use pyo3::{ + exceptions::{PyKeyError, PyRuntimeError, PyTypeError}, + pyclass, pymethods, + types::{PyAnyMethods, PyList, PyListMethods, PyMapping}, + Bound, IntoPyObjectExt, PyAny, PyResult, Python, +}; +use pythonize::{depythonize, pythonize}; +use serde::{Deserialize, Serialize}; + +#[pyclass(frozen, skip_from_py_object)] +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +#[serde(transparent)] +pub struct Unsigned { + inner: Arc>, +} + +/// The fields in the unsigned data of an event that are persisted in the +/// database. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +struct PersistedUnsignedFields { + #[serde(skip_serializing_if = "Option::is_none")] + age_ts: Option, + #[serde(skip_serializing_if = "Option::is_none")] + replaces_state: Option, + #[serde(skip_serializing_if = "Option::is_none")] + invite_room_state: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + knock_room_state: Option>, +} + +/// The inner representation of the unsigned data of an event, which includes +/// both the fields that are persisted in the database and the fields that are +/// only used in memory. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub struct UnsignedInner { + #[serde(flatten)] + persisted_fields: PersistedUnsignedFields, + #[serde(skip_serializing_if = "Option::is_none")] + prev_content: Option>, // We use Box to minimise stack space + #[serde(skip_serializing_if = "Option::is_none")] + prev_sender: Option, +} + +/// The fields that exist on the unsigned data of an event. +/// +/// This is used when converting from python to rust, to ensure that if we add a +/// new field we don't forget to add it to all the necessary places. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum UnsignedField { + AgeTs, + ReplacesState, + InviteRoomState, + KnockRoomState, + PrevContent, + PrevSender, +} + +impl std::str::FromStr for UnsignedField { + type Err = (); + + fn from_str(s: &str) -> Result { + match s { + "age_ts" => Ok(Self::AgeTs), + "replaces_state" => Ok(Self::ReplacesState), + "invite_room_state" => Ok(Self::InviteRoomState), + "knock_room_state" => Ok(Self::KnockRoomState), + "prev_content" => Ok(Self::PrevContent), + "prev_sender" => Ok(Self::PrevSender), + _ => Err(()), + } + } +} + +impl Unsigned { + fn py_read(&self) -> PyResult> { + self.inner + .read() + .map_err(|_| PyRuntimeError::new_err("Unsigned lock poisoned")) + } + + fn py_write(&self) -> PyResult> { + self.inner + .write() + .map_err(|_| PyRuntimeError::new_err("Unsigned lock poisoned")) + } +} + +#[pymethods] +impl Unsigned { + #[new] + fn py_new(unsigned: Bound<'_, PyMapping>) -> PyResult { + let inner = depythonize(&unsigned)?; + + Ok(Self { + inner: Arc::new(RwLock::new(inner)), + }) + } + + fn __getitem__<'py>( + &self, + py: Python<'py>, + key: Bound<'_, PyAny>, + ) -> PyResult> { + let key = key + .extract::<&str>() + .map_err(|_| PyTypeError::new_err("Unsigned keys must be strings"))?; + + let field: UnsignedField = key + .parse() + .map_err(|_| PyKeyError::new_err(format!("Unsigned has no key '{key}'")))?; + + let unsigned = self.py_read()?; + + match field { + UnsignedField::AgeTs => Ok(unsigned + .persisted_fields + .age_ts + .ok_or_else(|| PyKeyError::new_err("age_ts"))? + .into_bound_py_any(py)?), + UnsignedField::ReplacesState => Ok((unsigned.persisted_fields.replaces_state) + .as_ref() + .ok_or_else(|| PyKeyError::new_err("replaces_state"))? + .into_bound_py_any(py)?), + UnsignedField::InviteRoomState => Ok(room_state_to_py( + py, + unsigned + .persisted_fields + .invite_room_state + .as_ref() + .ok_or_else(|| PyKeyError::new_err("invite_room_state"))?, + )?), + UnsignedField::KnockRoomState => Ok(room_state_to_py( + py, + unsigned + .persisted_fields + .knock_room_state + .as_ref() + .ok_or_else(|| PyKeyError::new_err("knock_room_state"))?, + )?), + UnsignedField::PrevContent => Ok(pythonize( + py, + unsigned + .prev_content + .as_ref() + .ok_or_else(|| PyKeyError::new_err("prev_content"))?, + )?), + UnsignedField::PrevSender => Ok((unsigned.prev_sender) + .as_ref() + .ok_or_else(|| PyKeyError::new_err("prev_sender"))? + .into_bound_py_any(py)?), + } + } + + fn __contains__(&self, key: Bound<'_, PyAny>) -> PyResult { + let Ok(key) = key.extract::<&str>() else { + return Ok(false); + }; + + let Ok(field) = key.parse::() else { + return Ok(false); + }; + + let unsigned = self.py_read()?; + + let exists = match field { + UnsignedField::AgeTs => unsigned.persisted_fields.age_ts.is_some(), + UnsignedField::ReplacesState => unsigned.persisted_fields.replaces_state.is_some(), + UnsignedField::InviteRoomState => unsigned.persisted_fields.invite_room_state.is_some(), + UnsignedField::KnockRoomState => unsigned.persisted_fields.knock_room_state.is_some(), + UnsignedField::PrevContent => unsigned.prev_content.is_some(), + UnsignedField::PrevSender => unsigned.prev_sender.is_some(), + }; + + Ok(exists) + } + + fn __setitem__(&self, key: Bound<'_, PyAny>, value: Bound<'_, PyAny>) -> PyResult<()> { + let key = key + .extract::<&str>() + .map_err(|_| PyTypeError::new_err("Unsigned keys must be strings"))?; + + let field: UnsignedField = key + .parse() + .map_err(|_| PyKeyError::new_err(format!("Unsigned has no key '{key}'")))?; + + let mut unsigned = self.py_write()?; + + match field { + UnsignedField::AgeTs => unsigned.persisted_fields.age_ts = Some(value.extract()?), + UnsignedField::ReplacesState => { + unsigned.persisted_fields.replaces_state = Some(value.extract()?) + } + UnsignedField::InviteRoomState => { + unsigned.persisted_fields.invite_room_state = Some(room_state_from_py(value)?) + } + UnsignedField::KnockRoomState => { + unsigned.persisted_fields.knock_room_state = Some(room_state_from_py(value)?) + } + UnsignedField::PrevContent => { + unsigned.prev_content = Some(Box::new(depythonize(&value)?)) + } + UnsignedField::PrevSender => unsigned.prev_sender = Some(value.extract()?), + } + + Ok(()) + } + + fn __delitem__(&self, key: Bound<'_, PyAny>) -> PyResult<()> { + let key = key + .extract::<&str>() + .map_err(|_| PyTypeError::new_err("Unsigned keys must be strings"))?; + + let field: UnsignedField = key + .parse() + .map_err(|_| PyKeyError::new_err(format!("Unsigned has no key '{key}'")))?; + + let mut unsigned = self.py_write()?; + + match field { + UnsignedField::AgeTs => unsigned.persisted_fields.age_ts = None, + UnsignedField::ReplacesState => unsigned.persisted_fields.replaces_state = None, + UnsignedField::InviteRoomState => unsigned.persisted_fields.invite_room_state = None, + UnsignedField::KnockRoomState => unsigned.persisted_fields.knock_room_state = None, + UnsignedField::PrevContent => unsigned.prev_content = None, + UnsignedField::PrevSender => unsigned.prev_sender = None, + } + + Ok(()) + } + + #[pyo3(signature = (key, default=None))] + fn get<'py>( + &self, + py: Python<'py>, + key: Bound<'py, PyAny>, + default: Option>, + ) -> PyResult>> { + match self.__getitem__(py, key) { + Ok(value) => Ok(Some(value)), + Err(err) => { + if err.is_instance_of::(py) { + Ok(default) + } else { + Err(err) + } + } + } + } + + fn for_persistence<'py>(&self, py: Python<'py>) -> PyResult> { + Ok(pythonize(py, &self.py_read()?.persisted_fields)?) + } + + fn for_event<'py>(&self, py: Python<'py>) -> PyResult> { + Ok(pythonize(py, &*self.py_read()?)?) + } +} + +fn room_state_to_py<'py>( + py: Python<'py>, + state: &[serde_json::Value], +) -> PyResult> { + let py_list = PyList::empty(py); + + for item in state { + py_list.append(pythonize(py, item)?)?; + } + + py_list.into_bound_py_any(py) +} + +fn room_state_from_py(value: Bound<'_, PyAny>) -> PyResult> { + let py_list = value.cast::()?; + + let mut state = Vec::with_capacity(py_list.len()); + for item in py_list.iter() { + state.push(pythonize::depythonize(&item)?); + } + + Ok(state) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn test_unsigned_field_from_str_valid() { + assert_eq!("age_ts".parse(), Ok(UnsignedField::AgeTs)); + assert_eq!("replaces_state".parse(), Ok(UnsignedField::ReplacesState)); + assert_eq!( + "invite_room_state".parse(), + Ok(UnsignedField::InviteRoomState) + ); + assert_eq!( + "knock_room_state".parse(), + Ok(UnsignedField::KnockRoomState) + ); + assert_eq!("prev_content".parse(), Ok(UnsignedField::PrevContent)); + assert_eq!("prev_sender".parse(), Ok(UnsignedField::PrevSender)); + } + + #[test] + fn test_unsigned_field_from_str_invalid() { + assert_eq!("".parse::(), Err(())); + assert_eq!("unknown".parse::(), Err(())); + // Case-sensitive: upper-case should not match. + assert_eq!("AGE_TS".parse::(), Err(())); + // Must be an exact match, no whitespace. + assert_eq!(" age_ts".parse::(), Err(())); + } + + #[test] + fn test_persisted_fields_serialize_empty_is_empty_object() { + let fields = PersistedUnsignedFields::default(); + let json = serde_json::to_value(&fields).unwrap(); + assert_eq!(json, json!({})); + } + + #[test] + fn test_persisted_fields_serialize_populated() { + let fields = PersistedUnsignedFields { + age_ts: Some(1234), + replaces_state: Some("$prev:example.com".to_string()), + invite_room_state: Some(vec![json!({"type": "m.room.name"})]), + knock_room_state: Some(vec![json!({"type": "m.room.topic"})]), + }; + let json = serde_json::to_value(&fields).unwrap(); + assert_eq!( + json, + json!({ + "age_ts": 1234, + "replaces_state": "$prev:example.com", + "invite_room_state": [{"type": "m.room.name"}], + "knock_room_state": [{"type": "m.room.topic"}], + }) + ); + } + + #[test] + fn test_unsigned_inner_flattens_persisted_fields() { + let inner = UnsignedInner { + persisted_fields: PersistedUnsignedFields { + age_ts: Some(99), + ..Default::default() + }, + prev_content: Some(Box::new(json!({"body": "hi"}))), + prev_sender: Some("@alice:example.com".to_string()), + }; + + let json = serde_json::to_value(&inner).unwrap(); + assert_eq!( + json, + json!({ + "age_ts": 99, + "prev_content": {"body": "hi"}, + "prev_sender": "@alice:example.com", + }) + ); + } + + #[test] + fn test_unsigned_inner_roundtrip() { + let original = UnsignedInner { + persisted_fields: PersistedUnsignedFields { + age_ts: Some(10), + replaces_state: Some("$state:example.com".to_string()), + invite_room_state: None, + knock_room_state: None, + }, + prev_content: Some(Box::new(json!({"membership": "join"}))), + prev_sender: None, + }; + + let json = serde_json::to_string(&original).unwrap(); + let roundtripped: UnsignedInner = serde_json::from_str(&json).unwrap(); + + assert_eq!(roundtripped.persisted_fields.age_ts, Some(10)); + assert_eq!( + roundtripped.persisted_fields.replaces_state.as_deref(), + Some("$state:example.com") + ); + assert_eq!( + roundtripped.prev_content.as_deref(), + Some(&json!({"membership": "join"})) + ); + assert_eq!(roundtripped.prev_sender, None); + } + + #[test] + fn test_unsigned_serializes_transparently() { + // `Unsigned` is `#[serde(transparent)]` over its inner, so serializing + // an empty default should yield an empty object rather than a wrapper. + let unsigned = Unsigned::default(); + let json = serde_json::to_value(&unsigned).unwrap(); + assert_eq!(json, json!({})); + } + + #[test] + fn test_unsigned_deserialize_from_flat_object() { + let json = json!({ + "age_ts": 5, + "prev_sender": "@bob:example.com", + }); + let unsigned: Unsigned = serde_json::from_value(json).unwrap(); + let inner = unsigned.inner.read().unwrap(); + assert_eq!(inner.persisted_fields.age_ts, Some(5)); + assert_eq!(inner.prev_sender.as_deref(), Some("@bob:example.com")); + } +} diff --git a/synapse/events/__init__.py b/synapse/events/__init__.py index fc0f6aadbd..0f850d19b1 100644 --- a/synapse/events/__init__.py +++ b/synapse/events/__init__.py @@ -44,7 +44,7 @@ from synapse.api.constants import ( StickyEvent, ) from synapse.api.room_versions import EventFormatVersions, RoomVersion, RoomVersions -from synapse.synapse_rust.events import EventInternalMetadata, Signatures +from synapse.synapse_rust.events import EventInternalMetadata, Signatures, Unsigned from synapse.types import ( JsonDict, StateKey, @@ -208,7 +208,7 @@ class EventBase(metaclass=abc.ABCMeta): self.room_version = room_version self.signatures = Signatures(signatures) - self.unsigned = unsigned + self.unsigned = Unsigned(unsigned) self.rejected_reason = rejected_reason self._dict = event_dict @@ -258,9 +258,25 @@ class EventBase(metaclass=abc.ABCMeta): return self._dict.get("state_key") def get_dict(self) -> JsonDict: + """Convert the event to a dictionary suitable for serialisation.""" d = dict(self._dict) d.update( - {"signatures": self.signatures.as_dict(), "unsigned": dict(self.unsigned)} + { + "signatures": self.signatures.as_dict(), + "unsigned": self.unsigned.for_event(), + } + ) + + return d + + def get_dict_for_persistence(self) -> JsonDict: + """Convert the event to a dictionary suitable for persistence.""" + d = dict(self._dict) + d.update( + { + "signatures": self.signatures.as_dict(), + "unsigned": self.unsigned.for_persistence(), + } ) return d @@ -401,7 +417,7 @@ class FrozenEvent(EventBase): for name, sigs in event_dict.pop("signatures", {}).items() } - unsigned = dict(event_dict.pop("unsigned", {})) + unsigned = event_dict.pop("unsigned", {}) # We intern these strings because they turn up a lot (especially when # caching). @@ -455,7 +471,7 @@ class FrozenEventV2(EventBase): assert "event_id" not in event_dict - unsigned = dict(event_dict.pop("unsigned", {})) + unsigned = event_dict.pop("unsigned", {}) # We intern these strings because they turn up a lot (especially when # caching). diff --git a/synapse/events/utils.py b/synapse/events/utils.py index f038fb5578..926c81b83d 100644 --- a/synapse/events/utils.py +++ b/synapse/events/utils.py @@ -47,6 +47,7 @@ from synapse.api.constants import ( from synapse.api.errors import Codes, SynapseError from synapse.api.room_versions import RoomVersion from synapse.logging.opentracing import SynapseTags, set_tag, trace +from synapse.synapse_rust.events import Unsigned from synapse.types import JsonDict, Requester from . import EventBase, FrozenEventV2, StrippedStateEvent, make_event_from_dict @@ -987,7 +988,7 @@ def validate_canonicaljson(value: Any) -> None: def maybe_upsert_event_field( - event: EventBase, container: JsonDict, key: str, value: object + event: EventBase, container: Unsigned, key: str, value: object ) -> bool: """Upsert an event field, but only if this doesn't make the event too large. diff --git a/synapse/federation/federation_base.py b/synapse/federation/federation_base.py index fe0710a0bf..1631f021ca 100644 --- a/synapse/federation/federation_base.py +++ b/synapse/federation/federation_base.py @@ -307,20 +307,27 @@ def _is_invite_via_3pid(event: EventBase) -> bool: def parse_events_from_pdu_json( - pdus_json: Sequence[JsonDict], room_version: RoomVersion + pdus_json: Sequence[JsonDict], + room_version: RoomVersion, + received_time: int | None = None, ) -> list[EventBase]: return [ - event_from_pdu_json(pdu_json, room_version) + event_from_pdu_json(pdu_json, room_version, received_time=received_time) for pdu_json in filter_pdus_for_valid_depth(pdus_json) ] -def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventBase: +def event_from_pdu_json( + pdu_json: JsonDict, room_version: RoomVersion, received_time: int | None = None +) -> EventBase: """Construct an EventBase from an event json received over federation Args: pdu_json: pdu as received over federation room_version: The version of the room this event belongs to + received_time: timestamp in ms that the event was received at. If + `None` then any `age` field in the `unsigned` block will be + dropped. Raises: SynapseError: if the pdu is missing required fields or is otherwise @@ -333,6 +340,25 @@ def event_from_pdu_json(pdu_json: JsonDict, room_version: RoomVersion) -> EventB if "unsigned" in pdu_json: _strip_unsigned_values(pdu_json) + # Handle the `age` field, which is sent by some servers as part of the + # `unsigned` block. We convert this into an `age_ts` field, which is + # what Synapse uses internally. We also remove the `age` field to avoid + # confusion. + # + # c.f. https://github.com/matrix-org/synapse/issues/8429 + unsigned = pdu_json["unsigned"] + age = unsigned.pop("age", None) + + # We check that the `age` is actually an int before using it below. We + # don't error here as the `age` a) doesn't affect the validity of the + # event, and b) is best effort anyway. + if not isinstance(age, int): + age = None + + unsigned.pop("age_ts", None) + if received_time is not None and age is not None: + unsigned["age_ts"] = received_time - int(age) + depth = pdu_json["depth"] if type(depth) is not int: # noqa: E721 raise SynapseError(400, "Depth %r not an intger" % (depth,), Codes.BAD_JSON) diff --git a/synapse/federation/federation_client.py b/synapse/federation/federation_client.py index 78a1900c73..2b5ef5fbac 100644 --- a/synapse/federation/federation_client.py +++ b/synapse/federation/federation_client.py @@ -1574,10 +1574,15 @@ class FederationClient(FederationBase): min_depth=min_depth, timeout=timeout, ) + received_time = self._clock.time_msec() room_version = await self.store.get_room_version(room_id) - events = parse_events_from_pdu_json(content.get("events", []), room_version) + events = parse_events_from_pdu_json( + content.get("events", []), + room_version, + received_time=received_time, + ) signed_events = await self._check_sigs_and_hash_for_pulled_events_and_fetch( destination, events, room_version=room_version diff --git a/synapse/federation/federation_server.py b/synapse/federation/federation_server.py index 1bbe144422..6069287975 100644 --- a/synapse/federation/federation_server.py +++ b/synapse/federation/federation_server.py @@ -451,16 +451,6 @@ class FederationServer(FederationBase): newest_pdu_ts = 0 for p in transaction.pdus: - # FIXME (richardv): I don't think this works: - # https://github.com/matrix-org/synapse/issues/8429 - if "unsigned" in p: - unsigned = p["unsigned"] - if "age" in unsigned: - p["age"] = unsigned["age"] - if "age" in p: - p["age_ts"] = request_time - int(p["age"]) - del p["age"] - # We try and pull out an event ID so that if later checks fail we # can log something sensible. We don't mandate an event ID here in # case future event formats get rid of the key. @@ -488,10 +478,15 @@ class FederationServer(FederationBase): continue try: - event = event_from_pdu_json(p, room_version) + event = event_from_pdu_json(p, room_version, received_time=request_time) except SynapseError as e: logger.info("Ignoring PDU for failing to deserialize: %s", e) continue + except Exception as e: + # We catch all exceptions here as we don't want a single bad + # event to cause us to fail the whole transaction. + logger.exception("Error deserializing PDU: %s", e) + continue pdus_by_room.setdefault(room_id, []).append(event) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index ed3bce69ab..0687c9fa79 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -2089,7 +2089,6 @@ class EventCreationHandler: returned_invite = await federation_handler.send_invite( invitee.domain, event ) - event.unsigned.pop("room_state", None) # TODO: Make sure the signatures actually are correct. event.signatures.update(returned_invite.signatures.as_dict()) diff --git a/synapse/storage/databases/main/events.py b/synapse/storage/databases/main/events.py index 12c918eca6..5f6b03a988 100644 --- a/synapse/storage/databases/main/events.py +++ b/synapse/storage/databases/main/events.py @@ -2711,7 +2711,7 @@ class PersistEventsStore: return def event_dict(event: EventBase) -> JsonDict: - d = event.get_dict() + d = event.get_dict_for_persistence() d.pop("redacted", None) d.pop("redacted_because", None) return d diff --git a/synapse/storage/databases/main/events_worker.py b/synapse/storage/databases/main/events_worker.py index cc79b8042b..6f26bd17ce 100644 --- a/synapse/storage/databases/main/events_worker.py +++ b/synapse/storage/databases/main/events_worker.py @@ -779,14 +779,26 @@ class EventsWorkerStore(SQLBaseStore): events.append(event) if get_prev_content: - if "replaces_state" in event.unsigned: + # The `event` here might be in the cache, and so might have + # already had the `prev_content` and `prev_sender` fields added + # to its unsigned. + # + # We check if a) we should add the previous content, and b) if + # we have already added it. + replaces_state = "replaces_state" in event.unsigned + has_prev = ( + "prev_content" in event.unsigned and "prev_sender" in event.unsigned + ) + if replaces_state and not has_prev: prev = await self.get_event( event.unsigned["replaces_state"], get_prev_content=False, allow_none=True, ) if prev: - event.unsigned = dict(event.unsigned) + # This mutates the cached event, but that's fine as the + # previous content/sender will be the same for all + # requests for this event. event.unsigned["prev_content"] = prev.content event.unsigned["prev_sender"] = prev.sender diff --git a/synapse/synapse_rust/events.pyi b/synapse/synapse_rust/events.pyi index 40cf3f59f2..5ae2bb880a 100644 --- a/synapse/synapse_rust/events.pyi +++ b/synapse/synapse_rust/events.pyi @@ -12,7 +12,7 @@ from typing import Any, Mapping -from synapse.types import JsonDict +from synapse.types import JsonDict, JsonMapping class EventInternalMetadata: def __init__(self, internal_metadata_dict: JsonDict): ... @@ -183,3 +183,33 @@ class Signatures: def as_dict(self) -> dict[str, dict[str, str]]: ... """Return a copy of the signatures as a dictionary.""" + +class Unsigned: + """A class representing the unsigned data of an event.""" + + def __init__(self, unsigned_dict: JsonMapping): ... + def __getitem__(self, key: str) -> Any: ... + """Get the value for the given key. + + Raises KeyError if the key is unset or not recognised.""" + + def __setitem__(self, key: str, value: Any) -> None: ... + """Set the value for the given key. + + Raises KeyError if the key is not recognised.""" + + def __delitem__(self, key: str) -> None: ... + """Delete the value for the given key. + + Raises KeyError if the key is unset or not recognised.""" + + def __contains__(self, key: Any) -> bool: ... + def get(self, key: str, default: Any = None) -> Any: ... + """Get the value for the given key, or ``default`` if the key is unset.""" + + def for_persistence(self) -> JsonDict: ... + """Return a dict of the fields that should be persisted to the database.""" + + def for_event(self) -> JsonDict: ... + """Return a dict of all unsigned fields, including those only kept in + memory, suitable for inclusion in an event.""" diff --git a/tests/events/test_utils.py b/tests/events/test_utils.py index 12ef42866d..a402dd70d1 100644 --- a/tests/events/test_utils.py +++ b/tests/events/test_utils.py @@ -67,25 +67,31 @@ def MockEvent(**kwargs: Any) -> EventBase: class TestMaybeUpsertEventField(stdlib_unittest.TestCase): def test_update_okay(self) -> None: event = make_event_from_dict({"event_id": "$1234"}) - success = maybe_upsert_event_field(event, event.unsigned, "key", "value") + success = maybe_upsert_event_field( + event, event.unsigned, "replaces_state", "value" + ) self.assertTrue(success) - self.assertEqual(event.unsigned["key"], "value") + self.assertEqual(event.unsigned["replaces_state"], "value") def test_update_not_okay(self) -> None: event = make_event_from_dict({"event_id": "$1234"}) LARGE_STRING = "a" * 100_000 - success = maybe_upsert_event_field(event, event.unsigned, "key", LARGE_STRING) + success = maybe_upsert_event_field( + event, event.unsigned, "replaces_state", LARGE_STRING + ) self.assertFalse(success) - self.assertNotIn("key", event.unsigned) + self.assertNotIn("replaces_state", event.unsigned) def test_update_not_okay_leaves_original_value(self) -> None: event = make_event_from_dict( - {"event_id": "$1234", "unsigned": {"key": "value"}} + {"event_id": "$1234", "unsigned": {"replaces_state": "value"}} ) LARGE_STRING = "a" * 100_000 - success = maybe_upsert_event_field(event, event.unsigned, "key", LARGE_STRING) + success = maybe_upsert_event_field( + event, event.unsigned, "replaces_state", LARGE_STRING + ) self.assertFalse(success) - self.assertEqual(event.unsigned["key"], "value") + self.assertEqual(event.unsigned["replaces_state"], "value") class PruneEventTestCase(stdlib_unittest.TestCase): @@ -623,7 +629,7 @@ class CloneEventTestCase(stdlib_unittest.TestCase): { "type": "A", "event_id": "$test:domain", - "unsigned": {"a": 1, "b": 2}, + "unsigned": {"age_ts": 1, "replaces_state": "2"}, }, RoomVersions.V1, {"txn_id": "txn"}, @@ -634,10 +640,14 @@ class CloneEventTestCase(stdlib_unittest.TestCase): self.assertEqual(original.internal_metadata.instance_name, "worker1") cloned = clone_event(original) - cloned.unsigned["b"] = 3 + cloned.unsigned["age_ts"] = 3 - self.assertEqual(original.unsigned, {"a": 1, "b": 2}) - self.assertEqual(cloned.unsigned, {"a": 1, "b": 3}) + self.assertEqual( + original.unsigned.for_event(), {"age_ts": 1, "replaces_state": "2"} + ) + self.assertEqual( + cloned.unsigned.for_event(), {"age_ts": 3, "replaces_state": "2"} + ) self.assertEqual(cloned.internal_metadata.stream_ordering, 1234) self.assertEqual(cloned.internal_metadata.instance_name, "worker1") self.assertEqual(cloned.internal_metadata.txn_id, "txn") diff --git a/tests/federation/test_federation_server.py b/tests/federation/test_federation_server.py index a40e0b0680..7b1a2a5adc 100644 --- a/tests/federation/test_federation_server.py +++ b/tests/federation/test_federation_server.py @@ -754,9 +754,9 @@ class StripUnsignedFromEventsTestCase(unittest.TestCase): }, } - filtered_event2 = event_from_pdu_json(event2, RoomVersions.V1) - self.assertIn("age", filtered_event2.unsigned) - self.assertEqual(14, filtered_event2.unsigned["age"]) + filtered_event2 = event_from_pdu_json(event2, RoomVersions.V1, received_time=20) + self.assertIn("age_ts", filtered_event2.unsigned) + self.assertEqual(6, filtered_event2.unsigned["age_ts"]) self.assertNotIn("more warez", filtered_event2.unsigned) # Invite_room_state is allowed in events of type m.room.member self.assertIn("invite_room_state", filtered_event2.unsigned) @@ -779,8 +779,8 @@ class StripUnsignedFromEventsTestCase(unittest.TestCase): "invite_room_state": [], }, } - filtered_event3 = event_from_pdu_json(event3, RoomVersions.V1) - self.assertIn("age", filtered_event3.unsigned) + filtered_event3 = event_from_pdu_json(event3, RoomVersions.V1, received_time=20) + self.assertIn("age_ts", filtered_event3.unsigned) # Invite_room_state field is only permitted in event type m.room.member self.assertNotIn("invite_room_state", filtered_event3.unsigned) self.assertNotIn("more warez", filtered_event3.unsigned)