Implement filtering for /roomId/messages #1809
Conversation
Signed-off-by: Joakim Hulthe <[email protected]>
|
Not sure why SyTest is failing in the CI. It seems to work on my machine 😕 |
There was a problem hiding this comment.
We need to implement the topological filtering bit.
limit can be set both as a query parameter, and as a part of RoomEventFilter.
The spec doesn't seem to specify which takes precedence if both are specified.
For now I've assumed that the query parameter takes precedence.
Your assumption is the same as mine: limit takses precedence.
I would go for a 20 limit, not 10.
Unlike /sync, the spec doesn't seem to allow for the filter payload to be a filter_id
instead of a JSON RoomEventFilter. Is this intended, or a spec oversight?
Almost certainly a spec oversight. I didn't even know ?filter= was A Thing on /messages!
| ) | ||
| } else { | ||
| streamEvents, err = r.db.GetEventsInTopologicalRange( | ||
| r.ctx, r.from, r.to, r.roomID, r.limit, r.backwardOrdering, | ||
| r.ctx, r.from, r.to, r.roomID, r.filter.Limit, r.backwardOrdering, |
There was a problem hiding this comment.
We need the filter object passed through to GetEventsInTopologicalRange and for it to be implemented there unfortunately!
kegsay
added
the
stale
|
Stale. Please re-open when this has been brough up-to-date and the outstanding issues resolved. Thanks! |
Make
GET /_matrix/client/r0/rooms/{roomId}/messagesaccept afilterparameter inaccordance with the spec (hopefully).
Related issue: #587
This is my first PR to a matrix-project, so there are a couple of thing I'm unsure about:
Database.GetEventsInTopologicalRangedoesn't support filters, so if the client specifies atopological
from-token filtering wont work. Not sure if this is intended.limitcan be set both as a query parameter, and as a part of RoomEventFilter.The spec doesn't seem to specify which takes precedence if both are specified.
For now I've assumed that the query parameter takes precedence.
Curiously, the default
limitvalue ingomatrixserverlib.RoomEventFilteris 20. I'm not sureif this is something I should adhere to, or event if it conforms to the spec, since the default for
the query parameter is 10.
Unlike
/sync, the spec doesn't seem to allow for the filter payload to be afilter_idinstead of a JSON RoomEventFilter. Is this intended, or a spec oversight?
SyTest
Sadly, no new tests are passing (there doesn't seem to be many tests which handle filtering /messages),
although a couple of tests seem to be closer to passing than before:
Ephemeral messages received from clients are correctly expiredEphemeral messages received from servers are correctly expiredThese now fail for other reasons than filtering not working :)
Pull Request Checklist
sytest-whitelistas specified in docs/sytest.mdSigned-off-by:
Joakim Hulthe <[email protected]>