From 84024f6cdc4085076f2beb1bde0407df4e0cd214 Mon Sep 17 00:00:00 2001 From: shenlong <139912620+shenlong-tanwen@users.noreply.github.com> Date: Mon, 9 Jun 2025 08:26:44 +0530 Subject: [PATCH] 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> --- .../interfaces/local_album.interface.dart | 13 ++-- .../lib/domain/models/local_album.model.dart | 10 ++- mobile/lib/domain/services/hash.service.dart | 29 ++++---- .../domain/services/local_sync.service.dart | 6 +- .../repositories/local_album.repository.dart | 28 ++++++-- .../domain/services/hash_service_test.dart | 58 ++++++---------- .../local_album_repository_test.dart | 66 +++++++++++++++++++ mobile/test/test_utils/medium_factory.dart | 65 ++++++++++++++++++ 8 files changed, 198 insertions(+), 77 deletions(-) create mode 100644 mobile/test/infrastructure/repositories/local_album_repository_test.dart create mode 100644 mobile/test/test_utils/medium_factory.dart diff --git a/mobile/lib/domain/interfaces/local_album.interface.dart b/mobile/lib/domain/interfaces/local_album.interface.dart index d7b38c567f..1df62954d2 100644 --- a/mobile/lib/domain/interfaces/local_album.interface.dart +++ b/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> getAll({SortLocalAlbumsBy? sortBy}); + Future> getAll({Set sortBy = const {}}); - Future> getAssetsForAlbum(String albumId); + Future> getAssets(String albumId); - Future> getAssetIdsForAlbum(String albumId); + Future> getAssetIds(String albumId); Future upsert( LocalAlbum album, { @@ -25,12 +25,9 @@ abstract interface class ILocalAlbumRepository implements IDatabaseRepository { required Map> assetAlbums, }); - Future syncAlbumDeletes( - String albumId, - Iterable assetIdsToKeep, - ); + Future syncDeletes(String albumId, Iterable assetIdsToKeep); Future> getAssetsToHash(String albumId); } -enum SortLocalAlbumsBy { id } +enum SortLocalAlbumsBy { id, backupSelection, isIosSharedAlbum } diff --git a/mobile/lib/domain/models/local_album.model.dart b/mobile/lib/domain/models/local_album.model.dart index ee27d91a71..b0b08937ab 100644 --- a/mobile/lib/domain/models/local_album.model.dart +++ b/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; - const BackupSelection._(this.sortOrder); + // Do not change the order of these values + selected, + none, + excluded, } class LocalAlbum { diff --git a/mobile/lib/domain/services/hash.service.dart b/mobile/lib/domain/services/hash.service.dart index 9820453db1..d8ad3dc2dc 100644 --- a/mobile/lib/domain/services/hash.service.dart +++ b/mobile/lib/domain/services/hash.service.dart @@ -33,18 +33,12 @@ class HashService { Future hashAssets() async { final Stopwatch stopwatch = Stopwatch()..start(); // Sorted by backupSelection followed by isCloud - final localAlbums = await _localAlbumRepository.getAll(); - localAlbums.sort((a, b) { - final backupComparison = - a.backupSelection.sortOrder.compareTo(b.backupSelection.sortOrder); - - if (backupComparison != 0) { - return backupComparison; - } - - // Local albums come before iCloud albums - return (a.isIosSharedAlbum ? 1 : 0).compareTo(b.isIosSharedAlbum ? 1 : 0); - }); + final localAlbums = await _localAlbumRepository.getAll( + sortBy: { + SortLocalAlbumsBy.backupSelection, + SortLocalAlbumsBy.isIosSharedAlbum, + }, + ); for (final album in localAlbums) { final assetsToHash = @@ -96,13 +90,18 @@ class HashService { final hashed = []; 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) { - final asset = toHash[index].asset; + for (int i = 0; i < hashes.length; i++) { + 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}"); } } diff --git a/mobile/lib/domain/services/local_sync.service.dart b/mobile/lib/domain/services/local_sync.service.dart index 037179c966..ff77ebd83e 100644 --- a/mobile/lib/domain/services/local_sync.service.dart +++ b/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()) : []; final assetsInDb = dbAlbum.assetCount > 0 - ? await _localAlbumRepository.getAssetsForAlbum(dbAlbum.id) + ? await _localAlbumRepository.getAssets(dbAlbum.id) : []; if (deviceAlbum.assetCount == 0) { diff --git a/mobile/lib/infrastructure/repositories/local_album.repository.dart b/mobile/lib/infrastructure/repositories/local_album.repository.dart index 075b6e1805..4f46b9b408 100644 --- a/mobile/lib/infrastructure/repositories/local_album.repository.dart +++ b/mobile/lib/infrastructure/repositories/local_album.repository.dart @@ -17,7 +17,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository super(_db); @override - Future> getAll({SortLocalAlbumsBy? sortBy}) { + Future> getAll({Set 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 = []; + 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 syncAlbumDeletes( + Future syncDeletes( String albumId, Iterable assetIdsToKeep, ) async { @@ -172,7 +186,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository } @override - Future> getAssetsForAlbum(String albumId) { + Future> getAssets(String albumId) { final query = _db.localAlbumAssetEntity.select().join( [ innerJoin( @@ -189,7 +203,7 @@ class DriftLocalAlbumRepository extends DriftDatabaseRepository } @override - Future> getAssetIdsForAlbum(String albumId) { + Future> getAssetIds(String albumId) { final query = _db.localAlbumAssetEntity.selectOnly() ..addColumns([_db.localAlbumAssetEntity.assetId]) ..where(_db.localAlbumAssetEntity.albumId.equals(albumId)); diff --git a/mobile/test/domain/services/hash_service_test.dart b/mobile/test/domain/services/hash_service_test.dart index 1401f5d2a0..623aed5409 100644 --- a/mobile/test/domain/services/hash_service_test.dart +++ b/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)) diff --git a/mobile/test/infrastructure/repositories/local_album_repository_test.dart b/mobile/test/infrastructure/repositories/local_album_repository_test.dart new file mode 100644 index 0000000000..827d81c79b --- /dev/null +++ b/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(); + 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 + }); + }); +} diff --git a/mobile/test/test_utils/medium_factory.dart b/mobile/test/test_utils/medium_factory.dart new file mode 100644 index 0000000000..64c23321aa --- /dev/null +++ b/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() { + switch (T) { + case const (ILocalAlbumRepository): + return DriftLocalAlbumRepository(_db) as T; + default: + throw Exception('Unknown repository: $T'); + } + } +}