New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor event building into EventBuilder #4481

Merged
merged 8 commits into from Jan 29, 2019

Conversation

3 participants
@erikjohnston
Copy link
Member

erikjohnston commented Jan 25, 2019

This is so that everything is done in one place, making it easier to change the event format based on room version.

The crux of what's happening here is that the EventBuilder is now meant to be independent of the format version, allowing the rest of the code to build up events without worrying about different versions. This is done by making it an attr class with well defined fields that are of interest when building an event (mainly type/state_key/content/etc), and only adding the rest of the fields when we finally build the event (which handles things like adding prev events, signing and hashing it, using the right format etc)

Hopefully the commits are relatively stand alone.

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 25, 2019

@erikjohnston erikjohnston force-pushed the erikj/event_builder branch from b0b1161 to 9fb9205 Jan 25, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 25, 2019

Codecov Report

Merging #4481 into develop will increase coverage by 6.89%.
The diff coverage is 96.8%.

@@             Coverage Diff             @@
##           develop    #4481      +/-   ##
===========================================
+ Coverage    67.87%   74.76%   +6.89%     
===========================================
  Files          336      336              
  Lines        34163    34198      +35     
  Branches      5556     5564       +8     
===========================================
+ Hits         23188    25568    +2380     
+ Misses        9362     7054    -2308     
+ Partials      1613     1576      -37
Show resolved Hide resolved synapse/crypto/event_signing.py Outdated
Show resolved Hide resolved synapse/events/builder.py Outdated
Show resolved Hide resolved synapse/events/builder.py Outdated
Show resolved Hide resolved synapse/handlers/message.py
Show resolved Hide resolved synapse/events/builder.py
Show resolved Hide resolved synapse/events/builder.py

@erikjohnston erikjohnston added this to To Do in Homeserver Task Board via automation Jan 28, 2019

@erikjohnston erikjohnston moved this from To Do to In progress in Homeserver Task Board Jan 28, 2019

@erikjohnston

This comment has been minimized.

Copy link
Member Author

erikjohnston commented Jan 28, 2019

Now based on #4493 and #4494

erikjohnston added some commits Jan 25, 2019

Refactor event building into EventBuilder
This is so that everything is done in one place, making it easier to
change the event format based on room version
Replace usage of builder.user_id with builder.sender
`.user_id` is proxed to `.sender` in FrozenEvent, so this has no
functional change

@erikjohnston erikjohnston force-pushed the erikj/event_builder branch from 6b70f5e to aee39f7 Jan 29, 2019

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 29, 2019

@richvdh
Copy link
Member

richvdh left a comment

generally seems plausible. a few questions.

Show resolved Hide resolved synapse/events/builder.py Outdated
Show resolved Hide resolved synapse/handlers/message.py
Show resolved Hide resolved synapse/handlers/message.py Outdated
Deferred[FrozenEvent]
"""

state_ids = yield self._state.get_current_state_ids(

This comment has been minimized.

@richvdh

richvdh Jan 29, 2019

Member

Slightly concerned that we used to do context.get_prev_state_ids, which might return something different to this?

This comment has been minimized.

@erikjohnston

erikjohnston Jan 29, 2019

Author Member

In practice, they're exactly the same, compute_event_context just resolves the state across the prev_events to get prev_state_ids. From a protocol perspective, auth_events should be based on the state at the event.

We could make compute_event_context work with an EventBuilder, but I was slightly shying away from doing so in the interests of not changing too much. We could probably do so though

@erikjohnston erikjohnston requested a review from matrix-org/synapse-core Jan 29, 2019

@richvdh
Copy link
Member

richvdh left a comment

lgtm I think...

@erikjohnston erikjohnston merged commit b8d75ef into develop Jan 29, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hawkowl hawkowl moved this from In progress to Done in Homeserver Task Board Jan 29, 2019

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