Add timestamp to room responses
#247
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kegsay
requested changes
Aug 11, 2023
sync3/handler/connstate.go
Outdated
| @@ -624,6 +624,7 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu | |||
| JoinedCount: metadata.JoinCount, | |||
| InvitedCount: &metadata.InviteCount, | |||
| PrevBatch: userRoomData.RequestedLatestEvents.PrevBatch, | |||
| Timestamp: metadata.LastMessageTimestamp, | |||
There was a problem hiding this comment.
You don't know if this is what they want. getInitialRoomData is a central point called for multiple lists: you need to know the bump_event_types values to know what timestamp to pull here. Add that as a param to getInitialRoomData
sync3/handler/connstate_live.go
Outdated
| @@ -187,6 +187,14 @@ func (s *connStateLive) processLiveUpdate(ctx context.Context, up caches.Update, | |||
| // include this update in the rooms response TODO: filters on event type? | |||
| userRoomData := roomUpdate.UserRoomMetadata() | |||
| r := response.Rooms[roomUpdate.RoomID()] | |||
|
|
|||
| conMeta := s.lists.ReadOnlyRoom(roomUpdate.RoomID()) | |||
| for _, ts := range conMeta.LastInterestedEventTimestamps { | |||
There was a problem hiding this comment.
Needs comments. This is using the highest timestamp from each list for this room I guess?
tests-e2e/timestamp_test.go
Outdated
|
|
||
| // Bob should see a different timestamp than alice, as he just joined | ||
| bobTimestampBefore := resBob.Rooms[roomID].Timestamp | ||
| if bobTimestampBefore == timestampBeforeBobJoined { |
There was a problem hiding this comment.
Really this should equal res.Rooms[roomID].Timestamp as that is going to be bob's join due to :30.
tests-e2e/timestamp_test.go
Outdated
|
|
||
| resBob = bob.SlidingSyncUntilEventID(t, resBob.Pos, roomID, eventID) | ||
| bobTimestampAfter = resBob.Rooms[roomID].Timestamp | ||
| if bobTimestampBefore == bobTimestampAfter { |
There was a problem hiding this comment.
Again, we need to know exactly what timestamp this should be - use alice to figure this out.
| if bobTimestampBefore != bobTimestampAfter { | ||
| t.Fatalf("expected timestamps to be the same, but they aren't: %v vs %v", bobTimestampBefore, bobTimestampAfter) | ||
| } | ||
| } |
There was a problem hiding this comment.
Needs more tests. Specifically:
- If you now do an initial sync (no pos) as Bob with the same lists, I don't think you'll get the right value. You'll get the latest timestamp (m.room.name) and not the m.reaction timestamp.
- Ensure you
time.Sleep(time.Millisecond)after each send to ensure that timestamps will change. - If Charlie now joins the room with the same bump_event_types as bob, he should see the timestamp of his join.
kegsay
approved these changes
Aug 15, 2023
Hywan
added a commit
to Hywan/ruma
that referenced
this pull request
Aug 16, 2023
This has been added in matrix-org/sliding-sync#247. This is not part of the MSC yet.
Hywan
added a commit
to Hywan/ruma
that referenced
this pull request
Aug 17, 2023
This has been added in matrix-org/sliding-sync#247. This is not part of the MSC yet.
jplatte
pushed a commit
to ruma/ruma
that referenced
this pull request
Aug 17, 2023
This has been added in matrix-org/sliding-sync#247. This is not part of the MSC yet.
Merged
3 tasks
MadLittleMods
added a commit
to element-hq/synapse
that referenced
this pull request
Jul 8, 2024
… sorting (#17395) `bump_stamp` corresponds to the `stream_ordering` of the latest `DEFAULT_BUMP_EVENT_TYPES` in the room. This helps clients sort more readily without them needing to pull in a bunch of the timeline to determine the last activity. `bump_event_types` is a thing because for example, we don't want display name changes to mark the room as unread and bump it to the top. For encrypted rooms, we just have to consider any activity as a bump because we can't see the content and the client has to figure it out for themselves. Outside of Synapse, `bump_stamp` is just a free-form counter so other implementations could use `received_ts`or `origin_server_ts` (see the [*Security considerations* section in MSC3575 about the potential pitfalls of using `origin_server_ts`](https://github.com/matrix-org/matrix-spec-proposals/blob/kegan/sync-v3/proposals/3575-sync.md#security-considerations)). It doesn't have any guarantee about always going up. In the Synapse case, it could go down if an event was redacted/removed (or purged in cases of retention policies). In the future, we could add `bump_event_types` as [MSC3575](matrix-org/matrix-spec-proposals#3575) mentions if people need to customize the event types. --- In the Sliding Sync proxy, a similar [`timestamp` field was added](matrix-org/sliding-sync#247) for the same purpose but the name is not obvious what it pertains to or what it's for. The `timestamp` field was also added to Ruma in ruma/ruma#1622
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Attempt for #245