diff --git a/mobile/lib/data/db/main/table/local/asset.dart b/mobile/lib/data/db/main/table/local/asset.dart index 956cd9f04..31f09e26c 100644 --- a/mobile/lib/data/db/main/table/local/asset.dart +++ b/mobile/lib/data/db/main/table/local/asset.dart @@ -28,8 +28,7 @@ class LocalAssetEntity extends Table with DriftDefaultsMixin, AssetEntityMixin { IntColumn get playbackStyle => intEnum().withDefault(const Constant(0))(); - /// Checksum this asset had before its bytes last changed, so the upload of the - /// edited asset can be stacked over the remote asset that still has the old bytes. + /// Checksum of the last synced version of this asset TextColumn get previousChecksum => text().nullable()(); @override diff --git a/mobile/lib/domain/services/asset.service.dart b/mobile/lib/domain/services/asset.service.dart index 8e490920b..7e40f426c 100644 --- a/mobile/lib/domain/services/asset.service.dart +++ b/mobile/lib/domain/services/asset.service.dart @@ -198,10 +198,18 @@ class AssetService { return _localRepository.get(id); } - Future stackEditedUpload(String localId, String remoteId) async { - final previousId = await _localRepository.getPreviousRemoteId(localId); - if (previousId != null) { - await _apiRepository.stack([remoteId, previousId]); + Future stackEditedUpload(String localId, String remoteId, String? checksum) async { + // a manual upload of an asset that was never hashed has no checksum yet, so we need to fetch it from the server + final uploadedChecksum = checksum ?? await _apiRepository.getChecksum(remoteId); + try { + // previous_checksum still points at the version the server had before this upload + final previousId = await _localRepository.getPreviousRemoteId(localId); + if (previousId != null) { + await _apiRepository.stack([remoteId, previousId]); + } + } finally { + // the upload went through even when the stack call did not, so this version is the new base + await _localRepository.updatePreviousChecksum(localId, uploadedChecksum); } } } diff --git a/mobile/lib/infrastructure/repositories/local_album.repository.dart b/mobile/lib/infrastructure/repositories/local_album.repository.dart index 2930221a6..342195ed0 100644 --- a/mobile/lib/infrastructure/repositories/local_album.repository.dart +++ b/mobile/lib/infrastructure/repositories/local_album.repository.dart @@ -287,7 +287,7 @@ class LocalAlbumRepository extends DatabaseAccessor with $LocalAlbumRepos final companion = LocalAssetEntityCompanion.custom( checksum: const Constant(null), adjustmentTime: Variable(asset.adjustmentTime), - previousChecksum: _db.localAssetEntity.checksum, + previousChecksum: coalesce([_db.localAssetEntity.previousChecksum, _db.localAssetEntity.checksum]), ); batch.update( _db.localAssetEntity, @@ -350,8 +350,10 @@ class LocalAlbumRepository extends DatabaseAccessor with $LocalAlbumRepos companion, onConflict: DoUpdate( // SET reads the existing row, so this keeps the checksum being cleared - (_) => - RawValuesInsertable({...companion.toColumns(true), 'previous_checksum': _db.localAssetEntity.checksum}), + (_) => RawValuesInsertable({ + ...companion.toColumns(true), + 'previous_checksum': coalesce([_db.localAssetEntity.previousChecksum, _db.localAssetEntity.checksum]), + }), where: (old) => old.updatedAt.isNotValue(asset.updatedAt), ), ); diff --git a/mobile/lib/infrastructure/repositories/local_asset.repository.dart b/mobile/lib/infrastructure/repositories/local_asset.repository.dart index 8d9806043..481ace2c7 100644 --- a/mobile/lib/infrastructure/repositories/local_asset.repository.dart +++ b/mobile/lib/infrastructure/repositories/local_asset.repository.dart @@ -254,4 +254,9 @@ class LocalAssetRepository extends DatabaseAccessor with $LocalAssetRepos return query.map((row) => row.read(_db.remoteAssetEntity.id)).getSingleOrNull(); } + + Future updatePreviousChecksum(String id, String checksum) async { + final query = _db.localAssetEntity.update()..where((row) => row.id.equals(id)); + await query.write(LocalAssetEntityCompanion(previousChecksum: Value(checksum))); + } } diff --git a/mobile/lib/repositories/asset_api.repository.dart b/mobile/lib/repositories/asset_api.repository.dart index 50d8431de..25ff71333 100644 --- a/mobile/lib/repositories/asset_api.repository.dart +++ b/mobile/lib/repositories/asset_api.repository.dart @@ -66,6 +66,10 @@ class AssetApiRepository extends ApiRepository { return response.originalMimeType.orElse(null); } + Future getChecksum(String id) async { + return (await checkNull(_api.getAssetInfo(id))).checksum; + } + Future updateDescription(String assetId, String description) { return _api.updateAsset(assetId, UpdateAssetDto(description: Optional.present(description))); } diff --git a/mobile/lib/services/background_upload.service.dart b/mobile/lib/services/background_upload.service.dart index 43ee76f09..7e21dbdec 100644 --- a/mobile/lib/services/background_upload.service.dart +++ b/mobile/lib/services/background_upload.service.dart @@ -54,6 +54,7 @@ abstract class UploadTaskMetadata with _$UploadTaskMetadata { required String localAssetId, required bool isLivePhotos, required String livePhotoVideoId, + String? checksum, }) = _UploadTaskMetadata; Map toMap() { @@ -61,6 +62,7 @@ abstract class UploadTaskMetadata with _$UploadTaskMetadata { 'localAssetId': localAssetId, 'isLivePhotos': isLivePhotos, 'livePhotoVideoId': livePhotoVideoId, + 'checksum': checksum, }; } @@ -69,6 +71,7 @@ abstract class UploadTaskMetadata with _$UploadTaskMetadata { localAssetId: map['localAssetId'] as String, isLivePhotos: map['isLivePhotos'] as bool, livePhotoVideoId: map['livePhotoVideoId'] as String, + checksum: map['checksum'] as String?, ); } @@ -256,7 +259,11 @@ class BackgroundUploadService { return; } - await _assetService.stackEditedUpload(metadata.localAssetId, jsonDecode(update.responseBody!)['id'] as String); + await _assetService.stackEditedUpload( + metadata.localAssetId, + jsonDecode(update.responseBody!)['id'] as String, + metadata.checksum, + ); } catch (error, stackTrace) { dPrint(() => "Error stacking edited asset upload: $error $stackTrace"); } @@ -302,6 +309,7 @@ class BackgroundUploadService { localAssetId: asset.id, isLivePhotos: entity.isLivePhoto, livePhotoVideoId: '', + checksum: asset.checksum, ).toJson(); final requiresWiFi = _shouldRequireWiFi(asset); @@ -353,6 +361,7 @@ class BackgroundUploadService { localAssetId: asset.id, isLivePhotos: false, livePhotoVideoId: livePhotoVideoId, + checksum: asset.checksum, ).toJson(), fields: fields, group: kBackupLivePhotoGroup, diff --git a/mobile/lib/services/foreground_upload.service.dart b/mobile/lib/services/foreground_upload.service.dart index e58c65d8b..bd6a65f45 100644 --- a/mobile/lib/services/foreground_upload.service.dart +++ b/mobile/lib/services/foreground_upload.service.dart @@ -385,7 +385,7 @@ class ForegroundUploadService { if (result.isSuccess && result.remoteAssetId != null) { callbacks.onSuccess?.call(asset.localId!, result.remoteAssetId!); try { - await _assetService.stackEditedUpload(asset.localId!, result.remoteAssetId!); + await _assetService.stackEditedUpload(asset.localId!, result.remoteAssetId!, asset.checksum); } catch (error) { _logger.warning("Failed to stack the upload of ${asset.localId}: $error"); } diff --git a/mobile/test/medium/repositories/local_album_repository_test.dart b/mobile/test/medium/repositories/local_album_repository_test.dart index b261c4294..54a05a084 100644 --- a/mobile/test/medium/repositories/local_album_repository_test.dart +++ b/mobile/test/medium/repositories/local_album_repository_test.dart @@ -3,6 +3,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:immich_mobile/domain/models/album/local_album.model.dart'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/infrastructure/repositories/local_album.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/local_asset.repository.dart'; import '../repository_context.dart'; @@ -28,41 +29,80 @@ void main() { return (checksum: data.checksum, previousChecksum: data.previousChecksum); } - for (final (platform, scenario, edited, expected) in [ + for (final (platform, scenario, before, edited, after) in [ ( TargetPlatform.android, 'updatedAt changed clears the checksum and remembers it', + (checksum: 'a', previousChecksum: null), _localAsset('edited', updatedAt: DateTime(2025)), (checksum: null, previousChecksum: 'a'), ), ( TargetPlatform.android, 'adjustmentTime changed keeps the checksum', + (checksum: 'a', previousChecksum: null), _localAsset('edited', adjustmentTime: DateTime(2025)), (checksum: 'a', previousChecksum: null), ), + ( + TargetPlatform.android, + 'updatedAt changed again before the upload keeps the uploaded checksum', + (checksum: 'b', previousChecksum: 'a'), + _localAsset('edited', updatedAt: DateTime(2025)), + (checksum: null, previousChecksum: 'a'), + ), ( TargetPlatform.iOS, 'adjustmentTime changed clears the checksum and remembers it', + (checksum: 'a', previousChecksum: null), _localAsset('edited', adjustmentTime: DateTime(2025)), (checksum: null, previousChecksum: 'a'), ), ( TargetPlatform.iOS, 'updatedAt changed keeps the checksum', + (checksum: 'a', previousChecksum: null), _localAsset('edited', updatedAt: DateTime(2025)), (checksum: 'a', previousChecksum: null), ), + ( + TargetPlatform.iOS, + 'adjustmentTime changed again before the upload keeps the uploaded checksum', + (checksum: 'b', previousChecksum: 'a'), + _localAsset('edited', adjustmentTime: DateTime(2025)), + (checksum: null, previousChecksum: 'a'), + ), ]) { test('${platform.name}: $scenario', () async { debugDefaultTargetPlatformOverride = platform; - await ctx.newLocalAsset(id: 'edited', checksum: 'a', updatedAt: DateTime(2024), adjustmentTime: DateTime(2024)); + await ctx.newLocalAsset( + id: 'edited', + checksum: before.checksum, + previousChecksum: before.previousChecksum, + updatedAt: DateTime(2024), + adjustmentTime: DateTime(2024), + ); await sut.upsert(album, toUpsert: [edited]); - expect(await row('edited'), expected); + expect(await row('edited'), after); }); } + + test('android: previous follows the uploads across two edits', () async { + debugDefaultTargetPlatformOverride = TargetPlatform.android; + final assets = LocalAssetRepository(ctx.db); + await ctx.newLocalAsset(id: 'edited', checksum: 'a', updatedAt: DateTime(2024), adjustmentTime: DateTime(2024)); + + await assets.updatePreviousChecksum('edited', 'a'); + await sut.upsert(album, toUpsert: [_localAsset('edited', updatedAt: DateTime(2025))]); + expect(await row('edited'), (checksum: null, previousChecksum: 'a')); + + await assets.updateHashes({'edited': 'b'}); + await assets.updatePreviousChecksum('edited', 'b'); + await sut.upsert(album, toUpsert: [_localAsset('edited', updatedAt: DateTime(2026))]); + expect(await row('edited'), (checksum: null, previousChecksum: 'b')); + }); }); } diff --git a/mobile/test/services/background_upload.service_test.dart b/mobile/test/services/background_upload.service_test.dart index 75bad9dd8..64e89935e 100644 --- a/mobile/test/services/background_upload.service_test.dart +++ b/mobile/test/services/background_upload.service_test.dart @@ -54,7 +54,7 @@ void main() { mockBackupRepository = MockBackupRepository(); mockAssetMediaRepository = MockAssetMediaRepository(); mockAssetService = MockAssetService(); - when(() => mockAssetService.stackEditedUpload(any(), any())).thenAnswer((_) async {}); + when(() => mockAssetService.stackEditedUpload(any(), any(), any())).thenAnswer((_) async {}); sut = BackgroundUploadService( mockUploadRepository, @@ -416,7 +416,7 @@ void main() { group('onUploadStatus', () { test('stacks a plain photo after its upload', () async { - final asset = LocalAssetStub.image1; + final asset = LocalAssetStub.image1.copyWith(checksum: 'sha'); final mockEntity = MockAssetEntity(); final mockFile = File('/path/to/photo.jpg'); final void Function(TaskStatusUpdate) onStatus = verify( @@ -431,12 +431,12 @@ void main() { final task = await sut.getUploadTask(asset); onStatus(TaskStatusUpdate(task!, TaskStatus.complete, null, '{"id": "remote"}')); - verify(() => mockAssetService.stackEditedUpload(asset.id, 'remote')).called(1); + verify(() => mockAssetService.stackEditedUpload(asset.id, 'remote', 'sha')).called(1); verifyNoMoreInteractions(mockAssetService); }); test('stacks the still of a live photo, not its video', () async { - final asset = LocalAssetStub.image1; + final asset = LocalAssetStub.image1.copyWith(checksum: 'sha'); final mockEntity = MockAssetEntity(); final stillFile = File('/path/to/still.heic'); final videoFile = File('/path/to/motion.mov'); @@ -456,7 +456,7 @@ void main() { onStatus(TaskStatusUpdate(video!, TaskStatus.complete, null, '{"id": "video"}')); onStatus(TaskStatusUpdate(still!, TaskStatus.complete, null, '{"id": "still"}')); - verify(() => mockAssetService.stackEditedUpload(asset.id, 'still')).called(1); + verify(() => mockAssetService.stackEditedUpload(asset.id, 'still', 'sha')).called(1); verifyNoMoreInteractions(mockAssetService); }); }); diff --git a/mobile/test/services/foreground_upload.service_test.dart b/mobile/test/services/foreground_upload.service_test.dart index d8cac0838..bbfe2e649 100644 --- a/mobile/test/services/foreground_upload.service_test.dart +++ b/mobile/test/services/foreground_upload.service_test.dart @@ -55,7 +55,7 @@ void main() { mockConnectivityApi = MockConnectivityApi(); mockAssetMediaRepository = MockAssetMediaRepository(); mockAssetService = MockAssetService(); - when(() => mockAssetService.stackEditedUpload(any(), any())).thenAnswer((_) async {}); + when(() => mockAssetService.stackEditedUpload(any(), any(), any())).thenAnswer((_) async {}); sut = ForegroundUploadService( mockUploadRepository, @@ -203,7 +203,7 @@ void main() { }); test('stacks a plain photo after its upload', () async { - final asset = LocalAssetStub.image1; + final asset = LocalAssetStub.image1.copyWith(checksum: 'sha'); final mockEntity = MockAssetEntity(); final stillFile = File('/path/to/photo.jpg'); @@ -216,12 +216,12 @@ void main() { await sut.uploadSingleAsset(asset, null, callbacks: const UploadCallbacks()); - verify(() => mockAssetService.stackEditedUpload(asset.localId!, 'remote-1')).called(1); + verify(() => mockAssetService.stackEditedUpload(asset.localId!, 'remote-1', 'sha')).called(1); verifyNoMoreInteractions(mockAssetService); }); test('stacks the still of a live photo, not its video', () async { - final asset = LocalAssetStub.image1; + final asset = LocalAssetStub.image1.copyWith(checksum: 'sha'); final mockEntity = MockAssetEntity(); final stillFile = File('/path/to/still.heic'); final videoFile = File('/path/to/motion.mov'); @@ -236,7 +236,7 @@ void main() { await sut.uploadSingleAsset(asset, null, callbacks: const UploadCallbacks()); - verify(() => mockAssetService.stackEditedUpload(asset.localId!, 'remote-2')).called(1); + verify(() => mockAssetService.stackEditedUpload(asset.localId!, 'remote-2', 'sha')).called(1); verifyNoMoreInteractions(mockAssetService); }); }); diff --git a/mobile/test/unit/services/asset_service_test.dart b/mobile/test/unit/services/asset_service_test.dart index 9b116dd1a..c43daa1fe 100644 --- a/mobile/test/unit/services/asset_service_test.dart +++ b/mobile/test/unit/services/asset_service_test.dart @@ -3,6 +3,7 @@ import 'package:drift/native.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter_test/flutter_test.dart'; import 'package:immich_mobile/data/db/main/database.dart'; +import 'package:immich_mobile/domain/models/stack.model.dart'; import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/services/asset.service.dart'; import 'package:immich_mobile/domain/services/store.service.dart'; @@ -119,4 +120,54 @@ void main() { verifyNever(() => mocks.trashedAsset.applyTrashedAssets(any())); }); }); + + group('AssetService.stackEditedUpload', () { + const stack = StackResponse(id: 'stack', primaryAssetId: 'remote', assetIds: ['remote', 'previous']); + + setUp(() { + when(() => mocks.localAsset.repo.updatePreviousChecksum(any(), any())).thenAnswer((_) async {}); + }); + + test('stacks over the previous version, then records the uploaded checksum', () async { + when(() => mocks.localAsset.repo.getPreviousRemoteId('local')).thenAnswer((_) async => 'previous'); + when(() => apiRepository.stack(any())).thenAnswer((_) async => stack); + + await sut.stackEditedUpload('local', 'remote', 'sha'); + + verifyInOrder([ + () => apiRepository.stack(['remote', 'previous']), + () => mocks.localAsset.repo.updatePreviousChecksum('local', 'sha'), + ]); + verifyNever(() => apiRepository.getChecksum(any())); + }); + + test('records the uploaded checksum when there is nothing to stack on', () async { + when(() => mocks.localAsset.repo.getPreviousRemoteId('local')).thenAnswer((_) async => null); + + await sut.stackEditedUpload('local', 'remote', 'sha'); + + verifyNever(() => apiRepository.stack(any())); + verify(() => mocks.localAsset.repo.updatePreviousChecksum('local', 'sha')).called(1); + }); + + test('records the uploaded checksum even when the stack call fails', () async { + when(() => mocks.localAsset.repo.getPreviousRemoteId('local')).thenAnswer((_) async => 'previous'); + when(() => apiRepository.stack(any())).thenThrow(Exception('offline')); + + await expectLater(sut.stackEditedUpload('local', 'remote', 'sha'), throwsException); + + verify(() => mocks.localAsset.repo.updatePreviousChecksum('local', 'sha')).called(1); + }); + + test('asks the server for the checksum when the upload did not know it', () async { + when(() => mocks.localAsset.repo.getPreviousRemoteId('local')).thenAnswer((_) async => 'previous'); + when(() => apiRepository.getChecksum('remote')).thenAnswer((_) async => 'srv'); + when(() => apiRepository.stack(any())).thenAnswer((_) async => stack); + + await sut.stackEditedUpload('local', 'remote', null); + + verify(() => apiRepository.stack(['remote', 'previous'])).called(1); + verify(() => mocks.localAsset.repo.updatePreviousChecksum('local', 'srv')).called(1); + }); + }); }