fix: Request user causing state update loops for apps

This completely redoes the requestUser function.

It now doesn't stop requesting a user forever just because of a network
error. But redundant calls are still deduplicated, however their
parameters are taken into account now.

It also now only calls onUpdate and onRoomState when there is actually
new state. This way apps can just listen to updates to rerender instead
of having to implement the deduplication themselves.

It also now doesn't treat member events without membership as "join"
anymore. And lookup errors don't cause an empty user to get inserted
into the room state.

In general this should still request the profile from the server, if the
displayname is unset in a leave event, but it should also allow the app
to actually settle in the tests.

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 {