From 0b3c86f483a3616b9bc4b70897d83884982ca229 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 11:52:50 +0100 Subject: [PATCH 01/30] Define Room.Avatar --- sync3/room.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sync3/room.go b/sync3/room.go index 6bf7c35d..40738da3 100644 --- a/sync3/room.go +++ b/sync3/room.go @@ -9,7 +9,10 @@ import ( ) type Room struct { - Name string `json:"name,omitempty"` + Name string `json:"name,omitempty"` + // Avatar is set to the MXC url of the room if it has changed, or to "none" if + // it has been removed. An empty string represents no change to the avatar. + Avatar string `json:"avatar,omitempty"` RequiredState []json.RawMessage `json:"required_state,omitempty"` Timeline []json.RawMessage `json:"timeline,omitempty"` InviteState []json.RawMessage `json:"invite_state,omitempty"` From 569c44177122e9b4bfe76ed7e77be5571b072be5 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 13:55:10 +0100 Subject: [PATCH 02/30] WIP test cases --- tests-e2e/client_test.go | 9 ++ tests-e2e/lists_test.go | 272 +++++++++++++++++++++++++++++++++++++++ testutils/m/match.go | 26 ++++ 3 files changed, 307 insertions(+) diff --git a/tests-e2e/client_test.go b/tests-e2e/client_test.go index 3ef9c817..de713ba9 100644 --- a/tests-e2e/client_test.go +++ b/tests-e2e/client_test.go @@ -159,6 +159,15 @@ func (c *CSAPI) UploadContent(t *testing.T, fileBody []byte, fileName string, co return GetJSONFieldStr(t, body, "content_uri") } +func (c *CSAPI) SetAvatar(t *testing.T, avatarURL string) { + t.Helper() + reqBody := map[string]interface{}{} + if avatarURL != "" { + reqBody["avatar_url"] = avatarURL + } + c.MustDoFunc(t, "PUT", []string{"_matrix", "client", "v3", "profile", c.UserID, "avatar_url"}, WithJSONBody(t, reqBody)) +} + // DownloadContent downloads media from the server, returning the raw bytes and the Content-Type. Fails the test on error. func (c *CSAPI) DownloadContent(t *testing.T, mxcUri string) ([]byte, string) { t.Helper() diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 0d60b783..a77e8130 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1297,3 +1297,275 @@ func TestRangeOutsideTotalRooms(t *testing.T) { ), ) } + +// Nicked from Synapse's tests, see +// https://github.com/matrix-org/synapse/blob/2cacd0849a02d43f88b6c15ee862398159ab827c/tests/test_utils/__init__.py#L154-L161 +// Resolution: 1×1, MIME type: image/png, Extension: png, Size: 67 B +var smallPNG = []byte( + "\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\nIDATx\x9cc\x00\x01\x00\x00\x05\x00\x01\r\n-\xb4\x00\x00\x00\x00IEND\xaeB`\x82", +) + +func TestAvatarFieldInRoomResponse(t *testing.T) { + alice := registerNewUser(t) + bob := registerNewUser(t) + chris := registerNewUser(t) + + t.Log("Alice, Bob and Chris upload an avatar.") + aliceAvatar := alice.UploadContent(t, smallPNG, "alice.png", "image/png") + bobAvatar := bob.UploadContent(t, smallPNG, "bob.png", "image/png") + chrisAvatar := chris.UploadContent(t, smallPNG, "chris.png", "image/png") + + alice.SetAvatar(t, aliceAvatar) + bob.SetAvatar(t, bobAvatar) + chris.SetAvatar(t, chrisAvatar) + + t.Log("Alice makes a public room, a DM with herself, a DM with Bob, a DM with Chris, and a group-DM with Bob and Chris.") + public := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) + dmAlice := alice.CreateRoom(t, map[string]interface{}{ + "preset": "trusted_private_chat", + "is_direct": true, + }) + dmBob := alice.CreateRoom(t, map[string]interface{}{ + "preset": "trusted_private_chat", + "is_direct": true, + "invite": []string{bob.UserID}, + }) + dmChris := alice.CreateRoom(t, map[string]interface{}{ + "preset": "trusted_private_chat", + "is_direct": true, + "invite": []string{chris.UserID}, + }) + dmBobChris := alice.CreateRoom(t, map[string]interface{}{ + "preset": "trusted_private_chat", + "is_direct": true, + "invite": []string{bob.UserID, chris.UserID}, + }) + + t.Logf("Rooms: public=%s dmAlice=%s dmBob=%s dmChris=%s dmBobChris=%s", public, dmAlice, dmBob, dmChris, dmBobChris) + t.Log("Bob accepts his invites. Chris accepts none.") + bob.JoinRoom(t, dmBob, nil) + bob.JoinRoom(t, dmBobChris, nil) + + var res *sync3.Response + t.Run("Initial avatars", func(t *testing.T) { + t.Log("Alice makes an initial sliding sync.") + res = alice.SlidingSync(t, sync3.Request{ + Lists: map[string]sync3.RequestList{ + "rooms": { + Ranges: sync3.SliceRanges{{0, 4}}, + }, + }, + }) + + t.Log("Alice should see each room in the sync response with an appropriate avatar") + m.MatchResponse( + t, + res, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar("none")}, + dmAlice: {m.MatchRoomAvatar(aliceAvatar)}, + dmBob: {m.MatchRoomAvatar(bobAvatar)}, + dmChris: {m.MatchRoomAvatar(chrisAvatar)}, + dmBobChris: {m.MatchRoomAvatar("none")}, + }), + ) + }) + + t.Run("DM declined", func(t *testing.T) { + t.Log("Chris leaves his DM with Alice.") + chris.LeaveRoom(t, dmChris) + + t.Log("Alice syncs until she sees Chris's leave.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "leave") + + t.Log("Alice sees no change in the room's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(""))) + }) + + t.Run("Group DM declined", func(t *testing.T) { + t.Log("Chris leaves his group DM with Alice and Bob.") + chris.LeaveRoom(t, dmBobChris) + + t.Log("Alice syncs until she sees Chris's leave.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmBobChris, chris, "leave") + + t.Log("Alice sees the room's avatar change to Bob's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(bobAvatar))) + }) + + t.Run("Alice's avatar change propagates", func(t *testing.T) { + t.Log("Alice changes her avatar.") + aliceAvatar2 := alice.UploadContent(t, smallPNG, "alice2.png", "image/png") + if aliceAvatar == aliceAvatar2 { + t.Fatalf("Alice's new avatar had the same MXC URL %s", aliceAvatar) + } + alice.SetAvatar(t, aliceAvatar2) + + t.Log("Alice syncs until she sees her new avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + dmAlice: {m.MatchRoomAvatar(aliceAvatar2)}, + }), + ) + + t.Log("Alice removes her avatar.") + alice.SetAvatar(t, "") + + t.Log("Alice syncs until she sees her avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + dmAlice: {m.MatchRoomAvatar("none")}, + }), + ) + }) + + t.Run("Bob's avatar change propagates", func(t *testing.T) { + t.Log("Bob changes his avatar.") + bobAvatar2 := bob.UploadContent(t, smallPNG, "bob2.png", "image/png") + if bobAvatar == bobAvatar2 { + t.Fatalf("Bob's new avatar had the same MXC URL %s", bobAvatar) + } + bob.SetAvatar(t, bobAvatar2) + + t.Log("Alice syncs until she sees Bob's new avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + dmBob: {m.MatchRoomAvatar(bobAvatar2)}, + dmBobChris: {m.MatchRoomAvatar(bobAvatar2)}, + }), + ) + + t.Log("Bob removes his avatar.") + bob.SetAvatar(t, "") + + t.Log("Alice syncs until she sees Bob's avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + dmBob: {m.MatchRoomAvatar("none")}, + dmBobChris: {m.MatchRoomAvatar("none")}, + }), + ) + }) + + t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { + t.Log("Alice sets an avatar for the public room.") + publicAvatar := alice.UploadContent(t, smallPNG, "public.png", "image/png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar, + }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar)}, + }), + ) + + t.Log("Alice changes the avatar for the public room.") + publicAvatar2 := alice.UploadContent(t, smallPNG, "public2.png", "image/png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar2, + }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar2)}, + }), + ) + + t.Log("Alice removes the avatar for the public room.") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{}) + t.Log("Alice syncs until she sees that avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar("none")}, + }), + ) + }) + + t.Run("Explicit avatar propagates in DM room", func(t *testing.T) { + t.Log("Alice re-invites Chris to their DM. He accepts.") + alice.InviteRoom(t, dmChris, chris.UserID) + chris.JoinRoom(t, dmChris, nil) + + t.Log("Alice syncs until she sees Chris's join.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") + + t.Log("Alice should see the DM with Chris's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) + + t.Log("Chris gives their DM a bespoke avatar.") + dmAvatar := chris.UploadContent(t, smallPNG, "dm.png", "image/png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar, + }) + + t.Log("Alice syncs until she sees that avatar.") + alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) + + t.Log("Chris changes his global avatar.") + chrisAvatar2 := chris.UploadContent(t, smallPNG, "chris2.png", "image/png") + chris.SetAvatar(t, chrisAvatar2) + if chrisAvatar == chrisAvatar2 { + t.Fatalf("Chris's new avatar had the same MXC URL %s", chrisAvatar) + } + chris.SetAvatar(t, chrisAvatar2) + + t.Log("Chris sends a sentinel message.") + sentinel := chris.SendEventSynced(t, dmBobChris, Event{ + Type: "m.room.message", + Content: map[string]interface{}{ + "body": "Hello world", + "msgtype": "m.text", + }, + }) + + t.Log("Alice syncs until she sees the sentinel. She should not see the DM avatar change.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error { + matchNoAvatarChange := m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar("")) + if err := matchNoAvatarChange(response); err != nil { + t.Errorf("Saw DM avatar change: %s", err) + } + matchSentinel := m.MatchRoomSubscription(dmChris, m.MatchRoomTimelineEndsWithID(sentinel)) + return matchSentinel(response) + }) + + t.Log("Chris updates the DM's avatar.") + dmAvatar2 := chris.UploadContent(t, smallPNG, "dm2.png", "image/png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar2, + }) + + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar2))) + + t.Log("Chris removes the DM's avatar.") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) + + t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) + }) + + // TODO unset DM's avatar, then set a custom one in that room. + // TODO when you're invited, you see the avatar +} diff --git a/testutils/m/match.go b/testutils/m/match.go index a7b2f635..f485f415 100644 --- a/testutils/m/match.go +++ b/testutils/m/match.go @@ -39,6 +39,18 @@ func MatchRoomName(name string) RoomMatcher { } } +func MatchRoomAvatar(avatar string) RoomMatcher { + return func(r sync3.Room) error { + if avatar == "" { + return nil + } + if r.Avatar != avatar { + return fmt.Errorf("avatar mismatch, got %s want %s", r.Avatar, avatar) + } + return nil + } +} + func MatchJoinCount(count int) RoomMatcher { return func(r sync3.Room) error { if r.JoinedCount != count { @@ -139,6 +151,20 @@ func MatchRoomTimelineMostRecent(n int, events []json.RawMessage) RoomMatcher { } } +func MatchRoomTimelineEndsWithID(wantID string) RoomMatcher { + return func(r sync3.Room) error { + if len(r.Timeline) == 0 { + return fmt.Errorf("MatchRoomTimelineMostRecent: empty timeline") + } + event := gjson.ParseBytes(r.Timeline[len(r.Timeline)-1]) + gotID := event.Get("event_id").Str + if gotID != wantID { + return fmt.Errorf("MatchRoomTimelineEndsWithID: got %s, want %s", gotID, wantID) + } + return nil + } +} + func MatchRoomPrevBatch(prevBatch string) RoomMatcher { return func(r sync3.Room) error { if prevBatch != r.PrevBatch { From 4edec892f35b98cbc117e306a33b1f35e80530ce Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 14:50:42 +0100 Subject: [PATCH 03/30] Global cache: track m.room.avatar --- internal/roomname.go | 1 + state/storage.go | 4 +++- sync3/caches/global.go | 4 ++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/roomname.go b/internal/roomname.go index 73eb0205..c04e5f32 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -25,6 +25,7 @@ type RoomMetadata struct { RoomID string Heroes []Hero NameEvent string // the content of m.room.name, NOT the calculated name + AvatarURL string // the content of m.room.avatar, NOT the resolved avatar CanonicalAlias string JoinCount int InviteCount int diff --git a/state/storage.go b/state/storage.go index b51026cc..8f84ce2f 100644 --- a/state/storage.go +++ b/state/storage.go @@ -232,7 +232,7 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result // Select the name / canonical alias for all rooms roomIDToStateEvents, err := s.currentNotMembershipStateEventsInAllRooms(txn, []string{ - "m.room.name", "m.room.canonical_alias", + "m.room.name", "m.room.canonical_alias", "m.room.avatar", }) if err != nil { return fmt.Errorf("failed to load state events for all rooms: %s", err) @@ -244,6 +244,8 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result metadata.NameEvent = gjson.ParseBytes(ev.JSON).Get("content.name").Str } else if ev.Type == "m.room.canonical_alias" && ev.StateKey == "" { metadata.CanonicalAlias = gjson.ParseBytes(ev.JSON).Get("content.alias").Str + } else if ev.Type == "m.room.avatar" && ev.StateKey == "" { + metadata.AvatarURL = gjson.ParseBytes(ev.JSON).Get("content.avatar_url").Str } } result[roomID] = metadata diff --git a/sync3/caches/global.go b/sync3/caches/global.go index 36c2867a..cea1e5d9 100644 --- a/sync3/caches/global.go +++ b/sync3/caches/global.go @@ -285,6 +285,10 @@ func (c *GlobalCache) OnNewEvent( if ed.StateKey != nil && *ed.StateKey == "" { metadata.NameEvent = ed.Content.Get("name").Str } + case "m.room.avatar": + if ed.StateKey != nil && *ed.StateKey == "" { + metadata.AvatarURL = ed.Content.Get("avatar_url").Str + } case "m.room.encryption": if ed.StateKey != nil && *ed.StateKey == "" { metadata.Encrypted = true From 5c717c061b477ed2468e070fee7dcc54631d708a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 14:52:37 +0100 Subject: [PATCH 04/30] RoomDelta: track when m.room.avatar changes --- internal/roomname.go | 6 ++++++ sync3/lists.go | 2 ++ 2 files changed, 8 insertions(+) diff --git a/internal/roomname.go b/internal/roomname.go index c04e5f32..cd0b282f 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -66,6 +66,12 @@ func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool { sameHeroes(m.Heroes, other.Heroes)) } +// SameRoomAvatar checks if the fields relevant for room names have changed between the two metadatas. +// Returns true if there are no changes. +func (m *RoomMetadata) SameRoomAvatar(other *RoomMetadata) bool { + return m.AvatarURL == other.AvatarURL +} + func (m *RoomMetadata) SameJoinCount(other *RoomMetadata) bool { return m.JoinCount == other.JoinCount } diff --git a/sync3/lists.go b/sync3/lists.go index f5e1dfbf..587bc1c9 100644 --- a/sync3/lists.go +++ b/sync3/lists.go @@ -33,6 +33,7 @@ type RoomListDelta struct { type RoomDelta struct { RoomNameChanged bool + RoomAvatarChanged bool JoinCountChanged bool InviteCountChanged bool NotificationCountChanged bool @@ -73,6 +74,7 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"), ) } + delta.RoomAvatarChanged = !existing.SameRoomName(&r.RoomMetadata) // Interpret the timestamp map on r as the changes we should apply atop the // existing timestamps. From acf76b0983471afd6b31541d6b03180d1bc6cbc9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 15:20:51 +0100 Subject: [PATCH 05/30] Heroes: track avatar URLs --- internal/roomname.go | 5 +++-- state/storage.go | 5 +++-- sync3/caches/global.go | 5 +++-- sync3/caches/user.go | 1 + 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/internal/roomname.go b/internal/roomname.go index cd0b282f..8eaef4e9 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -109,8 +109,9 @@ func (m *RoomMetadata) IsSpace() bool { } type Hero struct { - ID string - Name string + ID string + Name string + Avatar string } func CalculateRoomName(heroInfo *RoomMetadata, maxNumNamesPerRoom int) string { diff --git a/state/storage.go b/state/storage.go index 8f84ce2f..a54fe748 100644 --- a/state/storage.go +++ b/state/storage.go @@ -284,8 +284,9 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result seen[key] = true metadata := result[roomID] metadata.Heroes = append(metadata.Heroes, internal.Hero{ - ID: targetUser, - Name: ev.Get("content.displayname").Str, + ID: targetUser, + Name: ev.Get("content.displayname").Str, + Avatar: ev.Get("content.avatar_url").Str, }) result[roomID] = metadata } diff --git a/sync3/caches/global.go b/sync3/caches/global.go index cea1e5d9..a2873078 100644 --- a/sync3/caches/global.go +++ b/sync3/caches/global.go @@ -359,8 +359,9 @@ func (c *GlobalCache) OnNewEvent( } if !found { metadata.Heroes = append(metadata.Heroes, internal.Hero{ - ID: *ed.StateKey, - Name: ed.Content.Get("displayname").Str, + ID: *ed.StateKey, + Name: ed.Content.Get("displayname").Str, + Avatar: ed.Content.Get("avatar_url").Str, }) } } diff --git a/sync3/caches/user.go b/sync3/caches/user.go index ef160f8b..0a498de7 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -110,6 +110,7 @@ func NewInviteData(ctx context.Context, userID, roomID string, inviteState []jso id.Heroes = append(id.Heroes, internal.Hero{ ID: target, Name: j.Get("content.displayname").Str, + Avatar: j.Get("content.avatar_url").Str, }) } case "m.room.name": From 9583663775f3848a08e9fcb0a0a1a8a0382bdef9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 15:21:51 +0100 Subject: [PATCH 06/30] Invites: extract room avatar_url --- sync3/caches/user.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sync3/caches/user.go b/sync3/caches/user.go index 0a498de7..03488620 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -73,6 +73,7 @@ type InviteData struct { Heroes []internal.Hero InviteEvent *EventData NameEvent string // the content of m.room.name, NOT the calculated name + AvatarURL string // the content of m.room.avatar, NOT the calculated avatar CanonicalAlias string LastMessageTimestamp uint64 Encrypted bool @@ -108,13 +109,15 @@ func NewInviteData(ctx context.Context, userID, roomID string, inviteState []jso id.IsDM = j.Get("is_direct").Bool() } else if target == j.Get("sender").Str { id.Heroes = append(id.Heroes, internal.Hero{ - ID: target, - Name: j.Get("content.displayname").Str, + ID: target, + Name: j.Get("content.displayname").Str, Avatar: j.Get("content.avatar_url").Str, }) } case "m.room.name": id.NameEvent = j.Get("content.name").Str + case "m.room.avatar": + id.AvatarURL = j.Get("content.avatar_url").Str case "m.room.canonical_alias": id.CanonicalAlias = j.Get("content.alias").Str case "m.room.encryption": @@ -148,6 +151,7 @@ func (i *InviteData) RoomMetadata() *internal.RoomMetadata { metadata := internal.NewRoomMetadata(i.roomID) metadata.Heroes = i.Heroes metadata.NameEvent = i.NameEvent + metadata.AvatarURL = i.AvatarURL metadata.CanonicalAlias = i.CanonicalAlias metadata.InviteCount = 1 metadata.JoinCount = 1 From 3e73fc8db4cb8cf7ec5369e50e1a239c068e67ae Mon Sep 17 00:00:00 2001 From: David Robertson Date: Thu, 13 Jul 2023 19:03:16 +0100 Subject: [PATCH 07/30] WIP UserRoomData has a ResolvedAvatarURL --- sync3/caches/user.go | 11 +++++++++++ sync3/lists.go | 9 +++++++++ 2 files changed, 20 insertions(+) diff --git a/sync3/caches/user.go b/sync3/caches/user.go index 03488620..ae56a2fa 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -50,6 +50,13 @@ type UserRoomData struct { CanonicalisedName string // stripped leading symbols like #, all in lower case // Set of spaces this room is a part of, from the perspective of this user. This is NOT global room data // as the set of spaces may be different for different users. + + // ResolvedAvatarURL is the avatar that should be displayed to this user to + // represent this room. Avatars set in m.room.avatar take precedence; if this is + // missing and the room is a DM with one other user, we fall back to that user's + // avatar (if any) as specified in their membership event in that room. + ResolvedAvatarURL string + Spaces map[string]struct{} // Map of tag to order float. // See https://spec.matrix.org/latest/client-server-api/#room-tagging @@ -577,6 +584,10 @@ func (c *UserCache) OnInvite(ctx context.Context, roomID string, inviteStateEven urd.HasLeft = false urd.HighlightCount = InvitesAreHighlightsValue urd.IsDM = inviteData.IsDM + urd.ResolvedAvatarURL = inviteData.AvatarURL + if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { + urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar + } urd.Invite = inviteData c.roomToDataMu.Lock() c.roomToData[roomID] = urd diff --git a/sync3/lists.go b/sync3/lists.go index 587bc1c9..9fcdf66c 100644 --- a/sync3/lists.go +++ b/sync3/lists.go @@ -75,6 +75,10 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { ) } delta.RoomAvatarChanged = !existing.SameRoomName(&r.RoomMetadata) + if delta.RoomAvatarChanged { + logger.Warn().Msg("m.room.avatar change, implementme") + r.ResolvedAvatarURL = "" // TODO + } // Interpret the timestamp map on r as the changes we should apply atop the // existing timestamps. @@ -99,6 +103,11 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { r.CanonicalisedName = strings.ToLower( strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"), ) + if r.AvatarURL != "" { + r.ResolvedAvatarURL = r.AvatarURL + } else if r.IsDM && len(r.Heroes) == 1 { + r.ResolvedAvatarURL = r.Heroes[0].Avatar + } // We'll automatically use the LastInterestedEventTimestamps provided by the // caller, so that recency sorts work. } From 7ace2d9810bfbbf65e90132bad776f2959881bad Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 11:51:04 +0100 Subject: [PATCH 08/30] Improve tests --- tests-e2e/lists_test.go | 44 ++++++++++++++++++++--------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index a77e8130..ddd35a28 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1310,10 +1310,20 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { bob := registerNewUser(t) chris := registerNewUser(t) - t.Log("Alice, Bob and Chris upload an avatar.") - aliceAvatar := alice.UploadContent(t, smallPNG, "alice.png", "image/png") - bobAvatar := bob.UploadContent(t, smallPNG, "bob.png", "image/png") - chrisAvatar := chris.UploadContent(t, smallPNG, "chris.png", "image/png") + avatarURLs := map[string]struct{}{} + uploadAvatar := func(client *CSAPI, filename string) string { + avatar := alice.UploadContent(t, smallPNG, "alice.png", "image/png") + if _, exists := avatarURLs[avatar]; exists { + t.Fatalf("New avatar %s has already been uploaded", avatar) + } + avatarURLs[avatar] = struct{}{} + return avatar + } + + t.Log("Alice, Bob and Chris upload and set an avatar.") + aliceAvatar := uploadAvatar(alice, "alice.png") + bobAvatar := uploadAvatar(bob, "bob.png") + chrisAvatar := uploadAvatar(chris, "chris.png") alice.SetAvatar(t, aliceAvatar) bob.SetAvatar(t, bobAvatar) @@ -1341,7 +1351,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { "invite": []string{bob.UserID, chris.UserID}, }) - t.Logf("Rooms: public=%s dmAlice=%s dmBob=%s dmChris=%s dmBobChris=%s", public, dmAlice, dmBob, dmChris, dmBobChris) + t.Logf("Rooms:\npublic=%s\ndmAlice=%s\ndmBob=%s\ndmChris=%s\ndmBobChris=%s", public, dmAlice, dmBob, dmChris, dmBobChris) t.Log("Bob accepts his invites. Chris accepts none.") bob.JoinRoom(t, dmBob, nil) bob.JoinRoom(t, dmBobChris, nil) @@ -1395,10 +1405,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Run("Alice's avatar change propagates", func(t *testing.T) { t.Log("Alice changes her avatar.") - aliceAvatar2 := alice.UploadContent(t, smallPNG, "alice2.png", "image/png") - if aliceAvatar == aliceAvatar2 { - t.Fatalf("Alice's new avatar had the same MXC URL %s", aliceAvatar) - } + aliceAvatar2 := uploadAvatar(alice, "alice2.png") alice.SetAvatar(t, aliceAvatar2) t.Log("Alice syncs until she sees her new avatar.") @@ -1427,10 +1434,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Run("Bob's avatar change propagates", func(t *testing.T) { t.Log("Bob changes his avatar.") - bobAvatar2 := bob.UploadContent(t, smallPNG, "bob2.png", "image/png") - if bobAvatar == bobAvatar2 { - t.Fatalf("Bob's new avatar had the same MXC URL %s", bobAvatar) - } + bobAvatar2 := uploadAvatar(bob, "bob2.png") bob.SetAvatar(t, bobAvatar2) t.Log("Alice syncs until she sees Bob's new avatar.") @@ -1461,7 +1465,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { t.Log("Alice sets an avatar for the public room.") - publicAvatar := alice.UploadContent(t, smallPNG, "public.png", "image/png") + publicAvatar := uploadAvatar(alice, "public.png") alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ "url": publicAvatar, }) @@ -1476,7 +1480,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { ) t.Log("Alice changes the avatar for the public room.") - publicAvatar2 := alice.UploadContent(t, smallPNG, "public2.png", "image/png") + publicAvatar2 := uploadAvatar(alice, "public2.png") alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ "url": publicAvatar2, }) @@ -1515,7 +1519,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) t.Log("Chris gives their DM a bespoke avatar.") - dmAvatar := chris.UploadContent(t, smallPNG, "dm.png", "image/png") + dmAvatar := uploadAvatar(chris, "dm.png") chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ "url": dmAvatar, }) @@ -1524,11 +1528,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) t.Log("Chris changes his global avatar.") - chrisAvatar2 := chris.UploadContent(t, smallPNG, "chris2.png", "image/png") - chris.SetAvatar(t, chrisAvatar2) - if chrisAvatar == chrisAvatar2 { - t.Fatalf("Chris's new avatar had the same MXC URL %s", chrisAvatar) - } + chrisAvatar2 := uploadAvatar(chris, "chris2.png") chris.SetAvatar(t, chrisAvatar2) t.Log("Chris sends a sentinel message.") @@ -1551,7 +1551,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { }) t.Log("Chris updates the DM's avatar.") - dmAvatar2 := chris.UploadContent(t, smallPNG, "dm2.png", "image/png") + dmAvatar2 := uploadAvatar(chris, "dm2.png") chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ "url": dmAvatar2, }) From e9ebe8afac1a71a0476d65bf78fb4b89bf9e3cb8 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 12:03:21 +0100 Subject: [PATCH 09/30] Remove redundant test fn --- tests-e2e/lists_test.go | 2 +- testutils/m/match.go | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index ddd35a28..09d91a8d 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1546,7 +1546,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { if err := matchNoAvatarChange(response); err != nil { t.Errorf("Saw DM avatar change: %s", err) } - matchSentinel := m.MatchRoomSubscription(dmChris, m.MatchRoomTimelineEndsWithID(sentinel)) + matchSentinel := m.MatchRoomSubscription(dmChris, MatchRoomTimelineMostRecent(1, []Event{{ID: sentinel}})) return matchSentinel(response) }) diff --git a/testutils/m/match.go b/testutils/m/match.go index f485f415..c8d200e5 100644 --- a/testutils/m/match.go +++ b/testutils/m/match.go @@ -151,20 +151,6 @@ func MatchRoomTimelineMostRecent(n int, events []json.RawMessage) RoomMatcher { } } -func MatchRoomTimelineEndsWithID(wantID string) RoomMatcher { - return func(r sync3.Room) error { - if len(r.Timeline) == 0 { - return fmt.Errorf("MatchRoomTimelineMostRecent: empty timeline") - } - event := gjson.ParseBytes(r.Timeline[len(r.Timeline)-1]) - gotID := event.Get("event_id").Str - if gotID != wantID { - return fmt.Errorf("MatchRoomTimelineEndsWithID: got %s, want %s", gotID, wantID) - } - return nil - } -} - func MatchRoomPrevBatch(prevBatch string) RoomMatcher { return func(r sync3.Room) error { if prevBatch != r.PrevBatch { From b2fd2db14d8cda6c579f2ea7c63f1f8b36ead9a3 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 14:22:26 +0100 Subject: [PATCH 10/30] Change avatar serialisation per MSC update --- sync3/room.go | 38 +++++++++++++++++++++++++++++++++----- tests-e2e/lists_test.go | 12 ++++++------ testutils/m/match.go | 20 +++++++++++++++----- 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/sync3/room.go b/sync3/room.go index 40738da3..91606745 100644 --- a/sync3/room.go +++ b/sync3/room.go @@ -1,18 +1,46 @@ package sync3 import ( + "bytes" "encoding/json" - "github.com/matrix-org/sliding-sync/internal" "github.com/matrix-org/sliding-sync/sync3/caches" ) +// sentinel value +const deletedAvatar = "" + +// An AvatarChange represents a change to a room's avatar. There are three cases: +// - an empty string represents no change, and should be omitted when JSON-serisalised; +// - the sentinel `avatarNotPresent` represents a room that has never had an avatar, +// or a room whose avatar has been removed. It is JSON-serialised as null. +// - All other strings represent the current avatar of the room and JSON-serialise as +// normal. +type AvatarChange string + +func (a AvatarChange) MarshalJSON() ([]byte, error) { + if a == deletedAvatar { + return []byte(`null`), nil + } else { + return json.Marshal(string(a)) + } +} + +func (a *AvatarChange) UnmarshalJSON(data []byte) error { + if bytes.Equal(data, []byte("null")) { + *a = deletedAvatar + return nil + } + return json.Unmarshal(data, (*string)(a)) +} + +var DeletedAvatar = AvatarChange(deletedAvatar) +var UnchangedAvatar AvatarChange + type Room struct { - Name string `json:"name,omitempty"` - // Avatar is set to the MXC url of the room if it has changed, or to "none" if - // it has been removed. An empty string represents no change to the avatar. - Avatar string `json:"avatar,omitempty"` + Name string `json:"name,omitempty"` + AvatarChange AvatarChange `json:"avatar,omitempty"` RequiredState []json.RawMessage `json:"required_state,omitempty"` Timeline []json.RawMessage `json:"timeline,omitempty"` InviteState []json.RawMessage `json:"invite_state,omitempty"` diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 09d91a8d..7a799bce 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1372,11 +1372,11 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t, res, m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar("none")}, + public: {m.MatchRoomUnsetAvatar()}, dmAlice: {m.MatchRoomAvatar(aliceAvatar)}, dmBob: {m.MatchRoomAvatar(bobAvatar)}, dmChris: {m.MatchRoomAvatar(chrisAvatar)}, - dmBobChris: {m.MatchRoomAvatar("none")}, + dmBobChris: {m.MatchRoomUnsetAvatar()}, }), ) }) @@ -1427,7 +1427,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res.Pos, sync3.Request{}, m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmAlice: {m.MatchRoomAvatar("none")}, + dmAlice: {m.MatchRoomUnsetAvatar()}, }), ) }) @@ -1457,8 +1457,8 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res.Pos, sync3.Request{}, m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmBob: {m.MatchRoomAvatar("none")}, - dmBobChris: {m.MatchRoomAvatar("none")}, + dmBob: {m.MatchRoomUnsetAvatar()}, + dmBobChris: {m.MatchRoomUnsetAvatar()}, }), ) }) @@ -1502,7 +1502,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res.Pos, sync3.Request{}, m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar("none")}, + public: {m.MatchRoomUnsetAvatar()}, }), ) }) diff --git a/testutils/m/match.go b/testutils/m/match.go index c8d200e5..3e9f7ad9 100644 --- a/testutils/m/match.go +++ b/testutils/m/match.go @@ -39,13 +39,23 @@ func MatchRoomName(name string) RoomMatcher { } } -func MatchRoomAvatar(avatar string) RoomMatcher { +// MatchRoomAvatar builds a RoomMatcher which checks that the given room response has +// set the room's avatar to the given value. +func MatchRoomAvatar(wantAvatar string) RoomMatcher { return func(r sync3.Room) error { - if avatar == "" { - return nil + if string(r.AvatarChange) != wantAvatar { + return fmt.Errorf("MatchRoomAvatar: got %s want %s", r.AvatarChange, wantAvatar) } - if r.Avatar != avatar { - return fmt.Errorf("avatar mismatch, got %s want %s", r.Avatar, avatar) + return nil + } +} + +// MatchRoomUnsetAvatar builds a RoomMatcher which checks that the given room has no +// avatar, or has had its avatar deleted. +func MatchRoomUnsetAvatar() RoomMatcher { + return func(r sync3.Room) error { + if r.AvatarChange != sync3.DeletedAvatar { + return fmt.Errorf("MatchRoomAvatar: got %s want %s", r.AvatarChange, sync3.DeletedAvatar) } return nil } From 2de017e8d1e8d6868cd50269d9e7019478da2d9a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 14:44:17 +0100 Subject: [PATCH 11/30] RoomMetadata.AvatarURL: rename to AvatarEvent, parse correctly --- internal/roomname.go | 4 ++-- state/storage.go | 2 +- sync3/caches/global.go | 2 +- sync3/caches/user.go | 2 +- sync3/lists.go | 4 ++-- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/roomname.go b/internal/roomname.go index 8eaef4e9..12cd6485 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -25,7 +25,7 @@ type RoomMetadata struct { RoomID string Heroes []Hero NameEvent string // the content of m.room.name, NOT the calculated name - AvatarURL string // the content of m.room.avatar, NOT the resolved avatar + AvatarEvent string // the content of m.room.avatar, NOT the resolved avatar CanonicalAlias string JoinCount int InviteCount int @@ -69,7 +69,7 @@ func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool { // SameRoomAvatar checks if the fields relevant for room names have changed between the two metadatas. // Returns true if there are no changes. func (m *RoomMetadata) SameRoomAvatar(other *RoomMetadata) bool { - return m.AvatarURL == other.AvatarURL + return m.AvatarEvent == other.AvatarEvent } func (m *RoomMetadata) SameJoinCount(other *RoomMetadata) bool { diff --git a/state/storage.go b/state/storage.go index a54fe748..5e1e9338 100644 --- a/state/storage.go +++ b/state/storage.go @@ -245,7 +245,7 @@ func (s *Storage) MetadataForAllRooms(txn *sqlx.Tx, tempTableName string, result } else if ev.Type == "m.room.canonical_alias" && ev.StateKey == "" { metadata.CanonicalAlias = gjson.ParseBytes(ev.JSON).Get("content.alias").Str } else if ev.Type == "m.room.avatar" && ev.StateKey == "" { - metadata.AvatarURL = gjson.ParseBytes(ev.JSON).Get("content.avatar_url").Str + metadata.AvatarEvent = gjson.ParseBytes(ev.JSON).Get("content.url").Str } } result[roomID] = metadata diff --git a/sync3/caches/global.go b/sync3/caches/global.go index a2873078..da16691e 100644 --- a/sync3/caches/global.go +++ b/sync3/caches/global.go @@ -287,7 +287,7 @@ func (c *GlobalCache) OnNewEvent( } case "m.room.avatar": if ed.StateKey != nil && *ed.StateKey == "" { - metadata.AvatarURL = ed.Content.Get("avatar_url").Str + metadata.AvatarEvent = ed.Content.Get("url").Str } case "m.room.encryption": if ed.StateKey != nil && *ed.StateKey == "" { diff --git a/sync3/caches/user.go b/sync3/caches/user.go index ae56a2fa..c33e39df 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -158,7 +158,7 @@ func (i *InviteData) RoomMetadata() *internal.RoomMetadata { metadata := internal.NewRoomMetadata(i.roomID) metadata.Heroes = i.Heroes metadata.NameEvent = i.NameEvent - metadata.AvatarURL = i.AvatarURL + metadata.AvatarEvent = i.AvatarURL metadata.CanonicalAlias = i.CanonicalAlias metadata.InviteCount = 1 metadata.JoinCount = 1 diff --git a/sync3/lists.go b/sync3/lists.go index 9fcdf66c..473cf4bf 100644 --- a/sync3/lists.go +++ b/sync3/lists.go @@ -103,8 +103,8 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { r.CanonicalisedName = strings.ToLower( strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"), ) - if r.AvatarURL != "" { - r.ResolvedAvatarURL = r.AvatarURL + if r.AvatarEvent != "" { + r.ResolvedAvatarURL = r.AvatarEvent } else if r.IsDM && len(r.Heroes) == 1 { r.ResolvedAvatarURL = r.Heroes[0].Avatar } From 9ecbb4828b3fcc166bd89abbe4757db51352e63a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 14:57:53 +0100 Subject: [PATCH 12/30] Same rename for InviteData --- sync3/caches/user.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sync3/caches/user.go b/sync3/caches/user.go index c33e39df..3dd9524e 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -80,7 +80,7 @@ type InviteData struct { Heroes []internal.Hero InviteEvent *EventData NameEvent string // the content of m.room.name, NOT the calculated name - AvatarURL string // the content of m.room.avatar, NOT the calculated avatar + AvatarEvent string // the content of m.room.avatar, NOT the calculated avatar CanonicalAlias string LastMessageTimestamp uint64 Encrypted bool @@ -124,7 +124,7 @@ func NewInviteData(ctx context.Context, userID, roomID string, inviteState []jso case "m.room.name": id.NameEvent = j.Get("content.name").Str case "m.room.avatar": - id.AvatarURL = j.Get("content.avatar_url").Str + id.AvatarEvent = j.Get("content.avatar_url").Str case "m.room.canonical_alias": id.CanonicalAlias = j.Get("content.alias").Str case "m.room.encryption": @@ -158,7 +158,7 @@ func (i *InviteData) RoomMetadata() *internal.RoomMetadata { metadata := internal.NewRoomMetadata(i.roomID) metadata.Heroes = i.Heroes metadata.NameEvent = i.NameEvent - metadata.AvatarEvent = i.AvatarURL + metadata.AvatarEvent = i.AvatarEvent metadata.CanonicalAlias = i.CanonicalAlias metadata.InviteCount = 1 metadata.JoinCount = 1 @@ -584,7 +584,7 @@ func (c *UserCache) OnInvite(ctx context.Context, roomID string, inviteStateEven urd.HasLeft = false urd.HighlightCount = InvitesAreHighlightsValue urd.IsDM = inviteData.IsDM - urd.ResolvedAvatarURL = inviteData.AvatarURL + urd.ResolvedAvatarURL = inviteData.AvatarEvent if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar } From e43beb2070ad945416a36c1e4263aa6ee350e001 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 16:37:39 +0100 Subject: [PATCH 13/30] Tweak test --- tests-e2e/lists_test.go | 363 ++++++++++++++++++++-------------------- testutils/m/match.go | 37 +++- 2 files changed, 207 insertions(+), 193 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 7a799bce..3282e0a4 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1306,16 +1306,17 @@ var smallPNG = []byte( ) func TestAvatarFieldInRoomResponse(t *testing.T) { - alice := registerNewUser(t) - bob := registerNewUser(t) - chris := registerNewUser(t) + alice := registerNamedUser(t, "alice") + bob := registerNamedUser(t, "bob") + chris := registerNamedUser(t, "chris") avatarURLs := map[string]struct{}{} uploadAvatar := func(client *CSAPI, filename string) string { - avatar := alice.UploadContent(t, smallPNG, "alice.png", "image/png") + avatar := alice.UploadContent(t, smallPNG, filename, "image/png") if _, exists := avatarURLs[avatar]; exists { t.Fatalf("New avatar %s has already been uploaded", avatar) } + t.Logf("%s is uploaded as %s", filename, avatar) avatarURLs[avatar] = struct{}{} return avatar } @@ -1331,10 +1332,12 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Log("Alice makes a public room, a DM with herself, a DM with Bob, a DM with Chris, and a group-DM with Bob and Chris.") public := alice.CreateRoom(t, map[string]interface{}{"preset": "public_chat"}) - dmAlice := alice.CreateRoom(t, map[string]interface{}{ - "preset": "trusted_private_chat", - "is_direct": true, - }) + // TODO: you can create a DM with yourself e.g. as below. It probably ought to have + // your own face as an avatar. + // dmAlice := alice.CreateRoom(t, map[string]interface{}{ + // "preset": "trusted_private_chat", + // "is_direct": true, + // }) dmBob := alice.CreateRoom(t, map[string]interface{}{ "preset": "trusted_private_chat", "is_direct": true, @@ -1351,36 +1354,30 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { "invite": []string{bob.UserID, chris.UserID}, }) - t.Logf("Rooms:\npublic=%s\ndmAlice=%s\ndmBob=%s\ndmChris=%s\ndmBobChris=%s", public, dmAlice, dmBob, dmChris, dmBobChris) + t.Logf("Rooms:\npublic=%s\ndmBob=%s\ndmChris=%s\ndmBobChris=%s", public, dmBob, dmChris, dmBobChris) t.Log("Bob accepts his invites. Chris accepts none.") bob.JoinRoom(t, dmBob, nil) bob.JoinRoom(t, dmBobChris, nil) - var res *sync3.Response - t.Run("Initial avatars", func(t *testing.T) { - t.Log("Alice makes an initial sliding sync.") - res = alice.SlidingSync(t, sync3.Request{ - Lists: map[string]sync3.RequestList{ - "rooms": { - Ranges: sync3.SliceRanges{{0, 4}}, - }, + t.Log("Alice makes an initial sliding sync.") + res := alice.SlidingSync(t, sync3.Request{ + Lists: map[string]sync3.RequestList{ + "rooms": { + Ranges: sync3.SliceRanges{{0, 4}}, }, - }) - - t.Log("Alice should see each room in the sync response with an appropriate avatar") - m.MatchResponse( - t, - res, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomUnsetAvatar()}, - dmAlice: {m.MatchRoomAvatar(aliceAvatar)}, - dmBob: {m.MatchRoomAvatar(bobAvatar)}, - dmChris: {m.MatchRoomAvatar(chrisAvatar)}, - dmBobChris: {m.MatchRoomUnsetAvatar()}, - }), - ) + }, }) + t.Log("Alice should see each room in the sync response with an appropriate avatar") + m.MatchResponse( + t, + res, + m.MatchRoomSubscription(public, m.MatchRoomUnsetAvatar()), + m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar)), + m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar)), + m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar()), + ) + t.Run("DM declined", func(t *testing.T) { t.Log("Chris leaves his DM with Alice.") chris.LeaveRoom(t, dmChris) @@ -1388,8 +1385,8 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Log("Alice syncs until she sees Chris's leave.") res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "leave") - t.Log("Alice sees no change in the room's avatar.") - m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(""))) + t.Log("Alice sees Chris's avatar vanish.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomUnsetAvatar())) }) t.Run("Group DM declined", func(t *testing.T) { @@ -1400,172 +1397,166 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res = alice.SlidingSyncUntilMembership(t, res.Pos, dmBobChris, chris, "leave") t.Log("Alice sees the room's avatar change to Bob's avatar.") - m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(bobAvatar))) + // Because this is now a DM room with exactly one other (joined|invited) member. + m.MatchResponse(t, res, m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar))) }) - t.Run("Alice's avatar change propagates", func(t *testing.T) { - t.Log("Alice changes her avatar.") - aliceAvatar2 := uploadAvatar(alice, "alice2.png") - alice.SetAvatar(t, aliceAvatar2) - - t.Log("Alice syncs until she sees her new avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmAlice: {m.MatchRoomAvatar(aliceAvatar2)}, - }), - ) - - t.Log("Alice removes her avatar.") - alice.SetAvatar(t, "") - - t.Log("Alice syncs until she sees her avatar vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmAlice: {m.MatchRoomUnsetAvatar()}, - }), - ) - }) - - t.Run("Bob's avatar change propagates", func(t *testing.T) { - t.Log("Bob changes his avatar.") - bobAvatar2 := uploadAvatar(bob, "bob2.png") - bob.SetAvatar(t, bobAvatar2) - - t.Log("Alice syncs until she sees Bob's new avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmBob: {m.MatchRoomAvatar(bobAvatar2)}, - dmBobChris: {m.MatchRoomAvatar(bobAvatar2)}, - }), - ) - - t.Log("Bob removes his avatar.") - bob.SetAvatar(t, "") - - t.Log("Alice syncs until she sees Bob's avatar vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmBob: {m.MatchRoomUnsetAvatar()}, - dmBobChris: {m.MatchRoomUnsetAvatar()}, - }), - ) - }) - - t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { - t.Log("Alice sets an avatar for the public room.") - publicAvatar := uploadAvatar(alice, "public.png") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ - "url": publicAvatar, - }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar(publicAvatar)}, - }), - ) - - t.Log("Alice changes the avatar for the public room.") - publicAvatar2 := uploadAvatar(alice, "public2.png") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ - "url": publicAvatar2, + /* + t.Run("Bob's avatar change propagates", func(t *testing.T) { + t.Log("Bob changes his avatar.") + bobAvatar2 := uploadAvatar(bob, "bob2.png") + bob.SetAvatar(t, bobAvatar2) + + avatarChangeInDM := false + avatarChangeInGroupDM := false + t.Log("Alice syncs until she sees Bob's new avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + func(response *sync3.Response) error { + if !avatarChangeInDM { + err := m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar2))(response) + if err == nil { + avatarChangeInDM = true + } + } + + if !avatarChangeInGroupDM { + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar2))(response) + if err == nil { + avatarChangeInGroupDM = true + } + } + + if avatarChangeInDM && avatarChangeInGroupDM { + return nil + } + m.LogRooms(t)(response) + return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + }, + ) + + t.Log("Bob removes his avatar.") + bob.SetAvatar(t, "") + + t.Log("Alice syncs until she sees Bob's avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + dmBob: {m.MatchRoomUnsetAvatar()}, + dmBobChris: {m.MatchRoomUnsetAvatar()}, + }), + ) }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar(publicAvatar2)}, - }), - ) - - t.Log("Alice removes the avatar for the public room.") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{}) - t.Log("Alice syncs until she sees that avatar vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomUnsetAvatar()}, - }), - ) - }) - - t.Run("Explicit avatar propagates in DM room", func(t *testing.T) { - t.Log("Alice re-invites Chris to their DM. He accepts.") - alice.InviteRoom(t, dmChris, chris.UserID) - chris.JoinRoom(t, dmChris, nil) - t.Log("Alice syncs until she sees Chris's join.") - res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") - - t.Log("Alice should see the DM with Chris's avatar.") - m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) - - t.Log("Chris gives their DM a bespoke avatar.") - dmAvatar := uploadAvatar(chris, "dm.png") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ - "url": dmAvatar, + t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { + t.Log("Alice sets an avatar for the public room.") + publicAvatar := uploadAvatar(alice, "public.png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar, + }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar)}, + }), + ) + + t.Log("Alice changes the avatar for the public room.") + publicAvatar2 := uploadAvatar(alice, "public2.png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar2, + }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar2)}, + }), + ) + + t.Log("Alice removes the avatar for the public room.") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{}) + t.Log("Alice syncs until she sees that avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomUnsetAvatar()}, + }), + ) }) - t.Log("Alice syncs until she sees that avatar.") - alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) + t.Run("Explicit avatar propagates in DM room", func(t *testing.T) { + t.Log("Alice re-invites Chris to their DM. He accepts.") + alice.InviteRoom(t, dmChris, chris.UserID) + chris.JoinRoom(t, dmChris, nil) - t.Log("Chris changes his global avatar.") - chrisAvatar2 := uploadAvatar(chris, "chris2.png") - chris.SetAvatar(t, chrisAvatar2) + t.Log("Alice syncs until she sees Chris's join.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") - t.Log("Chris sends a sentinel message.") - sentinel := chris.SendEventSynced(t, dmBobChris, Event{ - Type: "m.room.message", - Content: map[string]interface{}{ - "body": "Hello world", - "msgtype": "m.text", - }, - }) - - t.Log("Alice syncs until she sees the sentinel. She should not see the DM avatar change.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error { - matchNoAvatarChange := m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar("")) - if err := matchNoAvatarChange(response); err != nil { - t.Errorf("Saw DM avatar change: %s", err) - } - matchSentinel := m.MatchRoomSubscription(dmChris, MatchRoomTimelineMostRecent(1, []Event{{ID: sentinel}})) - return matchSentinel(response) - }) + t.Log("Alice should see the DM with Chris's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) - t.Log("Chris updates the DM's avatar.") - dmAvatar2 := uploadAvatar(chris, "dm2.png") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ - "url": dmAvatar2, - }) + t.Log("Chris gives their DM a bespoke avatar.") + dmAvatar := uploadAvatar(chris, "dm.png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar, + }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar2))) + t.Log("Alice syncs until she sees that avatar.") + alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) - t.Log("Chris removes the DM's avatar.") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) + t.Log("Chris changes his global avatar.") + chrisAvatar2 := uploadAvatar(chris, "chris2.png") + chris.SetAvatar(t, chrisAvatar2) - t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) - }) + t.Log("Chris sends a sentinel message.") + sentinel := chris.SendEventSynced(t, dmBobChris, Event{ + Type: "m.room.message", + Content: map[string]interface{}{ + "body": "Hello world", + "msgtype": "m.text", + }, + }) + + t.Log("Alice syncs until she sees the sentinel. She should not see the DM avatar change.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error { + matchNoAvatarChange := m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar("")) + if err := matchNoAvatarChange(response); err != nil { + t.Errorf("Saw DM avatar change: %s", err) + } + matchSentinel := m.MatchRoomSubscription(dmChris, MatchRoomTimelineMostRecent(1, []Event{{ID: sentinel}})) + return matchSentinel(response) + }) + + t.Log("Chris updates the DM's avatar.") + dmAvatar2 := uploadAvatar(chris, "dm2.png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar2, + }) + + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar2))) + + t.Log("Chris removes the DM's avatar.") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) + + t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) + }) + */ // TODO unset DM's avatar, then set a custom one in that room. // TODO when you're invited, you see the avatar + // TODO make a DM not a DM, loses its avatar; remake it a DM and it gains one. } diff --git a/testutils/m/match.go b/testutils/m/match.go index 3e9f7ad9..cc662c35 100644 --- a/testutils/m/match.go +++ b/testutils/m/match.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "github.com/matrix-org/sliding-sync/internal" "reflect" "sort" "testing" @@ -44,7 +45,7 @@ func MatchRoomName(name string) RoomMatcher { func MatchRoomAvatar(wantAvatar string) RoomMatcher { return func(r sync3.Room) error { if string(r.AvatarChange) != wantAvatar { - return fmt.Errorf("MatchRoomAvatar: got %s want %s", r.AvatarChange, wantAvatar) + return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, wantAvatar) } return nil } @@ -54,8 +55,19 @@ func MatchRoomAvatar(wantAvatar string) RoomMatcher { // avatar, or has had its avatar deleted. func MatchRoomUnsetAvatar() RoomMatcher { return func(r sync3.Room) error { - if r.AvatarChange != sync3.DeletedAvatar { - return fmt.Errorf("MatchRoomAvatar: got %s want %s", r.AvatarChange, sync3.DeletedAvatar) + if r.AvatarChange != internal.DeletedAvatar { + return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, internal.DeletedAvatar) + } + return nil + } +} + +// MatchRoomUnchangedAvatar builds a RoomMatcher which checks that the given room has no +// change to its avatar, or has had its avatar deleted. +func MatchRoomUnchangedAvatar() RoomMatcher { + return func(r sync3.Room) error { + if r.AvatarChange != internal.UnchangedAvatar { + return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, internal.UnchangedAvatar) } return nil } @@ -244,11 +256,11 @@ func MatchRoomSubscription(roomID string, matchers ...RoomMatcher) RespMatcher { return func(res *sync3.Response) error { room, ok := res.Rooms[roomID] if !ok { - return fmt.Errorf("MatchRoomSubscription: want sub for %s but it was missing", roomID) + return fmt.Errorf("MatchRoomSubscription[%s]: want sub but it was missing", roomID) } for _, m := range matchers { if err := m(room); err != nil { - return fmt.Errorf("MatchRoomSubscription: %s", err) + return fmt.Errorf("MatchRoomSubscription[%s]: %s", roomID, err) } } return nil @@ -666,6 +678,15 @@ func LogResponse(t *testing.T) RespMatcher { } } +// LogRooms is like LogResponse, but only logs the rooms section of the response. +func LogRooms(t *testing.T) RespMatcher { + return func(res *sync3.Response) error { + dump, _ := json.MarshalIndent(res.Rooms, "", " ") + t.Logf("Response rooms were: %s", dump) + return nil + } +} + func CheckList(listKey string, res sync3.ResponseList, matchers ...ListMatcher) error { for _, m := range matchers { if err := m(res); err != nil { @@ -708,13 +729,15 @@ func MatchLists(matchers map[string][]ListMatcher) RespMatcher { } } +const AnsiRedForeground = "\x1b[31m" +const AnsiResetForeground = "\x1b[39m " + func MatchResponse(t *testing.T, res *sync3.Response, matchers ...RespMatcher) { t.Helper() for _, m := range matchers { err := m(res) if err != nil { - b, _ := json.Marshal(res) - t.Errorf("MatchResponse: %s\n%+v", err, string(b)) + t.Errorf("%vMatchResponse: %s%v", AnsiRedForeground, err, AnsiResetForeground) } } } From 851b18d685c6a1fe52238a927ec5831153768262 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 19:15:33 +0100 Subject: [PATCH 14/30] Checkpoint --- internal/roomname.go | 42 +++++++++++++++++++++--- sync3/caches/user.go | 14 ++++---- sync3/handler/connstate.go | 1 + sync3/handler/connstate_live.go | 9 +++++- sync3/lists.go | 11 ++----- sync3/room.go | 57 ++++++++------------------------- 6 files changed, 70 insertions(+), 64 deletions(-) diff --git a/internal/roomname.go b/internal/roomname.go index 12cd6485..91e4fa90 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -13,7 +13,12 @@ type EventMetadata struct { Timestamp uint64 } -// RoomMetadata holds room-scoped data. It is primarily used in two places: +// RoomMetadata holds room-scoped data. +// TODO: This is a lie: we sometimes remove a user U from the list of heroes +// when calculating the sync response for that user U. Grep for `RemoveHero`. +// +// It is primarily used in two places: +// // - in the caches.GlobalCache, to hold the latest version of data that is consistent // between all users in the room; and // - in the sync3.RoomConnMetadata struct, to hold the version of data last seen by @@ -63,13 +68,13 @@ func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool { m.CanonicalAlias == other.CanonicalAlias && m.JoinCount == other.JoinCount && m.InviteCount == other.InviteCount && - sameHeroes(m.Heroes, other.Heroes)) + sameHeroNames(m.Heroes, other.Heroes)) } -// SameRoomAvatar checks if the fields relevant for room names have changed between the two metadatas. +// SameRoomAvatar checks if the fields relevant for room avatars have changed between the two metadatas. // Returns true if there are no changes. func (m *RoomMetadata) SameRoomAvatar(other *RoomMetadata) bool { - return m.AvatarEvent == other.AvatarEvent + return m.AvatarEvent == other.AvatarEvent && sameHeroAvatars(m.Heroes, other.Heroes) } func (m *RoomMetadata) SameJoinCount(other *RoomMetadata) bool { @@ -80,7 +85,7 @@ func (m *RoomMetadata) SameInviteCount(other *RoomMetadata) bool { return m.InviteCount == other.InviteCount } -func sameHeroes(a, b []Hero) bool { +func sameHeroNames(a, b []Hero) bool { if len(a) != len(b) { return false } @@ -95,6 +100,21 @@ func sameHeroes(a, b []Hero) bool { return true } +func sameHeroAvatars(a, b []Hero) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i].ID != b[i].ID { + return false + } + if a[i].Avatar != b[i].Avatar { + return false + } + } + return true +} + func (m *RoomMetadata) RemoveHero(userID string) { for i, h := range m.Heroes { if h.ID == userID { @@ -198,3 +218,15 @@ func disambiguate(heroes []Hero) []string { } return disambiguatedNames } + +func CalculateAvatar(metadata *RoomMetadata) AvatarChange { + if metadata.AvatarEvent != "" { + return AvatarChange(metadata.AvatarEvent) + } + // Assumption: metadata.RemoveHero has been called to remove the user who is syncing + // from the list of heroes. + if len(metadata.Heroes) == 1 { + return AvatarChange(metadata.Heroes[0].Avatar) + } + return DeletedAvatar +} diff --git a/sync3/caches/user.go b/sync3/caches/user.go index 3dd9524e..244dce67 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -55,7 +55,7 @@ type UserRoomData struct { // represent this room. Avatars set in m.room.avatar take precedence; if this is // missing and the room is a DM with one other user, we fall back to that user's // avatar (if any) as specified in their membership event in that room. - ResolvedAvatarURL string + ResolvedAvatarURL internal.AvatarChange Spaces map[string]struct{} // Map of tag to order float. @@ -224,7 +224,7 @@ func (c *UserCache) Unsubscribe(id int) { // OnRegistered is called after the sync3.Dispatcher has successfully registered this // cache to receive updates. We use this to run some final initialisation logic that // is sensitive to race conditions; confusingly, most of the initialisation is driven -// externally by sync3.SyncLiveHandler.userCache. It's importatn that we don't spend too +// externally by sync3.SyncLiveHandler.userCaches. It's important that we don't spend too // long inside this function, because it is called within a global lock on the // sync3.Dispatcher (see sync3.Dispatcher.Register). func (c *UserCache) OnRegistered(ctx context.Context) error { @@ -317,6 +317,7 @@ func (c *UserCache) LazyLoadTimelines(ctx context.Context, loadPos int64, roomID for _, requestedRoomID := range roomIDs { latestEvents := roomIDToLatestEvents[requestedRoomID] urd, ok := c.roomToData[requestedRoomID] + if !ok { urd = NewUserRoomData() } @@ -584,10 +585,11 @@ func (c *UserCache) OnInvite(ctx context.Context, roomID string, inviteStateEven urd.HasLeft = false urd.HighlightCount = InvitesAreHighlightsValue urd.IsDM = inviteData.IsDM - urd.ResolvedAvatarURL = inviteData.AvatarEvent - if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { - urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar - } + logger.Warn().Msgf("TODO set avatar on invite") + //urd.ResolvedAvatarURL = inviteData.AvatarEvent + //if urd.ResolvedAvatarURL == "" && urd.IsDM && len(inviteData.Heroes) == 1 { + // urd.ResolvedAvatarURL = inviteData.Heroes[0].Avatar + //} urd.Invite = inviteData c.roomToDataMu.Lock() c.roomToData[roomID] = urd diff --git a/sync3/handler/connstate.go b/sync3/handler/connstate.go index 9f3f79ab..a1ebdc2d 100644 --- a/sync3/handler/connstate.go +++ b/sync3/handler/connstate.go @@ -596,6 +596,7 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu } rooms[roomID] = sync3.Room{ Name: internal.CalculateRoomName(metadata, 5), // TODO: customisable? + AvatarChange: internal.CalculateAvatar(metadata), NotificationCount: int64(userRoomData.NotificationCount), HighlightCount: int64(userRoomData.HighlightCount), Timeline: roomToTimeline[roomID], diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go index a01a7ef6..5d5b4bcf 100644 --- a/sync3/handler/connstate_live.go +++ b/sync3/handler/connstate_live.go @@ -218,6 +218,11 @@ func (s *connStateLive) processLiveUpdate(ctx context.Context, up caches.Update, metadata.RemoveHero(s.userID) thisRoom.Name = internal.CalculateRoomName(metadata, 5) // TODO: customisable? } + if delta.RoomAvatarChanged { + metadata := roomUpdate.GlobalRoomMetadata() + metadata.RemoveHero(s.userID) + thisRoom.AvatarChange = internal.CalculateAvatar(metadata) + } if delta.InviteCountChanged { thisRoom.InvitedCount = &roomUpdate.GlobalRoomMetadata().InviteCount } @@ -290,8 +295,10 @@ func (s *connStateLive) processGlobalUpdates(ctx context.Context, builder *Rooms } } + metadata := *rup.GlobalRoomMetadata() + metadata.RemoveHero(s.userID) delta = s.lists.SetRoom(sync3.RoomConnMetadata{ - RoomMetadata: *rup.GlobalRoomMetadata(), + RoomMetadata: metadata, UserRoomData: *rup.UserRoomMetadata(), LastInterestedEventTimestamps: bumpTimestampInList, }) diff --git a/sync3/lists.go b/sync3/lists.go index 473cf4bf..d8b404df 100644 --- a/sync3/lists.go +++ b/sync3/lists.go @@ -74,10 +74,9 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"), ) } - delta.RoomAvatarChanged = !existing.SameRoomName(&r.RoomMetadata) + delta.RoomAvatarChanged = !existing.SameRoomAvatar(&r.RoomMetadata) if delta.RoomAvatarChanged { - logger.Warn().Msg("m.room.avatar change, implementme") - r.ResolvedAvatarURL = "" // TODO + r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata) } // Interpret the timestamp map on r as the changes we should apply atop the @@ -103,11 +102,7 @@ func (s *InternalRequestLists) SetRoom(r RoomConnMetadata) (delta RoomDelta) { r.CanonicalisedName = strings.ToLower( strings.Trim(internal.CalculateRoomName(&r.RoomMetadata, 5), "#!():_@"), ) - if r.AvatarEvent != "" { - r.ResolvedAvatarURL = r.AvatarEvent - } else if r.IsDM && len(r.Heroes) == 1 { - r.ResolvedAvatarURL = r.Heroes[0].Avatar - } + r.ResolvedAvatarURL = internal.CalculateAvatar(&r.RoomMetadata) // We'll automatically use the LastInterestedEventTimestamps provided by the // caller, so that recency sorts work. } diff --git a/sync3/room.go b/sync3/room.go index 91606745..159dcef7 100644 --- a/sync3/room.go +++ b/sync3/room.go @@ -1,57 +1,26 @@ package sync3 import ( - "bytes" "encoding/json" "github.com/matrix-org/sliding-sync/internal" "github.com/matrix-org/sliding-sync/sync3/caches" ) -// sentinel value -const deletedAvatar = "" - -// An AvatarChange represents a change to a room's avatar. There are three cases: -// - an empty string represents no change, and should be omitted when JSON-serisalised; -// - the sentinel `avatarNotPresent` represents a room that has never had an avatar, -// or a room whose avatar has been removed. It is JSON-serialised as null. -// - All other strings represent the current avatar of the room and JSON-serialise as -// normal. -type AvatarChange string - -func (a AvatarChange) MarshalJSON() ([]byte, error) { - if a == deletedAvatar { - return []byte(`null`), nil - } else { - return json.Marshal(string(a)) - } -} - -func (a *AvatarChange) UnmarshalJSON(data []byte) error { - if bytes.Equal(data, []byte("null")) { - *a = deletedAvatar - return nil - } - return json.Unmarshal(data, (*string)(a)) -} - -var DeletedAvatar = AvatarChange(deletedAvatar) -var UnchangedAvatar AvatarChange - type Room struct { - Name string `json:"name,omitempty"` - AvatarChange AvatarChange `json:"avatar,omitempty"` - RequiredState []json.RawMessage `json:"required_state,omitempty"` - Timeline []json.RawMessage `json:"timeline,omitempty"` - InviteState []json.RawMessage `json:"invite_state,omitempty"` - NotificationCount int64 `json:"notification_count"` - HighlightCount int64 `json:"highlight_count"` - Initial bool `json:"initial,omitempty"` - IsDM bool `json:"is_dm,omitempty"` - JoinedCount int `json:"joined_count,omitempty"` - InvitedCount *int `json:"invited_count,omitempty"` - PrevBatch string `json:"prev_batch,omitempty"` - NumLive int `json:"num_live,omitempty"` + Name string `json:"name,omitempty"` + AvatarChange internal.AvatarChange `json:"avatar,omitempty"` + RequiredState []json.RawMessage `json:"required_state,omitempty"` + Timeline []json.RawMessage `json:"timeline,omitempty"` + InviteState []json.RawMessage `json:"invite_state,omitempty"` + NotificationCount int64 `json:"notification_count"` + HighlightCount int64 `json:"highlight_count"` + Initial bool `json:"initial,omitempty"` + IsDM bool `json:"is_dm,omitempty"` + JoinedCount int `json:"joined_count,omitempty"` + InvitedCount *int `json:"invited_count,omitempty"` + PrevBatch string `json:"prev_batch,omitempty"` + NumLive int `json:"num_live,omitempty"` } // RoomConnMetadata represents a room as seen by one specific connection (hence one From 0da3a29cced1d026c3d68eb933bdbaccaf94e47d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 19:51:00 +0100 Subject: [PATCH 15/30] Fix weird hero duplication bug I can't explain --- sync3/handler/connstate_live.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go index 5d5b4bcf..eb379266 100644 --- a/sync3/handler/connstate_live.go +++ b/sync3/handler/connstate_live.go @@ -295,10 +295,8 @@ func (s *connStateLive) processGlobalUpdates(ctx context.Context, builder *Rooms } } - metadata := *rup.GlobalRoomMetadata() - metadata.RemoveHero(s.userID) delta = s.lists.SetRoom(sync3.RoomConnMetadata{ - RoomMetadata: metadata, + RoomMetadata: *rup.GlobalRoomMetadata(), UserRoomData: *rup.UserRoomMetadata(), LastInterestedEventTimestamps: bumpTimestampInList, }) From b943a60b9672d7f5b990c11d8208869ed74ab523 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 19:57:05 +0100 Subject: [PATCH 16/30] Fix not updating heroes' avatars --- sync3/caches/global.go | 1 + 1 file changed, 1 insertion(+) diff --git a/sync3/caches/global.go b/sync3/caches/global.go index da16691e..1efaddfc 100644 --- a/sync3/caches/global.go +++ b/sync3/caches/global.go @@ -353,6 +353,7 @@ func (c *GlobalCache) OnNewEvent( for i := range metadata.Heroes { if metadata.Heroes[i].ID == *ed.StateKey { metadata.Heroes[i].Name = ed.Content.Get("displayname").Str + metadata.Heroes[i].Avatar = ed.Content.Get("avatar_url").Str found = true break } From 5051c20bf7bae6c4b9823db91e2c184e14f1279a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 20:02:58 +0100 Subject: [PATCH 17/30] WIP on next test case --- tests-e2e/client_test.go | 6 +-- tests-e2e/lists_test.go | 105 +++++++++++++++++++++++---------------- 2 files changed, 65 insertions(+), 46 deletions(-) diff --git a/tests-e2e/client_test.go b/tests-e2e/client_test.go index de713ba9..641ff999 100644 --- a/tests-e2e/client_test.go +++ b/tests-e2e/client_test.go @@ -159,11 +159,11 @@ func (c *CSAPI) UploadContent(t *testing.T, fileBody []byte, fileName string, co return GetJSONFieldStr(t, body, "content_uri") } +// Use an empty string to remove your avatar. func (c *CSAPI) SetAvatar(t *testing.T, avatarURL string) { t.Helper() - reqBody := map[string]interface{}{} - if avatarURL != "" { - reqBody["avatar_url"] = avatarURL + reqBody := map[string]interface{}{ + "avatar_url": avatarURL, } c.MustDoFunc(t, "PUT", []string{"_matrix", "client", "v3", "profile", c.UserID, "avatar_url"}, WithJSONBody(t, reqBody)) } diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 3282e0a4..3150d808 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1401,56 +1401,75 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { m.MatchResponse(t, res, m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar))) }) - /* - t.Run("Bob's avatar change propagates", func(t *testing.T) { - t.Log("Bob changes his avatar.") - bobAvatar2 := uploadAvatar(bob, "bob2.png") - bob.SetAvatar(t, bobAvatar2) - - avatarChangeInDM := false - avatarChangeInGroupDM := false - t.Log("Alice syncs until she sees Bob's new avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - func(response *sync3.Response) error { - if !avatarChangeInDM { - err := m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar2))(response) - if err == nil { - avatarChangeInDM = true - } + t.Run("Bob's avatar change propagates", func(t *testing.T) { + t.Log("Bob changes his avatar.") + bobAvatar2 := uploadAvatar(bob, "bob2.png") + bob.SetAvatar(t, bobAvatar2) + + avatarChangeInDM := false + avatarChangeInGroupDM := false + t.Log("Alice syncs until she sees Bob's new avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + func(response *sync3.Response) error { + if !avatarChangeInDM { + err := m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar2))(response) + if err == nil { + avatarChangeInDM = true } + } - if !avatarChangeInGroupDM { - err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar2))(response) - if err == nil { - avatarChangeInGroupDM = true - } + if !avatarChangeInGroupDM { + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar2))(response) + if err == nil { + avatarChangeInGroupDM = true } + } - if avatarChangeInDM && avatarChangeInGroupDM { - return nil + if avatarChangeInDM && avatarChangeInGroupDM { + return nil + } + return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + }, + ) + + t.Log("Bob removes his avatar.") + bob.SetAvatar(t, "") + + avatarChangeInDM = false + avatarChangeInGroupDM = false + t.Log("Alice syncs until she sees Bob's avatars vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + func(response *sync3.Response) error { + if !avatarChangeInDM { + err := m.MatchRoomSubscription(dmBob, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInDM = true } - m.LogRooms(t)(response) - return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) - }, - ) + } - t.Log("Bob removes his avatar.") - bob.SetAvatar(t, "") + if !avatarChangeInGroupDM { + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInGroupDM = true + } + } - t.Log("Alice syncs until she sees Bob's avatar vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - dmBob: {m.MatchRoomUnsetAvatar()}, - dmBobChris: {m.MatchRoomUnsetAvatar()}, - }), - ) - }) + if avatarChangeInDM && avatarChangeInGroupDM { + return nil + } + m.LogRooms(t)(response) + return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + }, + ) + + }) + /* t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { t.Log("Alice sets an avatar for the public room.") From c26f29b06790e8d976efdf3b10839732817a7b1f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 20:05:40 +0100 Subject: [PATCH 18/30] It would help if I committed this --- internal/avatar.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 internal/avatar.go diff --git a/internal/avatar.go b/internal/avatar.go new file mode 100644 index 00000000..6ec89bba --- /dev/null +++ b/internal/avatar.go @@ -0,0 +1,37 @@ +package internal + +import ( + "bytes" + "encoding/json" +) + +// sentinel value +const deletedAvatar = "" + +// An AvatarChange represents a change to a room's avatar. There are three cases: +// - an empty string represents no change, and should be omitted when JSON-serisalised; +// - the sentinel `avatarNotPresent` represents a room that has never had an avatar, +// or a room whose avatar has been removed. It is JSON-serialised as null. +// - All other strings represent the current avatar of the room and JSON-serialise as +// normal. +type AvatarChange string + +func (a AvatarChange) MarshalJSON() ([]byte, error) { + if a == deletedAvatar { + return []byte(`null`), nil + } else { + return json.Marshal(string(a)) + } +} + +// Note: the unmarshalling is only used in tests. +func (a *AvatarChange) UnmarshalJSON(data []byte) error { + if bytes.Equal(data, []byte("null")) { + *a = deletedAvatar + return nil + } + return json.Unmarshal(data, (*string)(a)) +} + +var DeletedAvatar = AvatarChange(deletedAvatar) +var UnchangedAvatar AvatarChange From cc1681fb2c82083ba31b6851e9e8f8dddc4d45a2 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Fri, 14 Jul 2023 20:05:58 +0100 Subject: [PATCH 19/30] And these tests --- sync3/room_test.go | 75 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 sync3/room_test.go diff --git a/sync3/room_test.go b/sync3/room_test.go new file mode 100644 index 00000000..c8a00fab --- /dev/null +++ b/sync3/room_test.go @@ -0,0 +1,75 @@ +package sync3 + +import ( + "encoding/json" + "fmt" + "github.com/matrix-org/sliding-sync/internal" + "github.com/tidwall/gjson" + "reflect" + "testing" +) + +func TestAvatarChangeMarshalling(t *testing.T) { + var url = "mxc://..." + testCases := []struct { + Name string + AvatarChange internal.AvatarChange + Check func(avatar gjson.Result) error + }{ + { + Name: "Avatar exists", + AvatarChange: internal.AvatarChange(url), + Check: func(avatar gjson.Result) error { + if !(avatar.Exists() && avatar.Type == gjson.String && avatar.Str == url) { + return fmt.Errorf("unexpected marshalled avatar: got %#v want %s", avatar, url) + } + return nil + }, + }, + { + Name: "Avatar doesn't exist", + AvatarChange: internal.DeletedAvatar, + Check: func(avatar gjson.Result) error { + if !(avatar.Exists() && avatar.Type == gjson.Null) { + return fmt.Errorf("unexpected marshalled Avatar: got %#v want null", avatar) + } + return nil + }, + }, + { + Name: "Avatar unchanged", + AvatarChange: internal.UnchangedAvatar, + Check: func(avatar gjson.Result) error { + if avatar.Exists() { + return fmt.Errorf("unexpected marshalled Avatar: got %#v want omitted", avatar) + } + return nil + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + room := Room{AvatarChange: tc.AvatarChange} + marshalled, err := json.Marshal(room) + t.Logf("Marshalled to %s", string(marshalled)) + if err != nil { + t.Fatal(err) + } + avatar := gjson.GetBytes(marshalled, "avatar") + if err = tc.Check(avatar); err != nil { + t.Fatal(err) + } + + var unmarshalled Room + err = json.Unmarshal(marshalled, &unmarshalled) + if err != nil { + t.Fatal(err) + } + t.Logf("Unmarshalled to %#v", unmarshalled.AvatarChange) + if !reflect.DeepEqual(unmarshalled, room) { + t.Fatalf("Unmarshalled struct is different from original") + } + }) + } +} From df98a71a190565aa566b80614f4a4a7b8f6024f6 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 14:24:19 +0100 Subject: [PATCH 20/30] SetRoom: omit the current user as a hero --- internal/roomname.go | 26 ++++++++++++++++++++ internal/roomname_test.go | 42 +++++++++++++++++++++++++++++++++ sync3/caches/global.go | 8 +------ sync3/caches/user.go | 5 +++- sync3/handler/connstate_live.go | 4 +++- 5 files changed, 76 insertions(+), 9 deletions(-) diff --git a/internal/roomname.go b/internal/roomname.go index 91e4fa90..8e9a2104 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -60,6 +60,32 @@ func NewRoomMetadata(roomID string) *RoomMetadata { } } +// CopyHeroes returns a version of the current RoomMetadata whose Heroes field is +// a brand-new copy of the original Heroes. The return value's Heroes field can be +// safely modified by the caller, but it is NOT safe for the caller to modify any other +// fields. +func (m *RoomMetadata) CopyHeroes() *RoomMetadata { + newMetadata := *m + + // XXX: We're doing this because we end up calling RemoveHero() to omit the + // currently-sycning user in various places. But this seems smelly. The set of + // heroes in the room is a global, room-scoped fact: it is a property of the room + // state and nothing else, and all users see the same set of heroes. + // + // I think the data model would be cleaner if we made the hero-reading functions + // aware of the currently syncing user, in order to ignore them without having to + // change the underlying data. + // + // copy the heroes or else we may modify the same slice which would be bad :( + newMetadata.Heroes = make([]Hero, len(m.Heroes)) + copy(newMetadata.Heroes, m.Heroes) + + // ⚠️ NB: there are other pointer fields (e.g. PredecessorRoomID *string) or + // and pointer-backed fields (e.g. LatestEventsByType map[string]EventMetadata) + // which are not deepcopied here. + return &newMetadata +} + // SameRoomName checks if the fields relevant for room names have changed between the two metadatas. // Returns true if there are no changes. func (m *RoomMetadata) SameRoomName(other *RoomMetadata) bool { diff --git a/internal/roomname_test.go b/internal/roomname_test.go index 4e942f43..01349eed 100644 --- a/internal/roomname_test.go +++ b/internal/roomname_test.go @@ -247,3 +247,45 @@ func TestCalculateRoomName(t *testing.T) { } } } + +func TestCopyHeroes(t *testing.T) { + const alice = "@alice:test" + const bob = "@bob:test" + const chris = "@chris:test" + m1 := RoomMetadata{Heroes: []Hero{ + {ID: alice}, + {ID: bob}, + {ID: chris}, + }} + + m2 := m1.CopyHeroes() + // Uncomment this to see why CopyHeroes is necessary! + //m2 := m1 + + t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes) + + t.Log("Remove chris from m1") + m1.RemoveHero(chris) + t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes) + + assertSliceIDs(t, "m1.Heroes", m1.Heroes, []string{alice, bob}) + assertSliceIDs(t, "m2.Heroes", m2.Heroes, []string{alice, bob, chris}) + + t.Log("Remove alice from m1") + m1.RemoveHero(alice) + t.Logf("Compare heroes:\n\tm1=%v\n\tm2=%v", m1.Heroes, m2.Heroes) + + assertSliceIDs(t, "m1.Heroes", m1.Heroes, []string{bob}) + assertSliceIDs(t, "m2.Heroes", m2.Heroes, []string{alice, bob, chris}) +} + +func assertSliceIDs(t *testing.T, desc string, h []Hero, ids []string) { + if len(h) != len(ids) { + t.Errorf("%s has length %d, expected %d", desc, len(h), len(ids)) + } + for index, id := range ids { + if h[index].ID != id { + t.Errorf("%s[%d] ID is %s, expected %s", desc, index, h[index].ID, id) + } + } +} diff --git a/sync3/caches/global.go b/sync3/caches/global.go index 1efaddfc..537ce00b 100644 --- a/sync3/caches/global.go +++ b/sync3/caches/global.go @@ -118,13 +118,7 @@ func (c *GlobalCache) copyRoom(roomID string) *internal.RoomMetadata { logger.Warn().Str("room", roomID).Msg("GlobalCache.LoadRoom: no metadata for this room, returning stub") return internal.NewRoomMetadata(roomID) } - srCopy := *sr - // copy the heroes or else we may modify the same slice which would be bad :( - srCopy.Heroes = make([]internal.Hero, len(sr.Heroes)) - for i := range sr.Heroes { - srCopy.Heroes[i] = sr.Heroes[i] - } - return &srCopy + return sr.CopyHeroes() } // LoadJoinedRooms loads all current joined room metadata for the user given, together diff --git a/sync3/caches/user.go b/sync3/caches/user.go index 244dce67..ba5c3532 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -341,7 +341,10 @@ func (c *UserCache) LoadRoomData(roomID string) UserRoomData { } type roomUpdateCache struct { - roomID string + roomID string + // globalRoomData is a snapshot of the global metadata for this room immediately + // after this update. It is a copy, specific to the given user whose Heroes + // field can be freely modified. globalRoomData *internal.RoomMetadata userRoomData *UserRoomData } diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go index eb379266..1b1b23c1 100644 --- a/sync3/handler/connstate_live.go +++ b/sync3/handler/connstate_live.go @@ -295,8 +295,10 @@ func (s *connStateLive) processGlobalUpdates(ctx context.Context, builder *Rooms } } + metadata := rup.GlobalRoomMetadata().CopyHeroes() + metadata.RemoveHero(s.userID) delta = s.lists.SetRoom(sync3.RoomConnMetadata{ - RoomMetadata: *rup.GlobalRoomMetadata(), + RoomMetadata: *metadata, UserRoomData: *rup.UserRoomMetadata(), LastInterestedEventTimestamps: bumpTimestampInList, }) From bfd84488fb788e8ef44d7b907dd4ca15ebb2eac9 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 14:39:01 +0100 Subject: [PATCH 21/30] Checkpoint --- tests-e2e/lists_test.go | 56 +++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 3150d808..95045bb9 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1435,38 +1435,40 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { }, ) - t.Log("Bob removes his avatar.") - bob.SetAvatar(t, "") + /* + t.Log("Bob removes his avatar.") + bob.SetAvatar(t, "") - avatarChangeInDM = false - avatarChangeInGroupDM = false - t.Log("Alice syncs until she sees Bob's avatars vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - func(response *sync3.Response) error { - if !avatarChangeInDM { - err := m.MatchRoomSubscription(dmBob, m.MatchRoomUnsetAvatar())(response) - if err == nil { - avatarChangeInDM = true + avatarChangeInDM = false + avatarChangeInGroupDM = false + t.Log("Alice syncs until she sees Bob's avatars vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + func(response *sync3.Response) error { + if !avatarChangeInDM { + err := m.MatchRoomSubscription(dmBob, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInDM = true + } } - } - if !avatarChangeInGroupDM { - err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar())(response) - if err == nil { - avatarChangeInGroupDM = true + if !avatarChangeInGroupDM { + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInGroupDM = true + } } - } - if avatarChangeInDM && avatarChangeInGroupDM { - return nil - } - m.LogRooms(t)(response) - return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) - }, - ) + if avatarChangeInDM && avatarChangeInGroupDM { + return nil + } + m.LogRooms(t)(response) + return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + }, + ) + */ }) /* From 3213b9224b7a2582acbb06b7223a328ee73e4d7f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 16:18:24 +0100 Subject: [PATCH 22/30] Chceckpoint --- tests-e2e/lists_test.go | 60 +++++++++++++++++++++-------------------- 1 file changed, 31 insertions(+), 29 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 95045bb9..10720f6e 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1435,40 +1435,42 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { }, ) - /* - t.Log("Bob removes his avatar.") - bob.SetAvatar(t, "") + t.Log("Bob removes his avatar.") + bob.SetAvatar(t, "") - avatarChangeInDM = false - avatarChangeInGroupDM = false - t.Log("Alice syncs until she sees Bob's avatars vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - func(response *sync3.Response) error { - if !avatarChangeInDM { - err := m.MatchRoomSubscription(dmBob, m.MatchRoomUnsetAvatar())(response) - if err == nil { - avatarChangeInDM = true - } + avatarChangeInDM = false + avatarChangeInGroupDM = false + t.Log("Alice syncs until she sees Bob's avatars vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + func(response *sync3.Response) error { + if !avatarChangeInDM { + err := m.MatchRoomSubscription(dmBob, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInDM = true + } else { + t.Log(err) } + } - if !avatarChangeInGroupDM { - err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar())(response) - if err == nil { - avatarChangeInGroupDM = true - } + if !avatarChangeInGroupDM { + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar())(response) + if err == nil { + avatarChangeInGroupDM = true + } else { + t.Log(err) } + } - if avatarChangeInDM && avatarChangeInGroupDM { - return nil - } - m.LogRooms(t)(response) - return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) - }, - ) - */ + if avatarChangeInDM && avatarChangeInGroupDM { + return nil + } + m.LogRooms(t)(response) + return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + }, + ) }) /* From f68d73f6106ab9ecba9f1c6f7b34394c0443952f Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 16:32:36 +0100 Subject: [PATCH 23/30] fix rest of that test --- internal/roomname.go | 6 +++++- tests-e2e/lists_test.go | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/roomname.go b/internal/roomname.go index 8e9a2104..cd38da3b 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -252,7 +252,11 @@ func CalculateAvatar(metadata *RoomMetadata) AvatarChange { // Assumption: metadata.RemoveHero has been called to remove the user who is syncing // from the list of heroes. if len(metadata.Heroes) == 1 { - return AvatarChange(metadata.Heroes[0].Avatar) + avatar := metadata.Heroes[0].Avatar + if avatar == "" { + return DeletedAvatar + } + return AvatarChange(avatar) } return DeletedAvatar } diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 10720f6e..9ca6be2f 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1467,7 +1467,6 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { if avatarChangeInDM && avatarChangeInGroupDM { return nil } - m.LogRooms(t)(response) return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) }, ) From dd3f09ece8213fa4277d5c4bcf9374008d50fc2d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 17:01:02 +0100 Subject: [PATCH 24/30] Only use AvatarChange at the output --- internal/roomname.go | 19 +++++++++---------- {internal => sync3}/avatar.go | 27 ++++++++++++++++----------- sync3/caches/user.go | 7 ++++--- sync3/handler/connstate.go | 2 +- sync3/handler/connstate_live.go | 2 +- sync3/room.go | 26 +++++++++++++------------- sync3/room_test.go | 9 ++++----- testutils/m/match.go | 9 ++++----- 8 files changed, 52 insertions(+), 49 deletions(-) rename {internal => sync3}/avatar.go (61%) diff --git a/internal/roomname.go b/internal/roomname.go index cd38da3b..8e4ad465 100644 --- a/internal/roomname.go +++ b/internal/roomname.go @@ -245,18 +245,17 @@ func disambiguate(heroes []Hero) []string { return disambiguatedNames } -func CalculateAvatar(metadata *RoomMetadata) AvatarChange { +const noAvatar = "" + +// CalculateAvatar computes the avatar for the room, based on the global room metadata. +// Assumption: metadata.RemoveHero has been called to remove the user who is syncing +// from the list of heroes. +func CalculateAvatar(metadata *RoomMetadata) string { if metadata.AvatarEvent != "" { - return AvatarChange(metadata.AvatarEvent) + return metadata.AvatarEvent } - // Assumption: metadata.RemoveHero has been called to remove the user who is syncing - // from the list of heroes. if len(metadata.Heroes) == 1 { - avatar := metadata.Heroes[0].Avatar - if avatar == "" { - return DeletedAvatar - } - return AvatarChange(avatar) + return metadata.Heroes[0].Avatar } - return DeletedAvatar + return noAvatar } diff --git a/internal/avatar.go b/sync3/avatar.go similarity index 61% rename from internal/avatar.go rename to sync3/avatar.go index 6ec89bba..63e494b8 100644 --- a/internal/avatar.go +++ b/sync3/avatar.go @@ -1,23 +1,31 @@ -package internal +package sync3 import ( "bytes" "encoding/json" ) -// sentinel value -const deletedAvatar = "" - // An AvatarChange represents a change to a room's avatar. There are three cases: -// - an empty string represents no change, and should be omitted when JSON-serisalised; -// - the sentinel `avatarNotPresent` represents a room that has never had an avatar, +// - an empty string represents no change, and should be omitted when JSON-serialised; +// - the sentinel `` represents a room that has never had an avatar, // or a room whose avatar has been removed. It is JSON-serialised as null. // - All other strings represent the current avatar of the room and JSON-serialise as // normal. type AvatarChange string +const DeletedAvatar = AvatarChange("") +const UnchangedAvatar AvatarChange = "" + +// NewAvatarChange interprets an optional avatar string as an AvatarChange. +func NewAvatarChange(avatar string) AvatarChange { + if avatar == "" { + return DeletedAvatar + } + return AvatarChange(avatar) +} + func (a AvatarChange) MarshalJSON() ([]byte, error) { - if a == deletedAvatar { + if a == DeletedAvatar { return []byte(`null`), nil } else { return json.Marshal(string(a)) @@ -27,11 +35,8 @@ func (a AvatarChange) MarshalJSON() ([]byte, error) { // Note: the unmarshalling is only used in tests. func (a *AvatarChange) UnmarshalJSON(data []byte) error { if bytes.Equal(data, []byte("null")) { - *a = deletedAvatar + *a = DeletedAvatar return nil } return json.Unmarshal(data, (*string)(a)) } - -var DeletedAvatar = AvatarChange(deletedAvatar) -var UnchangedAvatar AvatarChange diff --git a/sync3/caches/user.go b/sync3/caches/user.go index ba5c3532..9d6b4e6e 100644 --- a/sync3/caches/user.go +++ b/sync3/caches/user.go @@ -52,10 +52,11 @@ type UserRoomData struct { // as the set of spaces may be different for different users. // ResolvedAvatarURL is the avatar that should be displayed to this user to - // represent this room. Avatars set in m.room.avatar take precedence; if this is - // missing and the room is a DM with one other user, we fall back to that user's + // represent this room. The empty string means that this room has no avatar. + // Avatars set in m.room.avatar take precedence; if this is missing and the room is + // a DM with one other user joined or invited, we fall back to that user's // avatar (if any) as specified in their membership event in that room. - ResolvedAvatarURL internal.AvatarChange + ResolvedAvatarURL string Spaces map[string]struct{} // Map of tag to order float. diff --git a/sync3/handler/connstate.go b/sync3/handler/connstate.go index a1ebdc2d..838c427c 100644 --- a/sync3/handler/connstate.go +++ b/sync3/handler/connstate.go @@ -596,7 +596,7 @@ func (s *ConnState) getInitialRoomData(ctx context.Context, roomSub sync3.RoomSu } rooms[roomID] = sync3.Room{ Name: internal.CalculateRoomName(metadata, 5), // TODO: customisable? - AvatarChange: internal.CalculateAvatar(metadata), + AvatarChange: sync3.NewAvatarChange(internal.CalculateAvatar(metadata)), NotificationCount: int64(userRoomData.NotificationCount), HighlightCount: int64(userRoomData.HighlightCount), Timeline: roomToTimeline[roomID], diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go index 1b1b23c1..817560b3 100644 --- a/sync3/handler/connstate_live.go +++ b/sync3/handler/connstate_live.go @@ -221,7 +221,7 @@ func (s *connStateLive) processLiveUpdate(ctx context.Context, up caches.Update, if delta.RoomAvatarChanged { metadata := roomUpdate.GlobalRoomMetadata() metadata.RemoveHero(s.userID) - thisRoom.AvatarChange = internal.CalculateAvatar(metadata) + thisRoom.AvatarChange = sync3.NewAvatarChange(internal.CalculateAvatar(metadata)) } if delta.InviteCountChanged { thisRoom.InvitedCount = &roomUpdate.GlobalRoomMetadata().InviteCount diff --git a/sync3/room.go b/sync3/room.go index 159dcef7..3a5c24e2 100644 --- a/sync3/room.go +++ b/sync3/room.go @@ -8,19 +8,19 @@ import ( ) type Room struct { - Name string `json:"name,omitempty"` - AvatarChange internal.AvatarChange `json:"avatar,omitempty"` - RequiredState []json.RawMessage `json:"required_state,omitempty"` - Timeline []json.RawMessage `json:"timeline,omitempty"` - InviteState []json.RawMessage `json:"invite_state,omitempty"` - NotificationCount int64 `json:"notification_count"` - HighlightCount int64 `json:"highlight_count"` - Initial bool `json:"initial,omitempty"` - IsDM bool `json:"is_dm,omitempty"` - JoinedCount int `json:"joined_count,omitempty"` - InvitedCount *int `json:"invited_count,omitempty"` - PrevBatch string `json:"prev_batch,omitempty"` - NumLive int `json:"num_live,omitempty"` + Name string `json:"name,omitempty"` + AvatarChange AvatarChange `json:"avatar,omitempty"` + RequiredState []json.RawMessage `json:"required_state,omitempty"` + Timeline []json.RawMessage `json:"timeline,omitempty"` + InviteState []json.RawMessage `json:"invite_state,omitempty"` + NotificationCount int64 `json:"notification_count"` + HighlightCount int64 `json:"highlight_count"` + Initial bool `json:"initial,omitempty"` + IsDM bool `json:"is_dm,omitempty"` + JoinedCount int `json:"joined_count,omitempty"` + InvitedCount *int `json:"invited_count,omitempty"` + PrevBatch string `json:"prev_batch,omitempty"` + NumLive int `json:"num_live,omitempty"` } // RoomConnMetadata represents a room as seen by one specific connection (hence one diff --git a/sync3/room_test.go b/sync3/room_test.go index c8a00fab..09ec058a 100644 --- a/sync3/room_test.go +++ b/sync3/room_test.go @@ -3,7 +3,6 @@ package sync3 import ( "encoding/json" "fmt" - "github.com/matrix-org/sliding-sync/internal" "github.com/tidwall/gjson" "reflect" "testing" @@ -13,12 +12,12 @@ func TestAvatarChangeMarshalling(t *testing.T) { var url = "mxc://..." testCases := []struct { Name string - AvatarChange internal.AvatarChange + AvatarChange AvatarChange Check func(avatar gjson.Result) error }{ { Name: "Avatar exists", - AvatarChange: internal.AvatarChange(url), + AvatarChange: NewAvatarChange(url), Check: func(avatar gjson.Result) error { if !(avatar.Exists() && avatar.Type == gjson.String && avatar.Str == url) { return fmt.Errorf("unexpected marshalled avatar: got %#v want %s", avatar, url) @@ -28,7 +27,7 @@ func TestAvatarChangeMarshalling(t *testing.T) { }, { Name: "Avatar doesn't exist", - AvatarChange: internal.DeletedAvatar, + AvatarChange: DeletedAvatar, Check: func(avatar gjson.Result) error { if !(avatar.Exists() && avatar.Type == gjson.Null) { return fmt.Errorf("unexpected marshalled Avatar: got %#v want null", avatar) @@ -38,7 +37,7 @@ func TestAvatarChangeMarshalling(t *testing.T) { }, { Name: "Avatar unchanged", - AvatarChange: internal.UnchangedAvatar, + AvatarChange: UnchangedAvatar, Check: func(avatar gjson.Result) error { if avatar.Exists() { return fmt.Errorf("unexpected marshalled Avatar: got %#v want omitted", avatar) diff --git a/testutils/m/match.go b/testutils/m/match.go index cc662c35..001135bc 100644 --- a/testutils/m/match.go +++ b/testutils/m/match.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/json" "fmt" - "github.com/matrix-org/sliding-sync/internal" "reflect" "sort" "testing" @@ -55,8 +54,8 @@ func MatchRoomAvatar(wantAvatar string) RoomMatcher { // avatar, or has had its avatar deleted. func MatchRoomUnsetAvatar() RoomMatcher { return func(r sync3.Room) error { - if r.AvatarChange != internal.DeletedAvatar { - return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, internal.DeletedAvatar) + if r.AvatarChange != sync3.DeletedAvatar { + return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, sync3.DeletedAvatar) } return nil } @@ -66,8 +65,8 @@ func MatchRoomUnsetAvatar() RoomMatcher { // change to its avatar, or has had its avatar deleted. func MatchRoomUnchangedAvatar() RoomMatcher { return func(r sync3.Room) error { - if r.AvatarChange != internal.UnchangedAvatar { - return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, internal.UnchangedAvatar) + if r.AvatarChange != sync3.UnchangedAvatar { + return fmt.Errorf("MatchRoomAvatar: got \"%s\" want \"%s\"", r.AvatarChange, sync3.UnchangedAvatar) } return nil } From 7cf1d23ced23342bf31612bf4a72a9773c3c2a41 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 17:43:09 +0100 Subject: [PATCH 25/30] SlidingSyncUntilMembership: ignore displayname and avatar --- tests-e2e/client_test.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/tests-e2e/client_test.go b/tests-e2e/client_test.go index 641ff999..38beca4f 100644 --- a/tests-e2e/client_test.go +++ b/tests-e2e/client_test.go @@ -687,16 +687,32 @@ func (c *CSAPI) SlidingSyncUntilMembership(t *testing.T, pos string, roomID stri }) } - return c.SlidingSyncUntilEvent(t, pos, sync3.Request{ + return c.SlidingSyncUntil(t, pos, sync3.Request{ RoomSubscriptions: map[string]sync3.RoomSubscription{ roomID: { TimelineLimit: 10, }, }, - }, roomID, Event{ - Type: "m.room.member", - StateKey: &target.UserID, - Content: content, + }, func(r *sync3.Response) error { + room, ok := r.Rooms[roomID] + if !ok { + return fmt.Errorf("missing room %s", roomID) + } + for _, got := range room.Timeline { + wantEvent := Event{ + Type: "m.room.member", + StateKey: &target.UserID, + } + if err := eventsEqual([]Event{wantEvent}, []json.RawMessage{got}); err == nil { + gotMembership := gjson.GetBytes(got, "content.membership") + if gotMembership.Exists() && gotMembership.Type == gjson.String && gotMembership.Str == membership { + return nil + } + } else { + t.Log(err) + } + } + return fmt.Errorf("found room %s but missing event", roomID) }) } From 49f96703bceb15a86dae4295c28a6ea324a5f956 Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 18:21:26 +0100 Subject: [PATCH 26/30] Extra test --- tests-e2e/lists_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 9ca6be2f..595e15ea 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1378,6 +1378,27 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar()), ) + t.Run("Avatar not resent on message", func(t *testing.T) { + t.Log("Bob sends a sentinel message.") + sentinel := bob.SendEventSynced(t, dmBob, Event{ + Type: "m.room.message", + Content: map[string]interface{}{ + "body": "Hello world", + "msgtype": "m.text", + }, + }) + + t.Log("Alice syncs until she sees the sentinel. She should not see the DM avatar change.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error { + matchNoAvatarChange := m.MatchRoomSubscription(dmBob, m.MatchRoomUnchangedAvatar()) + if err := matchNoAvatarChange(response); err != nil { + t.Fatalf("Saw DM avatar change: %s", err) + } + matchSentinel := m.MatchRoomSubscription(dmBob, MatchRoomTimelineMostRecent(1, []Event{{ID: sentinel}})) + return matchSentinel(response) + }) + }) + t.Run("DM declined", func(t *testing.T) { t.Log("Chris leaves his DM with Alice.") chris.LeaveRoom(t, dmChris) From 9e99476d5a9cb1555d8fbbc18f96978d316a706a Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 18:21:32 +0100 Subject: [PATCH 27/30] Fixup propagation tests --- tests-e2e/lists_test.go | 183 ++++++++++++++++++++-------------------- 1 file changed, 92 insertions(+), 91 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 595e15ea..1417d2be 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1493,112 +1493,113 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { ) }) - /* - - t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { - t.Log("Alice sets an avatar for the public room.") - publicAvatar := uploadAvatar(alice, "public.png") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ - "url": publicAvatar, - }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar(publicAvatar)}, - }), - ) - - t.Log("Alice changes the avatar for the public room.") - publicAvatar2 := uploadAvatar(alice, "public2.png") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ - "url": publicAvatar2, - }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomAvatar(publicAvatar2)}, - }), - ) - - t.Log("Alice removes the avatar for the public room.") - alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{}) - t.Log("Alice syncs until she sees that avatar vanish.") - res = alice.SlidingSyncUntil( - t, - res.Pos, - sync3.Request{}, - m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ - public: {m.MatchRoomUnsetAvatar()}, - }), - ) + + t.Run("Explicit avatar propagates in non-DM room", func(t *testing.T) { + t.Log("Alice sets an avatar for the public room.") + publicAvatar := uploadAvatar(alice, "public.png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar, }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar)}, + }), + ) - t.Run("Explicit avatar propagates in DM room", func(t *testing.T) { - t.Log("Alice re-invites Chris to their DM. He accepts.") - alice.InviteRoom(t, dmChris, chris.UserID) - chris.JoinRoom(t, dmChris, nil) + t.Log("Alice changes the avatar for the public room.") + publicAvatar2 := uploadAvatar(alice, "public2.png") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{ + "url": publicAvatar2, + }) + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomAvatar(publicAvatar2)}, + }), + ) - t.Log("Alice syncs until she sees Chris's join.") - res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") + t.Log("Alice removes the avatar for the public room.") + alice.SetState(t, public, "m.room.avatar", "", map[string]interface{}{}) + t.Log("Alice syncs until she sees that avatar vanish.") + res = alice.SlidingSyncUntil( + t, + res.Pos, + sync3.Request{}, + m.MatchRoomSubscriptions(map[string][]m.RoomMatcher{ + public: {m.MatchRoomUnsetAvatar()}, + }), + ) + }) - t.Log("Alice should see the DM with Chris's avatar.") - m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) + t.Run("Explicit avatar propagates in DM room", func(t *testing.T) { + t.Log("Alice re-invites Chris to their DM.") + alice.InviteRoom(t, dmChris, chris.UserID) - t.Log("Chris gives their DM a bespoke avatar.") - dmAvatar := uploadAvatar(chris, "dm.png") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ - "url": dmAvatar, - }) + t.Log("Alice syncs until she sees her invitation to Chris.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "invite") - t.Log("Alice syncs until she sees that avatar.") - alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) + t.Log("Alice should see the DM with Chris's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) - t.Log("Chris changes his global avatar.") - chrisAvatar2 := uploadAvatar(chris, "chris2.png") - chris.SetAvatar(t, chrisAvatar2) + t.Log("Chris joins the room.") + chris.JoinRoom(t, dmChris, nil) - t.Log("Chris sends a sentinel message.") - sentinel := chris.SendEventSynced(t, dmBobChris, Event{ - Type: "m.room.message", - Content: map[string]interface{}{ - "body": "Hello world", - "msgtype": "m.text", - }, - }) + t.Log("Alice syncs until she sees Chris's join.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") - t.Log("Alice syncs until she sees the sentinel. She should not see the DM avatar change.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, func(response *sync3.Response) error { - matchNoAvatarChange := m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar("")) - if err := matchNoAvatarChange(response); err != nil { - t.Errorf("Saw DM avatar change: %s", err) - } - matchSentinel := m.MatchRoomSubscription(dmChris, MatchRoomTimelineMostRecent(1, []Event{{ID: sentinel}})) - return matchSentinel(response) - }) + t.Log("Alice shouldn't see the DM's avatar change..") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomUnchangedAvatar())) - t.Log("Chris updates the DM's avatar.") - dmAvatar2 := uploadAvatar(chris, "dm2.png") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ - "url": dmAvatar2, - }) + t.Log("Chris gives their DM a bespoke avatar.") + dmAvatar := uploadAvatar(chris, "dm.png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar, + }) - t.Log("Alice syncs until she sees that avatar.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar2))) + t.Log("Alice syncs until she sees that avatar.") + alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar))) - t.Log("Chris removes the DM's avatar.") - chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) + t.Log("Chris changes his global avatar, which adds a join event to the room.") + chrisAvatar2 := uploadAvatar(chris, "chris2.png") + chris.SetAvatar(t, chrisAvatar2) - t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) + t.Log("Alice syncs until she sees that join event.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "join") + + t.Log("Her response should have either no avatar change, or the same bespoke avatar.") + // No change, ideally, but repeating the same avatar isn't _wrong_ + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, func(r sync3.Room) error { + noChangeErr := m.MatchRoomUnchangedAvatar()(r) + sameBespokeAvatarErr := m.MatchRoomAvatar(dmAvatar)(r) + if noChangeErr == nil || sameBespokeAvatarErr == nil { + return nil + } + return fmt.Errorf("expected no change or the same bespoke avatar (%s), got '%s'", dmAvatar, r.AvatarChange) + })) + + t.Log("Chris updates the DM's avatar.") + dmAvatar2 := uploadAvatar(chris, "dm2.png") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{ + "url": dmAvatar2, }) - */ + t.Log("Alice syncs until she sees that avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(dmAvatar2))) + + t.Log("Chris removes the DM's avatar.") + chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) + + t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) + }) + // TODO unset DM's avatar, then set a custom one in that room. // TODO when you're invited, you see the avatar // TODO make a DM not a DM, loses its avatar; remake it a DM and it gains one. From 432397a3da156ad2c3b370c6ff2668246d19904c Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 18:48:58 +0100 Subject: [PATCH 28/30] new test --- tests-e2e/client_test.go | 2 ++ tests-e2e/lists_test.go | 71 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 71 insertions(+), 2 deletions(-) diff --git a/tests-e2e/client_test.go b/tests-e2e/client_test.go index 38beca4f..a6982437 100644 --- a/tests-e2e/client_test.go +++ b/tests-e2e/client_test.go @@ -134,6 +134,7 @@ type CSAPI struct { Localpart string AccessToken string DeviceID string + AvatarURL string BaseURL string Client *http.Client // how long are we willing to wait for MustSyncUntil.... calls @@ -166,6 +167,7 @@ func (c *CSAPI) SetAvatar(t *testing.T, avatarURL string) { "avatar_url": avatarURL, } c.MustDoFunc(t, "PUT", []string{"_matrix", "client", "v3", "profile", c.UserID, "avatar_url"}, WithJSONBody(t, reqBody)) + c.AvatarURL = avatarURL } // DownloadContent downloads media from the server, returning the raw bytes and the Content-Type. Fails the test on error. diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index 1417d2be..c6f42e83 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1600,7 +1600,74 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) }) - // TODO unset DM's avatar, then set a custom one in that room. + t.Run("Changing DM flag", func(t *testing.T) { + t.Log("Alice clears the DM flag on Bob's room.") + alice.SetGlobalAccountData(t, "m.direct", map[string]interface{}{ + "content": map[string][]string{ + bob.UserID: {}, // no dmBob here + chris.UserID: {dmChris, dmBobChris}, + }, + }) + + t.Log("Alice syncs until she sees a new set of account data.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{ + Extensions: extensions.Request{ + AccountData: &extensions.AccountDataRequest{ + extensions.Core{Enabled: &boolTrue}, + }, + }, + }, func(response *sync3.Response) error { + if response.Extensions.AccountData == nil { + return fmt.Errorf("no account data yet") + } + if len(response.Extensions.AccountData.Global) == 0 { + return fmt.Errorf("no global account data yet") + } + return nil + }) + + t.Log("The DM with Bob should no longer be a DM and should no longer have an avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmBob, func(r sync3.Room) error { + if r.IsDM { + return fmt.Errorf("dmBob is still a DM") + } + return m.MatchRoomUnsetAvatar()(r) + })) + + t.Log("Alice sets the DM flag on Bob's room.") + alice.SetGlobalAccountData(t, "m.direct", map[string]interface{}{ + "content": map[string][]string{ + bob.UserID: {dmBob}, // dmBob reinstated + chris.UserID: {dmChris, dmBobChris}, + }, + }) + + t.Log("Alice syncs until she sees a new set of account data.") + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{ + Extensions: extensions.Request{ + AccountData: &extensions.AccountDataRequest{ + extensions.Core{Enabled: &boolTrue}, + }, + }, + }, func(response *sync3.Response) error { + if response.Extensions.AccountData == nil { + return fmt.Errorf("no account data yet") + } + if len(response.Extensions.AccountData.Global) == 0 { + return fmt.Errorf("no global account data yet") + } + return nil + }) + + t.Log("The room should have Bob's avatar again.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmBob, func(r sync3.Room) error { + if !r.IsDM { + return fmt.Errorf("dmBob is still not a DM") + } + return m.MatchRoomAvatar(bob.AvatarURL)(r) + })) + + }) + // TODO when you're invited, you see the avatar - // TODO make a DM not a DM, loses its avatar; remake it a DM and it gains one. } From 10bc83465834550449b257365be7455d6eccb33d Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 19:14:11 +0100 Subject: [PATCH 29/30] TODO note --- sync3/handler/connstate_live.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sync3/handler/connstate_live.go b/sync3/handler/connstate_live.go index 817560b3..d5b0975d 100644 --- a/sync3/handler/connstate_live.go +++ b/sync3/handler/connstate_live.go @@ -297,6 +297,13 @@ func (s *connStateLive) processGlobalUpdates(ctx context.Context, builder *Rooms metadata := rup.GlobalRoomMetadata().CopyHeroes() metadata.RemoveHero(s.userID) + // TODO: if we change a room from being a DM to not being a DM, we should call + // SetRoom and recalculate avatars. To do that we'd need to + // - listen to m.direct global account data events + // - compute the symmetric difference between old and new + // - call SetRooms for each room in the difference. + // I'm assuming this happens so rarely that we can ignore this for now. PRs + // welcome if you a strong opinion to the contrary. delta = s.lists.SetRoom(sync3.RoomConnMetadata{ RoomMetadata: *metadata, UserRoomData: *rup.UserRoomMetadata(), From 4fb80da8bbd2933dd3f0a76f1f50501ad92ffaac Mon Sep 17 00:00:00 2001 From: David Robertson Date: Mon, 17 Jul 2023 19:14:14 +0100 Subject: [PATCH 30/30] test updates --- tests-e2e/lists_test.go | 34 ++++++++++++++++++++++++---------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/tests-e2e/lists_test.go b/tests-e2e/lists_test.go index c6f42e83..11dd97c3 100644 --- a/tests-e2e/lists_test.go +++ b/tests-e2e/lists_test.go @@ -1373,8 +1373,8 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t, res, m.MatchRoomSubscription(public, m.MatchRoomUnsetAvatar()), - m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar)), - m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar)), + m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bob.AvatarURL)), + m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chris.AvatarURL)), m.MatchRoomSubscription(dmBobChris, m.MatchRoomUnsetAvatar()), ) @@ -1419,7 +1419,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { t.Log("Alice sees the room's avatar change to Bob's avatar.") // Because this is now a DM room with exactly one other (joined|invited) member. - m.MatchResponse(t, res, m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar))) + m.MatchResponse(t, res, m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bob.AvatarURL))) }) t.Run("Bob's avatar change propagates", func(t *testing.T) { @@ -1436,14 +1436,14 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { sync3.Request{}, func(response *sync3.Response) error { if !avatarChangeInDM { - err := m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bobAvatar2))(response) + err := m.MatchRoomSubscription(dmBob, m.MatchRoomAvatar(bob.AvatarURL))(response) if err == nil { avatarChangeInDM = true } } if !avatarChangeInGroupDM { - err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bobAvatar2))(response) + err := m.MatchRoomSubscription(dmBobChris, m.MatchRoomAvatar(bob.AvatarURL))(response) if err == nil { avatarChangeInGroupDM = true } @@ -1452,7 +1452,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { if avatarChangeInDM && avatarChangeInGroupDM { return nil } - return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + return fmt.Errorf("still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) }, ) @@ -1488,7 +1488,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { if avatarChangeInDM && avatarChangeInGroupDM { return nil } - return fmt.Errorf("Still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) + return fmt.Errorf("still waiting: avatarChangeInDM=%t avatarChangeInGroupDM=%t", avatarChangeInDM, avatarChangeInGroupDM) }, ) @@ -1546,7 +1546,7 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { res = alice.SlidingSyncUntilMembership(t, res.Pos, dmChris, chris, "invite") t.Log("Alice should see the DM with Chris's avatar.") - m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar))) + m.MatchResponse(t, res, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chris.AvatarURL))) t.Log("Chris joins the room.") chris.JoinRoom(t, dmChris, nil) @@ -1597,10 +1597,11 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { chris.SetState(t, dmChris, "m.room.avatar", "", map[string]interface{}{}) t.Log("Alice syncs until the DM avatar returns to Chris's most recent avatar.") - res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chrisAvatar2))) + res = alice.SlidingSyncUntil(t, res.Pos, sync3.Request{}, m.MatchRoomSubscription(dmChris, m.MatchRoomAvatar(chris.AvatarURL))) }) t.Run("Changing DM flag", func(t *testing.T) { + t.Skip("TODO: unimplemented") t.Log("Alice clears the DM flag on Bob's room.") alice.SetGlobalAccountData(t, "m.direct", map[string]interface{}{ "content": map[string][]string{ @@ -1669,5 +1670,18 @@ func TestAvatarFieldInRoomResponse(t *testing.T) { }) - // TODO when you're invited, you see the avatar + t.Run("See avatar when invited", func(t *testing.T) { + t.Log("Chris invites Alice to a DM.") + dmInvited := chris.CreateRoom(t, map[string]interface{}{ + "preset": "trusted_private_chat", + "is_direct": true, + "invite": []string{alice.UserID}, + }) + + t.Log("Alice syncs until she sees the invite.") + res = alice.SlidingSyncUntilMembership(t, res.Pos, dmInvited, alice, "invite") + + t.Log("The new room should use Chris's avatar.") + m.MatchResponse(t, res, m.MatchRoomSubscription(dmInvited, m.MatchRoomAvatar(chris.AvatarURL))) + }) }