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

Refactor Filter to handle data differently #11194

Merged
merged 12 commits into from
Oct 27, 2021
Merged

Refactor Filter to handle data differently #11194

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
c923cc8
Re-use calculated variables in filtering.
clokep Oct 25, 2021
32074ad
Pre-calculate limit.
clokep Oct 26, 2021
7004132
Pre-calculate lazy_load_members.
clokep Oct 26, 2021
590d131
Pre-calculate include_redundant_members.
clokep Oct 26, 2021
efb248f
Rename an internal method.
clokep Oct 25, 2021
ac9413b
Change arguments to _check_fields.
clokep Oct 26, 2021
69da896
Do not fetch content twice.
clokep Oct 26, 2021
5c00c85
Check different fields for presence vs. events.
clokep Oct 26, 2021
cf0f515
Simplify _check_fields.
clokep Oct 26, 2021
5f12887
Newsfragment
clokep Oct 27, 2021
2bb1462
Review comments.
clokep Oct 27, 2021
14a805c
Clarify use of content.
clokep Oct 27, 2021
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
3 changes: 1 addition & 2 deletions synapse/api/filtering.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,11 +323,11 @@ def check(self, event: FilterEvent) -> bool:
labels: List[str] = []
else:
sender = event.get("sender", None)
content = event.get("content") or {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I think we generally write this this as event.get("content", {})?

If event["content"] was an empty string, False or an empty list then we'd interpret the content as an empty dictionary. That feels like it might be unintentionally suppressing a type check?

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 was done on purpose to handle those cases actually! If it is of the wrong type we really just want to pretend it is a dictionary.

Regardless this is mostly just moving code which could be improved. I'll add some more error checking though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh, I guess this is an event we have already persisted and so it needs to remain as it is? (As in, we can't reject it and say "this is of the wrong format"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right they should always have content, but event is a misnomer -- this is sometimes account data or potentially other things.

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 tried to make this better with 14a805c.

if not sender:
# Presence events had their 'sender' in content.user_id, but are
# now handled above. We don't know if anything else uses this
# form. TODO: Check this and probably remove it.
content = event.get("content")
# account_data has been allowed to have non-dict content, so
# check type first
if isinstance(content, dict):
Expand All @@ -336,7 +336,6 @@ def check(self, event: FilterEvent) -> bool:
room_id = event.get("room_id", None)
ev_type = event.get("type", None)

content = event.get("content") or {}
# check if there is a string url field in the content for filtering purposes
contains_url = isinstance(content.get("url"), str)
labels = content.get(EventContentFields.LABELS, [])
Expand Down