Skip to content
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

State events filtering database api #438

Merged
merged 14 commits into from Aug 7, 2019

Conversation

@CromFr
Copy link
Contributor

commented May 9, 2018

This PR adds a gomatrixserverlib.Filter parameter to functions handling the syncapi_current_room_state table. It does not implement any filtering logic inside the syncapi IncrementalSync/CompleteSync functions, just the APIs for future use.

Default filters are provided as placeholders in IncrementalSync/CompleteSync, so behaviour should be unchanged (except the default 20 event limit)

SQL table will be changed. You can upgrade an existing database using:

ALTER TABLE syncapi_current_room_state  ADD COLUMN IF NOT EXISTS sender text;
UPDATE syncapi_current_room_state SET sender=(event_json::json->>'sender');
ALTER TABLE syncapi_current_room_state ALTER COLUMN sender SET NOT NULL;
ALTER TABLE syncapi_current_room_state  ADD COLUMN IF NOT EXISTS contains_url bool;
UPDATE syncapi_current_room_state SET contains_url=(event_json::json->>'content')::json->>'url' IS NOT NULL;
ALTER TABLE syncapi_current_room_state ALTER COLUMN contains_url SET NOT NULL;

Note: This depends on #436 (and includes all its commits). I'm not sure if Github will remove the duplicated commits once #436 is merged.

CromFr added some commits Apr 12, 2018

Store & retrieve filters as structs rather than []byte
Requires gomatrix to be updated with matrix-org/gomatrix#46

Signed-off-by: Thibaut CHARLES cromfr@gmail.com
gb vendor update github.com/matrix-org/gomatrix
Signed-off-by: Thibaut CHARLES cromfr@gmail.com
selectFilter details
Signed-off-by: Thibaut CHARLES cromfr@gmail.com
Typo fix
Signed-off-by: Thibaut CHARLES cromfr@gmail.com
s/gomatrix.Filter/gomatrixserverlib.Filter/
Signed-off-by: Thibaut CHARLES cromfr@gmail.com
Impl filtering ability in syncapi_current_room_state SQL queries
Signed-off-by: Thibaut CHARLES cromfr@gmail.com

@CromFr CromFr force-pushed the CromFr:state_filtering branch from 9225c30 to 9febd42 Jun 3, 2018

@anoadragon453
Copy link
Member

left a comment

Looking much better! A couple small things.

filterArray, err := json.Marshal(filter)
if err != nil {
// Validate generates a user-friendly error
if err = filter.Validate(); err != nil {

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

Does this still check if the JSON is in the correct format, as the code did before?

This comment has been minimized.

Copy link
@CromFr

CromFr Jun 4, 2018

Author Contributor

What do you mean by "if the JSON is in the correct format" ?

UnmarshalJSONRequest already checks if the JSON is valid and that types from the Filter struct match types from the json data.
Validate() only checks if values inside the Filter struct match those allowed by the documentation (ie the event_format field)

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

Ah, right. I was just looking at the Marshal being taken off and thinking that was the only line checking the JSON correctness.

@@ -44,7 +45,10 @@ func OnIncomingStateRequest(req *http.Request, db *storage.SyncServerDatabase, r
// TODO(#287): Auth request and handle the case where the user has left (where
// we should return the state at the poin they left)

stateEvents, err := db.GetStateEventsForRoom(req.Context(), roomID)
stateFilterPart := gomatrix.DefaultFilterPart()

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

A few gomatrix's still left in here.

@@ -166,9 +178,17 @@ func (s *currentRoomStateStatements) selectRoomIDsWithMembership(
// CurrentState returns all the current state events for the given room.
func (s *currentRoomStateStatements) selectCurrentState(
ctx context.Context, txn *sql.Tx, roomID string,
stateFilterPart *gomatrix.FilterPart,

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

and here

@@ -177,10 +178,10 @@ func (d *SyncServerDatabase) GetStateEvent(
// Returns an empty slice if no state events could be found for this room.
// Returns an error if there was an issue with the retrieval.
func (d *SyncServerDatabase) GetStateEventsForRoom(
ctx context.Context, roomID string,
ctx context.Context, roomID string, stateFilterPart *gomatrix.FilterPart,

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

and here

@@ -233,11 +234,13 @@ func (d *SyncServerDatabase) IncrementalSync(
var succeeded bool
defer common.EndTransaction(txn, &succeeded)

stateFilterPart := gomatrix.DefaultFilterPart() // TODO: use filter provided in request

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

and here

@@ -286,11 +289,13 @@ func (d *SyncServerDatabase) CompleteSync(
return nil, err
}

stateFilterPart := gomatrix.DefaultFilterPart() // TODO: use filter provided in request

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

and here

@@ -547,6 +552,7 @@ func (d *SyncServerDatabase) fetchMissingStateEvents(
func (d *SyncServerDatabase) getStateDeltas(
ctx context.Context, device *authtypes.Device, txn *sql.Tx,
fromPos, toPos types.StreamPosition, userID string,
stateFilterPart *gomatrix.FilterPart,

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

and here


// Validate checks if the filter contains valid property values
func (filter *Filter) Validate() error {
if filter.EventFormat != "client" && filter.EventFormat != "federation" {

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

Is this still relevant to the client-side gomatrix lib?

This comment has been minimized.

Copy link
@CromFr

CromFr Jun 4, 2018

Author Contributor

IMHO it's still interesting to have some kind of client-side validation before sending the query.
Though it's not very useful since you must rely on server response anyway.

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Jun 4, 2018

Member

Ah, well best a chat left to the gomatrix repo anyways.

Replaced gomatrix calls to gomatrixserverlib
Signed-off-by: Thibaut CHARLES cromfr@gmail.com
@anoadragon453

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

LGTM.

@Cnly

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2019

Since this PR has got an approval but stuck for a really long time, I'd like to help merge master onto it and see what we'll get.

CromFr and others added some commits Aug 5, 2019

@Cnly

Cnly approved these changes Aug 7, 2019

@anoadragon453 anoadragon453 merged commit 76e4eba into matrix-org:master Aug 7, 2019

8 checks passed

buildkite/dendrite Build #226 passed (5 minutes, 5 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-11 Passed (52 seconds)
Details
buildkite/dendrite/build-slash-go-1-dot-12 Passed (49 seconds)
Details
buildkite/dendrite/lint-slash-go-1-dot-12 Passed (1 minute, 54 seconds)
Details
buildkite/dendrite/pipeline Passed (8 seconds)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-11 Passed (59 seconds)
Details
buildkite/dendrite/unit-tests-slash-go-1-dot-12 Passed (51 seconds)
Details
ci/circleci: dendrite Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.