From a4ecedc0c8041a2c958ac65e8083d5007791e8a3 Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Fri, 26 Jun 2020 15:09:10 +0100 Subject: [PATCH] Default to the default timeline limit if it's unset, also strip the extra event correctly --- syncapi/routing/filter.go | 28 +++++++++++++++++-- .../postgres/output_room_events_table.go | 18 +++++++----- .../sqlite3/output_room_events_table.go | 18 +++++++----- syncapi/sync/notifier_test.go | 2 +- syncapi/sync/request.go | 4 +-- 5 files changed, 50 insertions(+), 20 deletions(-) diff --git a/syncapi/routing/filter.go b/syncapi/routing/filter.go index 1505e2a4b..baa4d841c 100644 --- a/syncapi/routing/filter.go +++ b/syncapi/routing/filter.go @@ -15,14 +15,17 @@ package routing import ( + "encoding/json" + "io/ioutil" "net/http" - "github.com/matrix-org/dendrite/clientapi/httputil" "github.com/matrix-org/dendrite/clientapi/jsonerror" "github.com/matrix-org/dendrite/syncapi/storage" + "github.com/matrix-org/dendrite/syncapi/sync" "github.com/matrix-org/dendrite/userapi/api" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" + "github.com/tidwall/gjson" ) // GetFilter implements GET /_matrix/client/r0/user/{userId}/filter/{filterId} @@ -81,8 +84,27 @@ func PutFilter( var filter gomatrixserverlib.Filter - if reqErr := httputil.UnmarshalJSONRequest(req, &filter); reqErr != nil { - return *reqErr + defer req.Body.Close() // nolint:errcheck + body, err := ioutil.ReadAll(req.Body) + if err != nil { + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON("The request body could not be read. " + err.Error()), + } + } + + if err = json.Unmarshal(body, &filter); err != nil { + return util.JSONResponse{ + Code: http.StatusBadRequest, + JSON: jsonerror.BadJSON("The request body could not be decoded into valid JSON. " + err.Error()), + } + } + // the filter `limit` is `int` which defaults to 0 if not set which is not what we want. We want to use the default + // limit if it is unset, which is what this does. + limitRes := gjson.GetBytes(body, "room.timeline.limit") + if !limitRes.Exists() { + util.GetLogger(req.Context()).Infof("missing timeline limit, using default") + filter.Room.Timeline.Limit = sync.DefaultTimelineLimit } // Validate generates a user-friendly error diff --git a/syncapi/storage/postgres/output_room_events_table.go b/syncapi/storage/postgres/output_room_events_table.go index 1d0e6eef4..c7c4dc63b 100644 --- a/syncapi/storage/postgres/output_room_events_table.go +++ b/syncapi/storage/postgres/output_room_events_table.go @@ -317,13 +317,6 @@ func (s *outputRoomEventsStatements) SelectRecentEvents( if err != nil { return nil, false, err } - // we queried for 1 more than the limit, so if we returned one more mark limited=true - limited := false - if len(events) > limit { - limited = true - // re-slice the extra event out - events = events[:len(events)-1] - } if chronologicalOrder { // The events need to be returned from oldest to latest, which isn't // necessary the way the SQL query returns them, so a sort is necessary to @@ -332,6 +325,17 @@ func (s *outputRoomEventsStatements) SelectRecentEvents( return events[i].StreamPosition < events[j].StreamPosition }) } + // we queried for 1 more than the limit, so if we returned one more mark limited=true + limited := false + if len(events) > limit { + limited = true + // re-slice the extra (oldest) event out: in chronological order this is the first entry, else the last. + if chronologicalOrder { + events = events[1:] + } else { + events = events[:len(events)-1] + } + } return events, limited, nil } diff --git a/syncapi/storage/sqlite3/output_room_events_table.go b/syncapi/storage/sqlite3/output_room_events_table.go index 2dbf8d6c1..0c909cc4d 100644 --- a/syncapi/storage/sqlite3/output_room_events_table.go +++ b/syncapi/storage/sqlite3/output_room_events_table.go @@ -328,13 +328,6 @@ func (s *outputRoomEventsStatements) SelectRecentEvents( if err != nil { return nil, false, err } - // we queried for 1 more than the limit, so if we returned one more mark limited=true - limited := false - if len(events) > limit { - limited = true - // re-slice the extra event out - events = events[:len(events)-1] - } if chronologicalOrder { // The events need to be returned from oldest to latest, which isn't // necessary the way the SQL query returns them, so a sort is necessary to @@ -343,6 +336,17 @@ func (s *outputRoomEventsStatements) SelectRecentEvents( return events[i].StreamPosition < events[j].StreamPosition }) } + // we queried for 1 more than the limit, so if we returned one more mark limited=true + limited := false + if len(events) > limit { + limited = true + // re-slice the extra (oldest) event out: in chronological order this is the first entry, else the last. + if chronologicalOrder { + events = events[1:] + } else { + events = events[:len(events)-1] + } + } return events, limited, nil } diff --git a/syncapi/sync/notifier_test.go b/syncapi/sync/notifier_test.go index ecc4fcbfc..f2a368ec2 100644 --- a/syncapi/sync/notifier_test.go +++ b/syncapi/sync/notifier_test.go @@ -363,7 +363,7 @@ func newTestSyncRequest(userID, deviceID string, since types.StreamingToken) syn timeout: 1 * time.Minute, since: &since, wantFullState: false, - limit: defaultTimelineLimit, + limit: DefaultTimelineLimit, log: util.GetLogger(context.TODO()), ctx: context.TODO(), } diff --git a/syncapi/sync/request.go b/syncapi/sync/request.go index 99633de66..41b18aa10 100644 --- a/syncapi/sync/request.go +++ b/syncapi/sync/request.go @@ -30,7 +30,7 @@ import ( ) const defaultSyncTimeout = time.Duration(0) -const defaultTimelineLimit = 20 +const DefaultTimelineLimit = 20 type filter struct { Room struct { @@ -68,7 +68,7 @@ func newSyncRequest(req *http.Request, device userapi.Device, syncDB storage.Dat tok := types.NewStreamToken(0, 0) since = &tok } - timelineLimit := defaultTimelineLimit + timelineLimit := DefaultTimelineLimit // TODO: read from stored filters too filterQuery := req.URL.Query().Get("filter") if filterQuery != "" {