Merge pull request #1883 from famedly/krille/refactor-profile-caching-in-database

refactor: Cache profiles in database and refactor API

lib/matrix.dart

@@ -52,6 +52,7 @@ export 'src/voip/utils/wrapped_media_stream.dart';
 export 'src/room.dart';
 export 'src/timeline.dart';
 export 'src/user.dart';
+export 'src/utils/cached_profile_information.dart';
 export 'src/utils/commands_extension.dart';
 export 'src/utils/crypto/encrypted_file.dart';
 export 'src/utils/device_keys_list.dart';

lib/msc_extensions/msc_1236_widgets/src/widget.dart

@@ -91,14 +91,14 @@ class MatrixWidget {
     // See https://github.com/matrix-org/matrix-doc/issues/1236 for a
     // description, specifically the section
     // `What does the other stuff in content mean?`
-    final userProfile = await room.client.fetchOwnProfile();
+    final userProfile = await room.client.getUserProfile(room.client.userID!);
     var parsedUri = url;
 
     // a key-value map with the strings to be replaced
     final replaceMap = {
-      r'$matrix_user_id': userProfile.userId,
+      r'$matrix_user_id': room.client.userID!,
       r'$matrix_room_id': room.id,
-      r'$matrix_display_name': userProfile.displayName ?? '',
+      r'$matrix_display_name': userProfile.displayname ?? '',
       r'$matrix_avatar_url': userProfile.avatarUrl?.toString() ?? '',
       // removing potentially dangerous keys containing anything but
       // `[a-zA-Z0-9_-]` as well as non string values

lib/src/client.dart

@@ -911,7 +911,7 @@ class Client extends MatrixApi {
     return id;
   }
 
-  @Deprecated('Use fetchOwnProfile() instead')
+  @Deprecated('Use getUserProfile(userID) instead')
   Future<Profile> get ownProfile => fetchOwnProfile();
 
   /// Returns the user's own displayname and avatar url. In Matrix it is possible that
@@ -919,6 +919,7 @@ class Client extends MatrixApi {
   /// Tries to get the profile from homeserver first, if failed, falls back to a profile
   /// from a room where the user exists. Set `useServerCache` to true to get any
   /// prior value from this function
+  @Deprecated('Use fetchOwnProfile() instead')
   Future<Profile> fetchOwnProfileFromServer(
       {bool useServerCache = false}) async {
     try {
@@ -943,64 +944,86 @@ class Client extends MatrixApi {
   /// This returns the profile from the first room by default, override `getFromRooms`
   /// to false to fetch from homeserver.
   Future<Profile> fetchOwnProfile({
-    bool getFromRooms = true,
-    bool cache = true,
+    @Deprecated('No longer supported') bool getFromRooms = true,
+    @Deprecated('No longer supported') bool cache = true,
   }) =>
-      getProfileFromUserId(
-        userID!,
-        getFromRooms: getFromRooms,
-        cache: cache,
-      );
-
-  final Map<String, ProfileInformation> _profileRoomsCache = {};
-  final Map<String, ProfileInformation> _profileServerCache = {};
-
-  /// Get the combined profile information for this user.
-  /// If [getFromRooms] is true then the profile will first be searched from the
-  /// room memberships. This is unstable if the given user makes use of different displaynames
-  /// and avatars per room, which is common for some bots and bridges.
-  /// If [cache] is true then
-  /// the profile get cached for this session. Please note that then the profile may
-  /// become outdated if the user changes the displayname or avatar in this session.
-  Future<Profile> getProfileFromUserId(String userId,
-      {bool cache = true, bool getFromRooms = true}) async {
-    var profile =
-        getFromRooms ? _profileRoomsCache[userId] : _profileServerCache[userId];
-    if (cache && profile != null) {
-      return Profile(
-        userId: userId,
-        displayName: profile.displayname,
-        avatarUrl: profile.avatarUrl,
+      getProfileFromUserId(userID!);
+
+  /// Get the combined profile information for this user. First checks for a
+  /// non outdated cached profile before requesting from the server. Cached
+  /// profiles are outdated if they have been cached in a time older than the
+  /// [maxCacheAge] or they have been marked as outdated by an event in the
+  /// sync loop.
+  /// In case of an
+  ///
+  /// [userId] The user whose profile information to get.
+  @override
+  Future<CachedProfileInformation> getUserProfile(
+    String userId, {
+    Duration timeout = const Duration(seconds: 30),
+    Duration maxCacheAge = const Duration(days: 1),
+  }) async {
+    final cachedProfile = await database?.getUserProfile(userId);
+    if (cachedProfile != null &&
+        !cachedProfile.outdated &&
+        DateTime.now().difference(cachedProfile.updated) < maxCacheAge) {
+      return cachedProfile;
+    }
+
+    final ProfileInformation profile;
+    try {
+      profile = await (_userProfileRequests[userId] ??=
+          super.getUserProfile(userId).timeout(timeout));
+    } catch (e) {
+      Logs().d('Unable to fetch profile from server', e);
+      if (cachedProfile == null) rethrow;
+      return cachedProfile;
+    } finally {
+      unawaited(_userProfileRequests.remove(userId));
+    }
+
+    final newCachedProfile = CachedProfileInformation.fromProfile(
+      profile,
+      outdated: false,
+      updated: DateTime.now(),
+    );
+
+    await database?.storeUserProfile(userId, newCachedProfile);
+
+    return newCachedProfile;
+  }
+
+  final Map<String, Future<ProfileInformation>> _userProfileRequests = {};
+
+  final CachedStreamController<String> onUserProfileUpdate =
+      CachedStreamController<String>();
+
+  /// Get the combined profile information for this user from the server or
+  /// from the cache depending on the cache value. Returns a `Profile` object
+  /// including the given userId but without information about how outdated
+  /// the profile is. If you need those, try using `getUserProfile()` instead.
+  Future<Profile> getProfileFromUserId(
+    String userId, {
+    @Deprecated('No longer supported') bool? getFromRooms,
+    @Deprecated('No longer supported') bool? cache,
+    Duration timeout = const Duration(seconds: 30),
+    Duration maxCacheAge = const Duration(days: 1),
+  }) async {
+    CachedProfileInformation? cachedProfileInformation;
+    try {
+      cachedProfileInformation = await getUserProfile(
+        userId,
+        timeout: timeout,
+        maxCacheAge: maxCacheAge,
       );
+    } catch (e) {
+      Logs().d('Unable to fetch profile for $userId', e);
     }
 
-    if (getFromRooms) {
-      final room = rooms.firstWhereOrNull((Room room) =>
-          room.getParticipants().indexWhere((User user) => user.id == userId) !=
-          -1);
-      if (room != null) {
-        final user =
-            room.getParticipants().firstWhere((User user) => user.id == userId);
-        final profileFromRooms = Profile(
-          userId: userId,
-          displayName: user.displayName,
-          avatarUrl: user.avatarUrl,
-        );
-        _profileRoomsCache[userId] = ProfileInformation(
-          avatarUrl: profileFromRooms.avatarUrl,
-          displayname: profileFromRooms.displayName,
-        );
-        return profileFromRooms;
-      }
-    }
-    profile = await getUserProfile(userId);
-    if (cache || _profileServerCache.containsKey(userId)) {
-      _profileServerCache[userId] = profile;
-    }
     return Profile(
       userId: userId,
-      displayName: profile.displayname,
-      avatarUrl: profile.avatarUrl,
+      displayName: cachedProfileInformation?.displayname,
+      avatarUrl: cachedProfileInformation?.avatarUrl,
     );
   }
 
@@ -2295,6 +2318,19 @@ class Client extends MatrixApi {
               content: update.content)));
         }
       }
+
+      // Any kind of member change? We should invalidate the profile then:
+      if (event is StrippedStateEvent && event.type == EventTypes.RoomMember) {
+        final userId = event.stateKey;
+        if (userId != null) {
+          // We do not re-request the profile here as this would lead to
+          // an unknown amount of network requests as we never know how many
+          // member change events can come down in a single sync update.
+          await database?.markUserProfileAsOutdated(userId);
+          onUserProfileUpdate.add(userId);
+        }
+      }
+
       if (event.type == EventTypes.Message &&
           !room.isDirectChat &&
           database != null &&

lib/src/database/database_api.dart

@@ -81,6 +81,13 @@ abstract class DatabaseApi {
 
   Future<void> forgetRoom(String roomId);
 
+  Future<CachedProfileInformation?> getUserProfile(String userId);
+
+  Future<void> storeUserProfile(
+      String userId, CachedProfileInformation profile);
+
+  Future<void> markUserProfileAsOutdated(String userId);
+
   Future<void> clearCache();
 
   Future<void> clear();

lib/src/database/hive_collections_database.dart

@@ -1597,6 +1597,22 @@ class HiveCollectionsDatabase extends DatabaseApi {
 
   @override
   Future<void> delete() => _collection.deleteFromDisk();
+
+  @override
+  Future<void> markUserProfileAsOutdated(userId) async {
+    return;
+  }
+
+  @override
+  Future<CachedProfileInformation?> getUserProfile(String userId) async {
+    return null;
+  }
+
+  @override
+  Future<void> storeUserProfile(
+      String userId, CachedProfileInformation profile) async {
+    return;
+  }
 }
 
 class TupleKey {

lib/src/database/hive_database.dart

@@ -1403,6 +1403,22 @@ class FamedlySdkHiveDatabase extends DatabaseApi with ZoneTransactionMixin {
 
   @override
   Future<void> delete() => Hive.deleteFromDisk();
+
+  @override
+  Future<void> markUserProfileAsOutdated(userId) async {
+    return;
+  }
+
+  @override
+  Future<CachedProfileInformation?> getUserProfile(String userId) async {
+    return null;
+  }
+
+  @override
+  Future<void> storeUserProfile(
+      String userId, CachedProfileInformation profile) async {
+    return;
+  }
 }
 
 dynamic _castValue(dynamic value) {

lib/src/database/matrix_sdk_database.dart

@@ -103,6 +103,8 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
 
   late Box<String> _seenDeviceKeysBox;
 
+  late Box<Map> _userProfilesBox;
+
   @override
   final int maxFileSize;
 
@@ -159,6 +161,8 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
 
   static const String _seenDeviceKeysBoxName = 'box_seen_device_keys';
 
+  static const String _userProfilesBoxName = 'box_user_profiles';
+
   Database? database;
 
   /// Custom IdbFactory used to create the indexedDB. On IO platforms it would
@@ -214,6 +218,7 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
         _eventsBoxName,
         _seenDeviceIdsBoxName,
         _seenDeviceKeysBoxName,
+        _userProfilesBoxName,
       },
       sqfliteDatabase: database,
       sqfliteFactory: sqfliteFactory,
@@ -283,6 +288,9 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
     _seenDeviceKeysBox = _collection.openBox(
       _seenDeviceKeysBoxName,
     );
+    _userProfilesBox = _collection.openBox(
+      _userProfilesBoxName,
+    );
 
     // Check version and check if we need a migration
     final currentVersion = int.tryParse(await _clientBox.get('version') ?? '');
@@ -340,6 +348,7 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
         await _timelineFragmentsBox.clear();
         await _outboundGroupSessionsBox.clear();
         await _presencesBox.clear();
+        await _userProfilesBox.clear();
         await _clientBox.delete('prev_batch');
       });
 
@@ -1618,4 +1627,32 @@ class MatrixSdkDatabase extends DatabaseApi with DatabaseFileStorage {
         name,
         sqfliteFactory ?? idbFactory,
       );
+
+  @override
+  Future<void> markUserProfileAsOutdated(userId) async {
+    final profile = await getUserProfile(userId);
+    if (profile == null) return;
+    await _userProfilesBox.put(
+      userId,
+      CachedProfileInformation.fromProfile(
+        profile as ProfileInformation,
+        outdated: true,
+        updated: profile.updated,
+      ).toJson(),
+    );
+  }
+
+  @override
+  Future<CachedProfileInformation?> getUserProfile(String userId) =>
+      _userProfilesBox.get(userId).then((json) => json == null
+          ? null
+          : CachedProfileInformation.fromJson(copyMap(json)));
+
+  @override
+  Future<void> storeUserProfile(
+          String userId, CachedProfileInformation profile) =>
+      _userProfilesBox.put(
+        userId,
+        profile.toJson(),
+      );
 }

lib/src/room.dart

@@ -1717,10 +1717,10 @@ class Room {
                 displayName: null
               )) {
         try {
-          final profile = await client.getProfileFromUserId(mxID);
+          final profile = await client.getUserProfile(mxID);
           foundUser = User(
             mxID,
-            displayName: profile.displayName,
+            displayName: profile.displayname,
             avatarUrl: profile.avatarUrl?.toString(),
             membership: foundUser?.membership.name ?? Membership.leave.name,
             room: this,

lib/src/utils/cached_profile_information.dart

@@ -0,0 +1,29 @@
+import 'package:matrix/matrix_api_lite.dart';
+
+class CachedProfileInformation extends ProfileInformation {
+  final bool outdated;
+  final DateTime updated;
+
+  CachedProfileInformation.fromProfile(
+    ProfileInformation profile, {
+    required this.outdated,
+    required this.updated,
+  }) : super(
+          avatarUrl: profile.avatarUrl,
+          displayname: profile.displayname,
+        );
+
+  factory CachedProfileInformation.fromJson(Map<String, Object?> json) =>
+      CachedProfileInformation.fromProfile(
+        ProfileInformation.fromJson(json),
+        outdated: json['outdated'] as bool,
+        updated: DateTime.fromMillisecondsSinceEpoch(json['updated'] as int),
+      );
+
+  @override
+  Map<String, Object?> toJson() => {
+        ...super.toJson(),
+        'outdated': outdated,
+        'updated': updated.millisecondsSinceEpoch,
+      };
+}

test/database_api_test.dart

@@ -616,6 +616,38 @@ void main() {
           storedPresence?.toJson(),
         );
       });
+      test(
+        'storeUserProfile',
+        () async {
+          final profile1 = await database.getUserProfile('@alice:example.com');
+          expect(profile1, null);
+
+          await database.storeUserProfile(
+            '@alice:example.com',
+            CachedProfileInformation.fromProfile(
+              ProfileInformation(
+                  avatarUrl: Uri.parse('mxc://test'), displayname: 'Alice M'),
+              outdated: false,
+              updated: DateTime.now(),
+            ),
+          );
+          // ignore: deprecated_member_use_from_same_package
+          if (database is! HiveCollectionsDatabase &&
+              // ignore: deprecated_member_use_from_same_package
+              database is! FamedlySdkHiveDatabase) {
+            final profile2 =
+                await database.getUserProfile('@alice:example.com');
+            expect(profile2?.displayname, 'Alice M');
+            expect(profile2?.outdated, false);
+            await database.markUserProfileAsOutdated('@alice:example.com');
+
+            final profile3 =
+                await database.getUserProfile('@alice:example.com');
+            expect(profile3?.displayname, 'Alice M');
+            expect(profile3?.outdated, true);
+          }
+        },
+      );
 
       // Clearing up from here
       test('clearSSSSCache', () async {

test_driver/matrixsdk_test.dart

@@ -490,6 +490,9 @@ void main() => group('Integration tests', () {
           if (testClientB.getRoomById(dmRoom) == null) {
             await testClientB.waitForRoomInSync(dmRoom, invite: true);
           }
+          // Wait at least for one additional sync to make sure the invite landed
+          // correctly. Workaround for synapse CI job failing.
+          await testClientB.onSync.stream.first;
 
           Logs().i('++++ (Bob) Create DM ++++');
           final dmRoomFromB =