Merge pull request #1854 from famedly/nico/fix-requestUser-update-spam

fix: Request user causing state update loops for apps

lib/fake_matrix_api.dart

@@ -168,8 +168,9 @@ class FakeMatrixApi extends BaseClient {
     } else if (method == 'GET' &&
         action.contains('/client/v3/rooms/') &&
         action.contains('/state/m.room.member/') &&
-        !action.endsWith('%40alicyy%3Aexample.com')) {
+        !action.endsWith('%40alicyy%3Aexample.com') &&
-      res = {'displayname': ''};
+        !action.contains('%40getme')) {
+      res = {'displayname': '', 'membership': 'ban'};
     } else if (method == 'PUT' &&
         action.contains(
             '/client/v3/rooms/!1234%3AfakeServer.notExisting/send/')) {
@@ -1522,15 +1523,28 @@ class FakeMatrixApi extends BaseClient {
           {'visibility': 'public'},
       '/client/v3/rooms/1/state/m.room.member/@alice:example.com': (var req) =>
           {'displayname': 'Alice'},
+      '/client/v3/profile/%40getmeprofile%3Aexample.com': (var req) => {
+            'avatar_url': 'mxc://test',
+            'displayname': 'You got me (profile)',
+          },
       '/client/v3/profile/%40getme%3Aexample.com': (var req) => {
             'avatar_url': 'mxc://test',
             'displayname': 'You got me',
           },
-      '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/@getme%3Aexample.com':
+      '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/%40getme%3Aexample.com':
           (var req) => {
                 'avatar_url': 'mxc://test',
                 'displayname': 'You got me',
+                'membership': 'knock',
               },
+      '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/%40getmeempty%3Aexample.com':
+          (var req) => {
+                'membership': 'leave',
+              },
+      '/client/v3/profile/%40getmeempty%3Aexample.com': (var req) => {
+            'avatar_url': 'mxc://test',
+            'displayname': 'You got me (empty)',
+          },
       '/client/v3/rooms/!localpart%3Aserver.abc/state': (var req) => [
             {
               'content': {'join_rule': 'public'},
@@ -1596,11 +1610,6 @@ class FakeMatrixApi extends BaseClient {
               'state_key': ''
             }
           ],
-      '/client/v3/rooms/!localpart:server.abc/state/m.room.member/@getme:example.com':
-          (var req) => {
-                'avatar_url': 'mxc://test',
-                'displayname': 'You got me',
-              },
       '/client/v3/rooms/!localpart:server.abc/event/1234': (var req) => {
             'content': {
               'body': 'This is an example text message',

lib/src/room.dart

@@ -18,7 +18,9 @@
 
 import 'dart:async';
 import 'dart:convert';
+import 'dart:math';
 
+import 'package:async/async.dart';
 import 'package:collection/collection.dart';
 import 'package:html_unescape/html_unescape.dart';
 
@@ -1648,100 +1650,169 @@ class Room {
     }
   }
 
-  final Set<String> _requestingMatrixIds = {};
+  // Internal helper to implement requestUser
+  Future<User?> _requestSingleParticipantViaState(
+    String mxID, {
+    required bool ignoreErrors,
+  }) async {
+    try {
+      Logs().v(
+        'Request missing user $mxID in room ${getLocalizedDisplayname()} from the server...',
+      );
+      final resp = await client.getRoomStateWithKey(
+        id,
+        EventTypes.RoomMember,
+        mxID,
+      );
+
+      // valid member events require a valid membership key
+      final membership = resp.tryGet<String>('membership', TryGet.required);
+      assert(membership != null);
+
+      final foundUser = User(
+        mxID,
+        room: this,
+        displayName: resp.tryGet<String>('displayname', TryGet.silent),
+        avatarUrl: resp.tryGet<String>('avatar_url', TryGet.silent),
+        membership: membership,
+      );
+
+      // Store user in database:
+      await client.database?.transaction(() async {
+        await client.database?.storeEventUpdate(
+          EventUpdate(
+            content: foundUser.toJson(),
+            roomID: id,
+            type: EventUpdateType.state,
+          ),
+          client,
+        );
+      });
+
+      return foundUser;
+    } on MatrixException catch (_) {
+      // Ignore if we have no permission
+      return null;
+    } catch (e, s) {
+      if (!ignoreErrors) {
+        rethrow;
+      } else {
+        Logs().w('Unable to request the user $mxID from the server', e, s);
+        return null;
+      }
+    }
+  }
+
+  // Internal helper to implement requestUser
+  Future<User?> _requestUser(
+    String mxID, {
+    required bool ignoreErrors,
+    required bool requestState,
+    required bool requestProfile,
+  }) async {
+    // Is user already in cache?
+    final userFromState = getState(EventTypes.RoomMember, mxID)?.asUser(this);
+
+    // If not in cache, try the database
+    var foundUser = userFromState;
+
+    // If the room is not postloaded, check the database
+    if (partial && foundUser == null) {
+      foundUser = await client.database?.getUser(mxID, this);
+    }
+
+    // If not in the database, try fetching the member from the server
+    if (requestState && foundUser == null) {
+      foundUser = await _requestSingleParticipantViaState(mxID,
+          ignoreErrors: ignoreErrors);
+    }
+
+    // If the user isn't found or they have left and no displayname set anymore, request their profile from the server
+    if (requestProfile) {
+      if (foundUser
+          case null ||
+              User(
+                membership: Membership.ban || Membership.leave,
+                displayName: null
+              )) {
+        try {
+          final profile = await client.getProfileFromUserId(mxID);
+          foundUser = User(
+            mxID,
+            displayName: profile.displayName,
+            avatarUrl: profile.avatarUrl?.toString(),
+            membership: foundUser?.membership.name ?? Membership.leave.name,
+            room: this,
+          );
+        } catch (e, s) {
+          if (!ignoreErrors) {
+            rethrow;
+          } else {
+            Logs()
+                .w('Unable to request the profile $mxID from the server', e, s);
+          }
+        }
+      }
+    }
+
+    if (foundUser == null) return null;
+
+    // Set user in the local state if the state changed.
+    // If we set the state unconditionally, we might end up with a client calling this over and over thinking the user changed.
+    if (userFromState == null ||
+        userFromState.displayName != foundUser.displayName) {
+      setState(foundUser);
+      // ignore: deprecated_member_use_from_same_package
+      onUpdate.add(id);
+    }
+
+    return foundUser;
+  }
+
+  final Map<
+      ({
+        String mxID,
+        bool ignoreErrors,
+        bool requestState,
+        bool requestProfile,
+      }),
+      AsyncCache<User?>> _inflightUserRequests = {};
 
   /// Requests a missing [User] for this room. Important for clients using
   /// lazy loading. If the user can't be found this method tries to fetch
-  /// the displayname and avatar from the profile if [requestProfile] is true.
+  /// the displayname and avatar from the server if [requestState] is true.
+  /// If that fails, it falls back to requesting the global profile if
+  /// [requestProfile] is true.
   Future<User?> requestUser(
     String mxID, {
     bool ignoreErrors = false,
+    bool requestState = true,
     bool requestProfile = true,
   }) async {
     assert(mxID.isValidMatrixId);
 
-    // Is user already in cache?
+    final parameters = (
-    var foundUser = getState(EventTypes.RoomMember, mxID)?.asUser(this);
+      mxID: mxID,
+      ignoreErrors: ignoreErrors,
+      requestState: requestState,
+      requestProfile: requestProfile,
+    );
 
-    // If not, is it in the database?
+    final cache = _inflightUserRequests[parameters] ??= AsyncCache.ephemeral();
-    foundUser ??= await client.database?.getUser(mxID, this);
 
-    // If not, can we request it from the server?
+    try {
-    if (foundUser == null) {
+      final user = await cache.fetch(() => _requestUser(
-      if (!_requestingMatrixIds.add(mxID)) return null;
+            mxID,
-      Map<String, dynamic>? resp;
+            ignoreErrors: ignoreErrors,
-      try {
+            requestState: requestState,
-        Logs().v(
+            requestProfile: requestProfile,
-            'Request missing user $mxID in room ${getLocalizedDisplayname()} from the server...');
+          ));
-        resp = await client.getRoomStateWithKey(
+      _inflightUserRequests.remove(parameters);
-          id,
+      return user;
-          EventTypes.RoomMember,
+    } catch (_) {
-          mxID,
+      _inflightUserRequests.remove(parameters);
-        );
+      rethrow;
-        foundUser = User(
-          mxID,
-          room: this,
-          displayName: resp['displayname'],
-          avatarUrl: resp['avatar_url'],
-          membership: resp['membership'],
-        );
-        _requestingMatrixIds.remove(mxID);
-
-        // Store user in database:
-        await client.database?.transaction(() async {
-          await client.database?.storeEventUpdate(
-            EventUpdate(
-              content: foundUser!.toJson(),
-              roomID: id,
-              type: EventUpdateType.state,
-            ),
-            client,
-          );
-        });
-      } on MatrixException catch (_) {
-        // Ignore if we have no permission
-      } catch (e, s) {
-        if (!ignoreErrors) {
-          _requestingMatrixIds.remove(mxID);
-          rethrow;
-        } else {
-          Logs().w('Unable to request the user $mxID from the server', e, s);
-        }
-      }
     }
-
-    // User not found anywhere? Set a blank one:
-    foundUser ??= User(mxID, room: this, membership: 'leave');
-
-    // Is it a left user without any displayname/avatar info? Try fetch profile:
-    if (requestProfile &&
-        {Membership.ban, Membership.leave}.contains(foundUser.membership) &&
-        foundUser.displayName == null &&
-        foundUser.avatarUrl == null) {
-      try {
-        final profile = await client.getProfileFromUserId(mxID);
-        foundUser = User(
-          mxID,
-          displayName: profile.displayName,
-          avatarUrl: profile.avatarUrl?.toString(),
-          membership: Membership.leave.name,
-          room: this,
-        );
-      } catch (e, s) {
-        if (!ignoreErrors) {
-          rethrow;
-        } else {
-          Logs().w('Unable to request the profile $mxID from the server', e, s);
-        }
-      }
-    }
-
-    // Set user in the local state
-    setState(foundUser!);
-    // ignore: deprecated_member_use_from_same_package
-    onUpdate.add(id);
-
-    return foundUser;
   }
 
   /// Searches for the event in the local cache and then on the server if not
@@ -2272,7 +2343,7 @@ class Room {
           'https://matrix.to/#/${Uri.encodeComponent(canonicalAlias)}');
     }
     final List queryParameters = [];
-    final users = await requestParticipants();
+    final users = await requestParticipants([Membership.join]);
     final currentPowerLevelsMap = getState(EventTypes.RoomPowerLevels)?.content;
 
     final temp = List<User>.from(users);
@@ -2301,18 +2372,17 @@ class Room {
       }
     }
     final sortedServers = Map.fromEntries(servers.entries.toList()
-      ..sort((e1, e2) => e1.value.compareTo(e2.value)));
+          ..sort((e1, e2) => e2.value.compareTo(e1.value)))
-    for (var i = 0; i <= 2; i++) {
+        .keys
-      if (!queryParameters.contains(sortedServers.keys.last)) {
+        .take(3);
-        queryParameters.add(sortedServers.keys.last);
+    for (final server in sortedServers) {
+      if (!queryParameters.contains(server)) {
+        queryParameters.add(server);
       }
-      sortedServers.remove(sortedServers.keys.last);
     }
 
     var queryString = '?';
-    for (var i = 0;
+    for (var i = 0; i < min(queryParameters.length, 3); i++) {
-        i <= (queryParameters.length > 2 ? 2 : queryParameters.length);
-        i++) {
       if (i != 0) {
         queryString += '&';
       }

test/room_test.dart

@@ -682,12 +682,83 @@ void main() {
     });
 
     test('getUserByMXID', () async {
-      User? user;
+      final List<String> called = [];
-      try {
+      final List<String> called2 = [];
-        user = await room.requestUser('@getme:example.com');
+      // ignore: deprecated_member_use_from_same_package
-      } catch (_) {}
+      final subscription = room.onUpdate.stream.listen((i) {
+        called.add(i);
+      });
+      final subscription2 = room.client.onRoomState.stream.listen((i) {
+        called2.add(i.roomId);
+      });
+
+      FakeMatrixApi.calledEndpoints.clear();
+      final user = await room.requestUser('@getme:example.com');
+      expect(FakeMatrixApi.calledEndpoints.keys, [
+        '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/%40getme%3Aexample.com'
+      ]);
       expect(user?.stateKey, '@getme:example.com');
-      expect(user?.calcDisplayname(), 'Getme');
+      expect(user?.calcDisplayname(), 'You got me');
+      expect(user?.membership, Membership.knock);
+
+      // Yield for the onUpdate
+      await Future.delayed(Duration(
+        milliseconds: 1,
+      ));
+      expect(called.length, 1);
+      expect(called2.length, 1);
+
+      FakeMatrixApi.calledEndpoints.clear();
+      final user2 = await room.requestUser('@getmeprofile:example.com');
+      expect(FakeMatrixApi.calledEndpoints.keys, [
+        '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/%40getmeprofile%3Aexample.com',
+        '/client/v3/profile/%40getmeprofile%3Aexample.com'
+      ]);
+      expect(user2?.stateKey, '@getmeprofile:example.com');
+      expect(user2?.calcDisplayname(), 'You got me (profile)');
+      expect(user2?.membership, Membership.leave);
+
+      // Yield for the onUpdate
+      await Future.delayed(Duration(
+        milliseconds: 1,
+      ));
+      expect(called.length, 2);
+      expect(called2.length, 2);
+
+      FakeMatrixApi.calledEndpoints.clear();
+      final userAgain = await room.requestUser('@getme:example.com');
+      expect(FakeMatrixApi.calledEndpoints.keys, []);
+      expect(userAgain?.stateKey, '@getme:example.com');
+      expect(userAgain?.calcDisplayname(), 'You got me');
+      expect(userAgain?.membership, Membership.knock);
+
+      // Yield for the onUpdate
+      await Future.delayed(Duration(
+        milliseconds: 1,
+      ));
+      expect(called.length, 2, reason: 'onUpdate should not have been called.');
+      expect(called2.length, 2,
+          reason: 'onRoomState should not have been called.');
+
+      FakeMatrixApi.calledEndpoints.clear();
+      final user3 = await room.requestUser('@getmeempty:example.com');
+      expect(FakeMatrixApi.calledEndpoints.keys, [
+        '/client/v3/rooms/!localpart%3Aserver.abc/state/m.room.member/%40getmeempty%3Aexample.com',
+        '/client/v3/profile/%40getmeempty%3Aexample.com'
+      ]);
+      expect(user3?.stateKey, '@getmeempty:example.com');
+      expect(user3?.calcDisplayname(), 'You got me (empty)');
+      expect(user3?.membership, Membership.leave);
+
+      // Yield for the onUpdate
+      await Future.delayed(Duration(
+        milliseconds: 1,
+      ));
+      expect(called.length, 3);
+      expect(called2.length, 3);
+
+      await subscription.cancel();
+      await subscription2.cancel();
     });
 
     test('setAvatar', () async {
@@ -1319,10 +1390,12 @@ void main() {
     });
     test('inviteLink', () async {
       // ensure we don't rerequest members
-      room.summary.mJoinedMemberCount = 4;
+      room.summary.mJoinedMemberCount = 3;
+
       var matrixToLink = await room.matrixToInviteLink();
       expect(matrixToLink.toString(),
           'https://matrix.to/#/%23testalias%3Aexample.com');
+
       room.setState(
         Event(
             senderId: '@test:example.com',
@@ -1333,9 +1406,10 @@ void main() {
             originServerTs: DateTime.now(),
             stateKey: ''),
       );
+
       matrixToLink = await room.matrixToInviteLink();
       expect(matrixToLink.toString(),
-          'https://matrix.to/#/!localpart%3Aserver.abc?via=example.com&via=test.abc&via=example.org');
+          'https://matrix.to/#/!localpart%3Aserver.abc?via=fakeServer.notExisting&via=matrix.org&via=test.abc');
     });
 
     test('EventTooLarge on exceeding max PDU size', () async {