refactor(mobile): simplify local sync and hash service (#18970)

* Hash service review changes

* local album repo test

* simplify local album repo method names

---------

Co-authored-by: shenlong-tanwen <139912620+shalong-tanwen@users.noreply.github.com>

mobile/lib/domain/interfaces/local_album.interface.dart

@@ -3,11 +3,11 @@ import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
 import 'package:immich_mobile/domain/models/local_album.model.dart';
 
 abstract interface class ILocalAlbumRepository implements IDatabaseRepository {
-  Future<List<LocalAlbum>> getAll({SortLocalAlbumsBy? sortBy});
+  Future<List<LocalAlbum>> getAll({Set<SortLocalAlbumsBy> sortBy = const {}});
 
-  Future<List<LocalAsset>> getAssetsForAlbum(String albumId);
+  Future<List<LocalAsset>> getAssets(String albumId);
 
-  Future<List<String>> getAssetIdsForAlbum(String albumId);
+  Future<List<String>> getAssetIds(String albumId);
 
   Future<void> upsert(
     LocalAlbum album, {
@@ -25,12 +25,9 @@ abstract interface class ILocalAlbumRepository implements IDatabaseRepository {
     required Map<String, List<String>> assetAlbums,
   });
 
-  Future<void> syncAlbumDeletes(
+  Future<void> syncDeletes(String albumId, Iterable<String> assetIdsToKeep);
-    String albumId,
-    Iterable<String> assetIdsToKeep,
-  );
 
   Future<List<LocalAsset>> getAssetsToHash(String albumId);
 }
 
-enum SortLocalAlbumsBy { id }
+enum SortLocalAlbumsBy { id, backupSelection, isIosSharedAlbum }

mobile/lib/domain/models/local_album.model.dart

@@ -1,12 +1,10 @@
 enum BackupSelection {
-  none._(1),
-  selected._(0),
-  excluded._(2);
-
   // Used to sort albums based on the backupSelection
   // selected -> none -> excluded
-  final int sortOrder;
+  // Do not change the order of these values
-  const BackupSelection._(this.sortOrder);
+  selected,
+  none,
+  excluded,
 }
 
 class LocalAlbum {

mobile/lib/domain/services/hash.service.dart

@@ -33,18 +33,12 @@ class HashService {
   Future<void> hashAssets() async {
     final Stopwatch stopwatch = Stopwatch()..start();
     // Sorted by backupSelection followed by isCloud
-    final localAlbums = await _localAlbumRepository.getAll();
+    final localAlbums = await _localAlbumRepository.getAll(
-    localAlbums.sort((a, b) {
+      sortBy: {
-      final backupComparison =
+        SortLocalAlbumsBy.backupSelection,
-          a.backupSelection.sortOrder.compareTo(b.backupSelection.sortOrder);
+        SortLocalAlbumsBy.isIosSharedAlbum,
-
+      },
-      if (backupComparison != 0) {
+    );
-        return backupComparison;
-      }
-
-      // Local albums come before iCloud albums
-      return (a.isIosSharedAlbum ? 1 : 0).compareTo(b.isIosSharedAlbum ? 1 : 0);
-    });
 
     for (final album in localAlbums) {
       final assetsToHash =
@@ -96,13 +90,18 @@ class HashService {
     final hashed = <LocalAsset>[];
     final hashes =
         await _nativeSyncApi.hashPaths(toHash.map((e) => e.path).toList());
+    assert(
+      hashes.length == toHash.length,
+      "Hashes length does not match toHash length: ${hashes.length} != ${toHash.length}",
+    );
 
-    for (final (index, hash) in hashes.indexed) {
+    for (int i = 0; i < hashes.length; i++) {
-      final asset = toHash[index].asset;
+      final hash = hashes[i];
+      final asset = toHash[i].asset;
       if (hash?.length == 20) {
         hashed.add(asset.copyWith(checksum: base64.encode(hash!)));
       } else {
-        _log.warning("Failed to hash file ${asset.id}");
+        _log.warning("Failed to hash file for ${asset.id}");
       }
     }
 

mobile/lib/domain/services/local_sync.service.dart

@@ -66,7 +66,7 @@ class LocalSyncService {
       if (_platform.isAndroid) {
         for (final album in dbAlbums) {
           final deviceIds = await _nativeSyncApi.getAssetIdsForAlbum(album.id);
-          await _localAlbumRepository.syncAlbumDeletes(album.id, deviceIds);
+          await _localAlbumRepository.syncDeletes(album.id, deviceIds);
         }
       }
 
@@ -113,7 +113,7 @@ class LocalSyncService {
       }
 
       final dbAlbums =
-          await _localAlbumRepository.getAll(sortBy: SortLocalAlbumsBy.id);
+          await _localAlbumRepository.getAll(sortBy: {SortLocalAlbumsBy.id});
 
       await diffSortedLists(
         dbAlbums,
@@ -252,7 +252,7 @@ class LocalSyncService {
               .then((a) => a.toLocalAssets())
           : <LocalAsset>[];
       final assetsInDb = dbAlbum.assetCount > 0
-          ? await _localAlbumRepository.getAssetsForAlbum(dbAlbum.id)
+          ? await _localAlbumRepository.getAssets(dbAlbum.id)
           : <LocalAsset>[];
 
       if (deviceAlbum.assetCount == 0) {

mobile/lib/infrastructure/repositories/local_album.repository.dart

@@ -17,7 +17,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
         super(_db);
 
   @override
-  Future<List<LocalAlbum>> getAll({SortLocalAlbumsBy? sortBy}) {
+  Future<List<LocalAlbum>> getAll({Set<SortLocalAlbumsBy> sortBy = const {}}) {
     final assetCount = _db.localAlbumAssetEntity.assetId.count();
 
     final query = _db.localAlbumEntity.select().join([
@@ -30,9 +30,23 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
     query
       ..addColumns([assetCount])
       ..groupBy([_db.localAlbumEntity.id]);
-    if (sortBy == SortLocalAlbumsBy.id) {
+
-      query.orderBy([OrderingTerm.asc(_db.localAlbumEntity.id)]);
+    if (sortBy.isNotEmpty) {
+      final orderings = <OrderingTerm>[];
+      for (final sort in sortBy) {
+        orderings.add(
+          switch (sort) {
+            SortLocalAlbumsBy.id => OrderingTerm.asc(_db.localAlbumEntity.id),
+            SortLocalAlbumsBy.backupSelection =>
+              OrderingTerm.asc(_db.localAlbumEntity.backupSelection),
+            SortLocalAlbumsBy.isIosSharedAlbum =>
+              OrderingTerm.asc(_db.localAlbumEntity.isIosSharedAlbum),
+          },
+        );
+      }
+      query.orderBy(orderings);
     }
+
     return query
         .map(
           (row) => row
@@ -49,7 +63,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
         // That is not the case on Android since asset <-> album has one:one mapping
         final assetsToDelete = _platform.isIOS
             ? await _getUniqueAssetsInAlbum(albumId)
-            : await getAssetIdsForAlbum(albumId);
+            : await getAssetIds(albumId);
         await _deleteAssets(assetsToDelete);
 
         // All the other assets that are still associated will be unlinked automatically on-cascade
@@ -59,7 +73,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
       });
 
   @override
-  Future<void> syncAlbumDeletes(
+  Future<void> syncDeletes(
     String albumId,
     Iterable<String> assetIdsToKeep,
   ) async {
@@ -172,7 +186,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
   }
 
   @override
-  Future<List<LocalAsset>> getAssetsForAlbum(String albumId) {
+  Future<List<LocalAsset>> getAssets(String albumId) {
     final query = _db.localAlbumAssetEntity.select().join(
       [
         innerJoin(
@@ -189,7 +203,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository
   }
 
   @override
-  Future<List<String>> getAssetIdsForAlbum(String albumId) {
+  Future<List<String>> getAssetIds(String albumId) {
     final query = _db.localAlbumAssetEntity.selectOnly()
       ..addColumns([_db.localAlbumAssetEntity.assetId])
       ..where(_db.localAlbumAssetEntity.albumId.equals(albumId));

mobile/test/domain/services/hash_service_test.dart

@@ -3,8 +3,8 @@ import 'dart:io';
 import 'dart:typed_data';
 
 import 'package:flutter_test/flutter_test.dart';
+import 'package:immich_mobile/domain/interfaces/local_album.interface.dart';
 import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
-import 'package:immich_mobile/domain/models/local_album.model.dart';
 import 'package:immich_mobile/domain/services/hash.service.dart';
 import 'package:mocktail/mocktail.dart';
 
@@ -21,6 +21,10 @@ void main() {
   late MockLocalAssetRepository mockAssetRepo;
   late MockStorageRepository mockStorageRepo;
   late MockNativeSyncApi mockNativeApi;
+  final sortBy = {
+    SortLocalAlbumsBy.backupSelection,
+    SortLocalAlbumsBy.isIosSharedAlbum,
+  };
 
   setUp(() {
     mockAlbumRepo = MockLocalAlbumRepository();
@@ -42,37 +46,8 @@ void main() {
   });
 
   group('HashService hashAssets', () {
-    test('processes albums in correct order', () async {
-      final album1 = LocalAlbumStub.recent
-          .copyWith(id: "1", backupSelection: BackupSelection.none);
-      final album2 = LocalAlbumStub.recent
-          .copyWith(id: "2", backupSelection: BackupSelection.excluded);
-      final album3 = LocalAlbumStub.recent
-          .copyWith(id: "3", backupSelection: BackupSelection.selected);
-      final album4 = LocalAlbumStub.recent.copyWith(
-        id: "4",
-        backupSelection: BackupSelection.selected,
-        isIosSharedAlbum: true,
-      );
-
-      when(() => mockAlbumRepo.getAll())
-          .thenAnswer((_) async => [album1, album2, album4, album3]);
-      when(() => mockAlbumRepo.getAssetsToHash(any()))
-          .thenAnswer((_) async => []);
-
-      await sut.hashAssets();
-
-      verifyInOrder([
-        () => mockAlbumRepo.getAll(),
-        () => mockAlbumRepo.getAssetsToHash(album3.id),
-        () => mockAlbumRepo.getAssetsToHash(album4.id),
-        () => mockAlbumRepo.getAssetsToHash(album1.id),
-        () => mockAlbumRepo.getAssetsToHash(album2.id),
-      ]);
-    });
-
     test('skips albums with no assets to hash', () async {
-      when(() => mockAlbumRepo.getAll()).thenAnswer(
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy)).thenAnswer(
         (_) async => [LocalAlbumStub.recent.copyWith(assetCount: 0)],
       );
       when(() => mockAlbumRepo.getAssetsToHash(LocalAlbumStub.recent.id))
@@ -89,7 +64,8 @@ void main() {
     test('skips assets without files', () async {
       final album = LocalAlbumStub.recent;
       final asset = LocalAssetStub.image1;
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset]);
       when(() => mockStorageRepo.getFileForAsset(asset))
@@ -109,7 +85,8 @@ void main() {
       when(() => mockFile.length()).thenAnswer((_) async => 1000);
       when(() => mockFile.path).thenReturn('image-path');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset]);
       when(() => mockStorageRepo.getFileForAsset(asset))
@@ -135,7 +112,8 @@ void main() {
       when(() => mockFile.length()).thenAnswer((_) async => 1000);
       when(() => mockFile.path).thenReturn('image-path');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset]);
       when(() => mockStorageRepo.getFileForAsset(asset))
@@ -159,7 +137,8 @@ void main() {
       when(() => mockFile.length()).thenAnswer((_) async => 1000);
       when(() => mockFile.path).thenReturn('image-path');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset]);
       when(() => mockStorageRepo.getFileForAsset(asset))
@@ -197,7 +176,8 @@ void main() {
       when(() => mockFile2.length()).thenAnswer((_) async => 100);
       when(() => mockFile2.path).thenReturn('path-2');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset1, asset2]);
       when(() => mockStorageRepo.getFileForAsset(asset1))
@@ -236,7 +216,8 @@ void main() {
       when(() => mockFile2.length()).thenAnswer((_) async => 100);
       when(() => mockFile2.path).thenReturn('path-2');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset1, asset2]);
       when(() => mockStorageRepo.getFileForAsset(asset1))
@@ -267,7 +248,8 @@ void main() {
       when(() => mockFile2.length()).thenAnswer((_) async => 100);
       when(() => mockFile2.path).thenReturn('path-2');
 
-      when(() => mockAlbumRepo.getAll()).thenAnswer((_) async => [album]);
+      when(() => mockAlbumRepo.getAll(sortBy: sortBy))
+          .thenAnswer((_) async => [album]);
       when(() => mockAlbumRepo.getAssetsToHash(album.id))
           .thenAnswer((_) async => [asset1, asset2]);
       when(() => mockStorageRepo.getFileForAsset(asset1))

mobile/test/infrastructure/repositories/local_album_repository_test.dart

@@ -0,0 +1,66 @@
+import 'package:drift/drift.dart';
+import 'package:drift/native.dart';
+import 'package:flutter_test/flutter_test.dart';
+import 'package:immich_mobile/domain/interfaces/local_album.interface.dart';
+import 'package:immich_mobile/domain/models/local_album.model.dart';
+import 'package:immich_mobile/infrastructure/repositories/db.repository.dart';
+
+import '../../test_utils/medium_factory.dart';
+
+void main() {
+  late Drift db;
+  late MediumFactory mediumFactory;
+
+  setUp(() {
+    db = Drift(
+      DatabaseConnection(
+        NativeDatabase.memory(),
+        closeStreamsSynchronously: true,
+      ),
+    );
+    mediumFactory = MediumFactory(db);
+  });
+
+  group('getAll', () {
+    test('sorts albums by backupSelection & isIosSharedAlbum', () async {
+      final localAlbumRepo =
+          mediumFactory.getRepository<ILocalAlbumRepository>();
+      await localAlbumRepo.upsert(
+        mediumFactory.localAlbum(
+          id: '1',
+          backupSelection: BackupSelection.none,
+        ),
+      );
+      await localAlbumRepo.upsert(
+        mediumFactory.localAlbum(
+          id: '2',
+          backupSelection: BackupSelection.excluded,
+        ),
+      );
+      await localAlbumRepo.upsert(
+        mediumFactory.localAlbum(
+          id: '3',
+          backupSelection: BackupSelection.selected,
+          isIosSharedAlbum: true,
+        ),
+      );
+      await localAlbumRepo.upsert(
+        mediumFactory.localAlbum(
+          id: '4',
+          backupSelection: BackupSelection.selected,
+        ),
+      );
+      final albums = await localAlbumRepo.getAll(
+        sortBy: {
+          SortLocalAlbumsBy.backupSelection,
+          SortLocalAlbumsBy.isIosSharedAlbum,
+        },
+      );
+      expect(albums.length, 4);
+      expect(albums[0].id, '4'); // selected
+      expect(albums[1].id, '3'); // selected & isIosSharedAlbum
+      expect(albums[2].id, '1'); // none
+      expect(albums[3].id, '2'); // excluded
+    });
+  });
+}

mobile/test/test_utils/medium_factory.dart

@@ -0,0 +1,65 @@
+import 'dart:math';
+
+import 'package:immich_mobile/domain/interfaces/local_album.interface.dart';
+import 'package:immich_mobile/domain/models/asset/base_asset.model.dart';
+import 'package:immich_mobile/domain/models/local_album.model.dart';
+import 'package:immich_mobile/infrastructure/repositories/db.repository.dart';
+import 'package:immich_mobile/infrastructure/repositories/local_album.repository.dart';
+
+class MediumFactory {
+  final Drift _db;
+
+  const MediumFactory(Drift db) : _db = db;
+
+  LocalAsset localAsset({
+    String? id,
+    String? name,
+    AssetType? type,
+    DateTime? createdAt,
+    DateTime? updatedAt,
+    String? checksum,
+  }) {
+    final random = Random();
+
+    return LocalAsset(
+      id: id ?? '${random.nextInt(1000000)}',
+      name: name ?? 'Asset ${random.nextInt(1000000)}',
+      checksum: checksum ?? '${random.nextInt(1000000)}',
+      type: type ?? AssetType.image,
+      createdAt: createdAt ??
+          DateTime.fromMillisecondsSinceEpoch(random.nextInt(1000000000)),
+      updatedAt: updatedAt ??
+          DateTime.fromMillisecondsSinceEpoch(random.nextInt(1000000000)),
+    );
+  }
+
+  LocalAlbum localAlbum({
+    String? id,
+    String? name,
+    DateTime? updatedAt,
+    int? assetCount,
+    BackupSelection? backupSelection,
+    bool? isIosSharedAlbum,
+  }) {
+    final random = Random();
+
+    return LocalAlbum(
+      id: id ?? '${random.nextInt(1000000)}',
+      name: name ?? 'Album ${random.nextInt(1000000)}',
+      updatedAt: updatedAt ??
+          DateTime.fromMillisecondsSinceEpoch(random.nextInt(1000000000)),
+      assetCount: assetCount ?? random.nextInt(100),
+      backupSelection: backupSelection ?? BackupSelection.none,
+      isIosSharedAlbum: isIosSharedAlbum ?? false,
+    );
+  }
+
+  T getRepository<T>() {
+    switch (T) {
+      case const (ILocalAlbumRepository):
+        return DriftLocalAlbumRepository(_db) as T;
+      default:
+        throw Exception('Unknown repository: $T');
+    }
+  }
+}