element-hq / element-hq/synapse

`event.freeze()` causes subtle downstream bugs that makes type comparisons brittle

Open
#18,117 2 comments 0 reactions 0 assignees View on GitHub
A-Modules T-Defect
Dominant language
Python
Stars
4.6k
Forks
600
Avg merge
5d 22h
Merged PRs (30d)
51

Description

*Spawning from reviewing https://github.com/element-hq/synapse/pull/18066 by @blackmad which has the original investigation on this,*

Normally, when using`EventBase`, you would expect to be able to do naive comparisons like this `isinstance(event.content.get("foo"), list)` or `isinstance(event.content.get("foo"), dict)`. But if `event.freeze()` was run beforehand, we actually need to take into account `(list, tuple)` and `(dict, immutabledict)`.

What makes this even more subtle is that [`event.freeze()`](https://github.com/element-hq/synapse/blob/aab3672037ee9ea71c54c89c86d9c0cc35ae184f/synapse/module_api/callbacks/third_party_event_rules_callbacks.py#L294) is only run when Synapse modules are configured.

It would be nice for the type system, linting, or anything to be able to catch this mistake. Or refactor things so that `event.freeze()` doesn't have such big downstream effects (perhaps we just make a copy that the Synapse module can do whatever it wants with).

Related issues:

- https://github.com/element-hq/synapse/pull/18066
- https://github.com/element-hq/synapse/issues/18101
- https://github.com/element-hq/synapse/issues/19689

Here is a list of places in the codebase where this matters (compiled from searching `isinstance\(.+list` and `isinstance\(.+dict` and manually inspecting if they're interacting with `EventBase`):

### Places we already take this into account ✅

It looks like we already take this into account in a few places:

`isinstance\(.+list`:

- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/events/auto_accept_invites.py#L160
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/message.py#L1780
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/message.py#L1789
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/push/bulk_push_rule_evaluator.py#L555
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/storage/controllers/state.py#L939
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/storage/controllers/state.py#L947

`isinstance\(.+dict`):

- *None*

### Places we still need to update :x:

But there are also other spots we also need to update:

`isinstance\(.+list`:

- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/event_auth.py#L329
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/room_summary.py#L958
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/sliding_sync/__init__.py#L821
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/sliding_sync/__init__.py#L827
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/sliding_sync/room_lists.py#L1440
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/push/bulk_push_rule_evaluator.py#L571
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/push/push_tools.py#L78
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/push/push_tools.py#L82
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/storage/databases/main/events.py#L2067

`isinstance\(.+dict`:

- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/event_auth.py#L335
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/events/utils.py#L864
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/events/utils.py#L874
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/event_auth.py#L335
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/room.py#L557
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/handlers/room.py#L576
- https://github.com/element-hq/synapse/blob/95a85b1129ddca60df920274edb9a2c1bf1d042c/synapse/push/bulk_push_rule_evaluator.py#L574

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.