/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement "event_fields" in filters #1638

Merged
merged 8 commits into from Nov 22, 2016

Conversation

Reviewers

@erikjohnston erikjohnston

Assignees

@erikjohnston erikjohnston

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Kegsay @erikjohnston
@Kegsay
Contributor

Kegsay commented Nov 22, 2016

http://matrix.org/docs/spec/client_server/r0.2.0.html#post-matrix-client-r0-user-userid-filter

event_fields	[string]

List of event fields to include. If this list is absent then all fields are included. The entries may include '.' charaters to indicate sub-fields. So ['content.body'] will include the 'body' field of the 'content' object. A literal '.' character in a field name may be escaped using a ''. A server may include more fields than were requested.

Kegsay added some commits Nov 21, 2016

@Kegsay
Add filter_event_fields and filter_field to FilterCollection
e90fcd9
@Kegsay
Move event_fields filtering to serialize_event 
Also make it an inclusive not exclusive filter, as the spec demands.

Some checks were not successful

1 errored and 1 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #1134 origin/kegan/sync-event-fields failed in 4 min 27 sec
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #2066 origin/kegan/sync-event-fields succeeded in 1 min 19 sec
Details
f97511a
@Kegsay
Start adding some tests

All checks have passed

3 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #2067 origin/kegan/sync-event-fields succeeded in 40 sec
Details
@matrixbot
Sytest Dendron (Commit) — Build #1135 origin/kegan/sync-event-fields succeeded in 14 min
Details
@matrixbot
Unit Tests (Commit) — Build #2098 origin/kegan/sync-event-fields succeeded in 2 min 49 sec
Details
70a2157
@Kegsay
Add remaining tests

All checks have passed

5 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #2068 origin/kegan/sync-event-fields succeeded in 36 sec
Details
@matrixbot
Sytest Dendron (Commit) — Build #1136 origin/kegan/sync-event-fields succeeded in 12 min
Details
@matrixbot
Sytest Postgres (Commit) — Build #1973 origin/kegan/sync-event-fields succeeded in 6 min 10 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #2018 origin/kegan/sync-event-fields succeeded in 8 min 23 sec
Details
@matrixbot
Unit Tests (Commit) — Build #2099 origin/kegan/sync-event-fields succeeded in 3 min 14 sec
Details
53b27bb
@Kegsay
More tests
0a8b0ee
@Kegsay
Glue only_event_fields into the sync rest servlet

All checks have passed

5 successful checks
@matrixbot
Flake8 + Packaging (Commit) — Build #2070 origin/kegan/sync-event-fields succeeded in 1 min 1 sec
Details
@matrixbot
Sytest Dendron (Commit) — Build #1138 origin/kegan/sync-event-fields succeeded in 11 min
Details
@matrixbot
Sytest Postgres (Commit) — Build #1975 origin/kegan/sync-event-fields succeeded in 8 min 54 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #2020 origin/kegan/sync-event-fields succeeded in 6 min 11 sec
Details
@matrixbot
Unit Tests (Commit) — Build #2101 origin/kegan/sync-event-fields succeeded in 3 min 28 sec
Details
cea4e4e
@Kegsay
Also check for dict since sometimes they aren't frozen

Some checks were not successful

1 failing and 9 successful checks
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #2075 origin/kegan/sync-event-fields succeeded in 46 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Commit) — Build #1141 origin/kegan/sync-event-fields succeeded in 12 min
Details
@matrixbot
Sytest Postgres (Commit) — Build #1978 origin/kegan/sync-event-fields succeeded in 8 min 35 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #2024 origin/kegan/sync-event-fields succeeded in 6 min 58 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #2106 origin/kegan/sync-event-fields succeeded in 3 min 10 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
6d4e6d4

@Kegsay Kegsay assigned erikjohnston Nov 22, 2016

@erikjohnston

erikjohnston reviewed Nov 22, 2016
View changes

You've mentioned performance a couple of times, I wonder if its worth doing a quick benchmark to see how long it takes to serialise events?

synapse/events/utils.py
+ for sub_field in field:
+ if sub_field not in sub_out_dict:
+ sub_out_dict[sub_field] = {}
+ sub_out_dict = sub_out_dict[sub_field]
@erikjohnston

erikjohnston Nov 22, 2016

Owner

You can replace the 3 lines with:

sub_out_dict = sub_out_dict.setdefault(sub_field, {})
@Kegsay

Kegsay Nov 22, 2016

Contributor

Done.

synapse/events/utils.py
+ # for each element of the output array of arrays:
+ # remove escaping so we can use the right key names. This purposefully avoids
+ # using list comprehensions to avoid needless allocations as this may be called
+ # on a lot of events.
@erikjohnston

erikjohnston Nov 22, 2016

Owner

I did a quick benchmark and the following list comprehension is actually quicker:

split_fields[:] = [ [field.replace(r'\.', r'.') for field in field_array] for field_array in split_fields]
@erikjohnston

erikjohnston Nov 22, 2016

Owner

And if we really cared about performance, we would pull this up to be calculated when we initially parsed the filter

@Kegsay

Kegsay Nov 22, 2016

Contributor

Done.

synapse/events/utils.py
+ d = event_format(d)
+
+ if (only_event_fields and isinstance(only_event_fields, list) and
+ all(isinstance(f, basestring) for f in only_event_fields)):
@erikjohnston

erikjohnston Nov 22, 2016

Owner

If the args are incorrect this should raise an exception.

@Kegsay

Kegsay Nov 22, 2016

Contributor

Done.

@Kegsay
Review comments

Some checks were not successful

1 errored and 9 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #1142 origin/kegan/sync-event-fields failed in 2 min 12 sec
Details
@matrixbot
Flake8 + Packaging (Commit) — Build #2076 origin/kegan/sync-event-fields succeeded in 55 sec
Details
@matrixbot
Flake8 + Packaging (Merged PR) — Build finished.
Details
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Commit) — Build #1979 origin/kegan/sync-event-fields succeeded in 9 min 20 sec
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Commit) — Build #2025 origin/kegan/sync-event-fields succeeded in 6 min 22 sec
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
@matrixbot
Unit Tests (Commit) — Build #2107 origin/kegan/sync-event-fields succeeded in 3 min 19 sec
Details
@matrixbot
Unit Tests (Merged PR) — Build finished.
Details
c3d963a
@erikjohnston
Owner

erikjohnston commented Nov 22, 2016

LGTM

@Kegsay Kegsay merged commit d4a459f into develop Nov 22, 2016
9 of 10 checks passed

9 of 10 checks passed

Sytest Dendron (Commit) Build #1142 origin/kegan/sync-event-fields failed in 2 min 12 sec
Details
Flake8 + Packaging (Commit) Build #2076 origin/kegan/sync-event-fields succeeded in 55 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #1979 origin/kegan/sync-event-fields succeeded in 9 min 20 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #2025 origin/kegan/sync-event-fields succeeded in 6 min 22 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #2107 origin/kegan/sync-event-fields succeeded in 3 min 19 sec
Details
Unit Tests (Merged PR) Build finished.
Details

@Kegsay Kegsay referenced this pull request in matrix-org/matrix-appservice-irc Nov 25, 2016

Closed

Freenode bridge outage (19-21 Nov) #288

@richvdh richvdh deleted the kegan/sync-event-fields branch Dec 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment