From 6733c14f76086ee387309d5d22820008edcf7fbb Mon Sep 17 00:00:00 2001 From: shenlong-tanwen <139912620+shalong-tanwen@users.noreply.github.com> Date: Sun, 21 Dec 2025 06:37:48 +0530 Subject: [PATCH] fix(mobile): handle missing extension during uploads --- mobile/lib/services/upload.service.dart | 81 +++++--- mobile/test/services/upload.service_test.dart | 173 +++++++++++++++++- 2 files changed, 232 insertions(+), 22 deletions(-) diff --git a/mobile/lib/services/upload.service.dart b/mobile/lib/services/upload.service.dart index 1ce0cf0322..b6b2648a3d 100644 --- a/mobile/lib/services/upload.service.dart +++ b/mobile/lib/services/upload.service.dart @@ -271,12 +271,12 @@ class UploadService { return null; } - final file = await _storageRepository.getFileForAsset(asset.id); - if (file == null) { + final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto); + if (uploadFileResult == null) { return null; } - final originalFileName = entity.isLivePhoto ? p.setExtension(asset.name, p.extension(file.path)) : asset.name; + final (:file, :originalFilename) = uploadFileResult; String metadata = UploadTaskMetadata( localAssetId: asset.id, @@ -290,7 +290,7 @@ class UploadService { file, createdAt: asset.createdAt, modifiedAt: asset.updatedAt, - originalFileName: originalFileName, + originalFileName: originalFilename, deviceAssetId: asset.id, metadata: metadata, group: "group", @@ -308,8 +308,6 @@ class UploadService { return null; } - File? file; - /// iOS LivePhoto has two files: a photo and a video. /// They are uploaded separately, with video file being upload first, then returned with the assetId /// The assetId is then used as a metadata for the photo file upload task. @@ -320,18 +318,12 @@ class UploadService { /// The cancel operation will only cancel the video group (normal group), the photo group will not /// be touched, as the video file is already uploaded. - if (entity.isLivePhoto) { - file = await _storageRepository.getMotionFileForAsset(asset); - } else { - file = await _storageRepository.getFileForAsset(asset.id); - } - - if (file == null) { + final uploadFileResult = await prepareUploadFile(asset, isLivePhoto: entity.isLivePhoto); + if (uploadFileResult == null) { return null; } - final fileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name; - final originalFileName = entity.isLivePhoto ? p.setExtension(fileName, p.extension(file.path)) : fileName; + final (:file, :originalFilename) = uploadFileResult; String metadata = UploadTaskMetadata( localAssetId: asset.id, @@ -345,7 +337,7 @@ class UploadService { file, createdAt: asset.createdAt, modifiedAt: asset.updatedAt, - originalFileName: originalFileName, + originalFileName: originalFilename, deviceAssetId: asset.id, metadata: metadata, group: group, @@ -362,21 +354,20 @@ class UploadService { return null; } - final file = await _storageRepository.getFileForAsset(asset.id); - if (file == null) { + final result = await prepareUploadFile(asset); + if (result == null) { return null; } final fields = {'livePhotoVideoId': livePhotoVideoId}; final requiresWiFi = _shouldRequireWiFi(asset); - final originalFileName = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name; return buildUploadTask( - file, + result.file, createdAt: asset.createdAt, modifiedAt: asset.updatedAt, - originalFileName: originalFileName, + originalFileName: result.originalFilename, deviceAssetId: asset.id, fields: fields, group: kBackupLivePhotoGroup, @@ -398,6 +389,54 @@ class UploadService { return requiresWiFi; } + @visibleForTesting + Future<({File file, String originalFilename})?> prepareUploadFile( + LocalAsset asset, { + bool isLivePhoto = false, + }) async { + final file = isLivePhoto + ? await _storageRepository.getMotionFileForAsset(asset) + : await _storageRepository.getFileForAsset(asset.id); + + if (file == null) { + return null; + } + + final originalFilename = await _assetMediaRepository.getOriginalFilename(asset.id) ?? asset.name; + + if (isLivePhoto) { + final livePhotoFilename = p.setExtension(originalFilename, p.extension(file.path)); + return (file: file, originalFilename: livePhotoFilename); + } + + final filenameExt = p.extension(originalFilename); + if (filenameExt.isNotEmpty) { + return (file: file, originalFilename: originalFilename); + } + + final assetNameExt = p.extension(asset.name); + if (assetNameExt.isNotEmpty) { + final correctedFilename = p.setExtension(originalFilename, assetNameExt); + _logger.fine( + "Corrected filename $originalFilename to $correctedFilename using asset.name extension $assetNameExt", + ); + return (file: file, originalFilename: correctedFilename); + } + + final filePathExt = p.extension(file.path); + if (filePathExt.isEmpty) { + _logger.warning( + "Asset ${asset.id} has no file extension in any source, using original filename - $originalFilename", + ); + return (file: file, originalFilename: originalFilename); + } + + final correctedFilename = p.setExtension(originalFilename, filePathExt); + _logger.fine("Corrected filename $originalFilename to $correctedFilename using file path extension $filePathExt"); + + return (file: file, originalFilename: correctedFilename); + } + Future buildUploadTask( File file, { required String group, diff --git a/mobile/test/services/upload.service_test.dart b/mobile/test/services/upload.service_test.dart index d33126782f..72bb82570b 100644 --- a/mobile/test/services/upload.service_test.dart +++ b/mobile/test/services/upload.service_test.dart @@ -4,6 +4,7 @@ import 'package:drift/drift.dart' hide isNull, isNotNull; import 'package:drift/native.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/store.model.dart'; import 'package:immich_mobile/domain/services/store.service.dart'; import 'package:immich_mobile/entities/store.entity.dart'; @@ -16,8 +17,8 @@ import 'package:mocktail/mocktail.dart'; import '../domain/service.mock.dart'; import '../fixtures/asset.stub.dart'; import '../infrastructure/repository.mock.dart'; -import '../repository.mocks.dart'; import '../mocks/asset_entity.mock.dart'; +import '../repository.mocks.dart'; void main() { late UploadService sut; @@ -165,4 +166,174 @@ void main() { verify(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).called(1); }); }); + + group('prepareUploadFile', () { + test('should keep filename with existing extension unchanged', () async { + final asset = LocalAssetStub.image1; + final mockFile = File('/tmp/123.jpg'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.jpg'); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.file.path, equals('/tmp/123.jpg')); + expect(result.originalFilename, equals('photo.jpg')); + }); + + test('should use asset.name extension when original filename lacks one', () async { + final asset = LocalAssetStub.image1; + final mockFile = File('/tmp/cache/123.mov'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30'); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.originalFilename, equals('2024-10-23_17-00-30.jpg')); + }); + + test('should use file path extension as final fallback', () async { + final asset = LocalAssetStub.image1.copyWith(name: 'document'); + final mockFile = File('/tmp/cache/123.mov'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document'); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.originalFilename, equals('document.mov')); + }); + + test('should handle file without extension anywhere', () async { + final asset = LocalAssetStub.image1.copyWith(name: 'document'); + final mockFile = File('/tmp/temp'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'document'); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.originalFilename, equals('document')); + }); + + test('should preserve existing extension even if asset.name has different one', () async { + final asset = LocalAssetStub.image1; + final mockFile = File('/tmp/123.mov'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'photo.HEIC'); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.originalFilename, equals('photo.HEIC')); + }); + + test('should fall back to asset.name when getOriginalFilename returns null', () async { + final asset = LocalAssetStub.image1.copyWith(name: 'VID_1234.mp4'); + final mockFile = File('/tmp/video.mov'); + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => null); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNotNull); + expect(result!.originalFilename, equals('VID_1234.mp4')); // Uses asset.name directly + }); + + test('should return null when file is not found', () async { + final asset = LocalAssetStub.image1; + + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => null); + + final result = await sut.prepareUploadFile(asset); + expect(result, isNull); + }); + }); + + group('getUploadTask with missing extensions', () { + test('should add extension for regular photo without extension', () async { + final asset = LocalAssetStub.image1; + final mockEntity = MockAssetEntity(); + final mockFile = File('/path/to/file.jpg'); + + when(() => mockEntity.isLivePhoto).thenReturn(false); + when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity); + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => '2024-10-23_17-00-30'); + + final task = await sut.getUploadTask(asset); + + expect(task, isNotNull); + expect(task!.fields['filename'], equals('2024-10-23_17-00-30.jpg')); + }); + + test('should preserve existing extension for regular photo', () async { + final asset = LocalAssetStub.image1; + final mockEntity = MockAssetEntity(); + final mockFile = File('/path/to/file.jpg'); + + when(() => mockEntity.isLivePhoto).thenReturn(false); + when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity); + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyPhoto.HEIC'); + + final task = await sut.getUploadTask(asset); + + expect(task, isNotNull); + expect(task!.fields['filename'], equals('MyPhoto.HEIC')); + }); + + test('should add extension for video without extension', () async { + // Create a video asset using copyWith since image2 is a video type + final asset = LocalAssetStub.image1.copyWith(id: 'video1', name: 'VID_20241023_170030', type: AssetType.video); + final mockEntity = MockAssetEntity(); + final mockFile = File('/path/to/video.mov'); + + when(() => mockEntity.isLivePhoto).thenReturn(false); + when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity); + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'VID_20241023_170030'); + + final task = await sut.getUploadTask(asset); + + expect(task, isNotNull); + expect(task!.fields['filename'], equals('VID_20241023_170030.mov')); + }); + }); + + group('getLivePhotoUploadTask with missing extensions', () { + test('should add extension when live photo filename lacks one', () async { + final asset = LocalAssetStub.image1.copyWith(name: 'IMG_1234.heic'); + final mockEntity = MockAssetEntity(); + final mockFile = File('/path/to/photo.heic'); + + when(() => mockEntity.isLivePhoto).thenReturn(true); + when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity); + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'IMG_1234'); + + final task = await sut.getLivePhotoUploadTask(asset, 'video-id-123'); + + expect(task, isNotNull); + expect(task!.fields['filename'], equals('IMG_1234.heic')); + expect(task.fields['livePhotoVideoId'], equals('video-id-123')); + }); + + test('should preserve extension when live photo filename has one', () async { + final asset = LocalAssetStub.image1; + final mockEntity = MockAssetEntity(); + final mockFile = File('/path/to/photo.heic'); + + when(() => mockEntity.isLivePhoto).thenReturn(true); + when(() => mockStorageRepository.getAssetEntityForAsset(asset)).thenAnswer((_) async => mockEntity); + when(() => mockStorageRepository.getFileForAsset(asset.id)).thenAnswer((_) async => mockFile); + when(() => mockAssetMediaRepository.getOriginalFilename(asset.id)).thenAnswer((_) async => 'MyLivePhoto.HEIC'); + + final task = await sut.getLivePhotoUploadTask(asset, 'video-id-456'); + + expect(task, isNotNull); + expect(task!.fields['filename'], equals('MyLivePhoto.HEIC')); + }); + }); }